Enable ability to prevent scrolling in Element.focus()

• Component: Scrolling
• Status: RESOLVED FIXED

[Jihye Hong]

There is a spec proposal to disable automatic scroll into view on Element.focus().

Here is the summary of current situation: https://github.com/whatwg/html/pull/2787#issuecomment-338193107

The new dictionary type "FocusOptions" with the “preventScroll" dictionary member is introduced.

The IDL of Element.focus() will change to: 

dictionary FocusOptions {
  boolean preventScroll = false;
};
void focus(optional FocusOptions options);

If preventScroll is omitted or false, then the element will be scrolled into view with UA-defined manners.
Otherwise, it disables scrolling triggered by focus().

Related tests:
https://github.com/w3c/web-platform-tests/pull/7915
https://github.com/w3c/web-platform-tests/pull/7917

[Jihye Hong]

The spec proposal related to this issue was finally merged to the HTML Spec.
See the spec in here: 
https://html.spec.whatwg.org/#focus-management-apis

[Alden Daniels]

Update: This is now supported in both Chrome and 

See https://bugzilla.mozilla.org/show_bug.cgi?id=1374045 and

https://wpt.fyi/results/html/interaction/focus/processing-model/preventScroll.html?label=master&product=chrome%5Bstable%5D&product=edge%5Bstable%5D&product=firefox%5Bexperimental%5D&product=safari%5Bexperimental%5D&aligned

[Radar WebKit Bug Importer]

<rdar://problem/65544961>

[Rob Buis]

Created attachment 409976 [details]
Patch

[Rob Buis]

Created attachment 409979 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 409979 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409979&action=review

> Source/WebCore/dom/Element.h:405
> +    virtual void focus(bool restorePreviousSelection = true, FocusDirection = FocusDirectionNone, bool preventScroll = false);

I think it would be nicer for preventScroll to be an enum class.

[Rob Buis]

Created attachment 410105 [details]
Patch

[Rob Buis]

Created attachment 410308 [details]
Patch

[Sam Weinig]

Comment on attachment 410308 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=410308&action=review

> Source/WebCore/dom/Element.h:406
> +    void focus(const FocusOptions&);
> +    enum class PreventScroll { No, Yes };
> +    virtual void focus(bool restorePreviousSelection = true, FocusDirection = FocusDirectionNone, PreventScroll = PreventScroll::No);

I think this might be a bit nicer if we just had the one focus(const FocusOptions&) function, and added restorePreviousSelection and FocusDirection to the  FocusOptions struct (without exposing them to IDL of course). What do you think?

[Rob Buis]

