1796690 - scrollintoview triggers a scrollend event event in parent element even if no scrolling needs to be done

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Dan Robertson (:dlrobertson)]

Attachment: scrollinto-view-scrollend.html

Steps to reproduce

• Navigate to the attached page.
• Watch the document scrollend count.

Expected results.

• The document scrollend event listener should not be triggered from the initial setup scroll nor the scrollIntoView.

Actual results.

• On scrollIntoView the documents scrollend listener is called.

Discussion

It appears that the AsyncSmoothMSDScrollCallback or AsyncScrollCallback is called after the scrollIntoView with a zero range, and we fire the scrollend event regardless of the range covered. This is could be the right thing to do in the case that the scroll frame was the caller of scrollIntoView, but it appears that we also call scrollIntoView on the parent elements of the caller.

Comment 1

[Dan Robertson (:dlrobertson)]

Attachment: Bug 1796690 - Fire scrollend for scroll that updates scroll position. r=hiro,botond

Add simple test to trigger a scrollend due to a call to scrollIntoView.

Comment 2

[Dan Robertson (:dlrobertson)]

Interestingly, this spec change means that we shouldn't be firing scrollend events for scrolls that do not cause any scroll anyways. So just never firing a scrollend event for any scroll that scrolled over an empty range should fix this.

Comment 3

[Dan Robertson (:dlrobertson)]

Attachment: Bug 1796690 - Restore the scroll position before each subtest. r=hiro

Restore the scroll position before running each subtest in the scrollend event
handler content attributes tests.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/887104a60dc4
Restore the scroll position before each subtest. r=hiro
https://hg.mozilla.org/integration/autoland/rev/d86f25e5b636
Fire scrollend for scroll that updates scroll position. r=hiro

Comment 5

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/36849 for changes under testing/web-platform/tests

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5040c6736ee7
Mark scrollend-event-for-user-scroll.html subtest as intermittent on Android. a=test-only CLOSED TREE

Comment 7

[Sandor Molnar[:smolnar]]

Backed out for causing mochitest failures in browser/components/urlbar/tests/browser/browser_UrlbarInput_overflow.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/af34ce4e5c1ad767a1916764496a5ebe14703e95

Push with failures

Failure log

FYI: also I backed out Cosmin's wpt fix

Failure log wpt

Comment 8

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 9

[Dan Robertson (:dlrobertson)]

Ah it does look like my fix does not take into account overscroll. I'll have to figure out a way around this.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

There are two test cases stuck by Dan's change here, these cases, I wonder our mochitest harness supports IPv6 addresses? CCing :mak who is the author of the test cases.

Note that with D160156 we no longer fire scrollend events in cases where the scroll position is unchanged, in other words we've been firing scrollend events even if the scroll position is unchanged, and I suppose the test cases have been passing unintentionally because of the scrollend events?

Comment 11

[Dan Robertson (:dlrobertson)]

Attachment: Bug 1796690 - Update urlbar text overflow direction calculation. r=mak,hiro

Update the urlbar text overflow direction calculation after recent scrollend
changes do not result in scrollend events fired for scrolls that do not change
the scroll position.

Depends on D160156

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

I wonder our mochitest harness supports IPv6 addresses? CCing :mak who is the author of the test cases.

The test is not actually causing visits or any network traffic, it's just checking how the urlbar input field reacts to the given string.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/66ddf8488393
Restore the scroll position before each subtest. r=hiro
https://hg.mozilla.org/integration/autoland/rev/70903d77e4f0
Fire scrollend for scroll that updates scroll position. r=hiro
https://hg.mozilla.org/integration/autoland/rev/28f9e04d3500
Update urlbar text overflow direction calculation. r=mak

Comment 14

[Cristian Tuns]

Backed out for causing mochitest failures on browser_primaryUI.js

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | browser/tools/mozscreenshots/primaryUI/browser_primaryUI.js | Unexpected exception in [ tabsInTitlebar, twoPinnedWithOverflow, maximized, onlyNavBar, noLWT, compactDensity ]: Timed out -

Comment 15

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 16

[Dan Robertson (:dlrobertson)]

Attachment: Bug 1796690 - Use instant scrolls in test setup. r=dao,bgrins,hiro

Do not wait for scrollend after a scroll that will not trigger a change in scroll
position. A scrollend event will not be fired for a programatic scroll that will
not change the scroll position.

Depends on D161846

Comment 17

[Dan Robertson (:dlrobertson)]

D161846 should fix the failing test.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/daafa9467f36
Restore the scroll position before each subtest. r=hiro
https://hg.mozilla.org/integration/autoland/rev/79b8fc96663e
Fire scrollend for scroll that updates scroll position. r=hiro
https://hg.mozilla.org/integration/autoland/rev/deb588549764
Update urlbar text overflow direction calculation. r=mak
https://hg.mozilla.org/integration/autoland/rev/9062c3b9febf
Use instant scrolls in test setup. r=dao

Comment 19

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/daafa9467f36
https://hg.mozilla.org/mozilla-central/rev/79b8fc96663e
https://hg.mozilla.org/mozilla-central/rev/deb588549764
https://hg.mozilla.org/mozilla-central/rev/9062c3b9febf

Comment 20

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 21

[Monica Chiorean]

I used two build before and after the fix on Win10x64:

1. FF build 108.0a1(20221020215126) and
2. Beta 109.0b1(20221212143511),

• the only difference is that on Beta the "Scrollend Event Count: 18" number keeps rising if continue scrolling compared to Nightly were it remains at zero "Scrollend Event Count: 0". Is this the expected result before and after the fix? Thank you so much.

Comment 22

[Dan Robertson (:dlrobertson)]

(In reply to Monica Chiorean from comment #21)

I used two build before and after the fix on Win10x64:

1. FF build 108.0a1(20221020215126) and
2. Beta 109.0b1(20221212143511),

• the only difference is that on Beta the "Scrollend Event Count: 18" number keeps rising if continue scrolling compared to Nightly were it remains at zero "Scrollend Event Count: 0". Is this the expected result before and after the fix? Thank you so much.

Note that for 108 builds the scrollend event will not be enabled to fire to content by default. Ensure that the preference apz.scrollend-event.content.enabled is set to true before testing this bug (default true for 109 AFAIK). The key counter to watch is the Scrollend Event Count: in the Document section. For builds before this was landed, the value should be greater than 0, but for builds after this landed, the value should be 0.

The "real" answer is that scrollend events should only be fired when a scroll occurred. As a result, where the Scroll Event Count: is 0, the Scrollend Event Count: should also be 0.

Comment 23

[Monica Chiorean]

I was able to get value '1' for "Scrollend Event Count: in the Document section", on a Win10x64 machine using FF build 108.0.1(20221215175817).
Verified as fixed on Win10x64/Mac 10.13/Ubuntu 20.04 using FF builds 109.0b5(20221220185745) and Nightly 110.0a1(20221220214632).