1806591 - Implementation of <tree> in XUL widgets prevents scrolling further through a page

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect, P3)

Description

[Mehmet]

Attachment: demo.mov

Split from bug 1790253 - Hope it the right title for the bug :)

Mehmet Reporter:

Comment 26
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.
Flags: needinfo?([e-mail])

Botond Ballo [:botond]

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

Botond Ballo [:botond]

Comment 28
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 1

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

The severity field is not set for this bug.
:mstriemer, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 2

[Mark Striemer [:mstriemer]]

Bug 1790253 comment 28 has the suggested fix for this. When the container isn't able to scroll more in that direction it shouldn't call event.preventDefault()

Comment 3

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1806591: Implement scroll handoff for tree widgets

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e49dbf193eac
Implement scroll handoff for tree widgets r=mstriemer,masayuki

Comment 5

[Natalia Csoregi [:nataliaCs]]

Backed out for failures on test_tree_scroll.xhtml

• backout: https://hg.mozilla.org/integration/autoland/rev/6a1a5d1407272e604cec233e442177e750051fb2
• push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&revision=e49dbf193eac41c5b0b04dc179a473112739f5d7
• tier 1 failure: seen on the next push
• failure log: https://treeherder.mozilla.org/logviewer?job_id=413497611&repo=autoland&lineNumber=4874

[task 2023-04-23T22:14:24.603Z] 22:14:24     INFO - TEST-PASS | toolkit/content/tests/chrome/test_tree_scroll.xhtml | Scroll of container is expected 
[task 2023-04-23T22:14:24.604Z] 22:14:24     INFO - Buffered messages logged at 22:14:20
[task 2023-04-23T22:14:24.605Z] 22:14:24     INFO - Check whether the tree is not scrolled when the parent is scrolling
[task 2023-04-23T22:14:24.606Z] 22:14:24     INFO - Buffered messages finished
[task 2023-04-23T22:14:24.608Z] 22:14:24     INFO - TEST-UNEXPECTED-FAIL | toolkit/content/tests/chrome/test_tree_scroll.xhtml | The tree should not be scrolled - got "0", expected "168"
[task 2023-04-23T22:14:24.609Z] 22:14:24     INFO - SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:507:14
[task 2023-04-23T22:14:24.610Z] 22:14:24     INFO - doScrollWhileScrollingParent@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/tree_shared.js:1837:5
[task 2023-04-23T22:14:24.611Z] 22:14:24     INFO - async*testtag_tree_scroll@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/tree_shared.js:1814:9
[task 2023-04-23T22:14:24.612Z] 22:14:24     INFO - async*SimpleTest_setTimeoutShim@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:920:41
[task 2023-04-23T22:14:24.613Z] 22:14:24     INFO - onload@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_tree_scroll.xhtml:1:11
[task 2023-04-23T22:14:24.614Z] 22:14:24     INFO - callStackHandler@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:285:24
[task 2023-04-23T22:14:24.615Z] 22:14:24     INFO - EventHandlerNonNull*this.addLoadEvent@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:314:7
[task 2023-04-23T22:14:24.616Z] 22:14:24     INFO - @chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1764:15
[task 2023-04-23T22:14:24.617Z] 22:14:24     INFO - GECKO(1588) | MEMORY STAT | vsize 20256MB | residentFast 617MB | heapAllocated 286MB
[task 2023-04-23T22:14:24.619Z] 22:14:24     INFO - TEST-OK | toolkit/content/tests/chrome/test_tree_scroll.xhtml | took 9513ms

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1d6ce8d3346e
Implement scroll handoff for tree widgets r=mstriemer,masayuki

Comment 7

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/1d6ce8d3346e

Comment 8

[Mehmet]

Attachment: demo.mov

Hi Daisuke, thanks for fixing this bug. In latest Nightly scrolling through the page is no longer prevented when the mouse cursor is over the search engine widget. The only difference to HTML scrollboxes I noticed is, that scrolling within the box is not possible until you move the mouse cursor within the box. But then scrolling only works in one direction until you move the mouse cursor again. Not sure if this is intended, but is feels like a bug to me :( Please let me know if I should open a new report for it. A demo is attached. Thanks.

Comment 10

[Daisuke Akatsuka (:daisuke)]

Hi Mehmet!
Ah, yeah, indeed. Please let me take a look.

Comment 11

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

The following field has been copied from a duplicate bug:

Field	Value	Source
Regressed by	bug 1790253	bug 1822642

For more information, please visit auto_nag documentation.

Comment 12

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

Set release status flags based on info from the regressing bug 1790253

Comment 13

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1806591: Consume wheel event as long as being able to handle the event as the same series

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/12c1b6ca6f89
Consume wheel event as long as being able to handle the event as the same series r=masayuki,mstriemer

Comment 15

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/12c1b6ca6f89

Comment 16

[Mehmet]

Hi Daisuke, thanks for looking again into this issue. Unfortunately there is still a bug, at least on Mac with Trackpad or Magic Mouse. It is hard to explain, but I try with steps to reproduce.

Bug 1:

1.) Scroll from outside the box with the cursor into the box
2.) When you are with the cursor in the box lift your fingers from the trackpad
3.) Immediately put your fingers again on the trackpad and scroll in the box

