Add scrollOptions for Element.focus() #2787

jihyerish · 2017-06-23T08:05:03Z

Add scrollOptions to Element.focus()

This option allows the author to define the scroll behavior triggered when a focused element is partially viewed within the viewport.

scrollOptions
{
  behavior: "auto" | "none" | "smooth",
  block: "start" | "end"
 }

auto: A focused element scrolls into viewport instantly
none: A focused element doesn't scroll into viewport
smooth: A focused element scrolls into viewport smoothly
start: A focused element scrolls to the start (top) of the viewport.
end: A focused element scrolls to the end (bottom) of the viewport.

scrollOptions refers to the scrollIntoViewOptions of Element.scrollIntoView().

Closes #834

domenic · 2017-06-23T16:59:06Z

Hi @jihyerish, as you can see from the Travis CI results, the build is failing. This makes it hard to review your changes. Can you fix the patch so that it compiles and builds, at least locally on your computer? Let us know if you need any help.

domenic · 2017-06-23T16:59:33Z

You also seem to have made a lot of changes that revert recent work we've done on HTML. If you could please not do that, that would be ideal.

jihyerish · 2017-06-26T00:53:34Z

@domenic Sorry! I didn't know that. I'll fix this.

NavidZ · 2017-06-26T16:17:44Z

I had a quick question. Why don't we use the same options as ScrollIntoViewOptions in this case as well? Is there any attribute there that doesn't apply here?

jihyerish · 2017-06-27T01:12:57Z

@NavidZ You're right. As ScrollIntoViewOptions, the scrollOptions specifies the scrolling behavior and the scrolling position, but there is none value for the scrolling behavior which makes no scrolling of an element.

From that point of view, ScrollIntoViewOptions may have none value and use it in here.

NavidZ · 2017-06-27T15:09:11Z

Basically that is what I was suggesting. Instead of defining a new dictionary why not just reuse the ScrollIntoViewOption which also has other applicable options like inline and then somewhere add a none behavior and in the steps of the focus just call into ScrollIntoView or something. This way you don't even need to redefine all the behavior of values for the keys in ScrollIntoViewOptions.
Although I wonder whether just adding a none value to the ScrollIntoViewOptions or ScrollBehavior makes sense or not. Since when we have those dictionaries we always do want to scroll.
I wonder if it is worth doing a little future proofing here. Say we can have a dictionary here for the focus function only which has bunch of attributes regarding the focus action. One of which is Scroll and can be either none meaning no scrolling at all or can be a ScrollIntoViewOptions. This way if we want to add other behaviors regarding focus we can easily extend this dictionary with other keys. @domenic @RByers @dtapuska @jihyerish WDYT?

domenic

I like @NavidZ's idea. If I am understanding that will require IDL like:

dictionary FocusOptions {
  ScrollIntoViewOptions scroll;
};

Then the usage would be either .focus() (equivalent to .focus({ scroll: undefined })), or .focus({ scroll: { behavior: "smooth" } }) or similar.

This needs to be added to this spec patch, however; right now there are no IDL changes at all.

domenic · 2017-06-27T17:39:51Z

source

@@ -73384,14 +73384,16 @@
   </dd>
 -->



You need to update the IDL, around line 9871.

Didn't we want to define something like this:
dictionary FocusOptions {
ScrollIntoViewOptions scroll;
};

and use that dictionary here?

Yes, we could use ScrollIntoViewOptions here if the "none" value is added to it.
I made an issue about it in CSSOM - w3c/csswg-drafts#1608.

I specified the naming of the option for Element.focus() as "scrollOptions".
But there is already "ScrollOptions" in CSSOM.
I'll change "scrollOptions" to "focusOptions".

Regarding the "none" value in the ScrollBehavior I don't think that will be the best option here. How about just relying on the scrolloptions key in the FocusOptions dictionary in your last patch? So the scrolloptions is either undefined which we can interpret as no scrolling or it is a ScrollIntoViewOptions which then gets passed to the scrollIntoView algorithm. This way we do not need to add a "none" value to the "ScrollBehavior".

