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 ScrollIntoViewMode ("always", "if-needed"); add FocusScrollOptions #5677

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

zcorpan
Copy link
Member

@zcorpan zcorpan commented Oct 28, 2020

This is the same as #1805 but from a fork to address https://lists.w3.org/Archives/Member/w3c-css-wg/2020OctDec/0050.html

Agenda+ for #1805 (comment) and following comments. @frivoal @anawhj @jihyerish @stipsan


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 Oct 28, 2020

@anawhj asked (#1805 (comment))

How could we drive this item? What should we discuss with more?

For focus() extensions, see whatwg/html#834 (comment)

The interop problem of the default scrolling behavior for focus() still exists, but could be solved I believe by changing Gecko: https://bugzilla.mozilla.org/show_bug.cgi?id=1410112

For scrollIntoView() extensions, I think it would be good to review what parts of
https://github.com/stipsan/scroll-into-view-if-needed developers are using and should be in the web platform directly, and whether the API shape in this PR is the right one.

@stipsan
Copy link

stipsan commented Oct 28, 2020

For scrollIntoView() extensions, I think it would be good to review what parts of
https://github.com/stipsan/scroll-into-view-if-needed developers are using and should be in the web platform directly, and whether the API shape in this PR is the right one.

Let me know if there's anything I can do 🙂 In my experience scrollMode: "if-needed" is often enough for most.
Then we have the common use case where devs are confused/surprised when they experience overflow: hidden frames getting scrolled, common enough to warrant adding a workaround.

The custom behavior API was added as an escape hatch for the other corner cases instead of bloating the API surface further. Since then a growing number of libraries prefer this pattern, as they want control over how the scrolling operations are performed: https://www.npmjs.com/browse/depended/compute-scroll-into-view
It could be helpful to have a similar utility in the platform, allowing us to calculate what should scroll where, but let us handle the actual scrolling 🤔

@zcorpan
Copy link
Member Author

zcorpan commented Oct 28, 2020

@stipsan thanks!

For overflow: hidden, is there a reason overflow: clip when you want to disallow scrolling isn't a better solution?

The custom behavior API seems interesting!

@stipsan
Copy link

stipsan commented Oct 28, 2020

For overflow: hidden, is there a reason overflow: clip when you want to disallow scrolling isn't a better solution?

I should've been more clear about that one. I mentioned the overflow: hidden confusion to only highlight the fact, not to propose that the skipOverflowHiddenElements workaround should be in the scrollIntoView spec. The overflow: clip behavior is already supported by the polyfill since it's in the spec. Once Safari and Chrome support it we'll drop the workaround 😄

@othermaciej
Copy link
Member

The title mentioned "notIfViewed" but I don't see that term or anything like it anywhere in the PR.

@zcorpan zcorpan changed the title [cssom-view] Add notIfViewed to ScrollIntoViewOptions; add FocusScrollOptions [cssom-view] Add ScrollIntoViewMode ("always", "if-needed"); add FocusScrollOptions Nov 11, 2020
@zcorpan
Copy link
Member Author

zcorpan commented Nov 11, 2020

@othermaciej thanks, fixed the title. (The API shape changed since the original proposal in #1805.)

Apologies for missing the telecon twice. I'll try again next week.

@anawhj
Copy link
Member

anawhj commented Nov 18, 2020

For focus() extensions, see whatwg/html#834 (comment)

In the thread(whatwg/html#834), there were requirements as follows:

  • Disabling scrolling on focus()
    -> Now, we can disable to scroll the element with element.focus({preventScroll: true});. (Add scrollOptions for Element.focus() whatwg/html#2787)
  • Smooth scrolling on focus()
    -> Now, we can smoothly scroll to the element by element.focus() with scroll-behavior: smooth.
  • Alignment of scrolling on focus()
    -> We can't do it for now. The default alignment is even different between Chrome(center) and Firefox(nearest).

If we have FocusScrollOptions for the last remaining requirement, I believe it could be used for several use cases.
(e.g. handle browser compatibility issue, avoid to use custom scroller, support several UX req.)

For the purpose, we could simply refer to scrollIntoviewOptions with block and inline properties.

My proposal is:
Can we rename the dictionary from scrollIntoviewOptions to `scrollAllignmentOptions' as more general term?
I referred Scroll Alignment term to https://scroll-into-view-if-needed.netlify.app/#scroll-alignment.
The renamed dictionary can be referred for the additional focus() extension in whatwg side.
It doesn't affect to web developers without any WebAPI change.

@zcorpan
Copy link
Member Author

zcorpan commented Nov 18, 2020

I think the interop difference in alignment on focus() should be fixed by browsers directly, and not be used as motivation for adding new API. We should only add alignment API if it's useful on its own.

I wonder if it makes sense to have a CSS property for focus alignment, which could take effect both for focus() and when the user moves focus with the Tab key. I see that the Scroll Snap spec has 'scroll-snap-align' which seems related: https://drafts.csswg.org/css-scroll-snap-1/#scroll-snap-align

My proposal is:
Can we rename the dictionary from scrollIntoviewOptions to `scrollAllignmentOptions' as more general term?
I referred Scroll Alignment term to https://scroll-into-view-if-needed.netlify.app/#scroll-alignment.
The renamed dictionary can be referred for the additional focus() extension in whatwg side.
It doesn't affect to web developers without any WebAPI change.

This PR adds FocusScrollOptions, intended to be used by focus(). It's a separate dictionary from ScrollIntoViewOptions because scrollIntoView() and focus() have different default behavior (always vs. if-needed, and likely also default alignment).

@anawhj
Copy link
Member

anawhj commented Nov 18, 2020

the interop difference in alignment on focus() should be fixed by browsers directly

I agree with it, but it hasn't been able to be unified for a long time, so authors need to have an option for handling it as one behavior now. Hopefully, it can be completed by https://bugzilla.mozilla.org/show_bug.cgi?id=1410112.

I wonder if it makes sense to have a CSS property for focus alignment, which could take effect both for focus() and when the user moves focus with the Tab key.

If we define a new CSS property for focus alignment, it needs to work as scroll-behavior does. The scroll-behavior takes effect both cases now. (focus() method and seq. navigation by tab key) Do we have further consideration for defining the new CSS property? If it needs much works to do, it could be simply handled by FocusScrollOptions as parameters inside focus() without a new CSS property.
e.g. scroll-alignment-[x|y]: [auto | start | end | center]

It's a separate dictionary from ScrollIntoViewOptions because scrollIntoView() and focus() have different default behavior (always vs. if-needed, and likely also default alignment).

If we can define the different default behaviors as a unified one in CSSOM-VIEW spec, ScrollIntoViewOptions could be used for several usages incl. focus(). If it would be difficult, I'm okay to put FocusScrollOptions separately from ScrollIntoViewOptions.

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [cssom-view] Add notIfViewed to ScrollIntoViewOptions; add FocusScrollOptions, and agreed to the following:

  • RESOLVED: change the PR to just ifNeeded bits with a reference to scroll snap as long as it's handling the use cases
The full IRC log of that discussion <dael> Topic: [cssom-view] Add notIfViewed to ScrollIntoViewOptions; add FocusScrollOptions
<dael> github: https://github.com//pull/5677
<dael> zcorpan_: A brief explanation. We wanted to extend scrollIntoView and Focus API to control if scroll should happen and if it does what the alignment should be; center, start, end alignment
<dael> zcorpan_: I think the question is what the API shape should look like. Which use cases we're trying to solve.
<dael> zcorpan_: There's at least one polyfill impl these behaviors so we have some data on what webdevs want
<dael> zcorpan_: Question is what to build intot he platform and how
<dael> zcorpan_: One option is add or utilize CSS properties for this. Or having a dictionary you pass into Focus or scrollIntoView methods. Other both
<dael> zcorpan_: Scroll behavior smooth you can do both. CSS property for smooth scrolling and you can opt in/out from JS
<dael> astearns: When you say that there's JS and CSS options for smooth scrolling it's just opting into it not any of the alignment?
<dael> zcorpan_: Smooth scrolling exists for scrollingINtoView. Issue is adding scroll alignment and if needed
<dael> fantasai: Shouldn't scroll alignment be handled by scroll snap alignment property?
<dael> zcorpan_: Maybe. Not familiar
<smfr> q+
<dael> fantasai: Has a scroll snap align with the values you typed. Supposed to take into account when scrolling even when scroll snap is not on.
<smfr> q-
<dael> faceless2: afaict that defines the alignemnt. There's a margin for spacing. All supposed to be for scrollIntoView type operations
<dael> f/faceless2/fantasai
<chris> I can't join the WebEx. On IRC only for now
<dael> zcorpan_: If needed means if a scroll should happen or not depending on if the element is already visible
<dael> fantasai: The default behavior is if needed. So if I tab to something not in view it scrolls to view?
<dael> zcorpan_: Exactly. Default is different for focus and scroll into view. Scroll into view always scrolls and focus is only if needed. You can override the behavior for both APIs if we extend the methods
<dael> fantasai: Always means what? Scrolls even if in view?
<dael> zcorpan_: Yeah, scrolls to spec alignment
<dael> fantasai: Like a target. Gotcha.
<dael> fantasai: We don't have anything like that.
<dael> fantasai: For alignment I recommend reading scroll snap
<dael> smfr: Also scroll padding that does scroll margin and padding
<dael> fantasai: All take effect even when scroll snapping not on
<dael> zcorpan_: That's good. Then possible CSS property is suffiecient and we don't need to override in API. Depends on if you want different alignment for different API calls
<dael> fantasai: Start with the CSS property, make sure it's impl, and wait if people ask for different behavior in different APIs
<dael> zcorpan_: But ifNeeded primitive still needs to be added if I understand
<dael> astearns: Is that an extra value or a new property?
<dael> zcorpan_: Not sure if it makes sense as a CSS property. Could add to focus and ScrollIntoView mentor
<dael> astearns: And determine if it ineeds to be in CSS on usage
<dael> fantasai: If you need to bring something into an alignment on scroll when it's target of ScrollIntoView I think you can turn on scroll snap. Don't know if there's a use case for a CSS property. I can see wanting to get one or other behavior out of APIs with different behaviors
<dael> zcorpan_: Agree
<dael> zcorpan_: Other issue is that browsers have different default behavior for scroll alignment on focus
<dael> zcorpan_: I think the way to fix that is by changing FF to match WK and Blink
<dael> zcorpan_: There is a Mozilla bug about that. Not fixed yet.
<dael> astearns: Mozilla bug is backed up by spec text?
<dael> zcorpan_: The PR leaves scroll alignment on focus undefinedd. We'd need to spec what we want
<dael> zcorpan_: Two behaviors are centering or start alignment if I remember correctly. FF does start alignment, WK and Blink do center. That's on focus
<dael> astearns: And is there an issue about the default alignment behavior? Did we resolve on what we wanted here and get spec updated to back getting it fixed in Gecko?
<dael> zcorpan_: It's part of this discussion, not a sep issue
<dael> astearns: Does the PR define the behavior?
<dael> zcorpan_: No, not for focus
<dael> zcorpan_: If the WG thinks one behavior is better we can resolve and spec. Otherwise we can wait for FF to change or WK or Blink to change to match FF
<dael> astearns: I don't know that I have an opinion which behavior is better
<emilio> q+
<dael> astearns: Could see an argument for Gecko b/c want to start reading at the top instead of having to scroll after focus
<astearns> ack emilio
<zcorpan_> The bug is https://bugzilla.mozilla.org/show_bug.cgi?id=1410112
<dael> emilio: I don't know if it's the same bug but we've discussed changes a couple times. We needed feedback from our a11y folks.
<dael> emilio: Other thing is if you do scroll if you're already visible. I haven't read the whole log so may have discussed. I remember Blink havign a compat issue we didn't. In general I don't think we'd be opposed to changing
<dael> zcorpan_: Bugzilla I pasted in IRC explains the two behaviors
<dael> zcorpan_: Three cases we care about. Element entirely in view is no scrolling; interop. Partially in view where WK and Blink scroll to nearest edge in block and inline. Entirely out of view they scroll to center, center.
<dael> zcorpan_: Nearest edge is the edge of the viewport closest to the element
<dael> astearns: The least amount of scrolling to take the element entirely into view
<emilio> https://github.com//issues/4778
<dael> emilio: This is more subtile. In inline access WK and Blink used to have a magic value where if it was out of view by less than an amount it wouldn't scroll. That's another thing to take into account
<dael> emilio: Generally centering makes sense. For sites that don't use proper padding the element might hide under fixedPos which is unfortunate
<dael> astearns: Suggest we resolve to reduce the PR to just the if needed bits and wait to see if Gecko can change and we spec default behavior if they can
<dael> smfr: What about prevent scroll argument?
<dael> zcorpan_: That is spec and impl
<dael> zcorpan_: as far as I know. Not sure if it's everywhere. It is in Chromium at least.
<dael> fantasai: astearns there was an argument for why there's centering in Gecko?
<dael> emilio: WK and Blink center. Gecko is sometimes unfortunate with scroll padding
<dael> astearns: Prop: Reduce PR to just hte If Needed bits
<dael> zcorpan_: And check if scroll snapping is enough for alignment use cases
<dael> fantasai: If you work on scrolling zcorpan_ read the whole module and let us know if we need adjustments.
<dael> zcorpan_: Can do that
<dael> smfr: I feel like both css properties and API properties there's a lot about scroll. might need algo to desc interaction. If Needed take into acocunt scroll padding?
<dael> fantasai: Should. In scroll module it is expected to apply to all the APIs
<dael> smfr: Okay
<dael> astearns: If it isn't there might be work a reference to scroll snap in CSSOM View. Just so people can follow breadcrumbs
<dael> zcorpan_: Essentially spec ifNeeded behavior in terms of scroll snap
<fantasai> https://drafts.csswg.org/css-scroll-snap-1/#scroll-padding
<dael> astearns: Objections to change to PR to just ifNeeded bits with a reference to scroll snap as long as it's handling the use cases
<fantasai> https://drafts.csswg.org/css-scroll-snap-1/#scroll-margin
<dael> RESOLVED: change the PR to just ifNeeded bits with a reference to scroll snap as long as it's handling the use cases

@zcorpan
Copy link
Member Author

zcorpan commented Nov 18, 2020

Next steps:

  • Review Scroll Snap spec and see how well the scroll alignment use cases are handled already.
  • If the scroll snap scroll alignment properties are sufficient, remove the scroll alignment parts from this PR. (Otherwise, explain and discuss again with the WG.)
  • Specify "if-needed" in terms of Scoll Snap so that 'scroll-margin' and 'scroll-padding' are taken into account when determining whether an element is "in view".
  • Specify default alignment for focus() to match WebKit/Chromium, and fix Gecko to match. (AFAIK, so far no objections, and it's the shortest path to interop, and the behavior doesn't seem unreasonable.)

@stipsan
Copy link

stipsan commented Nov 28, 2020

Specify "if-needed" in terms of Scoll Snap so that 'scroll-margin' and 'scroll-padding' are taken into account when determining whether an element is "in view".

Great news! I'll update the polyfill to follow this spec when I have some time off soon 😄 .

@jihyerish
Copy link
Contributor

How does this option work with nested overflow or iframe scrolling? Is this option applied to all descendant elements by default?

@ShivamJoker
Copy link

Any updates on this?

@anawhj
Copy link
Member

anawhj commented Feb 24, 2022

@ShivamJoker
If you have any use case on this, could you briefly share it here? :)

@ShivamJoker
Copy link

ShivamJoker commented Feb 24, 2022

Yes sure.

I have made an accordion library which renders 100s of list but it get's to bottom and user expands it, it can't automatically scroll into view.

Demo screen recording -

Screen.Recording.2022-02-24.at.2.00.41.PM.mov

If I add scrollIntoView all the elements will move even if it is not needed, and scrollIntoViewIfNeeded doesn't have smooth scrolling option and is not supported in majority of the browsers.

It would be helpful in such cases to have this option baked into the scrollIntoView.

@zcorpan
Copy link
Member Author

zcorpan commented Mar 3, 2023

scrollIntoViewIfNeeded usage seems to be rising over time: https://chromestatus.com/metrics/feature/timeline/popularity/389

If scrollIntoViewIfNeeded needs to be part of the web platform for compat (currently it's not implemented in Gecko), another option would be to standardize it and add smooth scrolling and positioning capabilities to it, instead of adding "if needed" to scrollIntoView. Thoughts?

@stipsan
Copy link

stipsan commented Mar 17, 2023

@zcorpan do you also suggest adding scroll-behavior: smooth; to it as well?

And what about .focus()? There's been earlier proposals for aligning .focus() with .scrollIntoView, giving it .focus({scrollMode: "if-needed", block: "nearest", inline: "nearest", behavior: "smooth"}). But if scrollIntoViewIfNeeded is standardised instead of .scrollIntoView({scrollMode: "if-needed"}) does that mean we should create a .focusIntoViewIfNeeded()?

Also worth noting that the demand for the proposed spec is on a steady increase as well: https://npm-stat.com/charts.html?package=scroll-into-view-if-needed&from=2018-03-16&to=2023-03-16 🤔

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.

7 participants