392746 - Multiple selection can go wrong when selecting something before an existing selection

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Martijn Wargers (dead)]

This was mentioned from Hendrix feedback:
"
The function multiple select with ctrl/cmd is
amazing, but there is a problem, when i select
something and anotherthing below, everthing works,
but if i try to select something in top, minefield
select everthing in the middle.
"

Well, that says it all.
Just to be more clear, steps to reproduce:
- Select some text
- Then multiple select some text before the previous selection, by selecting, using the mouse while keeping the ctrl-key pressed.

Actual result:
- One selection instance where it has the boundaries of where you are with your mouse and the beginning of the previous selection. The previous selection is gone.

Expected result:
- Two selection instances, the previous selection and the one you just created.

This regressed between 2006-07-27 and 2006-07-28:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-07-27+05&maxdate=2006-07-28+09&cvsroot=%2Fcvsroot
I think a regression from bug 345099.

Comment 1

[Martijn Wargers (dead)]

Note that this isn't an issue on branch, because branch build don't allow multiple selection yet.

Comment 4

[Uri Bernstein]

Attachment: patch

nsTypedSelection::AddRange() was assuming that the new range was inserted in position count-1, an assumption that's no longer correct after the fix for bug 345099.

This also restores setting *aDidAddRange to PR_TRUE in nsTypedSelection::addTableCellRange, which was accidently (I believe) removed in bug 345099.

Comment 5

[Uri Bernstein]

I know this is now very late in the game, and this bug is technically not a regression from 1.8, but I think its existence makes the multiple-selection feature half-broken in a bad way, so we should consider either fixing this or backing out the feature.

Comment 6

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

Comment on attachment 311145 [details] [diff] [review]
patch

looks good

Comment 7

[Martijn Wargers (dead)]

Comment on attachment 311145 [details] [diff] [review]
patch

Uri, thanks for fixing this!

Comment 8

[Mike Schroepfer]

Given the age of this issue and lack of dupes we are not going to block on this.  But Uri/Martijn can we get any automated test coverage we'd strongly consider landing this for 1.9.
 

Comment 9

[Uri Bernstein]

I'm not sure how to do automated testing for this, and I have very little Mozilla time this week.

Martijn - can you help? Thanks.

Comment 10

[Martijn Wargers (dead)]

Attachment: testcase

I guess something like this can be converted into a mochitest case.

Comment 11

[Martijn Wargers (dead)]

Attachment: mochitest

Ok, this is a mochitest that fails without the patch attached to this bug.

Comment 12

[Uri Bernstein]

Thanks, Martijn!

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 311145 [details] [diff] [review]
patch

a=beltzner assuming the mochitest goes in with it

Comment 14

[Uri Bernstein]

Checked in with Martijn's mochitest:

Checking in mozilla/layout/generic/nsSelection.cpp;
/cvsroot/mozilla/layout/generic/nsSelection.cpp,v  <--  nsSelection.cpp
new revision: 3.307; previous revision: 3.306
done
Checking in mozilla/layout/generic/test/Makefile.in;
/cvsroot/mozilla/layout/generic/test/Makefile.in,v  <--  Makefile.in
new revision: 1.23; previous revision: 1.22
done
RCS file: /cvsroot/mozilla/layout/generic/test/test_bug392746.html,v
done
Checking in mozilla/layout/generic/test/test_bug392746.html;
/cvsroot/mozilla/layout/generic/test/test_bug392746.html,v  <--  test_bug392746.html
initial revision: 1.1
done

Comment 15

[Uri Bernstein]

Backed out, because even after some changes to the mochitest causing it not to fail (at least on Windows and mac), a different mochitest was also failing:

*** 49557 ERROR FAIL | key up while open | got "#CCFFFF", expected "#FF6666" | /tests/toolkit/content/tests/widgets/test_colorpicker_popup.xul
*** 49560 ERROR FAIL | key down while open | got "#66FFFF", expected "#FF0000" | /tests/toolkit/content/tests/widgets/test_colorpicker_popup.xul

Comment 16

[Uri Bernstein]

Attachment: mochitest v2

This passes (at least on Windows and Mac).

Comment 17

[Martijn Wargers (dead)]

Attachment: mochitest v3

Sorry, a.left/b.left/c.left/d.left also needs +1 here (I thought I didn't need that yesterday, oh well), otherwise I got test failures again.

I'm seeing the test failure (probably the same as mentioned in comment 15) in test_colorpicker_popup.xul when I the mochitest in from this bug.
But in this mochitest, I added:
+  wu.sendMouseEvent('mousedown', 0, 0, 0, 1, 0);
+  wu.sendMouseEvent('mousemove',  0, 0, 0, 0, 0);
+  wu.sendMouseEvent('mouseup',  0, 0, 0, 1, 0);
That seems to 'reset' stuff, and that seems to make test_colorpicker_popup.xul not fail anymore, afaict.
Obviously, adding this kind of thing is bogus, there seems to be something wrong with the sendMouseEvent internal code, or something.

Comment 18

[Martijn Wargers (dead)]

I've filed now bug 426195 for the mochitest failure issue.

Comment 19

[Uri Bernstein]

I re-checked-in this with test v3 only to discover that it still causes weird failures in subsequent tests on Linux (Mac and Windows are OK). I changed the test to bail out immediately on Linux, and hopefully this will do it for now (still waiting for the Linux test machine to cycle).

Comment 20

[Andrew Schultz]

FWIW,

  window.onload=function() {
        // XXX: This test causes subsequent tests to fail on Linux,
        // so disable on Linux for now.
        if (navigator.platform.indexOf("Linux") >= 0)
          return;
  
failed because it clobbered SimpleTest's own onload handler, which would have run the next test.

Comment 21

[Martijn Wargers (dead)]

So Mochikit is adding a window.onload handler? Why doesn't it use window.addEventLister('load', etc, true)? That seems more robust to me.

Anyway, I've now filed bug 426546 for the Linux tinderbox failing on the mochitest.

Comment 22

[Andrew Schultz]

Mochikit rolls it's own addEventListener (collective sigh).  It has an addLoadEvent method that wraps functions you pass in inside its own function and then chains together its functions (via window.onload=).

But then, I'm not sure that (even if it used addEventListener) the even would fire before an onload= defined in a page.  If the page also uses addEventListener, I'd expect the page's listener to get called later (which would be bad).  The most robust thing would seem to be calling SimpleTest.waitForExplicitFinish within the top-level script (not onload).