1300905 - [e10s] when selected text is pressed using finger for copying purpose, the selection disappears and context menu is not displayed

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Abe - QA (:Abe_LV)]

Attachment: textselection.mp4

[Tested Platform] 
Asus laptop (touch enabled)

Nightly 51.0a1
Build ID  20160906030431
User Agent  Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0

[Pre-Requisites] 
about:config settings:
javascript.options.asyncstack; false
browser.tabs.remote.force-enable; true
layers.async-pan-zoom.enabled; true 

about:support info
Multiprocess Windows 1/1 (Enabled by user)
about:support Asynchronous Pan/Zoom wheel input enabled; touch input enabled

Steps to Reproduce

1.Go to: https://en.wikipedia.org/wiki/Main_Page
2.Using your finger press to select text on the screen
3.Using your fingers drag the selection caret to capture more text
4.Using your finger press the selected text to display the context menu & apply the Copy feature

Actual Result

When the selected text is pressed to display the context menu, the selection disappears and copy feature is not accessible.

Expected Result

User should be able to get context menu and copy selected texts.

Note:

It is e10s specific. Works fine without e10s.

Screen capture is also available from here: https://testing-1.tinytake.com/sf/OTU1Nzg1XzQwMDczMzQ

Comment 1

[Kartikaya Gupta (email:[e-mail])]

According to Philipp in bug 1300723, we should show the context menu when the user does a long-press on the selected text.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: Don't SelectWordOrShortcut if long-pressing on a selection

What do you think of this approach? If you're good with this I'll flag smaug for review on the Selection.* changes.

Comment 3

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

Comment on attachment 8795848 [details] [diff] [review]
Don't SelectWordOrShortcut if long-pressing on a selection

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

The idea is good to me.  I remember I did something similar before, but only to bring back the carets when they're being blurred or something at [1].  The downside of this is that user cannot select a word within an existing selection, but I do not feel this is a common usage pattern.  The solution needs some refine.

[1] http://searchfox.org/mozilla-central/rev/5bdd2876aeb9dc97dc619ab3e067e86083dad023/layout/base/AccessibleCaretManager.cpp#616-624

