1790253 - [Settings Page] Scrolling over Search-Engines Box stucks when scroll bar is visible

Status: VERIFIED FIXED

(Toolkit :: UI Widgets, defect, P3)

Description

[Mehmet]

Attachment: Scrolling_Issue.mov

106.0a1 (2022-09-10) (64-Bit)
macOS 12.5.1

Step to reproduce:

Now, with QuickActions enabled in Nightly a further Search Engine for QuickActions appears in the Search Box of the Settings Page and the number of search engines expands from 11 to 12. With that, also scrollbar appears in the box, probably because with 12 Search engines the box needs to be scrollable.
Now, when you scroll through the search section in the Settings Page, scrolling stucks when you scroll with the MacBook Trackpad or Magic Mouse.

A screencast of the issue is attached.

Thanks for checking.

Comment 1

[Timothy Nikkel (:tnikkel)]

I'm not sure I'm understanding the issue. If your cursor is over a scrollable element then we expect that scrollable element to be scrolled?

Comment 2

[Botond Ballo [:botond]]

I think the complaint may be that the scroll gesture which is captured by the search engine list is not handed off to the enclosing page, you have to start a new scroll gesture to scroll the page.

However, this too is intentional, otherwise it could be frustrating to try and scroll the list while still keeping the list on the screen (since you'd get to the end of the list, and then the remainder of the scroll gesture would start scrolling the list off screen).

To my knowledge, this has always been the behaviour of mousewheel and touchpad scrolling on desktop.

Comment 3

[Alice0775 White]

I think Bug 312831 described the our behavior.

Comment 4

[Mehmet]

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

I think the complaint may be that the scroll gesture which is captured by the search engine list is not handed off to the enclosing page, you have to start a new scroll gesture to scroll the page.

However, this too is intentional, otherwise it could be frustrating to try and scroll the list while still keeping the list on the screen (since you'd get to the end of the list, and then the remainder of the scroll gesture would start scrolling the list off screen).

To my knowledge, this has always been the behaviour of mousewheel and touchpad scrolling on desktop.

Hi, yes that‘s correct. The problem in the scrollable box on the settings page is, that it doesn‘t scroll further when I do a further swipe with the trackpad when the top or the bottom of the box is reached with the scrollbar. It stutters/stucks with every further swipe until the cursor is out of the box. This isn‘t the way how it works in other scrollable boxes. Thanks for checking.

Comment 5

[Mehmet]

Attachment: demo.mov

Please compare with the behavior at https://www.quackit.com/html/codes/html_scroll_box.cfm

I attached a further screencast to demonstrate the issue. The scrollbox on the Settings-Page does not behave as expected compared to the scrollbox css example.

Thanks for checking.

Comment 6

[Botond Ballo [:botond]]

A few observations from playing around with this:

• Bug 1782974 is fairly incidental to this, it just adds one more default entry to that search engine line, and in your case that happens to make the list contents cross the threshold from not-scrollable to scrollable. Presumably, you would have run into the same issue had you added an extra search engine manually. So I'm going to remove bug 1782974 as the "regressed-by" bug.
• The list of search engines does not seem to be scrolling natively using APZ, rather it scrolls one entry at a time. My guess is it's a XUL <scrollbox> or something like that.
• Testing on Linux, I can't reproduce the stutter / getting stuck described in comment 4 and shown in the screencast in comment 5.
  • I do see something unexpected, which is that the scroll hands off immediately from the list to the page. This is not terribly surprising since the list is a <scrollbox> and the page is scrolling normally via APZ, and the interactions between the two are probably not ironed out very well.

That said, the behaviour you describe and show in the screencast definitely sounds like a bug and not expected behaviour. (Likely also caused by poor interaction between <scrollbox> and APZ.)

I wonder if the reason I can't reproduce it is that it's Mac specific? Perhaps someone using Mac can confirm.

Comment 7

[Botond Ballo [:botond]]

Note, trying to reproduce the issue requires having 12 total entries in the search engine list. In a fresh install there are 10 (including the newly added Actions), so two additional ones (for example, Ecosia and Startpage) need to be added manually to get 12.

Comment 8

[Mehmet]

@Botond: Thanks for your feedback. Yes, this is indeed probably mac-only due to the native „inertial scrolling“ feature.

„Flick down with two fingers on a trackpad or Magic Mouse in Mac OS X and you’ll experience inertial scrolling, where after your finger has stopped moving the page continues to scroll in the intended direction until it slowly stops. This fluid and natural scrolling experience comes from the iOS.“

I hope this helps :)

Comment 9

[Daisuke Akatsuka (:daisuke)]

Hello!
Just to confirm one thing, the problem is that scrolling in the search engine pane and scrolling on the page move at the same time by trackpad or Magic Mouse, right?
If so, it is reproducible on OSX(12.5.1), Windows 11 Pro, and Ubuntu22.04 as well in my env at least.

And I have investigated a bit, it seems that the preventDefault() of the following code does not work.
https://searchfox.org/mozilla-central/rev/b94f0c157227dadd3ecf119ce271035c52ed237b/toolkit/content/widgets/tree.js#751-760
If we change the target that we add the event listener to the tree directly like below, not shadow DOM, then the preventDefault() works, and it behaves expectedly.

this.addEventListener("MozMousePixelScroll", event => {...}

But, I’m not sure if we can change the target simply, or if the event handling with shadow DOM is not expected we should fix it.
Botond, do you have any ideas?
Thanks!

Comment 10

[Botond Ballo [:botond]]

(In reply to Daisuke Akatsuka (:daisuke) from comment #9)

Just to confirm one thing, the problem is that scrolling in the search engine pane and scrolling on the page move at the same time by trackpad or Magic Mouse, right?

Hmm, after looking at it more carefully, I think you're right -- what appeared to me as "immediate handoff" is actually "the two scroll frames scroll at the same time"! (It can be hard to tell the diference when the scroll range of the inner scroller is very small.)

If so, it is reproducible on OSX(12.5.1), Windows 11 Pro, and Ubuntu22.04 as well in my env at least.

And I have investigated a bit, it seems that the preventDefault() of the following code does not work.
https://searchfox.org/mozilla-central/rev/b94f0c157227dadd3ecf119ce271035c52ed237b/toolkit/content/widgets/tree.js#751-760
If we change the target that we add the event listener to the tree directly like below, not shadow DOM, then the preventDefault() works, and it behaves expectedly.

this.addEventListener("MozMousePixelScroll", event => {...}

But, I’m not sure if we can change the target simply, or if the event handling with shadow DOM is not expected we should fix it.
Botond, do you have any ideas?

Good find! I'm not sure whether this change is fine to make. It looks like the change to use the shadow root was made by Brian in bug 1575485, maybe he can weigh in on this.

Comment 11

[Botond Ballo [:botond]]

Anyways, given that the issue is related to the way the XUL widget does preventDefault(), I think we can move this to Toolkit :: XUL Widgets.

Comment 12

[Brian Grinstead [:bgrins]]

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

(In reply to Daisuke Akatsuka (:daisuke) from comment #9)

Just to confirm one thing, the problem is that scrolling in the search engine pane and scrolling on the page move at the same time by trackpad or Magic Mouse, right?

Hmm, after looking at it more carefully, I think you're right -- what appeared to me as "immediate handoff" is actually "the two scroll frames scroll at the same time"! (It can be hard to tell the diference when the scroll range of the inner scroller is very small.)

If so, it is reproducible on OSX(12.5.1), Windows 11 Pro, and Ubuntu22.04 as well in my env at least.

And I have investigated a bit, it seems that the preventDefault() of the following code does not work.
https://searchfox.org/mozilla-central/rev/b94f0c157227dadd3ecf119ce271035c52ed237b/toolkit/content/widgets/tree.js#751-760
If we change the target that we add the event listener to the tree directly like below, not shadow DOM, then the preventDefault() works, and it behaves expectedly.

this.addEventListener("MozMousePixelScroll", event => {...}

But, I’m not sure if we can change the target simply, or if the event handling with shadow DOM is not expected we should fix it.
Botond, do you have any ideas?

Good find! I'm not sure whether this change is fine to make. It looks like the change to use the shadow root was made by Brian in bug 1575485, maybe he can weigh in on this.

How I remember it is that we had to change it to the shadow root to get tree scrolling by mousewheel to work at all, because in the past that event actually just didn't fire on the tree element (as from the comment on the line: "This event doesn't retarget, so listen on the shadow DOM directly"). It was ported from a XBL component which had different semantics on events and anonymous content which would sometimes bite us in ways like this when migrating to Custom Element + Shadow DOM.

So based on that memory it's surprising to me that the event fires at all on this now. Unless if something has changed with the semantics of this event specifically or Shadow DOM events in general such that now the original code is working (without triggering bug 1575485 which presumably it didn't from Daisuke's testing if you could scroll at all). Emilio, do you have any ideas on why this might be the case?

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Not off-hand, but I can repro this on release if I artificially make the <treechildren> smaller, so it doesn't seem recent. Bug 1601184 made these composed events (which probably explains it).

Comment 14

[Brian Grinstead [:bgrins]]

That's good to know. I think it makes sense to change the target as Daisuke suggests if it fixes the bug and doesn't reintroduce some form of bug 1575485. It would be great to get some QA assistance as well since this is not well covered by automated tests.

Comment 15

[Alexandru Trif, Desktop QA [:atrif]]

Hello! I searched for a regression range with mozregression on macOS 11 and there are the results:
Last good revision: c00df77af50e0703d37188be7c57032713e6efec (2021-04-29)
First bad revision: b417d526e5fcdd4bfd59dc1b0599fd915b993969 (2021-04-30)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c00df77af50e0703d37188be7c57032713e6efec&tochange=b417d526e5fcdd4bfd59dc1b0599fd915b993969
Unfortunately, the mozregression cannot bisect further and I don't know which of the bugs can be the culprit.

Comment 16

[Daisuke Akatsuka (:daisuke)]

Thank you very much, everyone!
I have investigated deeper.

First, I made a patch as I suggested, then tried to write the test code, and realized that this problem does not happen on browser mochitest.
I investigated the reason, when running on the mochitest, the dom.w3c_touch_events.enabled pref is set as 1 against 0 for normal mode (I’m using OSX). And, I confirmed that the problem does not happen if change dom.w3c_touch_events.enabled to 1 in about:config even in normal mode.
https://searchfox.org/mozilla-central/rev/80e1bfa13c954e6450a13b3cc802d82773eba2e0/testing/profiles/unittest-features/user.js#14
That the pref changes the behavior means, the problem is not the event target, but event handling, I thought then investigated.

As the result, CancelableBlockState::SetContentResponse(false) in InputQueue below was called before handling child’s preventDefault().
https://searchfox.org/mozilla-central/rev/80e1bfa13c954e6450a13b3cc802d82773eba2e0/gfx/layers/apz/src/InputQueue.cpp#558
Once this handling is done, subsequent updates will not be possible, and the tree's preventDefault() handling will not be applied.
https://searchfox.org/mozilla-central/rev/80e1bfa13c954e6450a13b3cc802d82773eba2e0/gfx/layers/apz/src/InputBlockState.cpp#192-194
It seems that this is the reason for this issue.

For the trial, I tried the code as follows, and it gets the expected behavior.
https://hg.mozilla.org/try/rev/da1ae6cc42926ff95262781aa1232a1176c01622
(And the try: https://treeherder.mozilla.org/jobs?repo=try&revision=4c2d26ce5c6b3590e287b6fc366ff721b372a5da)

Botond, I have questions!

• That the result of CancelableBlockState::IsTargetConfirmed() is changed by dom.w3c_touch_events.enabled pref is expected?
• If IsTargetConfirmed() is true, then there is a comment "Content won't prevent-default this, so we can just set the flag directly.", but if there is a XUL, do we need special handling?

In the try-server with the above patch, it seems that only the unit test is failed.
https://treeherder.mozilla.org/jobs?repo=try&revision=4c2d26ce5c6b3590e287b6fc366ff721b372a5da&selectedTaskRun=I6nWi635T5SVU_8M2_DZlg.0

Thanks!

Comment 17

[Botond Ballo [:botond]]

Thanks for investigating, Daisuke, these are pretty interesting findings!

The meaning of IsTargetConfirmed() == true here is basically "APZ thinks there are no event listeners which might preventDefault() this event".

The reason dom.w3c_touch_events.enabled might affect the outcome of IsTargetConfirmed() is if the target element has a touch event listener, it will only be considered as a listener which might preventDefault() the event if touch events are enabled.

These findings suggest that the problem is that, for some reason, APZ is failing to notice the MozMousePixelScroll event listener which may in fact preventDefault() the event. The relevant logic for checking this is here.

I notice one difference is that the touchstart listener is added to this, but the MozMousePixelScroll listener is added to this.shadowRoot -- so maybe EventListenerManager needs to be modified to check for listeners on the shadow root as well?

Comment 18

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1790253: Check whether elements having 'display: contents' style is apz aware.

Comment 19

[Daisuke Akatsuka (:daisuke)]

Thank you very much, Botond!
As I tried to implement it, could you check the patch?

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

Based on comment #15, this bug contains a bisection range found by mozregression. However, the Regressed by field is still not filled.

:daisuke, if possible, could you fill the Regressed by field and investigate this regression?

For more information, please visit auto_nag documentation.

Comment 21

[Botond Ballo [:botond]]

(In reply to Release mgmt bot [:suhaib / :marco/ :calixte] from comment #20)

Based on comment #15, this bug contains a bisection range found by mozregression. However, the Regressed by field is still not filled.

It's not clear which commit in that range affected the behaviour here, but it also doesn't matter all that much, as the issue has been diagnosed and is being worked on.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/31b12711c229
Check whether elements having 'display: contents' style is apz aware. r=botond,smaug,emilio

Comment 23

[Norisz Fay [:noriszfay]]

Backed out for causing mochitest failures on test_scroll_on_display_contents.html

Backout link

Push with failures

Failure log

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6644ae3e7c9b
Check whether elements having 'display: contents' style is apz aware. r=botond,smaug,emilio

Comment 25

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/6644ae3e7c9b

Comment 26

[Mehmet]

Attachment: Bildschirmvideo aufnehmen 2022-12-20 um 08.16.15.mov

Hello daisuke@: I am on latest Nightly 110.0a1 (2022-12-19) on macOS and it seems that this change has caused a regression :( Now, when I am with the cursor in the search-engine box, I am not able to scroll further up or down through the page. The scrolling/cursor stucks in the box. A screencast is attached.

Comment 27

[Botond Ballo [:botond]]

Unfortunately, I believe this is an expected effect of the change that was made.

As discussed in comment 6 and comment 9, the search engine list is a XUL widget (I said at the time it was something like <scrollbox>, but it's actually a <tree>), which implements its own scrolling by listening for mouse wheel events and calling preventDefault() on them.

Calling preventDefault() prevents the browser's default behaviour for the wheel event, which would be scrolling the nearest natively scrollable element (which is the preferences page itself).

The bug was that the preventDefault() wasn't respected, so both the <tree> and the enclosing page would scroll. Now that the bug is fixed, only the <tree> scrolls and not the page.

The behaviour you're looking for ("if the <tree> is at the end of its scroll range, then scroll the enclosing page instead") is called "scroll handoff", and it's not implemented from a <tree> to a natively scrollable element.

Comment 28

[Botond Ballo [:botond]]

Perhaps the implementation of <tree> could be revised such that if it's at the end of its scroll range, it does not call preventDefault() on the wheel event, and then you'd get scrolling of the page. Anyways, that's best considered in a separate bug.

Comment 29

[Mehmet]

Alright, thanks for your your information. I'll open a separate bugreport for this.

Comment 30

[Mehmet]

(In reply to Mehmet from comment #29)

Alright, thanks for your your information. I'll open a separate bugreport for this.

Filed bug 1806591 (and removing needinfo from daisuke@)

Comment 31

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:daisuke, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.Also, don't forget to request an uplift for the patches in the regression caused by this fix.
• If no, please set status-firefox109 to wontfix.

For more information, please visit auto_nag documentation.

Comment 32

[Catalin Sasca, Desktop QA [:csasca]]

Reproduced the issue on Firefox 109.0b9 by following the STR and screen attachments on macOS 11.7.2.

Verified that the issue is fixed on Firefox 110.0a1 (2023-01-08) under macOS 11.7.2, Ubuntu 22.04 and Windows 11.