This is I believe what @domenic agreed to as well:
#2787 (review)

So if somebody calls .focus({ scrolloptions: undefined }) then we would not scroll (instead of adding "none" to ScrollBehavior). If somebody calls focus() it is as if they call .focus({scrolloptions: {behavior: auto}}) which is the current behavior of focus regarding scrolling. Also if they desire to pass a full scrolling options for focus they can do that as well.

btw shouldn't we use scrollOptions instead of scrolloptions?

I updated the IDL, around line 9939 and 9949.

domenic · 2017-06-27T17:42:51Z

source


    <p>If the element is a <span>browsing context container</span>, moves the focus to the <span>nested browsing context</span> instead.</p>

+    <p>If the focused element is in a scrollable region and partially visible within the <span>viewport</span>, it may scroll into the <span>viewport</span>. The scroll behavior of is decided by <var>scrollOptions</var>.


A few issues:

What does "in a scrollable region" mean? Is that term defined anywhere?

What does "partially visible within the viewport" mean? Is that defined anywhere?

You cannot use "may" inside non-normative text like this description. Probably this should be "can"? Or "will"?

"The scroll behavior of is" should just be "The scroll behavior is"

What does "in a scrollable region" mean? Is that term defined anywhere?

In the Focus section in HTML Standard, the term "scrollable regions" is used to describe the viewport of a box which an element can scroll into.
But there isn't any definition about it in the spec.
"scrolling area" in CSSOM is similar to "scrollable regions".

What does "partially visible within the viewport" mean? Is that defined anywhere?

It means that the entire area of an element isn't in the viewport.
The expression of "part of an element" is used in the spec, so I think it's better to change the sentence by using it.

You cannot use "may" inside non-normative text like this description.
Probably this should be "can"? Or "will"?
"The scroll behavior of is" should just be "The scroll behavior is"

Thanks for the correction! I'll fix those. : )

Thanks for helping me out here. "scrollable region" seems OK then, although maybe we should work in the future on updating to consolidate with CSSOM.

I'm OK with either of "part of an element" or "partially visible within the viewport". But I am still not sure I understand the meaning. Does this mean that if something is too far down the page to be visible, we won't scroll to it? Is that intentional? Why would we add such a restriction?

At the first time, I thought controlling the scroll behavior for the invisible element within the viewport is unnecessary. So I considered only the visible elements to apply this feature.

But now I also think we don't need the restriction on the visibility of an element.

But now I also think we don't need the restriction on the visibility of an element.

If an element is entirely inside the viewport, we shouldn't scroll it, right?

@EiraGe Yes, it is. This will work as "scrollIntoView()".

domenic · 2017-06-27T17:44:25Z

source

   <li><p>Mark the element as <dfn>locked for focus</dfn>.</p></li>

-   <li><p>Run the <span>focusing steps</span> for the element.</p></li>
+   <li><p>If the element is in the scrollable region, take <var>scrollOptions</var> as the scroll behavior.</p></li>


As noted above "is in the scrollable region" doesn't seem to be defined.

Also, the statement "take scrollOptions as the scroll behavior" is meaningless. Probably this should be something like "scroll the element into view using scrollOptions"?

Yes, it seems much better!

I suggest not adding any conditions here and other places in this patch. We already have these conditions in the scroll steps. So just mentioning the call into the scroll function will take care of the cases that there are no scrollable boxes and whatnot.
The only condition needed here is if ScrollIntoViewOptions is null/false/undefined (whichever we decide for not scrolling) then do not call scroll function and effectively don't scroll. Otherwise call it and pass the options to it.
We also need to define the default value that doesn't interfere with not scrolling value and also indicates the current scroll behavior for that default which I believe is doing the scroll. Right?

