Closed Bug 1092888 Opened 10 years ago Closed 9 years ago

[Text selection] Enable selection carets for non-editable fields on B2G and fix the existing test case failures

Categories

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

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla37
feature-b2g 2.2+

People

(Reporter: mtseng, Assigned: mtseng)

References

Details

(Whiteboard: [ft:system-platform])

Attachments

(5 files, 8 obsolete files)

4.56 KB, patch
Details | Diff | Splinter Review
2.11 KB, patch
Details | Diff | Splinter Review
1.19 KB, patch
Details | Diff | Splinter Review
6.65 KB, patch
mdas
: review+
Details | Diff | Splinter Review
4.37 KB, patch
Details | Diff | Splinter Review
Since non-editable fields selection is ready but behind the pref. We'll enable it by default in this bug.
Accidentally import wrong patches on previous try run.

new try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=789fc662e394
Priority: -- → P1
Remove non-editable preference because we don't need it anymore.
Attachment #8515739 - Attachment is obsolete: true
In previous try, we crash at SetDragState because null FrameSelection. So I add check before use.
Disable selection carets in those fail testcases.
Attachment #8516536 - Flags: review?(roc)
Attachment #8516537 - Flags: review?(roc)
Attachment #8516538 - Flags: review?(roc)
Depends on: 1092425
Although all patches are r+ and try looks good. We get too much noise from system app. So I'll check in this patch after bug 1092425 land.
patch failed to apply: 

renamed 1092888 -> Bug-1092888---Part-1-Remove-preference-for-selecti.patch
applying Bug-1092888---Part-1-Remove-preference-for-selecti.patch
patching file layout/base/SelectionCarets.cpp
Hunk #1 FAILED at 56
Hunk #2 FAILED at 95
2 out of 4 hunks FAILED -- saving rejects to file layout/base/SelectionCarets.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh Bug-1092888---Part-1-Remove-preference-for-selecti.patch

could you take a look, thanks!
Flags: needinfo?(mtseng)
Keywords: checkin-needed
Patches has been rebased. Sorry about it! Please check-in again, thanks.
Flags: needinfo?(mtseng)
Keywords: checkin-needed
The Marionette failures appear to be all on Windows builds, if that matters.
Non-editable selection is work well, so we need flip the assert funtion in existing test for non-editable test.
try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=933eef2e1609
Attachment #8520358 - Flags: review?(roc)
Comment on attachment 8520358 [details] [diff] [review]
Part 4: Flip testing function for non-editablt test at test_selectioncarets.py.

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

::: layout/base/tests/marionette/test_selectioncarets.py
@@ +215,5 @@
>      # <div> non-editable test cases with selection carets enabled
>      ########################################################################
>      def test_content_non_editable_minimum_select_one_character_by_selection(self):
>          self.openTestHtml(enabled=True)
>          # Currently, selection carets do not show on non-editable elements.

Please delete this comment since selection carets supports non-editable now.
Latest try run still have some fails. Those fails because context menu is shown after calling
long tap. Then context menu overlapp selection carets so that we cannot hit carets and tests
fail. So I add a parameter to indicate visibility of context menu when calling long press.
Attachment #8520431 - Flags: review?(mdas)
Comment on attachment 8520431 [details] [diff] [review]
Part 5: Add a parameter to toggle context menu when calling long_press.

There are something wrong in this patch. Clear review.
Attachment #8520431 - Flags: review?(mdas)
Comment on attachment 8520431 [details] [diff] [review]
Part 5: Add a parameter to toggle context menu when calling long_press.

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

