1074736 - [Text Selection] Selection carets should consider multiple ranges selection

Status: RESOLVED FIXED

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

Description

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

In Bug 739396, we split a range to multiple ranges if selection content contains some non-selectable element. In this case, selection carets should support multiple ranges as well.

Comment 1

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: wip:multiple-range

This is wip for supporting multiple range. The idea is using first range to show start caret and using last range to show end caret. roc, could you give some feedback about this patch? thanks.

Comment 2

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

Comment on attachment 8497389 [details] [diff] [review]
wip:multiple-range

Review of attachment 8497389 [details] [diff] [review]:
-----------------------------------------------------------------

Seems like a reasonable thing to do if we don't plan to have UX for multiple ranges.

Comment 3

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: bug1074736

Ready for review!

Comment 4

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: bug1074736 v2

Rebased.

Comment 5

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

Comment on attachment 8507679 [details] [diff] [review]
bug1074736 v2

Review of attachment 8507679 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/SelectionCarets.cpp
@@ +389,5 @@
>      return;
>    }
>  
> +  int32_t rangeCount = selection->GetRangeCount();
> +  if (rangeCount <= 0) {

A collapsed selection must have at least one range, so you can take this check out.

@@ +395,5 @@
>      return;
>    }
>  
> +  nsRefPtr<nsRange> rangeStart = selection->GetRangeAt(0);
> +  nsRefPtr<nsRange> rangeEnd = selection->GetRangeAt(rangeCount - 1);

Call these firstRange, lastRange. rangeStart sounds like the start of a range.

Comment 6

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

This needs tests.

Comment 7

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: bug1074736 v3 (carry r+: roc)

Fix addressed comment.

try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b918d73c5c22

I'll attach new testcase for this bug later.

Comment 8

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Marionette test part 1: selection_rect_list should consider multiple range as well

Comment 9

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Marionette test part 2: Marionette tests for selection carets with multiple range support

Comment 10

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8509245 [details] [diff] [review]
Marionette test part 1: selection_rect_list should consider multiple range as well

Review of attachment 8509245 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/marionette/client/marionette/selection.py
@@ +77,1 @@
>          '''Return the selection's DOMRectList object.

Please update the document. '''Returns the selection's DOMRectList object for the range at given idx.'''

