* TITLE/SUMMARY [PP] Triple-click selection blocked by style changes (writing up as courtesy for mjudge) * STEPS TO REPRODUCE 0) Launch Apprunner 1) View Demo 0 (from the Debug menu) 2) Attempt to triple-click in any of the paragraphs with style changes (e.g. "This is a basic paragraph..." or "This paragraph is aligned left..." * RESULT - What happened The selection will be "blocked" on each end upon reaching a style change (e.g. Bold, italics, etc). e.g. in the first example, the selection would be limited to, "This is a basic paragraph with" - What was expected Selection only to be blocked by a line break. * REGRESSION - Occurs On Mac OS Apprunner (1999101308 optimized build) Linux Apprunner (1999101308 optimized build) - Doesn't Occur On Win32 Apprunner (1999101308 optimized build [NT 4, Service Pack 5]) --- triple click not supported * CONFIGURATIONS TESTED - [Mac] Beige Power Mac G3 (266 MHz PowerPC 750), 96 MB RAM (VM on; 1 MB of VM used), 1024x768 (Thousands of Colors), Mac OS 8.6 - [Win32] Vectra VL (233 MHz P2), 96 MB RAM, 800x600 (True Color), NT 4.0 SP5. - [Linux] Vectra VL (266 MHz P2), 96 MB RAM. Red Hat Linux 6.0 (GNOME).

setting to M15, this can wait until after beta push

updating keyword and status whiteboard to reflect that this is a beta 2 feature work bug that the Composer team deems a must fix for beta 2.

I'll try to look at this -- it's related to the word-selection bug which I'm already working on.

This bug seems to have been fixed: triple-clicking selects the whole line even when there are style changes in it. In html, it should select by paragraph, not by line; that's covered by bug 32807. On Windows, triple-click events don't work at all; that's another bug (I don't have the number handy, but search for "triple").

Re-opening, per 5.3.00 builds on Win/Linux. Specifically, triple-clicking on the "This is a paragraph with font variations..." paragraph doesn't result in a complete selection. Instead, the selection stops before the 'COURIER' text. Same results occur by triple-clicking on the text after the 'COURIER' text (e.g. on 'Times New Roman').

How strange ... Sure enough, I see the same thing. The difference with the COURIER section (the reason it stops there) may have something to do with it having another font tag nested inside it.

[nsbeta2-]

Adding akkana email comments for PDT re-review on May 8: "The PDT team just marked 16311 as nsbeta2-, but 32807 as nsbeta2+, but with no associated comments. Does this mean that you want me to make paragraph selection work but leave line selection broken, or the other way around? (I admit, the bugs are confusing.) The most recent comments in the bugs seems to say that most people care more about having line selection on triple click than paragraph selection. Please let me know what the nsbeta2 priority is, so I know where to concentrate my efforts. I spent this week working on both of them together, but if PDT considers one of them expendable I can drop that part of the work and concentrate on the other."

Changing summary to make it clear that this is the line selection bug, and that it doesn't happen only on triple-click (but also on line motions initiated through key bindings). PDT: I would recommend that this one be nsbeta2+, ahead of paragraph selection (32807).

the vast majority of selection is working, this is not "new" feature work this is bug fixing. This needs to be fixed by beta3. I am recommending that beta2 not be held for this, but mark it for beta3.

I should have also stated that if Akkana can get this fix in for beta2 then that would be outstanding

updating whiteboard

Long-term problems, at risk for nsbeta2.

Akkana -- I can't reproduce the initial problem on mac or windows using the current build. I can select a line (triple-click) regardless of the in-line style on the line. I will attach a test case.

Beth: have you tried it with Eli's original instructions using test0? I agree, there are lots of cases where this doesn't happen, but I think you'll find that the original case still happens. I can't test this on today's build to make sure, because today's build loops forever waiting for test0 to load, whether I click on the url in this bug or call up test0 from the debug menu.

moving thisone out to beta3, removing nsbeta2 data

setting to nsbeta3+

adding brackets to status whiteboard

Changing platform to all.

setting priority in status whiteboard

*SPAM*: Changing the QA contact of all open/resolved Selection bugs from elig@netscape.com to BlakeR1234@aol.com. After the many great years of service Eli has given to Mozilla, it's time for him to move on; he has accepted a position at Eazel. We'll be sad to see him go, and I'll do my best to fill his spot...

moving out to future, this can wait until the next release

Anthony is taking over selection bugs.

setting to 0.9 to get it into 6.5. anthonyd

