Closed Bug 1652288 Opened 2 months ago Closed 2 months ago

Can't click on link at bottom of page

Categories

(Core :: Layout, defect)

80 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla80
Tracking Status
firefox80 --- fixed

People

(Reporter: ethan.glasser.camp, Assigned: botond)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.

Status: UNCONFIRMED → RESOLVED
Closed: 2 months ago
Resolution: --- → MOVED

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.

Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: MOVED → ---

(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.

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?

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.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.

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.

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

Assignee: nobody → botond
Component: General → Panning and Zooming
Product: Firefox for Android → Core
Version: Firefox 80 → 80 Branch

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.

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.

(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.

(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.)

Component: Panning and Zooming → Layout

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.

(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.)

Severity: -- → S2

(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.

Attachment #9165825 - Attachment description: Bug 1652288 - Expand the RCD-RSF's visual overflow rect by the dynamic toolbar height in DescendIntoChild(). r=hiro → Bug 1652288 - Expand the RCD-RSF's ink overflow rect by the dynamic toolbar height in DescendIntoChild(). r=hiro
Pushed by bballo@mozilla.com:
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

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....

Flags: needinfo?(botond)

Oops, I introduced a compiler error during rebasing.

Flags: needinfo?(botond)
Pushed by bballo@mozilla.com:
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
Status: REOPENED → RESOLVED
Closed: 2 months ago2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla80
Blocks: 1649865
You need to log in before you can comment on or make changes to this bug.