We already have these conditions in the scroll steps. So just mentioning the call into the scroll function will take care of the cases that there are no scrollable boxes and whatnot.

The scroll steps that you mentioned above is described in scrollIntoView function.
Does scrolling effect triggered by focus always call the scrollIntoView function?

We can call that out in the spec that focus calls into scrollIntoView. I don't see any reason not to.

domenic · 2017-06-27T17:45:40Z

source


  </ol>

+  User agents may scroll the focused element if it is partially within the <span>viewport</span> of the scrollable region. The behavior of scroll is specified with <var>scrollOptions</var>.


This algorithm is in general pretty confusing; it's not clear when it applies, and it takes no inputs or outputs. Probably we want to delegate to https://drafts.csswg.org/cssom-view/#scroll-an-element-into-view if at all possible instead.

If https://drafts.csswg.org/cssom-view/#scroll-an-element-into-view is not good, could you describe the intent of the algorithm first, and then we can work together on crafting the right text for it?

I'll take the algorithm mentioned in https://drafts.csswg.org/cssom-view/#scroll-an-element-into-view as mush as possible. Thanks! : )

NavidZ · 2017-07-10T16:22:27Z

source

   <li><p>Mark the element as <dfn>locked for focus</dfn>.</p></li>

-   <li><p>Run the <span>focusing steps</span> for the element.</p></li>
+   <li><p>If the element is in the scrollable region, take <var>scrollOptions</var> as the scroll behavior.</p></li>


I suggest not adding any conditions here and other places in this patch. We already have these conditions in the scroll steps. So just mentioning the call into the scroll function will take care of the cases that there are no scrollable boxes and whatnot.
The only condition needed here is if ScrollIntoViewOptions is null/false/undefined (whichever we decide for not scrolling) then do not call scroll function and effectively don't scroll. Otherwise call it and pass the options to it.
We also need to define the default value that doesn't interfere with not scrolling value and also indicates the current scroll behavior for that default which I believe is doing the scroll. Right?

NavidZ · 2017-07-10T16:29:56Z

source

@@ -73384,14 +73384,16 @@
   </dd>
 -->



Didn't we want to define something like this:
dictionary FocusOptions {
ScrollIntoViewOptions scroll;
};

and use that dictionary here?

mcmurraywanda101 · 2017-07-10T17:11:31Z

thank u

jihyerish

I updated IDL in the commit 4bf4a2c

…ntoViewOptions

NavidZ · 2017-07-17T15:54:27Z

@domenic
I had a question regarding the behavior of not doing the scroll. I assume when someone calls .focus() it is as if they called .focus({scrollOptions: {behavior: auto}}) with us specifying default values in the IDL. Now if they want to not to scroll which one is better/correct: .focus({scrollOptions: false}) or .focus({scrollOptions: undefined})

domenic · 2017-07-17T21:19:53Z

I had a question regarding the behavior of not doing the scroll. I assume when someone calls .focus() it is as if they called .focus({scrollOptions: {behavior: auto}}) with us specifying default values in the IDL. Now if they want to not to scroll which one is better/correct: .focus({scrollOptions: false}) or .focus({scrollOptions: undefined})

If someone calls .focus(), that is always equivalent to calling .focus({scrollOptions: undefined}). What that does is dependent on how we spec it, but presumably we want it to be backward-compatible, so we will spec it to do no scrolling.

.focus({scrollOptions: false}) will usually behave the same as .focus({scrollOptions: {}). The exception is if someone has added properties to Boolean.prototype; if that happens, those can impact the scroll options.

domenic · 2017-07-17T21:20:31Z

source

  [<span>CEReactions</span>] attribute [TreatNullAs=EmptyString] DOMString <span data-x="dom-innerText">innerText</span>;
 };
