1650644 - Can't scroll list of items on the column of a project page on GitHub

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Rafał Opiłowski]

Attachment: Screencast of the issue (on Custom Tab) - cut as it includes another issue

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:80.0) Gecko/20100101 Firefox/80.0

Steps to reproduce:

• Open up e.g. "Hershey's 🍫" project (on the browser or as a Custom Tab)
• Scroll list of items on e.g. "Backlog" column

Actual results:

List of items doesn't scroll.

Expected results:

List of items scrolls properly.

Comment 1

[Emily Toop (:fluffyemily)]

Possibly a dupe of bug 1649865?

:botond is this a toolbar issue?

Comment 2

[Rafał Opiłowski]

Attachment: Possible screencast of the bug 1649865 (on Custom Tab) - github.com

Hi :fluffyemily, thank you for picking up this bug. :-)

You've suggested this issue might be related to bug 1649865.
I've revisited the URL mentioned in this bug to check this theory as you can see in the screencast I'm attaching.
In my opinion, it seems that's a good hint.

I'm leaving judgment to you, that's why I've set feedback flags for you and :botond to decide.

Comment 3

[Botond Ballo [:botond]]

It looks like a dynamic toolbar issue, yeah. It doesn't reproduce in GVE.

It seems like we are expecting the toolbar to disappear when you scroll down, and the bottom-fixed "Project menu" div to move down along with it, making the entire issues list visible, but that toolbar movement does not happen for some reason.

Comment 4

[Botond Ballo [:botond]]

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

It seems like we are expecting the toolbar to disappear when you scroll down, [...]

At least, the video from comment 2 is consistent with this explanation.

The video from comment 0 is stranger, because there the height of the page content is such that it wouldn't be scrollable with the toolbar hidden, but it should be with the toolbar shown. This might be an edge case that the dynamic toolbar mechanism doesn't handle well.

Comment 5

[Botond Ballo [:botond]]

(In reply to Emily Toop (:fluffyemily) from comment #1)

Possibly a dupe of bug 1649865?

It's possible, but I'm not sure yet. On BMO, there is an additional complicating factor (that the element that's scrolling at unit zoom is a <div>, not the entire page) which is not the case in the GitHub project view.

I think the next step here is to understand why, in a scenario like in comment 2 (i.e. on a page with enough content height that you can do at least some scrolling) the scrolling does not cause the dynamic toolbar to hide.

Comment 6

[Botond Ballo [:botond]]

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

I think the next step here is to understand why, in a scenario like in comment 2 (i.e. on a page with enough content height that you can do at least some scrolling) the scrolling does not cause the dynamic toolbar to hide.

It looks like the dynamic toolbar does not move in response to scrolling on a large variety of GitHub pages, including:

• the https://github.com home page
• an org page like https://github.com/mozilla-mobile
• a repo page like https://github.com/mozilla-mobile/android-components
• an issues list like https://github.com/mozilla-mobile/android-components/issues

I believe the relevant logic here is at higher levels (android-components / fenix). Perhaps Christian can shed some light on why this might be happening?

Comment 7

[Hiroyuki Ikezoe (:hiro)]

I tried the github page on a 2020-05-13 build, sometimes the dynamic toolbar moves (it often gets stuck in the middle of moving), once after the dynamic toolbar has been completely hidden, all contents looks in view correctly to me.

So, this is an annoying edge case of hit result handling?

Comment 8

[Botond Ballo [:botond]]

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

I tried the github page on a 2020-05-13 build, sometimes the dynamic toolbar moves (it often gets stuck in the middle of moving), once after the dynamic toolbar has been completely hidden, all contents looks in view correctly to me.

The behaviour has definitely changed since then. In a more recent build (I think I tested 2020-07-01, around the time this bug was filed), I can't get the dynamic toolbar to move at all on any of the mentioned GitHub pages.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

This is definitely related to bug 1652190. But my current WIP patch for bug 1652190 doesn't fix this github case properly (the position:fixed element at the bottom page moves along with scrolling for some reasons). I am going to investigate it tomorrow.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

I was totally wrong. I thought the behavior I saw was caused by the patch I wrote. But in fact, it's not related, it seems that github has changed something. The bottom toolbar now is able to move. But the position fixed element at the bottom of page jumps when the toolbar is completely hidden.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Rafał, could you please do double-check the behavior on the github page has been changed since then? (It's still broken though)

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Weird, I can no longer see the jumping position:fixed element on Android Emulator, I can still see it on my pixel 3.

Comment 13

[Rafał Opiłowski]

Attachment: Issue from comment #2 possibly resolved - screencast

Hi :hiro, I'm testing build #21960614 of Firefox Nightly on OnePlus 6 (Android 10).

In this build, with GitHub website overflowing no matter toolbar's present or not (comment #2), position:fixed element at this point scrolls down correctly with the toolbar. It applies to both browser and a Custom Tab.

I'll respond about case from comment #0 shortly, when I'll have a screencast.

Comment 14

[Rafał Opiłowski]

Attachment: Edge case from comment #0 not resolved - screencast

Hi :hiro, this time about edge case from comment #0.

For this case, I've checked another project - "Feature engagement" with columns "Eng Ready" and "In progress".
Both of them at this point have 4 elements.
In case of "In progress" columns, issues have enough labels, that they overflow only when toolbar is present.

Opinion:

As so, browser doesn't seem to understand it - thinks there's no overflow,
because it doesn't exclude the area of the toolbar while checking it.

At this point, I can't hide the toolbar by scrolling on the screen and still parts of the content are covered by the toolbar and position: fixed element doesn't move.

Adding feedback flag for your opinion.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Attachment: A test case to see the issue in comment 0

This is indeed an edge case.

This test case has a green 100vh height element. That means the bottom part of the element is covered by the bottom dynamic toolbar initially. But the content is not scrollable at all.

I think APZ needs to tell GeckoView (and GeckoView tells to android-components) that "the input wasn't handled in APZC but need to move the dynamic toolbar".

To do that, FrameMetrics needs to have a new parameter to represent this situation.

Comment 16

[Rafał Opiłowski]

:hiro, :botond I recently found bug 1633322 and relevant GitHub issue. Don't they cover the edge case from comment #15 and comment #0?

Comment 17

[Hiroyuki Ikezoe (:hiro)]

No, I don't think bug 1633322 fixes the case (though I haven't looked the patches carefully). Because in the case in comment 15 APZ doesn't know the size of the area which is NOT including the dynamic toolbar area, APZ just knows the size INCLUDING the dynamic toolbar area, so APZ can just tell the content is not scrollable, can't tell "it's not scrollable but there is area covered by the dynamic toolbar so please move the dynamic toolbar".