using the given testcase (viewer debug demo #0) this works for me on windows. does anyone have any information on other platforms? anthonyd

okay i tested this on todays trunk builds for win32, linux, and mac. I could not reproduce this, marking bug as WORKSFORME. anthonyd

This has never been working. I can't test the viewer demo as is described in the original comment, since the Debug menu has been broken undex linux for about 9 months now, but here's another URL: http://www.theregister.co.uk/content/4/21288.html You can triple click to select the first line in the story, but other lines, you can't. Triple click selects the first word in the line. (no matter where the mouse was). Exception: Lines with style (like the third, with "Poteau Daily News" in italics, select all of the line up to the END of the style, then stop. Anyway, idealy the whole line selection thing is tossed and bug 32807 is fixed, since HTML has no concept of lines.

Due to screwy cache/form interaction, Cc addition disapeared... sigh.

*** Bug 90984 has been marked as a duplicate of this bug. ***

setting milestone from duplicate bug

the following line on debug->viewer demos->#0 is still broken: This is a paragraph with font variations: Bold Arial, Italic Verdana, COURIER, Times New Roman triple clicking anywhere on that line causes the line to be selected fromt he beginning, untill the end of Verdana.

--> mjudge

-> 0.9.5. sorry, we can't deal w/ regressions like this right now.

If this is important to someone for 0.9.5, tell me now so I can find another owner for it. mjudge won't be back until 0.9.6.

-> 0.9.6

0.9.6 is gone -> 0.9.7

changing selection qa to tpreston.

removing myself from the cc list

Both the attached testcase, and the site mentioned in comment #28, WFM with: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2) Gecko/20050603 Firefox/1.0+ Is anyone still seeing this bug on any testcase?

The bug is actually still visible in the case described in comment #32. Changing the URL to one which can be accesed in Firefox. The problem is that geometric scanning (nsFrame::GetContentAndOffsetsFromPoint) is used to find the last (and first) frame on the line. This fails if the top of such frame is lower than the top of its parent. Taking. Patch will follow.

This solution is similar to the one used for bug 263309, and makes use of some of the code written for that bugfix. The idea is that there is no need to use a (buggy) geometric algorithm to find the first and last frames (and contents) on the line, since the line iterator (together with the frame tree) give us this information. This works just fine in the BiDi case as well, and actually fixes bug 260781. I created DrillDownToBeginningOfLine() for the symmetry with DrillDownToEndOfLine(). Both methods are now relatively simple and are called only from one place - so maybe they should be inlined?

Comment on attachment 186741 [details] [diff] [review] patch +DrillDownToBeginningOfLine(nsIFrame* aFrame, + nsPeekOffsetStruct* aPos) Fix indentation + aPos->mContentOffset = firstFrame->GetOffsets(startOffset, endOffset); That returns an nsresult. So you should do "nsresult rv = ...; if (NS_FAILED(rv)) return rv;" + return PR_TRUE; return NS_OK; + for (i=1; i<aLineFrameCount && nextFrame; i++) //already have 1st frame This is confusing. Why can't you just do for (i = 0; i < aLineFrameCount; ++i) { NS_ASSERTION(currentFrame, "lineFrame Count lied to us from nsILineIterator!"); currentFrame = currentFrame->GetNextSibling(); } and you don't need nextFrame at all? + aPos->mContentOffset = nextFrame->GetOffsets(startOffset, endOffset); + aPos->mContentOffset = endOffset; + + return PR_TRUE; as above

Thanks for the comments. Some of my mistakes were more embarassing than others. I fixed everything according to your suggestions, except for one issue: the loop over siblings in DrillDownToEndOfLine() absolutely needs to start from 1, not 0 (because if aLineFrameCount==1 you don't want to get into the loop even once!). Changing i++ to ++i doesn't help, of course. Also, I placed the assertion such that it guarantees that currentFrame is not null when the loop is exited (that it is not null to begin with is ensured at the very beginning of the method). Note that this patch (unlike the original code and the preivious patch) is not tolerant to "lineFrame Count lying" - i.e. it will ASSERT if a too large count is given, whereas the original code would complain but still work. Hopefully this is a theoretical issue (in my testing I haven't come accross a case where a wrong count was provided).

*** Bug 175658 has been marked as a duplicate of this bug. ***

If the line frame count is wrong, we are already doomed in so many other places.

Comment on attachment 186841 [details] [diff] [review] patch v2 Actually, I just discovered that this patch causes a crash when using cmd-arrow ("End" on PC) to navigate to the end of a line ending with a hard line break (<BR>), when in BiDi mode. (nsSelection::GetFrameForNodeOffset failes on the BRFrame, and nsSelectionMoveCaret() does not check its return value). I'll work on a fix.

This includes a fix for a regression caused by the previous patch. nsSelection::GetFrameForNodeOffset() might now get a childless node (specifically, a BR node). It shouldn't fail in this case - just return the frame assosiated with this node itself. Also, in MoveCaret(), check the return value of GetFrameForNodeOffset(), so that a failure won't become a crash (this is just for good measure).

checked in. Thanks!

Please note that the fix for this bug was checked in together with the fix to bug 32807. Therefore, to verify that this bug is fixed, you must first set browser.triple_click_selects_paragraph to false.

This checkin caused bug 305239 (tested by locally backing this patch out -- that fixes that bug).

*** Bug 126290 has been marked as a duplicate of this bug. ***