1929707 - Remove dead scrollend specific event queue.

Status: RESOLVED FIXED

(Core :: Panning and Zooming, task, P3)

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1929707 - Remove dead scrollend specific event queue. r=dlrobertson

Comment 2

[Emilio Cobos Álvarez [:emilio]]

So I was going to submit this because I realized it's dead code while reviewing bug 1801658.

However I think the fact that is dead code is mostly an accident, because this line:

https://searchfox.org/mozilla-central/rev/5b288ed276a9580f2bedd4f67543940667d6a8c0/layout/generic/ScrollContainerFrame.cpp#5840

Didn't change to PostScrollEndEvent in bug 1881974.

Dan, wdyt? Should we simplify the code for now, or should we fix the bit in bug 1881974 with something like this?

diff --git a/layout/generic/ScrollContainerFrame.cpp b/layout/generic/ScrollContainerFrame.cpp
index 41f275f507708..ff23202be5705 100644
--- a/layout/generic/ScrollContainerFrame.cpp
+++ b/layout/generic/ScrollContainerFrame.cpp
@@ -5822,7 +5822,7 @@ void ScrollContainerFrame::CurPosAttributeChangedInternal(nsIContent* aContent,
 ScrollContainerFrame::ScrollEvent::ScrollEvent(ScrollContainerFrame* aHelper,
                                                bool aDelayed)
     : Runnable("ScrollContainerFrame::ScrollEvent"), mHelper(aHelper) {
-  mHelper->PresContext()->RefreshDriver()->PostScrollEvent(this, aDelayed);
+  mHelper->PresContext()->RefreshDriver()->PostScrollEndEvent(this, aDelayed);
 }

 // TODO: Convert this to MOZ_CAN_RUN_SCRIPT (bug 1415230, bug 1535398)

That said, the spec is not super clear on what the right thing to do is.

It seems Chrome also keeps a single queue and dispatches them all. In fact it seems it doesn't keep a separate queue for visual viewport events at all. That does seem a bit simpler? Should we just do that?

Comment 3

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1929707 - Use a single event queue for all scroll events. r=dlrobertson

This matches other browsers, see comments, and is generally simpler.

Comment 4

[Dan Robertson (:dlrobertson)]

Ah, we added this as part of a group of work to ensure that scrollend events are fired after all scroll relevant scroll events. I think the best fix would be to append the events to the scrollend array. The spec does say there should be a pending scrollend events array, so I also think two arrays one for scrollend and one for scroll might be the right approach.

Comment 5

[Emilio Cobos Álvarez [:emilio]]

I'm... not convinced. In any case the spec is super vague, and per spec aiui the scrollend should fire sync, which is clearly broken?

I filed https://github.com/w3c/csswg-drafts/issues/11164 about this.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3a159346c2cd
Remove dead scrollend specific event queue. r=dlrobertson
https://hg.mozilla.org/integration/autoland/rev/d58cb3443885
Use a single event queue for all scroll events. r=dlrobertson

Comment 7

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/3a159346c2cd
https://hg.mozilla.org/mozilla-central/rev/d58cb3443885