Comment 18

[Botond Ballo [:botond]]

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

This is indeed an edge case.

This test case has a green 100vh height element. That means the bottom part of the element is covered by the bottom dynamic toolbar initially. But the content is not scrollable at all.

I think APZ needs to tell GeckoView (and GeckoView tells to android-components) that "the input wasn't handled in APZC but need to move the dynamic toolbar".

To do that, FrameMetrics needs to have a new parameter to represent this situation.

What we have done in Fennec's dynamic toolbar implementation, is made the toolbar static for pages where the content height is in this range. So, the toolbar would never move, and accordingly APZ uses the smaller height (excluding the toolbar height) to determine whether the page should be vertically scrollable (and the answer is now "yes").

Comment 19

[Botond Ballo [:botond]]

(I agree bug 1633322 is unrelated here. It concerns cases where an element other than the <html> is the one which has scrollable content, whereas on these Github pages it's the <html>.)

Comment 20

[Botond Ballo [:botond]]

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

What we have done in Fennec's dynamic toolbar implementation, is made the toolbar static for pages where the content height is in this range. So, the toolbar would never move, and accordingly APZ uses the smaller height (excluding the toolbar height) to determine whether the page should be vertically scrollable (and the answer is now "yes").

It sounds like to implement this we would need:

• The patch from bug 1652190, with a tweak to handle the case where (screen height without toolbar) <= (content height) <= (screen height with toolbar).
• A change to android-components (perhaps in BrowserToolbarBottomBehavior and a corresponding place for when the toolbar is at the top) to not move the toolbar in such cases. (Or should this part be handled in Gecko by returning INPUT_RESULT_HANDLED_CONTENT in such cases?)

Comment 21

[Hiroyuki Ikezoe (:hiro)]

After chatting with Agi, I noticed that we don't either need to modify android-components or APZ at all if we return INPUT_RESULT_HANDLED from GeckoView.onTouchEventForResult in the case where

1. the PanzoomController.onTouchEvent() returns INPUT_RESULT_UNHANDLED AND
2. part of the current content is covered by the dynamic toolbar

:snorp, does this sound a reasonable approach in the near future? I am guessing it would also be useful for top toolbar issues, but if it's not sufficient we can add a new input result something like INPUT_RESULT_UNHANDLED_BUT_TOOLBAR_SHOULD_MOVE.

CCing Petru, if you have any suggestions/objections, please let me know.

I am going to attach the change I've locally done.

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1650644 - Factor out the function to tell whether part of the content is covered by the dynamic toolbar.

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1650644 - Tell a lie in GeckoView.onTouchEventForResult as if the touch event was handled in PanZoomController if part of the content is covered by the dynamic toolbar.

Depends on D84938

Comment 24

[Petru-Mugurel Lingurar [:petru]]

Regarding Fenix's behavior, the bottom toolbar has it's own scroll gesture detector.
So the toolbar would always be animated up/down for scroll gestures irrespective of what happens in GV.
To try and sync the toolbar and GV - only animate the toolbar if GV scrolls the page, for each scroll event Fenix uses the return from PZC talked about above to get if the scroll gesture should animate or not the toolbar. (Bottom toolbar is only animated for INPUT_RESULT_HANDLED).

So if I'm correctly understanding the proposed fix: have PZC inform about overscrolls for the height of the toolbar, seems like it would work great 👍

Comment 25

[Hiroyuki Ikezoe (:hiro)]

Thanks for the great feedback, Petru!

Comment 26

[Botond Ballo [:botond]]

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

(I agree bug 1633322 is unrelated here. It concerns cases where an element other than the <html> is the one which has scrollable content, whereas on these Github pages it's the <html>.)

This is only partially correct: bug 1633322 is also relevant in cases where the <html> element has scrollable content, but there is a non-passive touch event listener on the page. On such pages, we may currently never hide the toolbar. I expect those cases will not be fixed by the patches in this bug, but will be fixed by bug 1633322.

Comment 28

[Hiroyuki Ikezoe (:hiro)]

I am going to fix this in a different way in bug 1663000 (the way is not same as the way in bug 1663000 comment 0 though)

Comment 29

[Hiroyuki Ikezoe (:hiro)]

This was also fixed by bug 1663000.