+
+dictionary <dfn>FocusOptions</dfn> : <a href="https://drafts.csswg.org/cssom-view/#dictdef-scrollintoviewoptions">ScrollIntoViewOptions</a> {


This should not inherit from ScrollIntoViewOptions, since it contains ScrollIntoViewOptions.

domenic · 2017-07-17T21:21:07Z

source

 };
+
+dictionary <dfn>FocusOptions</dfn> : <a href="https://drafts.csswg.org/cssom-view/#dictdef-scrollintoviewoptions">ScrollIntoViewOptions</a> {
+  <a href="https://drafts.csswg.org/cssom-view/#dictdef-scrollintoviewoptions">ScrollIntoViewOptions</a> scrolloptions;


We don't use <a href=""> inside IDL blocks. Instead, we use <span>ScrollIntoViewOptions</span>, and add something to the "References" section. In this case it would be around line 3560.

Thanks for your comment! I'll fix this.

domenic · 2017-07-17T21:22:09Z

source

-   <li><p>Run the <span>focusing steps</span> for the element.</p></li>
+   <li><p>If the element is in the scrollable region, take <var>options</var> as the scroll behavior.</p></li>
+
+   <li><p>Run the <span>focusing steps</span> for the element with <var>options</var>.</p></li>


The focusing steps do not take options as an argument, so this doesn't work. It's like calling a function with arguments that it does not accept; it won't compile.

Yes, it's right. The option is irrelevant to the focusing steps.

NavidZ · 2017-07-18T02:17:40Z

If someone calls .focus(), that is always equivalent to calling .focus({scrollOptions: undefined}). What that does is dependent on how we spec it, but presumably we want it to be backward-compatible, so we will spec it to do no scrolling.

I was under this impression that .focus() is the same as .focus(undefined) and is different from .focus({scrollOptions: undefined}). Am I wrong?
Anyhow, my understanding is that currently calling focus() will scroll to the element. At least as far as I tested unless there are some weird scenarios. If that is true so focus() with no parameter should scroll (and if this is the same as .focus({scrollOptions: undefined})) what is the best option for indicating no scrolling in FocusOptions without adding the none to the ScrollBehavior?

domenic · 2017-07-18T02:28:12Z

Anyhow, my understanding is that currently calling focus()will scroll to the element.

Oh, sorry for misunderstanding this; I was assuming it did not, without testing.

I was under this impression that .focus() is the same as .focus(undefined) and is different from .focus({scrollOptions: undefined}). Am I wrong?

Yeah, that's not correct. In Web IDL, supplying undefined for a dictionary is the same as supplying a dictionary with all its values set to its defaults. (And undefined is how to trigger defaults for dictionary members.)

If that is true so focus() with no parameter should scroll (and if this is the same as .focus({scrollOptions: undefined})) what is the best option for indicating no scrolling in FocusOptions without adding the none to the ScrollBehavior?

I think adding to ScrollBehavior is the best option, as it's much more clear for developers. If you're not willing to do that... perhaps we should take a step back and think about what API we want to present to developers, before getting into the IDL details.

For example, one possibility I just came up with is:

el.focus({ scroll: "auto" });
el.focus(); // same as above
el.focus({ scroll: "none" });
el.focus({ scroll: "instant" });
el.focus({ scroll: "smooth" });
el.focus({ scroll: "smooth", scrollOptions: { block: "start", inline: "end" });

Getting this properly integrated with the various underlying specs is difficult, but I'd be happy to help there. But first I think we should work on agreement on what API we want, regardless of how it integrates with other specs.

NavidZ · 2017-07-18T14:28:13Z

My only concern for adding the "none" to ScrollBehavior is that it is a parameter to a function that its job is to scroll with different behaviors. So if one doesn't want to scroll they shouldn't even call into that function (i.e. scrollIntoView) in the first place rather than a parameter telling that function to do nothing. However, that was the only thing I had in mind which I agree it might not be the strongest point. So if we cannot come up with anything clean I certainly would be fine with adding none to ScrollBehavior as well.

One question regarding your suggestion:

el.focus({ scroll: "smooth", scrollOptions: { block: "start", inline: "end" });

Isn't scrollOptions's type ScrollIntoViewOptions already? Which means it has an attribute behavior to specify ScrollBehavior (e.g. "auto", "instant", "smooth"). So what does scroll represent in your example?

domenic · 2017-07-18T14:40:54Z

My point was that we should not worry about the IDL; we should figure out what user-facing API we want. Thus this may involve creating new enumerations, dictionaries, or any of that so as to get the better experience for users. I'd like us to figure out what that experience is, before worrying about how it fits with existing or new IDL constructs.

NavidZ · 2017-07-19T15:48:41Z

How about this:
.focus() (To be same as .focus({scrollOptions: {block:'center', inline:'center', behavior: 'auto'}}) or whatever right now the default behavior precisely is for compat.
.focus({scrollOptions: false}) or .focus({scrollOptions: {}}) -> Does no scrolling
.focus({scrollOptions: {block:X, inline:Y, behavior: Z}}) -> Does whatever X,Y,Z imply.

domenic · 2017-07-19T16:06:43Z

Hmm, that departs from JavaScript APIs in several ways that we should avoid:

It mixes types: sometimes focusOptions is a boolean, sometimes an object. Typically we do not allow such mixing in our APIs; booleans (either true or false) get treated as an empty object.
It treats {} as something other than "fill in the default values". In general, passing an empty options object should be the same as passing an options object with all its defaults filled in.

Can we come up with an API that avoids these pitfalls?

- Don't use dictionary member names to discuss options to the scroll into view algorithm - Move FocusOptions definition near focus()'s definition - Make the developer-facing documentation more explicit and don't mention spec-level concepts like dictionaries - Tag closing or whitespace tweaks especially to make the diff simpler

domenic · 2017-10-25T00:25:21Z

I did a review and decided to just push small tweaks for the editorial issues I found. Nothing major. I will switch my status to approved.

What remains before we can merge this is web-platform-tests and interest from another browser implementer. I understand Chrome is interested; was there any word on others?

jihyerish · 2017-10-25T03:06:31Z

@domenic Thanks for the review and approval!
I think Gecko showed interest about this in the comment (#834 (comment)).

@smaug---- Am I understanding correctly?

domenic · 2017-10-25T03:39:54Z

Ah, thank you, I forgot there was a corresponding issue thread. In that case all we need is tests!

domenic · 2017-10-25T03:40:35Z

Wait, no, @zcorpan wrote tests!! web-platform-tests/wpt#7915 and web-platform-tests/wpt#7917 .

I'll work on getting this all merged tomorrow then \o/

jihyerish · 2017-10-25T04:55:30Z

Yay, finally!! 😆 Thank you!

smaug---- · 2017-10-25T09:21:47Z

ugh, the discussion is again nicely split to PR and Issue.
(IMO PRs really should be only about the somewhat mechanical spec changes once the actual matter has been decided in the issue.)

But ok, initially there will be just a way to prevent scrolling, right and no other customizations? Sounds ok.

jihyerish · 2017-10-25T09:46:42Z

@smaug---- Yes, for now only the option to prevent scrolling will be added.
But there will be the discussion about other options too, on the seperate thread.

Jihye Hong added 2 commits June 26, 2017 15:21

Add the scrollOptions for Element.focus()

f11ebaa

Add scrollOptions to the Element.focus() algorithm

c411c4f

domenic requested changes Jun 27, 2017

View reviewed changes

Merge branch 'master' into focus-scroll-options

20abf8a

NavidZ suggested changes Jul 10, 2017

View reviewed changes

jihyerish mentioned this pull request Jul 17, 2017

[cssom-view] Add a none value to the ScrollBehavior w3c/csswg-drafts#1608

Closed

Jihye Hong added 6 commits July 17, 2017 14:10

Merge branch 'master' into focus-scroll-options

a91b5b6

Modify the description of Element.focus(scrollOptions)

5314878

Change the naming of the option for Element.focus()

eaace4e

Update the IDL of HTMLElement with the FocusOptions

d0f9a42

Update the description of Element.focus()following IDL

4bf4a2c

Merge branch 'master' into focus-scroll-options

0d43dd5

jihyerish commented Jul 17, 2017

View reviewed changes

Modify the algorithm of focus() with FocusOptions referencing ScrollI…

fc80561

…ntoViewOptions

domenic requested changes Jul 17, 2017

View reviewed changes

domenic added 2 commits October 24, 2017 20:20

Undo some line-wrapping changes in unrelated areas

15e66cf

domenic approved these changes Oct 25, 2017

View reviewed changes

domenic added addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest topic: focus needs tests Moving the issue forward requires someone to write tests labels Oct 25, 2017

domenic removed the needs implementer interest Moving the issue forward requires implementers to express interest label Oct 25, 2017

domenic removed the needs tests Moving the issue forward requires someone to write tests label Oct 25, 2017

domenic merged commit 7a579c1 into whatwg:master Oct 26, 2017

domenic mentioned this pull request Oct 26, 2017

Disable / customize scrolling behavior of Element.focus #834

Open

chromium-wpt-export-bot mentioned this pull request Oct 28, 2017

Enable prevent scrolling in Element.focus() web-platform-tests/wpt#7998

Merged

ryanjduffy mentioned this pull request Jun 4, 2018

ENYO-5217: Fix Spottable to prevent scroll on focus on webOS enactjs/enact#1703

Merged

calvellido mentioned this pull request Jun 25, 2018

Need pattern for feature detecting dictionary members whatwg/webidl#107

Open

annevk mentioned this pull request Apr 8, 2019

The behaviour for repeated calls to focus({ preventScroll }) should be clarified #4512

Open

zcorpan mentioned this pull request Nov 20, 2019

[css-scroll-snap] Add tests for scroll-into-view operations. web-platform-tests/wpt#8575

Merged

zcorpan mentioned this pull request Oct 28, 2020

[cssom-view] Add ScrollIntoViewMode ("always", "if-needed"); add FocusScrollOptions w3c/csswg-drafts#5677

Open


		<p>If the element is a <span>browsing context container</span>, moves the focus to the <span>nested browsing context</span> instead.</p>

		<p>If the focused element is in a scrollable region and partially visible within the <span>viewport</span>, it may scroll into the <span>viewport</span>. The scroll behavior of is decided by <var>scrollOptions</var>.


		</ol>

		User agents may scroll the focused element if it is partially within the <span>viewport</span> of the scrollable region. The behavior of scroll is specified with <var>scrollOptions</var>.

Add scrollOptions for Element.focus() #2787

Add scrollOptions for Element.focus() #2787

Uh oh!

Conversation

jihyerish commented Jun 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

domenic commented Jun 23, 2017

Uh oh!

domenic commented Jun 23, 2017

Uh oh!

jihyerish commented Jun 26, 2017

Uh oh!

NavidZ commented Jun 26, 2017

Uh oh!

jihyerish commented Jun 27, 2017

Uh oh!

NavidZ commented Jun 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

domenic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jihyerish Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

domenic Jun 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcmurraywanda101 commented Jul 10, 2017

Uh oh!

jihyerish left a comment

Choose a reason for hiding this comment

Uh oh!

NavidZ commented Jul 17, 2017

Uh oh!

domenic commented Jul 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jihyerish commented Jun 23, 2017 •

edited

Loading

NavidZ commented Jun 27, 2017 •

edited

Loading

jihyerish Jul 17, 2017 •

edited

Loading

domenic Jun 27, 2017 •

edited

Loading

jihyerish commented Oct 25, 2017 •

edited

Loading