::: layout/base/tests/marionette/test_selectioncarets.py
@@ +47,5 @@
>  
>          # Long press the caret position. Selection carets should appear, and the
>          # first word will be selected. On Windows, those spaces after the word
>          # will also be selected.
> +        self.actions.long_press(el, self._long_press_time, x, y,\

The arguments in the parentheses do not need a backslash at the end of the line.

::: testing/marionette/client/marionette/marionette.py
@@ +380,5 @@
>              self.action_chain.append(['wait', time_increment/1000])
>          self.action_chain.append(['release'])
>          return self
>  
> +    def long_press(self, element, time_in_seconds, x=None, y=None, with_contextmenu=False):

with_contextmenu should be default to True.

@@ +388,5 @@
>          :param element: The element to press.
>          :param time_in_seconds: Time in seconds to wait before releasing the press.
>          :param x: Optional, x-coordinate to tap, relative to the top-left
>           corner of the element.
>          :param y: Optional, y-coordinate to tap, relative to the top-left

Please add document for "with_contextmenu"

@@ +400,5 @@
>  
>          '''
>          element = element.id
>          self.action_chain.append(['press', element, x, y])
> +        if with_contextmenu == True:

if not with_contextmenu
Fix address comment.
Attachment #8520431 - Attachment is obsolete: true
Attachment #8520440 - Flags: feedback?(tlin)
Attachment #8520440 - Flags: feedback?(tlin) → feedback+
Attachment #8520440 - Flags: review?(mdas)
Comment on attachment 8520440 [details] [diff] [review]
Part 5: Add a parameter to toggle context menu when calling long_press v2.

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

r- just to move the call to gestures.py, looks good otherwise!

::: testing/marionette/client/marionette/marionette.py
@@ +404,5 @@
>          '''
>          element = element.id
>          self.action_chain.append(['press', element, x, y])
> +        if not with_contextmenu:
> +            self.action_chain.append(['moveByOffset', 0, 0])

I don't think this should be part of the marionette library. We want this library to be a direct interface to what the server side supports. Since the server doesn't support this, and the moveByOffset(0,0) is wholly dependent on the way gecko processes events, I don't want to add it here.

That being said, it can be added to gestures.py without problem: https://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/gestures.py
Attachment #8520440 - Flags: review?(mdas) → review-
Update address comment.
Attachment #8520440 - Attachment is obsolete: true
Attachment #8523605 - Flags: review?(mdas)
Comment on attachment 8523605 [details] [diff] [review]
Part 5: Add a parameter to toggle context menu when calling long_press v3.

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

::: testing/marionette/client/marionette/gestures.py
@@ +73,5 @@
> +#element: The element to press.
> +#time_in_seconds: Time in seconds to wait before releasing the press.
> +#x: Optional, x-coordinate to tap, relative to the top-left corner of the element.
> +#y: Optional, y-coordinate to tap, relative to the top-leftcorner of the element.
> +def long_press_without_contextmenu(marionette_session, element, time_in_seconds, x=None, y=None):

Nit: The triple quote docstring is the preferred way since it can be picked up by Python automatically.
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #28)
> Comment on attachment 8523605 [details] [diff] [review]
> Part 5: Add a parameter to toggle context menu when calling long_press v3.
> 
> Review of attachment 8523605 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/marionette/client/marionette/gestures.py
> @@ +73,5 @@
> > +#element: The element to press.
> > +#time_in_seconds: Time in seconds to wait before releasing the press.
> > +#x: Optional, x-coordinate to tap, relative to the top-left corner of the element.
> > +#y: Optional, y-coordinate to tap, relative to the top-leftcorner of the element.
> > +def long_press_without_contextmenu(marionette_session, element, time_in_seconds, x=None, y=None):
> 
> Nit: The triple quote docstring is the preferred way since it can be picked
> up by Python automatically.

Hmm, I just followed code style in gestures.py.
Comment on attachment 8523605 [details] [diff] [review]
Part 5: Add a parameter to toggle context menu when calling long_press v3.

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

Thanks for the fix!

And yes, feel free to change the quotes style in gestures.py if you prefer them to be triple-quoted.
Attachment #8523605 - Flags: review?(mdas) → review+
Backout in below commit.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0d50a3f1e96
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Should be ok for checking-in again.
Keywords: checkin-needed
Something in the push from comment 36 made Gij(2) permafail (see the log link below). In the interest of getting the trees reopened sooner, I've backed out the entire push for now. I've also kicked off Try runs of each bug individually backed out and will re-land the innocent patches. Sorry for the hassle.
https://hg.mozilla.org/integration/mozilla-inbound/rev/481af84aba2a

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4455263&repo=mozilla-inbound
Target Milestone: mozilla36 → ---
The Try push confirms that this was the cause of the permafail.
feature-b2g: --- → 2.2+
Whiteboard: [ft:system-platform]
This Gij permafail is caused by app crash at DispatchSelectionStateChangeEvent. Bug 1067728 fix some logic here. And it has been landed. After rebased my patches and re-try, I don't encounter this permafail again.
Here is my latest try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=11ebae746ac3

Ready for re-land again.
Keywords: checkin-needed
QA Whiteboard: [textselection]
QA Whiteboard: [textselection] → [COM=Text Selection]
You need to log in before you can comment on or make changes to this bug.