Closed Bug 391584 Opened 15 years ago Closed 14 years ago

double-clicking a word followed by link selects the first word of the link text

Categories

(Core :: DOM: Selection, defect, P2)

x86
Windows XP
defect

Tracking

()

VERIFIED FIXED
mozilla1.9beta2

People

(Reporter: info, Assigned: roc)

References

Details

(Whiteboard: [dbaron-1.9:RuCt])

Attachments

(5 files, 3 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007080910 Minefield/3.0a8pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007080910 Minefield/3.0a8pre ID:2007080910

(See summary.)

Reproducible: Always

Steps to Reproduce:
1. Double-click a word that is followed by a hyperlink.
   For example, clickthisword comment 0.

Actual Results:  
The selection extends to the first word of the hyperlink.

Expected Results:  
Selection should only be the word and trailing whitespace.

This over-selection is a bit like bug 206141, so I put it in component Selection instead of GFX:Thebes.
I bet this has the same cause as bug 391464, so adding a dependency there and confirming. I can reproduce this using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a8pre) Gecko/2007080815 Minefield/3.0a8pre
Status: UNCONFIRMED → NEW
Depends on: 391464
Ever confirmed: true
Caused on 20 June by bug 367177.
Blocks: 367177
This bug as described only occurs when layout.word_select.eat_space_to_next_word=true (i.e, by default on Windows). But the same occurs when clicking a word following a link (e.g. bug 391584 clickthisword), even if layout.word_select.eat_space_to_next_word is false.
Flags: blocking1.9?
Blocks: 391783
(In reply to comment #3)
> (e.g. bug 391584 clickthisword)

Sorry, this example doesn't actually show the bug. But try clicking your user name after the "Log out" on the top of this page, and the word "out" is selected.
No longer blocks: 391783
Flags: blocking1.9? → blocking1.9+
Assignee: nobody → roc
Attached file near-minimal testcase (obsolete) —
Double-clicking on 'roc' selects 'out' as well.
If a 62Kb patch replacing nsAutoBuffer with nsAutoTArray is a near-minimal testcase for this bug, I think we're in trouble ;)

Is this actually one, not two bugs? The minimal testcase for comment 0 is data:text/html,a%20<i>b but comment 3 seems to be something more involved.
Attached file I hate myself
Attachment #281396 - Attachment is obsolete: true
Attached patch fix (obsolete) — Splinter Review
Okay, this fixes it. The problem is when whitespace is collapsing; if the word break opportunity is associated with a space that collapses away, then we don't notice it. The solution is to remember when we pass over a collapsed space with a word-break opportunity.

This also fixes another bug in that testcase --- putting the caret in "out" and then doing a word-right advanes to the start of "roc" instead of stopping at the end of "out". This is because of the XXX-commented problem that we never have a word-break at the start of a frame. A simple way to improve this is to note a word break opportunity at the start of the frame when the frame starts with white-space.
Attachment #281407 - Flags: superreview?(smontagu)
Attachment #281407 - Flags: review?(smontagu)
Attachment #281407 - Flags: superreview?(smontagu)
Attached patch updated fixSplinter Review
Okay, here's an update of the patch, this time with a mochitest.

While I was writing the test I realized we should also record a word break opportunity at the end of a frame when there's white-space there. This is still a bit of a hack ... a genuine fix would provide all possible context from adjacent frames when we call nsIWordBreaker.
Attachment #281407 - Attachment is obsolete: true
Attachment #281601 - Flags: review?(smontagu)
Attachment #281407 - Flags: review?(smontagu)
Attachment #281601 - Flags: review?(smontagu) → review+
checked in
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
I backed this out because the tests don't pass on Linux or Windows.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch updated testSplinter Review
Same patch, but this time it sets the layout.word_select.eat_space_to_next_word pref to false while running the tests. I'll try relanding this.
I relanded this a while ago.
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
(In reply to comment #4)
> try clicking your user
> name after the "Log out" on the top of this page, and the word "out" is
> selected.

Incorrectly selecting the hyperlink before is fixed in Minefield, for at least a few days.  However, incorrectly selecting the hyperlink after still happens in build 2007101704.  I think the bug should be reopened, or maybe the most-excellent Mr. O'Callahan has fixed a branch?

File a new bug with a testcase and make sure it isn't a duplicate of bug 391464.
The exact behavior I describe at the top of this bug is still present in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007102105 Minefield/3.0a9pre ID:2007102105
Double-click any word in the comments that's followed by a link, the first word of the link text is selected as well.

Why file the same text in a new bug?  I'm reopening this.  Sorry if I missed something obvious.

This seems different from bug 391464; Uri Bernstein thinks bug 391783 is the same.  I just re-confirmed all three in my build.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007102205 Minefield/3.0a9pre

I confirm that this bug (steps-to-reproduce from comment 0) is this present in the latest trunk hourly.
Blocks: 391783
Indeed, it's not fixed when eat_space_next_to_word is false. Sorry about that.
I mean, it's not fixed when eat_space_next_to_word is *true*.
Attached patch further fix (obsolete) — Splinter Review
This patch makes us pass around the contents of earlier text frames as context for making word-breaking decisions. This way we can correctly identify a word-break opportunity at the edge of a frame, fixing this bug.
Attachment #285835 - Flags: review?(smontagu)
Whiteboard: [needs review]
Comment on attachment 285835 [details] [diff] [review]
further fix

>+  PRInt32 textStart;
>+  if (aDirection > 0) {
>+    if (aContext.IsEmpty()) {
>+      // No left context, so it must be the start of a line or text run
>+      mWordBreaks[0] = PR_TRUE;
>+    }
>+    textStart = aContext.Length();
>+    mFrag->AppendTo(aContext, textOffset, textLen);
>+  } else {
>+    if (aContext.IsEmpty()) {
>+      // No right context, so it must be the start of a line or text run
>+      mWordBreaks[textLen] = PR_TRUE;

s/start/end/ in the second comment, and please use "previous" and "subsequent" or something, not "right" and "left".
Attachment #285835 - Flags: review?(smontagu) → review+
Whiteboard: [needs review] → [needs landing]
Whiteboard: [needs landing] → [needs landing][dbaron-1.9:RuCt]
Attachment #285835 - Attachment is obsolete: true
Attachment #287633 - Flags: review+
Checking in layout/generic/nsFrame.cpp;
/cvsroot/mozilla/layout/generic/nsFrame.cpp,v  <--  nsFrame.cpp
new revision: 3.765; previous revision: 3.764
done
Checking in layout/generic/nsIFrame.h;
/cvsroot/mozilla/layout/generic/nsIFrame.h,v  <--  nsIFrame.h
new revision: 3.381; previous revision: 3.380
done
Checking in layout/generic/nsTextFrameThebes.cpp;
/cvsroot/mozilla/layout/generic/nsTextFrameThebes.cpp,v  <--  nsTextFrameThebes.cpp
new revision: 3.121; previous revision: 3.120
done
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing][dbaron-1.9:RuCt] → [dbaron-1.9:RuCt]
Target Milestone: --- → mozilla1.9 M10
Depends on: 403060
Attached patch mochitestsSplinter Review
I'll check these in when I get a chance
Blocks: 399652
Verified in Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050704 Minefield/3.0pre and  Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008050706 Minefield/3.0pre. 
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.