@@ +77,5 @@
>          '''Return the selection's DOMRectList object.
>  
>          If the element is either <input> or <textarea>, return the selection's
>          DOMRectList within the element. Otherwise, return the DOMRectList of the
>          current selection.

Ditto.

Comment 11

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8509246 [details] [diff] [review]
Marionette test part 2: Marionette tests for selection carets with multiple range support

Review of attachment 8509246 [details] [diff] [review]:
-----------------------------------------------------------------

Selection Carets should support multiple range in non-editable fields. How about we just name the files test_selectioncarets_noneditable.py and test_selectioncarets_noneditable.html?

::: layout/base/tests/marionette/test_selectioncarets_multiplerange.py
@@ +37,5 @@
> +        self._nonsel1 = self.marionette.find_element(By.ID, 'nonsel1')
> +
> +    def _long_press_without_contextmenu(self, el, x, y):
> +        return self.actions.press(el, x, y).move_by_offset(0, 0).\
> +            wait(self._long_press_time).release()

Perhaps marionette should support a mechanism for this. Reviewer in ateam might have some ideas. :-)

Comment 12

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #11)
> Comment on attachment 8509246 [details] [diff] [review]
> Marionette test part 2: Marionette tests for selection carets with multiple
> range support
> 
> Review of attachment 8509246 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Selection Carets should support multiple range in non-editable fields. How
> about we just name the files test_selectioncarets_noneditable.py and
> test_selectioncarets_noneditable.html?
This test is only for multiple range. I'll add tests for non-editable fields in another bug.

> 
> ::: layout/base/tests/marionette/test_selectioncarets_multiplerange.py
> @@ +37,5 @@
> > +        self._nonsel1 = self.marionette.find_element(By.ID, 'nonsel1')
> > +
> > +    def _long_press_without_contextmenu(self, el, x, y):
> > +        return self.actions.press(el, x, y).move_by_offset(0, 0).\
> > +            wait(self._long_press_time).release()
> 
> Perhaps marionette should support a mechanism for this. Reviewer in ateam
> might have some ideas. :-)

Comment 13

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Marionette test part 1: selection_rect_list should consider multiple range as well v2 (carry f+: tlin)

Fix addressed comment.

Comment 14

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 8509246 [details] [diff] [review]
Marionette test part 2: Marionette tests for selection carets with multiple range support

Review of attachment 8509246 [details] [diff] [review]:
-----------------------------------------------------------------

r+ provided it passes in b2g. You should run this against try's emulators just to make sure. If it doesn't, then add b2g = false to the manifest entry

Comment 15

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 8509246 [details] [diff] [review]
Marionette test part 2: Marionette tests for selection carets with multiple range support

Review of attachment 8509246 [details] [diff] [review]:
-----------------------------------------------------------------

I missed this question

::: layout/base/tests/marionette/test_selectioncarets_multiplerange.py
@@ +37,5 @@
> +        self._nonsel1 = self.marionette.find_element(By.ID, 'nonsel1')
> +
> +    def _long_press_without_contextmenu(self, el, x, y):
> +        return self.actions.press(el, x, y).move_by_offset(0, 0).\
> +            wait(self._long_press_time).release()

Are you trying to press something without the context menu event being fired?

Comment 16

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: bug1074736 v4 (carry r+: roc)

rebased

Comment 17

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Marionette test part 2: Marionette tests for selection carets with multiple range support v2 (carry f+: tlin, r+: mdas)

Fix test fail on b2g.

try run:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a6dd9d3f12c6

Comment 18

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to Malini Das [:mdas] from comment #15)
> Comment on attachment 8509246 [details] [diff] [review]
> Marionette test part 2: Marionette tests for selection carets with multiple
> range support
> 
> Review of attachment 8509246 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I missed this question
> 
> ::: layout/base/tests/marionette/test_selectioncarets_multiplerange.py
> @@ +37,5 @@
> > +        self._nonsel1 = self.marionette.find_element(By.ID, 'nonsel1')
> > +
> > +    def _long_press_without_contextmenu(self, el, x, y):
> > +        return self.actions.press(el, x, y).move_by_offset(0, 0).\
> > +            wait(self._long_press_time).release()
> 
> Are you trying to press something without the context menu event being fired?

Yes, according to the code at marionette-listener.js, we'll send context menu event when isTap is true. So I made a workaround here let it move by offset with (0, 0) so isTap status would be reset after this move and then context menu won't be fired.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/141ea70b1caf
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1ab04e807c3
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b1f330c9298

Comment 20

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures on windows like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3282505&repo=mozilla-inbound

Comment 21

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Marionette test part 2: Marionette tests for selection carets with multiple range support v3 (carry f+: tlin, r+: mdas)

Fix windows failure. This failure is because "select word" action on window would select extra tail space so our testcase would failed. So I strip space before doing test.

Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7ac982300815

Comment 22

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Marionette test part 2: Marionette tests for selection carets with multiple range support v4 (carry f+: tlin, r+: mdas)

Considering line ending different on all platforms.

new try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=15bf4d03b0ee

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e0a13090cd8
https://hg.mozilla.org/integration/mozilla-inbound/rev/f43962d6b41e
https://hg.mozilla.org/integration/mozilla-inbound/rev/94c5f5317e4b

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6e0a13090cd8
https://hg.mozilla.org/mozilla-central/rev/f43962d6b41e
https://hg.mozilla.org/mozilla-central/rev/94c5f5317e4b

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing frequent carat-related Marionette failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e17c9722713b

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=540335&repo=mozilla-central
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=539150&repo=mozilla-central
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3326139&repo=mozilla-inbound
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=711831&repo=b2g-inbound
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=1006929&repo=fx-team
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=1007524&repo=fx-team

Comment 26

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Marionette test part 2: Marionette tests for selection carets with multiple range support v5 (carry f+: tlin, r+: mdas)

This patch should fix the problem.

try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=efeddcc1b9c8

Comment 27

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

try looks good.

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb1c7fef9214
https://hg.mozilla.org/integration/mozilla-inbound/rev/3863e1d10b69
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ce56791b0b7

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fb1c7fef9214
https://hg.mozilla.org/mozilla-central/rev/3863e1d10b69
https://hg.mozilla.org/mozilla-central/rev/7ce56791b0b7