Actual: Scrolling has effect outside the box. Scrolling has also no effect in the box when I move the cursor.
Expected: Scrolling should have an effect in the box. Scrolling only starts when I wait f a few seconds before I start with step 3.

Bug 2:

1.) Scroll in the box until you reach the top or bottom.
2.) Now lift your fingers from the trackpad while the cursor is still in the box
3.) Immediately put your fingers again on the trackpad and scroll in the direction to scroll further through outside the box.

Actual: Scrolling hangs in the box
Expected: Scrolling should move further through the page.

If you need a demo, please let me know.

Thanks for checking again :)

Comment 17

[Daisuke Akatsuka (:daisuke)]

Hi Mehmet, thank you very much always!
Yes, I can imagine it.

Masayuki-san, I'd like to hear your opinion.
To resolve it, we need to know the special events eWheelOperationStart and eWheelOperationEnd happening on OSX only, I think.
What do you think is the easiest way to detect both events in the tree component?
Can we fire the event from our platform??

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah, WheelTransaction does not work on <tree> because all mouse scroll events are handled in a <tree>. Therefore, ending wheel transaction is not managed by it. That must be the reason why we fail to end transaction.

How about to add nsIDOMWindowUtils.startWheelScrollTransaction(Element, Event)? I think that it's hard to maintain same logic in the <tree>. Then, call WheelTransaction::BeginTransaction with proper frames for the given element (the scrollable element in the <tree>) and the received event or its corresponding wheel event. For doing that, WidgetMouseScrollEvent needs to have a pointer to a WidgetWheelEvent which caused the event only while dispatching the event. They can be set around these lines. Then, WheelTransaction can manage the <tree> scroll transaction too. So, <tree> should check whether getWheelScrollTarget returns its scroll target or not to consider whether it should scroll its target.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oh, and perhaps, the new API name should be requestWheelScrollTransaction. Then, it should be called for calling UpdateTransaction too. And it should return bool and if there is another active transaction, this should return false and <tree> shouldn't handle the event.

Comment 20

[Daisuke Akatsuka (:daisuke)]

Masayuki-san, thank you very much! I will try it when I can have time.
(ni to myself to not forget this)

Comment 21

[Mark Striemer [:mstriemer]]

While making this work entirely as expected seems like a nice thing to do I wonder how much real world benefit we get from it. I think we only have two places where this can happen in Firefox, about:sessionrestore and this Search Engines section.

The about:sessionrestore case it's the only thing on the page so it seems less likely to cause serious issues like this original bug where it seemed like scrolling was totally stuck suddenly. The Search Engines case doesn't seem like it's an exceptionally common case to be in, without looking at any data and just guessing I'd be surprised if there were a lot of users that had enough search engines added to hit this.

I don't know how often this is used in Thunderbird, maybe it's a bigger issue there?

So if you've got time to fix this and it doesn't seem like a huge lift then fixing it sounds nice. But also it doesn't feel super high priority (happy to see the case for it being a higher priority though). I'd personally like to see us move away from these tree elements, but my team doesn't really have time right now nor do we have designs for what to replace it with.

Comment 22

[Ina Popescu, Desktop QA]

Attachment: scrolling on trackpad macOS.mov

I've reproduced the first issue using Firefox 113.0b1 following the STR from Comment 0, and second issue using Nightly 114.0a1 (2023-04-25) following the STR from Comment 8 on Windows 10 x64.

Both issues no longer persist in the latest Firefox 114.0b4 on Windows 10 x64 and Ubuntu 22.04 when scrolling with mouse.

Regarding Comment 16, I'm not able to reproduce the issues described there. As it looks to me, when using the trackpad to scroll on the about:preferences#search page, the first issue from Comment 0 is reproduceable on macOS. Please see the attachment.

Mehmet, could you please attach demos for these two issue too?

Comment 23

[Mehmet]

Yes the initial issue from comment 0 has been fixed. Comment 16 is still valid for me. Looks Daisuke@ will work on it when he has time.