415707 - multiple text selection improvement doesn't work with double-click "word-by-word" or triple-click "paragraph" modes

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Aaron J. Peterson]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b2) Gecko/2007121014 Firefox/3.0b2
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b2) Gecko/2007121014 Firefox/3.0b2

Trying text selection improvements in the 3.0b2 rel notes, it behaves as if CMD was not held and forgets the first selection with CMD-double-click or CMD-triple-click.  It does not matter how the first selection was made.



Reproducible: Always

Steps to Reproduce:
1. Click & drag to create a selection.  
2. Then CMD-double-click or CMD-triple-click, with or without dragging, to add a selection.
Actual Results:  
first selection is forgotten, a new selection is made

Expected Results:  
A word or paragraph delimited selection should be added to the first selection, as should further selections of any type (char, word, pargraph) made with Ctrl/Cmd held down.

Comment 1

[Uri Bernstein]

Confirming.

Comment 4

[Elias Hasle]

(In reply to comment #3)

Bug 423624 shows the same problem in Beta 4 on Windows XP.

Comment 5

[Uri Bernstein]

(In reply to comment #4)
> (In reply to comment #3)
> 
> Bug 423624 shows the same problem in Beta 4 on Windows XP.
> 

Yeah, this is cross-platform. Marking All/All.

Comment 6

[Uri Bernstein]

Attachment: WIP (depends on bug 392746)

This would have fixed it, I think, if not for bug 392746.

Comment 7

[Uri Bernstein]

Comment on attachment 311130 [details] [diff] [review]
WIP (depends on bug 392746)

I verified that with my proposed fix for bug 392746, this patch does in fact fix this bug.

Comment 8

[Uri Bernstein]

Comment on attachment 311130 [details] [diff] [review]
WIP (depends on bug 392746)

Actually, this fixes double[triple]-click, but not double[triple]-click-drag. Will have to look further into that.

Comment 9

[Uri Bernstein]

Notes:

1. nsFrameSelection::MaintainSelection() uses the range at position 0. Should probably use mAnchorFocusRange instead.

2. nsFrameSelection::AdjustForMaintainedSelection() uses Collapse(), which collapses the entire selection. We need something that collapses just the current range.

Comment 10

[Uri Bernstein]

Attachment: patch v0.9

This adds the two items mentioned in comment #9, and also removes part of nsFrameSelection::AdjustForMaintainedSelection() that is not necessary now that bug 16203 is fixed.

Unfortunately, replacing the calls to Collapse() by calls to CopyRangeToAnchorFocus() causes "selection turds" (areas that remain highlighted after they're de-selected, until the window is re-focused) in certain cases. I tried calling Repaint() but that doesn't help. I'd appreciate help on this.

Comment 11

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

The patch looks good other than that last issue. nsTypedSelection::Collapse does selectFrames and NotifySelectionListeners, but CopyRangeToAnchorFocus does not (but its caller nsTypedSelection::Extend does). I think you need to do those yourself. Or maybe you could extend Collapse so that it can optionally just collapse the current range.

Comment 12

[Uri Bernstein]

Comment on attachment 341773 [details] [diff] [review]
patch v0.9

Thanks for the feedback. I'll have a closer look when I have some time. Removing the review request for now.

Comment 13

[Uri Bernstein]

Attachment: patch v1.0

This is based on v0.9, with the following changes:

- Added nsTypedSelection::ReplaceAnchorFocusRange(), that de-selects the frames of the current anchor/focus range before replacing it (and selecting the frames of the new range). It turns out that the de-selecting was the part I was actually missing (for the problem in comment 10), and since it's difficult to access the current anchor/focus range from nsFrameSelection, I had to implement this in nsTypedSelection.

- Changed the implementation of AdjustForMaintainedSelection() (again), and restored its original API (so to not rely on the fix for bug 16203). This implementation is the most simple and elegant I could come up with.

- Added nsTypedSelection::RemoveCollapsedRanges() and called it when adding a range for multiple-selection. This is not strictly necessary. I originally did it because I thought it would fix what turned out to be bug 459599. It does, in fact, reduce the probability of hitting that bug when ctrl-doubleclick-dragging, and I also think it's a reasonable thing to do (having collapsed ranges lying around doesn't help anyone). But if it's considered unnecessary, I'll remove it.

As a side note, I think the current structure of nsFrameSelection vs. nsTypedSelection is flawed. I have some ideas how to refactor those classes to make more sense. I'll probably lay them out in a separate bug.

Comment 14

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

Comment on attachment 342819 [details] [diff] [review]
patch v1.0

Looks good.

Would it be possible to write a test for this? I think you could, by synthesizing mouse events.

Comment 15

[Uri Bernstein]

(In reply to comment #14)
> Would it be possible to write a test for this? I think you could, by
> synthesizing mouse events.

I tried generating a double-click selection using repeated calls to nsIDOMWindowUtils::sendMouseEvent, but nothing was actually selected.

Martijn - have you ever done this? Can you point me to an example?

Comment 16

[Uri Bernstein]

http://hg.mozilla.org/mozilla-central/rev/3632fa9aad9b

Comment 17

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #15)
> I tried generating a double-click selection using repeated calls to
> nsIDOMWindowUtils::sendMouseEvent, but nothing was actually selected.

To simulate a double click, you need to send a mousedown then mouseup with clickCount=2, like this:

utils.sendMouseEvent("mousedown", x, y, 0, 2, 0);
utils.sendMouseEvent("mouseup", x, y, 0, 2, 0);

(To accurately simulate a double click, you should probably send a mousedown/mouseup with clickCount=1 first, but I don't think it makes a practical difference.)

If you have access to EventUtils.js, you can instead use synthesizeMouse() and pass {clickCount: 2} as the aEvent parameter.

Comment 18

[Uri Bernstein]

Attachment: mochitest

Thanks, Gavin. My problem turned out to be that I didn't delay the test running using setTimeout(), as I apparently should have.

This test *should* work cross-platform, but I only tested it on Mac. Could someone please test it on Windows/Linux? Also, any other comments are welcome, as I'm still pretty new to Mochitests.

Comment 19

[Uri Bernstein]

Comment on attachment 343978 [details] [diff] [review]
mochitest

>+  // It's cmd+click on mack, ctrl+click otherwise.
>+  var keyMask = (osString == "Darwin") ? 8 : 2;

Where by "mack" I mean "Mac".

Comment 20

[Uri Bernstein]

Attachment: better mochitest

Using EventUtils.js, and including a workaround for bug 426195.

Comment 21

[Uri Bernstein]

I'm not going to check in the test until bug 426195 is fixed, since the workaround seems to be unreliable (and even causes trouble on Linux). See also bug 459921.

Comment 22

[Boris Zbarsky [:bzbarsky]]

So... This patch changed the signature of a virtual nsFrame method, but didn't update any of the classes that overrode it.  When compiling layout/xul, I get:

In file included from /Users/bzbarsky/mozilla/css-frameconst/mozilla/layout/xul/base/src/nsScrollbarFrame.cpp:46:
/Users/bzbarsky/mozilla/css-frameconst/mozilla/layout/xul/base/src/../../../generic/nsFrame.h:348: warning: ‘virtual nsresult nsFrame::HandleMultiplePress(nsPresContext*, nsGUIEvent*, nsEventStatus*, PRBool)’ was hidden
/Users/bzbarsky/mozilla/css-frameconst/mozilla/layout/xul/base/src/nsScrollbarFrame.h:77: warning:   by ‘virtual nsresult nsScrollbarFrame::HandleMultiplePress(nsPresContext*, nsGUIEvent*, nsEventStatus*)’
In file included from /Users/bzbarsky/mozilla/css-frameconst/mozilla/layout/xul/base/src/nsScrollbarFrame.cpp:47:
/Users/bzbarsky/mozilla/css-frameconst/mozilla/layout/xul/base/src/../../../generic/nsFrame.h:348: warning: ‘virtual nsresult nsFrame::HandleMultiplePress(nsPresContext*, nsGUIEvent*, nsEventStatus*, PRBool)’ was hidden
/Users/bzbarsky/mozilla/css-frameconst/mozilla/layout/xul/base/src/nsScrollbarButtonFrame.h:79: warning:   by ‘virtual nsresult nsScrollbarButtonFrame::HandleMultiplePress(nsPresContext*, nsGUIEvent*, nsEventStatus*)’

etc, etc.