1652288 - Can't click on link at bottom of page

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Ethan Glasser-Camp]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:80.0) Gecko/20100101 Firefox/80.0

Steps to reproduce:

I'm working on building a web app at https://quartermaster.fyi/. On https://quartermaster.fyi/sightings, there is a button-styled link at the bottom of the page which is labeled "Add sighting". I tried to click this button in Firefox Nightly for Android.

It seems I have to fill out the Version field but I'm not really sure what version it is. Going to About Firefox Nightly reports:

Nightly 200711 06:00 (Build #21930608)
AC: 50.0.20200710130140, b54541be1
GV: 80.0a1-20200709093347
AS: 61.0.7

Saturday 7/11 @ 6:08 AM

Actual results:

Clicking this button on Firefox Nightly for Android does nothing, although it works fine for Firefox Desktop, even when in Responsive Design Mode.

Expected results:

It should take you to https://quartermaster.fyi/sightings/new.

I've been trying to debug this, thinking it was an issue in my application, and it seems that if I add another element to the bottom of the page with height of about 50px, I can interact with the element, but only if I touch the top part of it. For this reason, I wonder if it is being "covered" somehow by the address bar, even when the address bar is not visible.

Comment 1

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Moved to https://github.com/mozilla-mobile/fenix/issues/12557

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Reopening since I could reproduce the issue locally on Android Emulator. On the initial load of the page (https://quartermaster.fyi/sightings) I don't see any litst there, but once after I clicked at "Add sighting" and went back to the original page, there are a bunch of lists, then scrolled to the bottom of the page, the issue happens. And when I click the "Add sighting", I see "Got an unexpected touchend" message in adb log.

CCing kats and Botond.

Comment 3

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

(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)

Reopening since I could reproduce the issue locally on Android Emulator.

In Fenix? Or GVE? Presumably in Fenix, since GVE doesn't have the dynamic toolbar which seems like a prerequisite for this bug. I guess there's some confusion about bug workflow since I was under the impression that if the bug is not reproducible in GVE we should let the Fenix/A-C folks take the first crack at it because it could very well be a bug in one of those components rather than in Gecko. Particularly because of what kbrosnan said in this comment.

On the initial load of the page (https://quartermaster.fyi/sightings) I don't see any litst there, but once after I clicked at "Add sighting" and went back to the original page, there are a bunch of lists, then scrolled to the bottom of the page, the issue happens. And when I click the "Add sighting", I see "Got an unexpected touchend" message in adb log.

The warning message is kinda self-explanatory here; to get more info you'll want to run with MOZ_LOG="apz.inputqueue:5" or add logging to where the android nsWidget is sending touch events to APZ. Either way my suspicion is that if we're getting that warning, that APZ is not getting the touchstart at all and it's getting eaten somewhere earlier.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Sorry for the confusion. Indeed it's in Fenix. The reason why I re-open this bug is that it looked like an issue in APZ but may be not.

Anyways, with apz.inputqueue and apz.eventstate logs.

A failure case;

started new touch block 0x7930cd63a240 id 12 for target 0x7930ccee9000
not waiting for content response on block 0x7930cd63a240
processing input from block 0x7930cd63a240; preventDefault 0 shouldDropEvents 0 target 0x7930ccee9000
action: ProcessUntransformedAPZEvent 966
Handling event type 222
Event not prevented; pending response for 12 { l=0x100000002, p=1, v=2 }
got allowed touch behaviours; block=12
got a target apzc; block=12 guid={ l=0x100000002, p=1, v=2 }

A success case;

started new touch block 0x7930ccf0f190 id 32 for target 0x7930ccff8000
not waiting for content response on block 0x7930ccf0f190
processing input from block 0x7930ccf0f190; preventDefault 0 shouldDropEvents 0 target 0x7930ccff8000
action: ProcessUntransformedAPZEvent 966
got allowed touch behaviours; block=32
got a target apzc; block=32 guid={ l=0x100000002, p=1, v=2 }
Handling event type 222
Event not prevented; pending response for 32 { l=0x100000002, p=1, v=2 }

So a big difference is the order. In the failure case, it looks like we received "got allowed touch behaviours" after handling the event. Maybe it's a race condition that delivering the "got allowed" gets delay for some reasons?

Comment 5

[Botond Ballo [:botond]]

In this log, "got allowed touch behaviours" is printed by the parent process in response to a notification sent by the content process here, and "Handling event type" is printed by the content process in code called from here. So, I don't think their relative order is significant; it's likely just an artifact of the parent process getting scheduled to run in between those two points in the content process code.

Comment 6

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)

[...] we should let the Fenix/A-C folks take the first crack at it because it could very well be a bug in one of those components rather than in Gecko. Particularly because of what kbrosnan said in this comment.

Quoting that comment for context:

For some reason tapping into the site's search bar and dismissing the keyboard allows the navigation to work.

I wonder if this has to do with tapping into the search bar performing a zoom-to-focused-input that changes the site's zoom level (and as a result various metrics) slightly.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Thank you Botond for the clarification. I need to get more log..

(In reply to Botond Ballo [:botond] from comment #6)

Quoting that comment for context:

For some reason tapping into the site's search bar and dismissing the keyboard allows the navigation to work.

I wonder if this has to do with tapping into the search bar performing a zoom-to-focused-input that changes the site's zoom level (and as a result various metrics) slightly.

Yeah, possible. In my environment, the above setup (tapping the search bar, etc.) doesn't fix the problem. After some pinch zoom in/out, scrolling, etc. etc. the touch suddenly starts working.

Comment 8

[Botond Ballo [:botond]]

I think this is an issue with main-thread hit testing.

Comment 9

[Botond Ballo [:botond]]

In particular it seems similar to bug 1643212, except in that case the issue occurred even when the visual and layout viewports coincide. Here, the issue seems to only occur when the visual viewport has been scrolled away from the layout viewport origin.

Comment 10

[Botond Ballo [:botond]]

Display list building for the hit test fails to enter the scroll frame because this intersection in DescendIntoChild() fails; the scroll frame's visual overflow rect is not expanded to include the toolbar height.

Comment 11

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #10)

Display list building for the hit test fails to enter the scroll frame because this intersection in DescendIntoChild() fails; the scroll frame's visual overflow rect is not expanded to include the toolbar height.

It appears the only reason the page in bug 1643212 does not fail in the same way, is that on that page, DescendIntoChild() early-exits here.

Comment 12

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #8)

I think this is an issue with main-thread hit testing.

(Note: this does not appear to be a regression from bug 1556556, but rather a pre-existing dynamic toolbar issue.)

Comment 13

[Botond Ballo [:botond]]

A few things I discovered:

• The reason we early-exit DescendIntoChild() in the bug 1643212 case is that the scroll frame has a out-of-flow descendant which causes all of its ancestors to be marked for display.
• The out-of-flow descendant is a backdrop frame. It does not appear to be connected to any particular page element, it just seems to always be there on Android, including on the page in this bug.
• However, for the page in this bug, the call to MarkOutOfFlowFrameForDisplay() for the backdrop frame early-exits here.
• The reason for that early-exit is the intersection here. The backdrop frame's visual overflow rect seems to include the toolbar height (and thus intersect the hit-test point) on the page in bug 1643212, but not the page in this bug. I don't know why yet.

Comment 14

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #13)

• The reason for that early-exit is the intersection here. The backdrop frame's visual overflow rect seems to include the toolbar height (and thus intersect the hit-test point) on the page in bug 1643212, but not the page in this bug. I don't know why yet.

Investigating why the backdrop frame's visual overflow rect is sized the way it is may not be a useful direction. I don't think we want to be relying on the presence of the backdrop frame to force us to descend into the scroll frame via the mark-for-display mechanism; for all we know, a future optimization may result in there not being a backdrop frame at all.

Rather, I think the problem is the scroll frame's visual overflow rect: it should include the toolbar height. (Then we would descend into the scroll frame regardless of whether we take the early-exit it DescendIntoChild or not.)

Comment 15

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #14)

Rather, I think the problem is the scroll frame's visual overflow rect: it should include the toolbar height. (Then we would descend into the scroll frame regardless of whether we take the early-exit it DescendIntoChild or not.)

After thinking some more about this, this would be a fairly invasive and risky change, having various possible unrelated ramifications, such as altering the scroll frame's layout scroll range and various properties exposed to script.

Instead, I'm going to pursue the more targeted fix of expanding the visual overflow rect by the toolbar height in DescendIntoChild. This does fix the bug for me locally.

Comment 16

[Botond Ballo [:botond]]

Attachment: Bug 1652288 - Expand the RCD-RSF's ink overflow rect by the dynamic toolbar height in DescendIntoChild(). r=hiro

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/564081c2d0cd
Expand the RCD-RSF's ink overflow rect by the dynamic toolbar height in DescendIntoChild(). r=hiro

Comment 18

[Natalia Csoregi [:nataliaCs]]

Backed out for bustage on nsIFrame.cpp

backout: https://hg.mozilla.org/integration/autoland/rev/de0b9fd13f535d36aa55b4a87d6e0ab9f279371f

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=564081c2d0cdd4a047bf7ac02d786c89313133e2&group_state=expanded&selectedTaskRun=Ih2zhNBhTcy85eYNSZQQ3g.0

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=310989787&repo=autoland&lineNumber=24249

[task 2020-07-24T18:01:40.810Z] 18:01:40 INFO - In file included from Unified_cpp_layout_generic2.cpp:83:
[task 2020-07-24T18:01:40.810Z] 18:01:40 ERROR - /builds/worker/checkouts/gecko/layout/generic/nsIFrame.cpp:3853:29: error: no member named 'GetInkOverflowRect' in 'nsIFrame'; did you mean 'InkOverflowRect'?
[task 2020-07-24T18:01:40.810Z] 18:01:40 INFO - nsRect overflow = aChild->GetInkOverflowRect();
[task 2020-07-24T18:01:40.810Z] 18:01:40 INFO - ^~~~~~~~~~~~~~~~~~
[task 2020-07-24T18:01:40.810Z] 18:01:40 INFO - InkOverflowRect
[task 2020-07-24T18:01:40.810Z] 18:01:40 INFO - /builds/worker/checkouts/gecko/layout/generic/nsIFrame.h:3538:10: note: 'InkOverflowRect' declared here
[task 2020-07-24T18:01:40.811Z] 18:01:40 INFO - nsRect InkOverflowRect() const { return GetOverflowRect(eInkOverflow); }
[task 2020-07-24T18:01:40.811Z] 18:01:40 INFO - ^
[task 2020-07-24T18:01:40.811Z] 18:01:40 INFO - 1 error generated.
[task 2020-07-24T18:01:40.811Z] 18:01:40 ERROR - make[4]: *** [/builds/worker/checkouts/gecko/config/rules.mk:746: Unified_cpp_layout_generic2.o] Error 1
[task 2020-07-24T18:01:40.811Z] 18:01:40 INFO - make[4]: Leaving directory '/builds/worker/workspace/obj-build/layout/generic'
[task 2020-07-24T18:01:40.812Z] 18:01:40 ERROR - make[3]: *** [/builds/worker/checkouts/gecko/config/recurse.mk:72: layout/generic/target-objects] Error 2
[task 2020-07-24T18:01:40.812Z] 18:01:40 INFO - make[3]: *** Waiting for unfinished jobs....

Comment 19

[Botond Ballo [:botond]]

Oops, I introduced a compiler error during rebasing.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/236c6d4f049c
Expand the RCD-RSF's ink overflow rect by the dynamic toolbar height in DescendIntoChild(). r=hiro

Comment 21

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/236c6d4f049c