1858984 - Non-scrollable elements moves after focusing

Status: VERIFIED FIXED

(Core :: Layout: Scrolling and Overflow, defect, P2)

Description

[David Gausmann]

Attachment: SliderProblem.htm

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

Steps to reproduce:

Click on the sliders in the attached file.

Actual results:

The sliders will move to top and remain there, event after page reload (F5). After page reload without cache (Ctrl + F5) the position is ok, until you click again.

Expected results:

The sliders should remain in their position.
In Chrome and in old Firefox versions this worked fine.

I already did a bisection and got this result:

Element.focus() now centers an element in the block and inline
directions when the element should be scrolled into view. Update tests
to account for this.

Differential Revision: https://phabricator.services.mozilla.com/D183541

Comment 1

[David Gausmann]

The bisection result was not copied completely:

2023-10-13T16:54:46.943000: INFO : Narrowed integration regression window from [2d64faa1, 7ffbd729] (3 builds) to [ae30a1ac, 7ffbd729] (2 builds) (~1 steps left)
2023-10-13T16:54:46.955000: DEBUG : Starting merge handling...
2023-10-13T16:54:46.955000: DEBUG : Using url: https://hg.mozilla.org/integration/autoland/json-pushes?changeset=7ffbd7298f7b4d38aad4c5af4e08a00e626d2c09&full=1
2023-10-13T16:54:46.956000: DEBUG : redo: attempt 1/3
2023-10-13T16:54:46.956000: DEBUG : redo: retry: calling _default_get with args: ('https://hg.mozilla.org/integration/autoland/json-pushes?changeset=7ffbd7298f7b4d38aad4c5af4e08a00e626d2c09&full=1',), kwargs: {}, attempt #1
2023-10-13T16:54:46.962000: DEBUG : urllib3.connectionpool: Resetting dropped connection: hg.mozilla.org
2023-10-13T16:54:48.390000: DEBUG : urllib3.connectionpool: https://hg.mozilla.org:443 "GET /integration/autoland/json-pushes?changeset=7ffbd7298f7b4d38aad4c5af4e08a00e626d2c09&full=1 HTTP/1.1" 200 None
2023-10-13T16:54:48.474000: DEBUG : Found commit message:
Bug 1842679 - Update test expectations for Element.focus change. r=emilio,credential-management-reviewers,devtools-reviewers,sgalich,ochameau

Element.focus() now centers an element in the block and inline
directions when the element should be scrolled into view. Update tests
to account for this.

Differential Revision: https://phabricator.services.mozilla.com/D183541

2023-10-13T16:54:48.474000: DEBUG : Did not find a branch, checking all integration branches
2023-10-13T16:54:48.476000: INFO : The bisection is done.
2023-10-13T16:54:48.479000: INFO : Stopped

Comment 2

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=ae30a1ac484e1daea4fc77681a6edda7aba66aff&tochange=7ffbd7298f7b4d38aad4c5af4e08a00e626d2c09

Comment 3

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

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

:dlrobertson, since you are the author of the regressor, bug 1842679, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 4

[Dan Robertson (:dlrobertson)]

The diff listed is a testing diff. The regressing diff is https://phabricator.services.mozilla.com/D183512, but I've got some ideas as to what is happening here, so I'll investigate.

Comment 5

[Emilio Cobos Álvarez [:emilio]]

Ah, if it was my patch what regressed and you don't have the cycles lmk and I can look.

Comment 6

[Dan Robertson (:dlrobertson)]

Thanks! I might reach out with questions, but this is a good re-intro into rr... Its been a couple months and I've already forgotten things :p I'll be debugging this today ni? myself as a reminder to post what I've found.

Comment 7

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

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

Comment 8

[Dan Robertson (:dlrobertson)]

Took a bit to figure this out, but I'll post a fix shortly. The element is in a overflow:hidden container, so it is sort-of scrollable, so we will attempt to scroll the target into view when it is clicked. The bug appears to be in ComputeNeedToScroll (layout/base/PresShell.cpp:3318-3320). In this particular case the line size is greater than that of the target rect to scroll into view. As a result, ComputeNeedToScroll will always return true, even though the element is visible.

Comment 9

[Dan Robertson (:dlrobertson)]

Attachment: Bug 1858984 - Test that focus does not unnecessarily scroll. r=emilio,hiro

Add test to ensure that a element focus with a short line size does not
unnecessarily scroll an element.

Comment 10

[Dan Robertson (:dlrobertson)]

Attachment: Bug 1858984 - Fix ComputeNeedToScroll with large line sizes. r=emilio,hiro

When the line scroll amount is larger than that of the rectangle to
scroll into view, do not use the line size to determine if we should
scroll.

Depends on D192869

Comment 11

[Ryan VanderMeulen (PTO, back 6-April)]

Is this ready to land?

Comment 12

[Dan Robertson (:dlrobertson)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #11)

Is this ready to land?

Thanks for the ping! I returned from PTO today, and will land ASAP.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0aad09d9cd3f
Test that focus does not unnecessarily scroll. r=emilio
https://hg.mozilla.org/integration/autoland/rev/03881cb508be
Fix ComputeNeedToScroll with large line sizes. r=emilio

Comment 14

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

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

Comment 15

[Norisz Fay [:noriszfay]]

Backed out for causing dt failures on browser_markup_dragdrop_reorder.js

Backout link

Push with failures

Failure log

Comment 16

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

Upstream PR was closed without merging

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0eb9a79316cb
Test that focus does not unnecessarily scroll. r=emilio
https://hg.mozilla.org/integration/autoland/rev/271462f27193
Fix ComputeNeedToScroll with large line sizes. r=emilio,devtools-reviewers,ochameau

Comment 18

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/0eb9a79316cb
https://hg.mozilla.org/mozilla-central/rev/271462f27193

Comment 19

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

Upstream PR merged by moz-wptsync-bot

Comment 21

[dizhangg]

Newly introduced WPT test focus/focus-large-element-in-overflow-hidden-container.html is failing on Chrome and Safari. Can someone help double check if the test is written correctly and/or why this is the correct new behavior? Thanks.

Comment 22

[Dan Robertson (:dlrobertson)]

It would appear that the focus border might be a little bit smaller on chrome. It looks like if I reduce the error indicator box to 3px, the bug will still be caught (firefox release still fails), but chrome and firefox nightly still pass.

Comment 23

[Dan Robertson (:dlrobertson)]

(In reply to dizhangg from comment #21)

Newly introduced WPT test focus/focus-large-element-in-overflow-hidden-container.html is failing on Chrome and Safari. Can someone help double check if the test is written correctly and/or why this is the correct new behavior? Thanks.

Created an update to fix this here

Comment 24

[Daniel Holbert [:dholbert]]

I just merged Dan's PR to fix the test. The fixed version passes in WebKit and Chromium, as shown here in the wpt.fyi view of the PR results:
https://wpt.fyi/results/focus/focus-large-element-in-overflow-hidden-container.html?sha=499099f083&label=pr_head

(--> Canceling dizhangg's needinfo since I think that closes out that concern.)

Comment 26

[Alexandru Trif, Desktop Test Engineering [:atrif]]

Reproduced the issue with Firefox 120.0a1 (2023-10-13) on Windows 10x64. The sliders will move to the top and remain there after being activated and remain in that position after a refresh.
The issue is verified fixed with Firefox 123.0b2 on Windows 10x64, macOS 12 and Ubuntu 20.04. The slider will no longer change position after being activated.