1233272 - Long press on text opens context menu from adjacent links

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

* Go to http://www.theguardian.com/commentisfree/2015/dec/16/new-form-climate-denialism-dont-celebrate-yet-cop-21
* Long press a word to select it, in a line 1 or 2 lines below a link (e.g. on my device, I can long press "have denied" below "historic climate pact in Paris" in the first paragraph)

Expected: Text is selected
Actual: Context menu appears

I'm pretty sure I saw this bug go by in the past few days but I'm refiling just in case.

A similar behavior occurs for presses, but the taps are insignificant. I think this is a regression from bug 1222234 – see the discussion from bug 1222234 comment 10.

Comment 1

[Dominique Vincent [:domivinc]]

Michael, I tested with the previous preference values:

ui.touch.radius.leftmm, 3
ui.touch.radius.topmm, 5
ui.touch.radius.rightmm, 3
ui.touch.radius.bottommm, 2

The behavior you reported (I'm not convince it's a bug) is less visible but it is still here.
The touch area is bigger (with bug 1222234). You can see more easily the difference between a long press in a touch area without any links and a long press in a touch area with one link. 

With the previous values (without the code changes from bug 1222234), you can try to make a long press to select the words 'signing of' or 'we might now' near the link and you will see the same behavior.

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

As per comment 1, I backed out bug 1222234 and still see this. In particular, I can select text to the left and right of links, but selecting text a line above or below a link triggers the context menu.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

This sounds similar to what I'm seeing on reddit since I got updated to 45 on Aurora. Link targeting just seems "off" in a similar manner to what's described here.

Annoyingly, I tried tracking this down with mozregression and was unable to reproduce, so presumably it's somehow tied to my profile :(

Comment 4

[Dominique Vincent [:domivinc]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #3)
> This sounds similar to what I'm seeing on reddit since I got updated to 45
> on Aurora. Link targeting just seems "off" in a similar manner to what's
> described here.
> 
> Annoyingly, I tried tracking this down with mozregression and was unable to
> reproduce, so presumably it's somehow tied to my profile :(

I have the same behavior in versions 42 and 44 : selecting text a line above or below a link triggers the context menu. 

Regarding the link targeting issue in reddit, the issue could come from another code change (see bug 1190541 comment 23).

Comment 6

[Matt Brubeck (:mbrubeck)]

Hmm, I may have duplicated this too soon.  The regression described in comment 3 is definitely bug 1233056 which affects Aurora 45 but not Nightly.  But the other comments may describe a different bug.

Comment 7

[:Margaret Leibovic]

Maybe the patch for bug 1233056 will fix this, we should check once that lands.

Comment 8

[Mark Capella [:capella]]

I was watching all the stuff that landed last night ... the failures I'd observed here seem to work now.

Comment 9

[Randall Barker [:rbarker]]

(In reply to :Margaret Leibovic from comment #7)
> Maybe the patch for bug 1233056 will fix this, we should check once that
> lands.

The patch in bug 1233056 will only fix Aurora 45. Any issues seen in nightly are probably a different issues since 45 is using JPZ(Java Pan Zoom) and 46/nightly is C++APZ.

Comment 10

[Randall Barker [:rbarker]]

Using the link in comment #0 in nightly, with no zoom, I am unable to select text above or below links with a long press as the hit test seems to always find the link. When I zoom in, it is possible to select the text. However I'm see the same behavior in Aurora so this does not appear to be an C++APZ specific bug.

Comment 11

[Randall Barker [:rbarker]]

I think this is a dupe of Bug 1182804.

Comment 12

[Vincent Lefevre]

See also bug 1244498 (which was initially bug 826601, but I was asked to open a new bug).

Comment 13

[Randall Barker [:rbarker]]

I believe Bug 1247095 will address this issue.

Comment 14

[Randall Barker [:rbarker]]

This issue should be fixed in nightly with Bug 1247095.

Comment 16

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

Reopening this, as it has come back with the re-enabling of mouse event retargeting in bug 1618532. I intend to disable retargeting on long-presses to avoid this problem.

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Comment 18

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

I was looking at this and one of the gotchas is that the mousemove event that gets generated just before the contextmenu/mouselongtap events here triggers the nearest link to be highlighted as :hover, even if the contextmenu/mouselongtap are not retargeted. So we need to suppress retargeting that mousemove event as well. This is probably good from a consistency standpoint anyway, because scripts that are listening for move events and contextmenu events might expect the last move to have the same coordinates as the contextmenu event. So if we don't retarget any of those we can preserve that invariant.

Comment 20

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

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=a8527e24f9eff4dc5fcc24c69e459ca42fbd6ac6

Comment 21

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

Attachment: Bug 1233272 - Modernize log statement. r?snorp

Comment 22

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

Attachment: Bug 1233272 - Suppress event retargeting for mouse events generated via longpress. r?snorp

Instead of just special-casing the eContextMenu and eMouseLongTap events in
PositionedEventTargeting, this adds a mechanism to suppress retargeting for
any events dispatched from a particular code block. This allows us to also
suppress retargeting the mousemove that precedes the eContextMenu, while still
retargeting any other mousemove events (e.g. the one that precedes a click).
Doing this is important because if the mousemove is retargeted, it can trigger
the hover pseudoclass CSS on an element when that element is not actually
going to be activated at all, and can be confusing to the user.

Depends on D65211

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d59cc3f521d3
Modernize log statement. r=snorp
https://hg.mozilla.org/integration/autoland/rev/012a83782d3d
Suppress event retargeting for mouse events generated via longpress. r=snorp

Comment 24

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/d59cc3f521d3
https://hg.mozilla.org/mozilla-central/rev/012a83782d3d