Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[cssom-view] Add notIfViewed to ScrollIntoViewOptions; add FocusScrollOptions (Moved to #5677) #1805

Closed
wants to merge 2 commits into from

Conversation

zcorpan
Copy link
Member

@zcorpan zcorpan commented Sep 12, 2017

notIfViewed was originally suggested in
http://www.w3.org/mid/CAE3TfZPmNWvz1z7XPqFjtg5S+m_9BOmNNHsOZuSrR2z2AgyNHA@mail.gmail.com

Now it is needed for HTMLElement focus(options) to match the behavior
of focus() in browsers. Since the defaults for focus() is different
from the defaults for scrollIntoView(), a new dictionary
FocusScrollOptions is introduced. For now the defaults for block
and inline are UA-defined.

whatwg/html#2787 (comment)

…lOptions

notIfViewed was originally suggested in
http://www.w3.org/mid/CAE3TfZPmNWvz1z7XPqFjtg5S+m_9BOmNNHsOZuSrR2z2AgyNHA@mail.gmail.com

Now it is needed for HTMLElement focus(options) to match the behavior
of focus() in browsers. Since the defaults for focus() is different
from the defaults for scrollIntoView(), a new dictionary
FocusScrollOptions is introduced. For now the defaults for block
and inline are UA-defined.

whatwg/html#2787 (comment)
@zcorpan
Copy link
Member Author

zcorpan commented Sep 12, 2017

Note that this introduces a boolean dictionary member with default value true, which is generally not recommended.

It is strongly suggested not to use a default value of true for boolean-typed arguments, as this can be confusing for authors who might otherwise expect the default conversion of undefined to be used (i.e., false).

https://heycam.github.io/webidl/#dfn-optional-argument-default-value

However, the alternative would be to use different members that both default to false but have different semantics (i.e., notIfViewed = false and evenIfViewed = false), which also seems like it would cause confusion. cc @tobie

@tobie
Copy link
Member

tobie commented Sep 12, 2017

That's a tough one. There's no enum that would work, is there?

@zcorpan
Copy link
Member Author

zcorpan commented Sep 12, 2017

@tobie suggested an alternative name: unlessViewed.

I'm open for suggestions with enums or other API shapes.

@tobie
Copy link
Member

tobie commented Sep 12, 2017

This? :-/

scrollIntoView({ scroll: "always" });
scrollIntoView({ scroll: "only-when-hidden" });

or:

scrollIntoView({ avoidScrolling: "never" });
scrollIntoView({ avoidScrolling: "when-visible" });

@espadrine
Copy link

I like only-when-hidden. Maybe scrollIntoView({ onlyWhenHidden: true })?

@tobie
Copy link
Member

tobie commented Sep 12, 2017

Maybe scrollIntoView({ onlyWhenHidden: true })?

Well, then you're back with the boolean defaulting to true issue.

@espadrine
Copy link

espadrine commented Sep 12, 2017 via email

@tobie
Copy link
Member

tobie commented Sep 12, 2017

Well, the issue is both conditions are the default in different APIs.

@zcorpan
Copy link
Member Author

zcorpan commented Sep 13, 2017

focus({ preventScroll: true });
focus({ scrollOptions: { scroll: "always" } });
focus({ scrollOptions: { scroll: "if-needed" } });
scrollIntoView({ scroll: "always" });
scrollIntoView({ scroll: "if-needed" });

"if-needed" would be familiar for developers using scrollIntoViewIfNeeded().

@zcorpan
Copy link
Member Author

zcorpan commented Sep 13, 2017

OK updated the PR per my previous comment.

@zcorpan
Copy link
Member Author

zcorpan commented Sep 15, 2017

Another approach could be to add "-if-needed" suffixes to the keywords in ScrollLogicalPosition. The current nearest already has "if needed" semantics, as specified. This would give more control as it lets developers specify "if needed" separately for the two dimensions.

focus({ preventScroll: true });
focus({ scrollOptions: { block: "center", inline: "center" } });
focus({ scrollOptions: { block: "center-if-needed", inline: "center-if-needed" } });
scrollIntoView({ block: "start" });
scrollIntoView({ block: "start-if-needed" });

IDL

enum ScrollLogicalPosition { "start", "start-if-needed", "center", "center-if-needed", "end", "end-if-needed", "nearest" };

(No change to nearest for compat.)

@tobie
Copy link
Member

tobie commented Sep 15, 2017

That looks interesting.

@jihyerish
Copy link
Contributor

jihyerish commented Sep 21, 2017

I don't have a strong preference between these two:

  • Add scroll dictionary member scroll which can have always or if-needed as a value.
  • Add "-if-needed" suffixes to the keywords in ScrollLogicalPosition

But it looks like the second choice has more benefits for developers.

Apart from that, when focus({ scrollOptions: { scroll: "always" } });, does always mean that the scrolling happens to the element wherever it is positioned?
Or does it work only excluding the element which is entirely out of the view?
I think always should work for the element which is entirely or partially in the view.

If it's not, there is no way to do that case.

Element's position Scrolling Scrolling Scrolling Scrolling
Entirely in the view O X X X
Partially in the view O O X X
Entirely out of the view O O O X
IDL focus({ scrollOptions: { scroll: "always" } }); 😱 focus({ scrollOptions: { scroll: "if-needed" } }); focus({ preventScroll: true });

@zcorpan
Copy link
Member Author

zcorpan commented Sep 21, 2017

The proposal here says to abort if the element is entirely in view and "if-needed" is specified. If I understand correctly that should cover your use case. But always doesn't mean that scrolling will always take place--e.g., if nearest/nearest is specified and the element is already in view, no scrolling happens. (Or if there's no overflow, there's nothing to scroll.)

@jihyerish
Copy link
Contributor

Sorry, I mixed it up. I thought if "if-needed" is specified and the element is entirely out of view, then scrolling happens.
Then, the use case for scrolling only when the element is entirely out of view cannot be supported with this, right? That case is what I wanted to solve at the very beginning. (whatwg/html#834 (comment))
Also in Firefox, Element.focus() works like that.

So I think it also need to cover this case.

@jihyerish
Copy link
Contributor

jihyerish commented Sep 22, 2017

Currently, the result of Element.focus() on browsers is like this:

 Position of an element Chrome, Opera, Safari Firefox, Edge
Entirely in the view No scrolling No scrolling
Partially in the view Scrolling (block: nearest, inline: nearest) No scrolling
Entirely out of the view Scrolling (block: center, inline: center) Scrolling (block: nearest, inline: nearest)

You can see how it works here.

(Edited by @zcorpan to add Opera, Safari, and to merge same results into a single column.)

@zcorpan
Copy link
Member Author

zcorpan commented Sep 22, 2017

OK, then I think I have misunderstood the requirements. I thought you were happy with a way to turn off scrolling entirely (with preventScroll in focus()).

So you want to be able to have no scrolling for partially in view, and scroll into view for entirely out of view, correct?

Maybe instead of having increasingly complicated dictionaries to cover all possible cases, we could have only preventScroll in focus() (and select() or whatever else), then a separate API to check if an element is entirely in view, partially in view, or entirely out of view, and finally you can call (or not) scrollIntoView() with the positioning you want for the given case?

@jihyerish
Copy link
Contributor

So you want to be able to have no scrolling for partially in view, and scroll into view for entirely out of view, correct?

Yes, no scrolling for partially and entirely in view.
The separate API can solve the problem here. Then will it be the new API?

@anawhj
Copy link
Member

anawhj commented Oct 9, 2017

At this stage, I would like to summarize the current status and suggest some opinions on this PR.
I attached some comments(//) into the WebIDL below to clarify the meaning of the terms.

enum ScrollBehavior { "auto", "instant", "smooth" };
enum ScrollIntoViewMode { "always", "if-needed" }; // 1. prefer "if-needed" than suffixes"
enum ScrollLogicalPosition { "start", "center", "end", "nearest" };

dictionary ScrollOptions {
  ScrollBehavior behavior = "auto";
};

dictionary ScrollIntoViewOptions : ScrollOptions {
  ScrollIntoViewMode scroll = "always";
  ScrollLogicalPosition block = "start";
  ScrollLogicalPosition inline = "nearest";
};

dictionary FocusScrollOptions : ScrollOptions {
  ScrollIntoViewMode scroll = "if-needed"; // 2. scroll to scrollMode
  ScrollLogicalPosition block = "start"; // 3. default value: same as ScrollIntoViewOptions
  ScrollLogicalPosition inline = "nearest";
};

dictionary FocusOptions {
  boolean preventScroll = false;
  FocusScrollOptions scrollOptions;
};

void focus(optional FocusOptions options);

// For instances (applied the comments above)
elm.focus();
elm.focus({preventScroll: false});
elm.focus({scrollOptions: {behavior: "smooth"}});
elm.focus({scrollOptions: {behavior: "smooth", scrollMode: "always", block: "center"}});
elm.scrollIntoView({behavior: "smooth", scrollMode: "if-hidden", block: "nearest"});

First of all, the current IDL seems so complicated enough to support the scrolling options for the focus function, so I totally agree @zcorpan's opinion to prevent having increasingly complicated dictionaries.

Honestly, before the UA support for smooth scrolling, the feature has been supported by several libraries or polyfills using requestAnimationFrame(). However, in case of scrolling options for the focus function, there seems no way to control the scrolling behavior when the focus is moved to another element due to several reasons such as focus-related events with no cancelable. In addition, the important thing is that developers want to use the native focus function in order to support the focus-related a11y features and one of the biggest challenges is to handle the scrolling.

With such a background, I would strongly suggest that focus function can embrace the FocusScrollOptions dictionary as well as the preventScroll property. The WebIDL above couldn't be happier to me, while the case of partially view would be handled by other API (e.g. IntersectionObserver).

Regarding the WebIDL above, I would like to put some comments, but it's not the strong opinions. The basic design of WebIDL seems well-organized enough to me.

  1. prefer "if-needed" than suffixes of "-if-needed", prefer "if-hidden" than "if-needed"
    We need an option to specify whether to trigger scrolling or not when the element is fully in the view. Basically, scrollIntoView() enables the scrolling in that case, but focus() doesn't, so we defined the FocusScrollOption dictionary with different default value from the ScrollIntoViewOptions dictionary. As I see, there seems no correlation between "if-needed" that can control to happen scrolling and the ScrollLogicalPosition dictionary that specifies the position after scrolling. In many cases, developers want only to use the "if-needed" option, not to use the "block" or "inline" options, so I'm in favor of the ScrollIntoViewMode approach than the suffixes of "-if-needed" approach.
    In addition, it would be difficult to understand the meaning of "if-needed" even now. I agree on the design of "if-needed" in consideration of `scrollIntoViewIfNeeded", but it's not the standard API (only support in WebKit and Blink). If this work proceed well, scrollIntoView with "if-needed" could be same as the scrollIntoViewIfNeeded so that it could be deprecated naturally. It seems so difficult to define a new term instead of "if-needed", but I suggest "if-hidden" that means scrolling only if the element is hidden (partially or entirely out of view). I guess "if-hidden" could be used in scrollIntoView method to support several use cases while it has even been supported in JS libraries. Surely no problem to me with "if-needed" as is for BC.

  2. scroll to scrollMode
    scroll property name as the ScrollIntoViewMode type seems kind of general term, so I prefer to change the term like scrollMode.

elm.scrollIntoView({behavior: "smooth", scroll: "if-needed"});
elm.scrollIntoView({behavior: "smooth", scrollMode: "if-needed"});
  1. The default values of FocusScrollOptions would be same as ScrollIntoViewOptions
    {block: start, inline: nearest} would be good for FocusScrollOptions (same as ScrollIntoViewOptions)

@zcorpan
Copy link
Member Author

zcorpan commented Oct 16, 2017

Thank you for your analysis and comments @anawhj!

Let me try to state my understanding, so we can reach common ground.

  • preventScroll: true + IntersectionObserver + scrollIntoView() handles all use cases, but might not be so convenient.
  • Making https://drafts.csswg.org/cssom-view/#smooth-scrolling apply to scrolls caused by moved focus would be a convenient way to enable smooth scrolling for focus(). [cssom-view] Does scroll-behavior affect the sequential focus navigation? #1783
  • Customizing scrolling for focus() in a convenient manner, but less clear and less powerful compared to the first item in this list, could be done with a nested dictionary. The semantics and naming here is still under discussion. We agree that it shouldn't cover differentiating for the partially visible case, since that would make it too complicated.

To your points:

  1. I suggest "if-hidden" that means scrolling only if the element is hidden (partially or entirely in the view).

    Sounds good to me.

  2. scroll to scrollMode

    Sounds good to me.

  3. The default values of FocusScrollOptions would be same as ScrollIntoViewOptions

    Why? The primary reason for introducing FocusScrollOptions, as explained in Add scrollOptions for Element.focus() whatwg/html#2787 (comment) , is that omitting the dictionary should be equivalent to specifying an empty dictionary. Compare with [cssom-view] scrollIntoView({}) dictionary default should match scrollIntoView(true) #1720 . As noted by @jihyerish above [cssom-view] Add notIfViewed to ScrollIntoViewOptions; add FocusScrollOptions (Moved to #5677) #1805 (comment) the current browser behavior for focus() does not match that of scrollIntoView(). Therefore, the defaults for focus need to be different. Since we don't yet have interoperability on how focus() scrolls, what those defaults should be is up to the UA. We should work towards interoperability on that, but that can be separated imo.

zcorpan added a commit that referenced this pull request Oct 16, 2017
@zcorpan
Copy link
Member Author

zcorpan commented Oct 16, 2017

I rebased this branch (for cssom-viewcssom-view-1 rename) and added 2 new commits to rename if-needed to if-hidden, made it not scroll when partially visible, and renamed scroll to scrollMode, per above comments.

@frivoal
Copy link
Collaborator

frivoal commented Oct 17, 2017

preventScroll: true + IntersectionObserver + scrollIntoView() handles all use cases, but might not be so convenient.

I agree both that it would work, and that it's not so convenient.

Rather than adding another boolean or another value to an enum in the dictionary, which would force us to think hard about naming to make the whole thing non confusing, couldn't we add a numeric threshold, with the same meaning as the threshold in IntersectionObserver, applied only in the if-hidden case?

That seems to cover everything, without excessive complexity either for authors who want to use this, nor for those who don't care, nor on the implementation side.

@zcorpan
Copy link
Member Author

zcorpan commented Oct 19, 2017

Here is my recommendation on how to best proceed.

What we should focus on right now:

  • Add only preventScroll to focus() (without scrollOptions), and try to get that adopted cross-browser. This should be an easier sell than the whole package sans consensus. See https://extensiblewebmanifesto.org/
  • Develop a JS library that uses the primitives preventScroll: true + IntersectionObserver + scrollIntoView() to expose a convenient API for web developers.
  • Ask browser vendors to consider if they want to get full interoperability on default scrolling behavior for focus(), and if so what the behavior should be. I can file issues.

What we can do later:

  • When preventScroll is shipped and JS libraries exist to use it, study how these features are actually being used on the web, to inform what the for-convenience API should handle and how.
  • If/when focus() default scrolling behavior is more interoperable, we have a stable foundation to extend the API with scrollOptions (or something) that can explain the default scrolling behavior and allow customizing alignment etc.

1. If <var>block</var> is undefined, set <var>block</var> to a UA-defined value.
1. If <var>inline</var> is undefined, set <var>inline</var> to a UA-defined value.

ISSUE: Define defaults for <var>block</var> and <var>inline</var> for {{FocusScrollOptions}}.

1. Let <var>position</var> be the scroll position <var>scrolling box</var> would have by following these steps:
1. Let <var>hidden</var> be false.
1. If <var>element edge B</var> is outside (before) <var>scrolling box edge A</var>, or
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you push 'tap key' to move the focus to the element partially in view, the focus is moved to the element while scrolling happens to the nearest edge (Chrome and FF). It's the default behavior of focus() and we should support the default behavior even if the FocusScrollOptions is added into focus().

@anawhj
Copy link
Member

anawhj commented Oct 19, 2017

@zcorpan I agree to add only preventScroll to focus(), while other features such as smooth scrolling, alignment in the scrolling box could be handled using other APIs.

In addition, scrollOptions such as scrolling behavior and customizing alignment would be investigated aligned with CSS Scroll Snap. Regarding interoperability of alignment(block/inline) for focus() summarized in the comment above, interests from browser vendors seems to be important. Hopefully it will be unified as a single solution, but we should carefully consider it as it has a significant influence in terms of web UX.

@jihyerish
Copy link
Contributor

@zcorpan Thanks for wrapping up the current issue in here.
I'll be happy only if preventScroll is added to focus() (without scrollOptions) for right now.

Also, for the future work about FocusScrollOptions, it would be nice to discuss about:

  • Figuring out the default values of FocusScrollOptions
  • Finding the way to handle the partially viewed element (for now, it's intersectionObserver but is there any better solution?)
  • Considering the alignment with other focus cases. (scrolling by tab focusing, the fragment link scrolling, etc)
  • Investigating the alignment with CSS Scroll Snap

@zcorpan
Copy link
Member Author

zcorpan commented Oct 19, 2017

Ask browser vendors to consider if they want to get full interoperability on default scrolling behavior for focus(), and if so what the behavior should be. I can file issues.

https://bugzilla.mozilla.org/show_bug.cgi?id=1410112
https://bugs.webkit.org/show_bug.cgi?id=178515
https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/14298983/
https://bugs.chromium.org/p/chromium/issues/detail?id=776368

@Loirooriol
Copy link
Contributor

It seems all comments consider "Partially in the view" as a single case, but it can be subdivided depending on whether there is available space to show a greater fraction of the element or not. The "nearest" mode scrolls in the former but not in the latter. Some people might want this kind of behavior for other modes.

@stipsan
Copy link

stipsan commented Oct 6, 2018

Hey ya'll! I would love to see this move forward 😄

Are we waiting for vendors to give their feedback or can we move forward and get their input later?

I maintain a related polyfill for this proposal and based on feedback I'm getting, and the growing popularity of the package itself, I think it's safe to say that developers are very interested.

If there's anything I can do to help move things forward then just say the word! 👋

@jihyerish
Copy link
Contributor

@stipsan
Thanks for sharing your feedback, and the awesome polyfill! 👍
Yes, for now, we are gathering more feedback from vendors and developers.

As I mentioned in here, I'm planning to do items on the list in the near future!

@zcorpan
Copy link
Member Author

zcorpan commented Sep 18, 2020

Current status:

https://bugzilla.mozilla.org/show_bug.cgi?id=1410112

No change, but @annevk said it seems ok to align with Chrome and Safari.

https://bugs.webkit.org/show_bug.cgi?id=178515

No activity.

https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/14298983/

Edge has switched to Chromium.

https://bugs.chromium.org/p/chromium/issues/detail?id=776368

Closed as WontFix ("there is no update from the spec discussion in more than 1 year").

@frivoal
Copy link
Collaborator

frivoal commented Oct 23, 2020

This pull request is made with a branch in the main repo, rather than from a fork. This continuously puts strain on the build server, which is rebuilding it all the time to keep the bikeshed references current. This is OK for branches that need to have a preview, but this seems stale. It should be (updated and) merged or closed.

@zcorpan
Copy link
Member Author

zcorpan commented Oct 23, 2020

Has there been any movement on the default scrolling behavior for focus()?

I think it's ok to close this, it can be reopened if there is new interest. We could file an issue so it doesn't get forgotten.

@anawhj
Copy link
Member

anawhj commented Oct 26, 2020

Has there been any movement on the default scrolling behavior for focus()?

Yep! It has been merged via a commit in Oct 2017.
You can quickly understand how element.focus({preventScroll: true}); work on this.

I believe that the valuable changes in this thread would be needed for many developers.
Its polyfill has been widely used for several use cases such as custom scroller, arrow keys based navigation.

In my side(LG webOS), there are requirements about the following functionalities.

A: element.focus({block: start});
(It can be used for both aligning it in a customized way and mediating the different default behaviors between Chrome and Firefox.)

B: element.scrollIntoView({scroll: if-needed});
(It could be handled with IntersectionObserver for selecting the 'already inside view' case, so It seems less convincing though.)

All the cases are mostly covered with the already proposed changes in this thread.
How could we drive this item? What should we discuss with more?

@zcorpan
Copy link
Member Author

zcorpan commented Oct 28, 2020

Moved to #5677

@zcorpan zcorpan closed this Oct 28, 2020
@zcorpan zcorpan changed the title [cssom-view] Add notIfViewed to ScrollIntoViewOptions; add FocusScrollOptions [cssom-view] Add notIfViewed to ScrollIntoViewOptions; add FocusScrollOptions (Moved to #5677) Oct 28, 2020
@zcorpan zcorpan deleted the zcorpan/focusscrolloptions branch October 28, 2020 20:30
blm768 added a commit to blm768/oruga that referenced this pull request Dec 5, 2022
TODO: should only scroll into view if needed
May need a third-party dependency for that on many browsers. See:

w3c/csswg-drafts#1805
https://www.npmjs.com/package/scroll-into-view-if-needed
https://caniuse.com/mdn-api_element_scrollintoviewifneeded
blm768 added a commit to blm768/oruga that referenced this pull request Dec 6, 2022
TODO: should only scroll into view if needed
May need a third-party dependency for that on many browsers. See:

w3c/csswg-drafts#1805
https://www.npmjs.com/package/scroll-into-view-if-needed
https://caniuse.com/mdn-api_element_scrollintoviewifneeded
blm768 added a commit to blm768/oruga that referenced this pull request Dec 22, 2022
TODO: should only scroll into view if needed
May need a third-party dependency for that on many browsers. See:

w3c/csswg-drafts#1805
https://www.npmjs.com/package/scroll-into-view-if-needed
https://caniuse.com/mdn-api_element_scrollintoviewifneeded
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants