787427 - Long press on links makes page scroll to top

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Description

[Chris AtLee [:catlee]]

STR:

1) Go to http://www.marksdailyapple.com/
2) Enter in "apple" in the search box at the top right
3) Scroll down the search results
4) Long press on one of the links
5) Page jumps to top of page

Expected: popup menu appears giving options such as "open in new tab"

Comment 1

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

The link is in an iframe and for some reason we're not picking it up on long press properly and showing the context menu. Then, because we don't have a context menu, we end up starting a selection which presumably throws us back to the top of the page.

Comment 2

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

The iframe is contained inside a div that is slightly smaller than it. This means that our getClosest() code in browser.js (and more specifically _computeDistanceFromRect) decides that the container div is the best element at that touch point, rather than the iframe inside the div.

wesj, any thoughts on tuning _computeDistanceFromRect, maybe so that it returns zero if the touch point is inside the rect? That way getClosest() will return the innermost element that contains the touch point, if there is one, and if there isn't it'll fall back to the current behaviour.

Comment 3

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

Attachment: Patch

i.e. this. It fixes the problem on this page, not sure about other side-effects it'll have. Also added braces and a missing "else"

Comment 4

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

Attachment: Patch

K fine, I'll take out the braces. But I'm still going to try and insert them in the future at every chance I get!

Comment 5

[Wesley Johnston (:wesj)]

Comment on attachment 658065 [details] [diff] [review]
Patch

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

Sorry for the delay. I'm not sure why we think this will return the innermost element? Does nodesFromRect have some guarantee about the order of results it returns? Otherwise this will just result in us the first item in the list where the touch is inside the node (i.e. distance == 0).

We could rewrite these though:
y = Math.min( Math.abs(aRect.top - aY), Math.abs(ymost - aY) );

Comment 6

[Wesley Johnston (:wesj)]

I am not quite sure how we manage to succeed to highlight the node and fail to show a context menu for it too. Bug 708048 has a different fix that should fix this though. Maybe I'll send that review to you kats, since mfinkle has ignored it for awhile....

Comment 7

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

(In reply to Wesley Johnston (:wesj) from comment #5)
> Sorry for the delay. I'm not sure why we think this will return the
> innermost element? Does nodesFromRect have some guarantee about the order of
> results it returns? Otherwise this will just result in us the first item in
> the list where the touch is inside the node (i.e. distance == 0).

Good point. I did assume nodesFromRect returned innermost elements earlier in the list, and the way the implementation code works I think that will always be true, but it's probably a bad idea to rely on that.

> We could rewrite these though:
> y = Math.min( Math.abs(aRect.top - aY), Math.abs(ymost - aY) );

I don't see how this would help; we already know that aY is between aRect.top and ymost, so this line is equivalent to the original. What if I updated my patch to ensure that it picks the innermost element of distance zero?

Comment 8

[Scott Johnson (:jwir3)]

Attachment: b787427 (WIP)

Wes:

What about something like the following? Basically, what I'm doing is if there's an element in the node list that's a parent of another element in the node list, then I skip that one (as we probably want the child instead).

Comment 9

[Wesley Johnston (:wesj)]

Comment on attachment 673603 [details] [diff] [review]
b787427 (WIP)

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

Interesting idea. This code is going to go away as soon as I can figure out some test failures, but its just going to use the C++ implementation:

http://mxr.mozilla.org/mozilla-central/source/layout/base/PositionedEventTargeting.cpp

I think this is probably ok. In cases where the parent is important, say you are tapping near a node that's inside an anchor, I think we'll throw away the node anyway since it likely will not pass isElementClickable (I'm curious what we do now if you put an element inside an anchor and then do a transform to move it outside....) i.e. we should be looking for the innermost element that's clickable.

Comment 10

[Wesley Johnston (:wesj)]

Comment on attachment 673603 [details] [diff] [review]
b787427 (WIP)

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

Interesting idea. This code is going to go away as soon as I can figure out some test failures, but its just going to use the C++ implementation:

http://mxr.mozilla.org/mozilla-central/source/layout/base/PositionedEventTargeting.cpp

I think this is probably ok. In cases where the parent is important, say you are tapping near a node that's inside an anchor, I think we'll throw away the node anyway since it likely will not pass isElementClickable (I'm curious what we do now if you put an element inside an anchor and then do a transform to move it outside....) i.e. we should be looking for the innermost element that's clickable.

Comment 11

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.