(In reply to Sam Weinig from comment #9)
> Comment on attachment 410308 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=410308&action=review
> 
> > Source/WebCore/dom/Element.h:406
> > +    void focus(const FocusOptions&);
> > +    enum class PreventScroll { No, Yes };
> > +    virtual void focus(bool restorePreviousSelection = true, FocusDirection = FocusDirectionNone, PreventScroll = PreventScroll::No);
> 
> I think this might be a bit nicer if we just had the one focus(const
> FocusOptions&) function, and added restorePreviousSelection and
> FocusDirection to the  FocusOptions struct (without exposing them to IDL of
> course). What do you think?

That is a good idea, I will give it a shot.

[Rob Buis]

Created attachment 410349 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 410349 [details]
Patch

How is the prevent scrolling stuff spec'ed to work with nested overflow/iframe scrolling? Is scrolling prevented on all ancestors? If so, that needs testing.

[Loïc Yhuel]

Comment on attachment 410349 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=410349&action=review

I'm not sure whether the code style guidelines mandate spaces inside braced init lists, and the WebKit code isn't consistent.
Here when initializing the different FocusOptions arguments, sometimes there are spaces, sometimes not.

> Source/WebCore/ChangeLog:29
> +        * dom/FocusOptions.h: Copied from Source/WebCore/html/HTMLOrForeignElement.idl.

not copied

> Source/WebCore/dom/Element.h:405
> +    enum class PreventScroll { No, Yes };

This is no longer used in this patch revision.

[Rob Buis]

Created attachment 417535 [details]
Patch

[Rob Buis]

Created attachment 417687 [details]
Patch

[Rob Buis]

Comment on attachment 410349 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=410349&action=review

Loic, are you interested in taking over this bug? I will not have time to finish this patch for a while...

>> Source/WebCore/ChangeLog:29
>> +        * dom/FocusOptions.h: Copied from Source/WebCore/html/HTMLOrForeignElement.idl.
> 
> not copied

Removed.

>> Source/WebCore/dom/Element.h:405
>> +    enum class PreventScroll { No, Yes };
> 
> This is no longer used in this patch revision.

Removed.

[Matt Ryan]

Commenting as an +1 for prioritization of this bug, as Safari support is the one remaining blocker for wide availability of this feature and support has been in other browsers for a long time.

[Charlie Croom]

Throwing a +1 from Twitter into the mix too. We found this to be a really useful a11y tool to help with focus restoration from focus bound/locked contexts like dropdowns. It allows manual focus management and scrolling to play nicer together. (Although can be a foot gun if used wrong)

[Chris J. Shull]

+1 from the Google Maps API.

[Rob Buis]

(In reply to Simon Fraser (smfr) from comment #12)
> Comment on attachment 410349 [details]
> Patch
> 
> How is the prevent scrolling stuff spec'ed to work with nested
> overflow/iframe scrolling? Is scrolling prevented on all ancestors? If so,
> that needs testing.

Jihye, as spec author do you have some ideas about this? Are tests needed? This is the part the needs to be clarified before landing this.

[Jihye Hong]

>> Comment on attachment 410349 [details]
>> Patch
>> 
>> How is the prevent scrolling stuff spec'ed to work with nested
>> overflow/iframe scrolling? Is scrolling prevented on all ancestors? If so,
>> that needs testing.

>Jihye, as spec author do you have some ideas about this? Are tests needed? This is the part the needs to be clarified before landing this.


For the nested scroll elements, the preventScroll option 
* Doesn't affect the ancestors of the element
* Affects the descenders of the element

The initial intention of the spec is to avoid the frequent scroll behavior triggered by focus.
So preventScroll option is given to a certain element and if the element is a scroll container, the option applies to all elements that belong to it.

I think it would be better to have TC about this such as:

if there are nested scroll elements
<div id="scrollBox" style="overflow-y: scroll;">
  <div class="bigbox" id="1" style="overflow-y: scroll;" tabindex=1>
    <div class="box" id="1_1" tabindex=1>1_1</div>
    <div class="box" id="1_2" tabindex=1>1_2</div>
    <div class="box" id="1_3" tabindex=1>1_3</div>
  </div>
  <div class="bigbox" id="2" style="overflow-y: scroll;" tabindex=1>
    <div class="box" id="2_1" tabindex=1>2_1</div>
    <div class="box" id="2_2" tabindex=1>2_2</div>
    <div class="box" id="2_3" tabindex=1>2_3</div>
  </div>
  <div class="bigbox" id="3" style="overflow-y: scroll;" tabindex=1>
    <div class="box" id="3_1" tabindex=1>3_1</div>
    <div class="box" id="3_2" tabindex=1>3_2</div>
    <div class="box" id="3_3" tabindex=1>3_3</div>
  </div>
</div>

1. Check whether preventScroll effects ancestor or not 
1) #2 element and #2_1 element are both partially visible from the viewport of #scrollBox
2) document.getElementById('2_1').focus({preventScroll: true});
Expected result: Focus moves to the element with id "2_1" and both #2 element and #2_1 element don't scroll inside the viewport of #scrollBox element

2. Check whether preventScroll effects descender or not 
1) #2 element and #2_1 element are both partially visible from the viewport of #scrollBox
2) document.getElementById('2').focus({preventScroll: true});
Expected result: Focus moves to the element with id "2" and both #2 element and #2_1 element don't scroll inside the viewport of #scrollBox element

[Rob Buis]

Created attachment 420239 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Rob Buis]

Created attachment 420442 [details]
Patch

[Rob Buis]

(In reply to Jihye Hong from comment #21)
> I think it would be better to have TC about this such as:

Thanks you for the TC idea and review!

[Rob Buis]

This is ready for review :)

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 420442 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Rob Buis]

Created attachment 423926 [details]
Patch

[EWS]

Committed r274812: <https://commits.webkit.org/r274812>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423926 [details].

[Simon Fraser (smfr)]

*** Bug 219093 has been marked as a duplicate of this bug. ***

[Ryosuke Niwa]

Comment on attachment 423926 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=423926&action=review

> Source/WebCore/dom/Element.cpp:3092
> -    newTarget->revealFocusedElement(restorationMode);
> +    if (!options.preventScroll)
> +        newTarget->revealFocusedElement(options.selectionRestorationMode);

This is clearly wrong. We don't have to scroll but we most certainly need to update the selection.