1842679 - Element.focus() should scrollIntoView with block/inline set to "center" when a scroll is needed

Status: RESOLVED FIXED

(Core :: Layout: Scrolling and Overflow, task, P3)

Description

[Dan Robertson (:dlrobertson)]

Attachment: element-focus.html

Summary

When Element.focus() is used, the element should be scrolled into view such that the element is centered when the scroll completes

Steps to reproduce

1. Navigate to the attached test case
2. Click the "Focus" button
3. Observe the position of the red element

Expected results

The red element should be in the center of the viewport

Actual results

The red element is at the very bottom of the viewport

Discussion

This is technically allowed by the spec, as dom-focus states:

... block flow direction position set to an implementation-defined value, and inline base direction position set to an implementation-defined value.

However, both chrome and safari both use block: "center" and inline: "center". This might make it worthwhile to update to the same behavior as chrome and safari. In particular Element.focus() is used in some web-platform-tests. It may be that we fail or pass for invalid reasons due to our behavior here.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

This will some impacts where documents depend on our current behavior.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

I think we should do this in this release cycle, wdyt? It'd be bad shipping inconsistent behavior between keyboard and regular .focus(), in particular because script focus is commonly used by pages to implement custom keyboard / mouse behavior.

Comment 4

[Dan Robertson (:dlrobertson)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

I think we should do this in this release cycle, wdyt?

Yeah, I can try to get this done. There were quite a few tests that expect the current behavior of Element.focus(), but that makes this a reasonable background task to work on. If I don't make it to the top of the try highscores chart, I'll be disappointed :p

It'd be bad shipping inconsistent behavior between keyboard and regular .focus(), in particular because script focus is commonly used by pages to implement custom keyboard / mouse behavior.

Is this severe enough to warrant feature gating or backing out the current fix for keyboard scrolls?

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(In reply to Dan Robertson (:dlrobertson) from comment #4)

Is this severe enough to warrant feature gating or backing out the current fix for keyboard scrolls?

I think we should avoid shipping the inconsistent behavior yes.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

At a glance try doesn't seem that unhappy:

• https://treeherder.mozilla.org/jobs?repo=try&revision=68a0a4e7c8cad02fa5e29e0ab830d6d991738040
• https://treeherder.mozilla.org/jobs?repo=try&revision=768d27c9738c805ae992e0eafa8ecea971218b8b (a more complete try run)

I can take this I guess, if you're busy with other stuff.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1842679 - Consistently scroll to center for focus. r=dlrobertson

Comment 8

[Dan Robertson (:dlrobertson)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #6)

I can take this I guess, if you're busy with other stuff.

I have time to work on it and I have an old revision that also changed the behavior of Element.focus() and fixed some of the tests... Most of them were reasonably easy fixes.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Ah, alright then! Feel free to steal :)

Comment 10

[Dan Robertson (:dlrobertson)]

Attachment: Bug 1842679 - Update test expectations for Element.focus change. r=emilio,hiro

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.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f02a0e3200b0
Consistently scroll to center for focus. r=dlrobertson
https://hg.mozilla.org/integration/autoland/rev/7ffbd7298f7b
Update test expectations for Element.focus change. r=emilio,credential-management-reviewers,devtools-reviewers,sgalich,ochameau

Comment 12

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

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

Comment 13

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/f02a0e3200b0
https://hg.mozilla.org/mozilla-central/rev/7ffbd7298f7b

Comment 14

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

Upstream PR merged by moz-wptsync-bot