Closed Bug 787427 Opened 9 years ago Closed 1 year ago

Long press on links makes page scroll to top

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set
normal

Tracking

(firefox16 affected, firefox17 affected, firefox18 affected)

RESOLVED INCOMPLETE
Tracking Status
firefox16 --- affected
firefox17 --- affected
firefox18 --- affected

People

(Reporter: catlee, Assigned: jwir3)

References

Details

Attachments

(1 file, 2 obsolete files)

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"
Product: Fennec → Firefox for Android
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.
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.
Attached patch Patch (obsolete) — Splinter Review
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"
Attachment #657360 - Flags: review?(wjohnston)
Attached patch Patch (obsolete) — Splinter Review
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!
Attachment #657360 - Attachment is obsolete: true
Attachment #657360 - Flags: review?(wjohnston)
Attachment #658065 - Flags: review?(wjohnston)
Assignee: nobody → bugmail.mozilla
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) );
Attachment #658065 - Flags: review?(wjohnston) → review-
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....
(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?
Attached patch b787427 (WIP)Splinter Review
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).
Assignee: bugmail.mozilla → sjohnson
Attachment #658065 - Attachment is obsolete: true
Attachment #673603 - Flags: feedback?(wjohnston)
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 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.
Attachment #673603 - Flags: feedback?(wjohnston) → feedback+
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.
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → INCOMPLETE
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.