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

Status: VERIFIED FIXED

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

Description

[skierpage]

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.

Comment 1

[Samuel Sidler (old account; do not CC)]

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

Comment 2

[Ria Klaassen (not reading all bugmail)]

Caused on 20 June by bug 367177.

Comment 3

[Uri Bernstein]

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.

Comment 4

[Uri Bernstein]

(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.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: near-minimal testcase

Double-clicking on 'roc' selects 'out' as well.

Comment 6

[Phil Ringnalda (:philor)]

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.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: I hate myself

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

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.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: updated fix

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.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I backed this out because the tests don't pass on Linux or Windows.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: updated test

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.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I relanded this a while ago.

Comment 14

[skierpage]

(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?

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

File a new bug with a testcase and make sure it isn't a duplicate of bug 391464.

Comment 16

[skierpage]

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.

Comment 17

[Ria Klaassen (not reading all bugmail)]

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.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Indeed, it's not fixed when eat_space_next_to_word is false. Sorry about that.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I mean, it's not fixed when eat_space_next_to_word is *true*.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: further fix

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.

Comment 21

[Simon Montagu :smontagu]

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".

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: updated for checking

Comment 23

[Reed Loden [:reed]]

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

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: mochitests

I'll check these in when I get a chance

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Test checked in

Comment 26

[Al Billings [:abillings - ex-MoCo]]

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.