::: layout/base/AccessibleCaretEventHub.cpp
@@ +326,5 @@
>    {
> +    // If the long-tap is landing on a pre-existing selection, don't replace
> +    // it with a new one. Instead just return and let the context menu pop up
> +    // on the pre-existing selection.
> +    if (aContext->mManager->SelectionContainsPoint(aPoint)) {

I'd like to keep AccessibleCaretEventHub simple without involving which action to do with long press, i.e. to select or not to select.  Let's defer the job to AccessibleCaretManager::SelectWordOrShortcut.

Could you try to change the if-statement at [1] to something like

 if (GetCaretMode() == CaretMode::Selection &&
     SelectionContainsPoint(aPoint))

so that we always do not initialize a new selection when user pressing on an existing one? (Note: aPoint is relative to root frame.)  If it works, please update the comments below accordingly, and we still want to update carets in this case. Thanks.

::: layout/base/AccessibleCaretManager.cpp
@@ +531,5 @@
> +bool
> +AccessibleCaretManager::SelectionContainsPoint(const nsPoint& aPoint)
> +{
> +  Selection* selection = GetSelection();
> +  return selection ? selection->ContainsPoint(aPoint) : false;

We have nsLayoutUtils::GetSelectionBoundingRect() to get the rect of selection. You might want to use it instead of PointInRectChecker(). And this lifts the review burden for smaug if it works here ;)

Comment 4

[Kartikaya Gupta (email:[e-mail])]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #3)
> > +  Selection* selection = GetSelection();
> > +  return selection ? selection->ContainsPoint(aPoint) : false;
> 
> We have nsLayoutUtils::GetSelectionBoundingRect() to get the rect of
> selection. You might want to use it instead of PointInRectChecker(). And
> this lifts the review burden for smaug if it works here ;)

That's not quite the same, since it gives the bounding box of the selection. The selection as you know can have an irregular shape and and I would like to ensure that long-tapping inside the bounding box but outside the actual selection behaves the same as tapping far away.

I'll make the other changes you suggested and repost the patch for review. Thanks!

Comment 5

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

Re comment 4:

> That's not quite the same, since it gives the bounding box of the selection.
> The selection as you know can have an irregular shape and and I would like
> to ensure that long-tapping inside the bounding box but outside the actual
> selection behaves the same as tapping far away.

Ouch. You're right! The selection shape is irregular.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #3)
> Note: aPoint is relative to root frame.

As far as I can tell aPoint is in the right coordinate space for using the way I am doing in the patch. I tested it with selections in the top-level document as well as iframes, both scrolled and not scrolled, and they all worked as expected.

I moved the code into AccessibleCaretManager as you requested, that made it simpler. Patch coming up.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Attachment: When long-pressing on a selection just show the context menu instead of re-selecting a word

r?mats for the Selection.h/nsSelection.cpp changes since smaug is overloaded. mats, let me know if you'd prefer smaug reviewing this; I just figured the new function was pretty straightforward so it shouldn't be too hard to review.

Comment 8

[Mats Palmgren (inactive)]

Comment on attachment 8796295 [details] [diff] [review]
When long-pressing on a selection just show the context menu instead of re-selecting a word

>+  bool ContainsPoint(const nsPoint& aClientPoint);

Please make it a "const" method if possible.
Please add a doc comment saying what it does, including
a @param saying that aClientPoint is a point relative to
the root frame.  I don't understand what "client" is meant
convey, perhaps just name it aPoint?

>+  PointInRectChecker(const nsPoint& aPoint)

Do we need "explicit" here?

r=mats with those addressed as you see fit

Comment 9

[Kartikaya Gupta (email:[e-mail])]

(In reply to Mats Palmgren (:mats) from comment #8)
> >+  bool ContainsPoint(const nsPoint& aClientPoint);
> 
> Please make it a "const" method if possible.

I had to make IsCollapsed() and GetRangeAt() const as well but that allowed me to make the new method const.

> Please add a doc comment saying what it does, including
> a @param saying that aClientPoint is a point relative to
> the root frame.  I don't understand what "client" is meant
> convey, perhaps just name it aPoint?

Done.

> >+  PointInRectChecker(const nsPoint& aPoint)
> 
> Do we need "explicit" here?

good catch, done.

I also kicked off a try push on the updated patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=20fee4afd4e0

Comment 10

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

Comment on attachment 8796295 [details] [diff] [review]
When long-pressing on a selection just show the context menu instead of re-selecting a word

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

::: layout/base/AccessibleCaretManager.cpp
@@ +534,5 @@
> +  // If the long-tap is landing on a pre-existing selection, don't replace
> +  // it with a new one. Instead just return and let the context menu pop up
> +  // on the pre-existing selection.
> +  if (GetCaretMode() == CaretMode::Selection &&
> +      GetSelection()->ContainsPoint(aPoint)) {

Generally, this change looks good to me except it'll break a minor scenario [1] had fixed.

The issue [1] tried to fix could be reproduced on Fennec as follows:
1. Open https://en.m.wikipedia.org/wiki/Main_Page
2. Make an arbitrary selection.
3. Click an arbitrary image. (The image will enlarge to the entire screen, but the selection will remain underneath. Carets will hide)
4. Close the image.
5. Long press on the existing selection (We won't make a new selection, but we'll need to restore the caret as well as show the copy/paste menu)

So I would suggest move the two following lines in [1] here, and delete the entire if-block of [1].
    AC_LOG("%s: UpdateCarets() for current selection", __FUNCTION__);
    UpdateCaretsWithHapticFeedback();

r=me with that change. If it causes problem on Windows, please let me know. (I've tested it on Fennec, and my Windows machine with touch support is still on its way ... )

[1] http://searchfox.org/mozilla-central/rev/5bdd2876aeb9dc97dc619ab3e067e86083dad023/layout/base/AccessibleCaretManager.cpp#616-624

Comment 11

[Kartikaya Gupta (email:[e-mail])]

That makes sense. I updated the patch with the change you requested and tested it again on Windows. Seems good so I'll land it.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/abd0f2b6a2d4
When long-pressing on a selection, don't dismiss the selection and start a new one. Show the context menu instead. r=TYLin,mats

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/abd0f2b6a2d4