Non-scrollable elements moves after focusing
Categories
(Core :: Layout: Scrolling and Overflow, defect, P2)
Tracking
()
People
(Reporter: David.Gausmann, Assigned: dlrobertson)
References
(Regression)
Details
(Keywords: nightly-community, regression)
Attachments
(3 files)
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
Reporter | ||
Comment 1•9 months ago
|
||
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•9 months ago
|
||
Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=ae30a1ac484e1daea4fc77681a6edda7aba66aff&tochange=7ffbd7298f7b4d38aad4c5af4e08a00e626d2c09
Comment 3•9 months ago
|
||
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.
Assignee | ||
Comment 4•9 months ago
|
||
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•9 months ago
|
||
Ah, if it was my patch what regressed and you don't have the cycles lmk and I can look.
Assignee | ||
Comment 6•9 months ago
|
||
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.
Updated•8 months ago
|
Updated•8 months ago
|
Comment 7•8 months ago
|
||
Set release status flags based on info from the regressing bug 1842679
Updated•8 months ago
|
Assignee | ||
Comment 8•8 months ago
|
||
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.
Assignee | ||
Comment 9•8 months ago
|
||
Add test to ensure that a element focus with a short line size does not
unnecessarily scroll an element.
Updated•8 months ago
|
Assignee | ||
Comment 10•8 months ago
|
||
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
Updated•8 months ago
|
Assignee | ||
Comment 12•7 months ago
|
||
(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•7 months ago
|
||
Pushed by drobertson@mozilla.com: 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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/43396 for changes under testing/web-platform/tests
Comment 15•7 months ago
|
||
Backed out for causing dt failures on browser_markup_dragdrop_reorder.js
Upstream PR was closed without merging
Updated•7 months ago
|
Updated•7 months ago
|
Assignee | ||
Updated•7 months ago
|
Updated•6 months ago
|
Comment 17•6 months ago
|
||
Pushed by drobertson@mozilla.com: 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•6 months ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0eb9a79316cb
https://hg.mozilla.org/mozilla-central/rev/271462f27193
Upstream PR merged by moz-wptsync-bot
Comment 21•5 months ago
|
||
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.
Assignee | ||
Comment 22•5 months ago
|
||
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.
Assignee | ||
Comment 23•5 months ago
|
||
(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•5 months ago
|
||
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.)
Updated•5 months ago
|
Comment 26•5 months ago
|
||
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.
Description
•