Adds `skipOverflowHiddenElements` boolean option #225

icd2k3 · 2018-05-07T17:57:47Z

Hey there, if this is intentional please let me know, but I noticed a change in my app after upgrading from v1 to 2.

I had an element I wanted to scroll to that was a child of an element with overflow: hidden set. Essentially the behavior was:

1.) click a button to open an accordion-like drawer with content inside
-- a.) this drawer is initially set to overflow: hidden, max-height: 0
-- b.) in active state, drawer is set to overflow: hidden, max-height: 500px
2.) auto-scroll to the (now-expanded) content drawer.

This behavior worked in v1, but in v2 it detects the drawer container as scrollable and produced some weird side-effects by trying to scroll it.

Adding an overflowValue !== 'hidden' check to canOverflow which (I think) is reasonable, because a container with overflow: hidden set can not be scrolled.

stipsan · 2018-05-07T20:23:11Z

Hi @icd2k3, thanks for initiating a discussion about this! 😄

It's intended to be the first step towards implementing the Element.scrolIntoView spec.

Time for some back story!

The spec, at least to me, can take some effort to wrap your head around. Here's some info-nuggets that might help:
The spec calls elements that are scrolled during a Element.scrollIntoView or Element.focus for scrolling boxes.
A scrolling box can be either an Element or a viewport (e.g. browser Window, or iframe, etc) when certain criteria is met.

And I must confess

I did not implement both parts concerning overflow: hidden, take a look:

Elements and viewports have an associated scrolling box if has a scrolling mechanism or it overflows its content area and the used value of the overflow-x or overflow-y property is hidden.

This should be implemented instead of just a naive is overflow hidden check.
The reason why overflow: hidden counts is because it's been used to allow programmatic scrolling. This is also why overflow: hidden is close to just as expensive to render for the browser as overflow: scroll | auto because it needs to prerender the content in case scrolling happens.
There's a proposal to add overflow: clip to remedy this, which gives the same visual result as hidden but without the side effect of being a scrolling box.
Sadly not even any of the nightly or canary browsers support that property yet, the closest thing to it that I know is the Firefox specific -moz-hidden-unscrollable.

We're between a rock and a hard place

If this library deviates from the spec and choose to count overflow: hidden as no longer a scrolling box it'll break a lot of sites that are using the native Element.scrollIntoView API today and switch to this library. In fact, even the logo animation on the demo site itself rely on it: https://scroll-into-view-if-needed.netlify.com/

At the same time, following the spec leaves its own problems, as there's probably just as many who rely on overflow: hidden not being scrolled, as those who do.
Try it yourself if you login to Codepen and then try the demo.

In Chrome 66 it scrolls the document.body because it's overflow: hidden and when you're logged in there's some overflow happening:

Firefox 56 produces the same result:

I've noticed that most scrolling libraries handle this like v1 without exposing an option, while a few lets you decide to include overflow: hidden as a scrolling box if you want (check isScrollable in scroll-into-view).

The ideal solution

The most common use case is that you would either want all overflow: hidden to be either ignored or scrolled. IMO that justifies exposing it as a simple boolean option. The pdfjs library used by Firefox implement its own scrollIntoView logic with an option called skipOverflowHiddenElements, I think this could work here.

Can you update your PR to do that? 😃

For those who want to skip some overflow elements, but not all, I plan on introducing another callback option for that, similar to options.boundary where you can make your own assertions and decide what should count as a scrolling box or not.

icd2k3 · 2018-05-07T20:51:37Z

@stipsan thanks for the super detailed response - interesting stuff!

I'm happy to update this PR with a boolean option for skipOverflowHiddenElements! I will try to find some time for that soon.

I also really like your idea as well for really fine-grained control:

For those who want to skip some overflow elements, but not all, I plan on introducing another callback option for that, similar to options.boundary where you can make your own assertions and decide what should count as a scrolling box or not.

stipsan · 2018-05-07T21:00:27Z

You're welcome! And take your time, no rush 😄

Btw I escalated merging #223 so that we avoid merge conflicts, as it touched upon the type interfaces used to declare options.

danbrianwhite · 2018-05-08T16:10:07Z

I would also like to use the "hidden" check feature as this resolves my issues. Any thoughts on when the update will be merged and pushed to NPM?

icd2k3 · 2018-05-08T16:23:23Z

@danbrianwhite I should have time this week to do a first pass of this boolean option (new 👶 at home is making things a bit hectic 😄). Afterwards, it will have to go through normal code review cycle and approval / release which is up to @stipsan.

stipsan · 2018-05-08T21:19:47Z

@danbrianwhite once the changes we've agreed upon is implemented we'll merge the PR and the CI will publish to NPM right away. I gotta say I love Semantic Release for making this so automatic and painless 😄 Don't even need access to my laptop, if I can merge I can publish 🚀

@icd2k3 Congrats on the 👶 🎉!

I'm willing to fast track this as I expect many will need this ability.

By fast tracking I mean that I won't require new tests to be added for it, only that existing tests don't break.
These two specifically:

These tests are executed by using a reverse proxy here: https://github.com/stipsan/scroll-into-view-if-needed/tree/master/tests/web-platform
There are no other tests yet, not even for the boundary or behavior: Function features.
I'll get around to do that during the weekend. After that's in place I'll start expecting reasonable test coverage on PR contributions.

There's a few scenarios that the web-platform tests don't cover that I'd also like to add while at it:

testing scrolling boxes within scrolling boxes, like in the Codepen.
overflow: hidden without overflowing content.
scrolling inside iframes (same-origin and cross-origin).
ensure CSS borders with various widths behave correctly.
watch out for position: fixed and position: sticky quirks.
CSS zoom edge cases.
CSS transforms like rotate and 3D, make sure it doesn't break.
handle writing-mode correctly, like RTL.

Basically stuff that is really frustrating to face when you just want to scroll a thing into view :P

icd2k3 · 2018-05-09T06:02:30Z

A naive first pass of this feature is committed. Let me know if this is along the lines of what you were thinking @stipsan. Happy to make edits at your direction!

All of the current automated tests are passing:

I can also write a new test as a follow-up.

However, I still have not tested the changes locally, or on codepen (by the way, the link you posted goes to the semantic release repo - can you edit the link?). Going through that checklist seems like a good idea before merge consideration.

stipsan · 2018-05-09T08:25:41Z

This is perfect 👌

I updated the link to the CodePen. The logo animation on the website is the first thing that would break if the there’s a regression in the PR. If you can yarn link it in and verify it works then I think we’re good to go 😄

stipsan

Oops, dunno why the rest if the review got left out. Retrying

stipsan · 2018-05-09T08:09:04Z

src/types.ts

@@ -3,12 +3,16 @@ export type ScrollBehavior = 'auto' | 'instant' | 'smooth'
 export type ScrollLogicalPosition = 'start' | 'center' | 'end' | 'nearest'
 // This new option is tracked in this PR, which is the most likely candidate at the time: https://github.com/w3c/csswg-drafts/pull/1805
 export type ScrollMode = 'always' | 'if-needed'
+// New option that skips auto-scrolling all nodes with overflow: hidden set


Good work on making sure things are well documented 👍

stipsan · 2018-05-09T08:11:44Z

README.md

@@ -250,6 +250,16 @@ scrollIntoView(target, {
 })
 ```

+#### skipOverflowHiddenElements
+
+Type: `Boolean`


It would make sense to add that it’s false by default, like block is doing :)

stipsan · 2018-05-09T08:16:43Z

README.md

+
+By default the [spec](https://drafts.csswg.org/cssom-view/#scrolling-box) states that `overflow: hidden` elements should be scrollable because it has [been used to allow programatic scrolling](https://drafts.csswg.org/css-overflow-3/#valdef-overflow-hidden). This behavior can sometimes lead to [scrolling issues](https://github.com/stipsan/scroll-into-view-if-needed/pull/225#issue-186419520) when you have a node that is a child of an `overflow: hidden` node.
+
+This package follows the convention [adopted by Firefox](https://hg.mozilla.org/integration/fx-team/rev/c48c3ec05012#l7.18) of setting a boolean option to _not_ scroll all nodes with `overflow: hidden` set.


Nice level of detail on the documentation here 😄

chore(deps): update dependency lint-staged to v7.1.0 (scroll-into-view#224) chore(deps): update dependency lint-staged to v7.1.0 (scroll-into-view#224) fix(typings): Improve TS typings (scroll-into-view#223) * prepare to add flowtype tests * improve the quality of the typescript libdefs * Improve typescript declarations * improve further * update readme etc * Update index.ts chore(deps): update dependency flowgen to v1.2.2 (scroll-into-view#226) fix(compute): custom behavior lack block and inline defaults (scroll-into-view#227) fix(compute): custom behavior missing block and inline defaults chore(deps): update dependency semantic-release to v15.4.0 (scroll-into-view#229) chore(readme): I think about Jest… a LOT! (scroll-into-view#230)

icd2k3 · 2018-05-10T03:46:48Z

Thanks @stipsan!

The logo animation on the website is the first thing that would break if the there’s a regression in the PR. If you can yarn link it in and verify it works then I think we’re good to go 😄

Sorry, but I'm not following. When you have a moment could you please provide instructions on how to do this?

stipsan · 2018-05-10T18:05:10Z

Sure.
cd into your local checkout of scroll-into-view-if-needed and run yarn link.
cd into a local checkout of https://github.com/stipsan/scroll-into-view-if-needed-website and do yarn link scroll-into-view-if-needed.
Next run yarn dev and you'll be able to check http://localhost:3000 with the website running on the new version.

If this logo animation is still working then all is well:

I am going to setup the CI to do these kind of checks automatically, in the future 😄

icd2k3 · 2018-05-10T18:30:11Z

Sounds good - I should have time to verify this tonight. Thanks!

icd2k3 · 2018-05-11T03:18:22Z

Confirmed (gif below)

stipsan · 2018-05-11T14:24:39Z

Awesome work! Time to release 🎉

stipsan · 2018-05-11T14:28:35Z

🎉 This PR is included in version 2.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

icd2k3 force-pushed the add-type-hidden-to-can-overflow-check branch 3 times, most recently from 320fecc to b381d07 Compare May 9, 2018 06:23

stipsan reviewed May 9, 2018

View reviewed changes

icd2k3 changed the title ~~Adds 'hidden' check to canOverflow function~~ Adds skipOverflowHiddenElements boolean option May 9, 2018

icd2k3 force-pushed the add-type-hidden-to-can-overflow-check branch from 2ecf48d to 1fed7e8 Compare May 10, 2018 03:41

stipsan merged commit 21cea6f into scroll-into-view:master May 11, 2018

stipsan mentioned this pull request Jul 18, 2018

Feature Request: ability to "continue" scrolling if needed scroll-into-view/smooth-scroll-into-view-if-needed#140

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds `skipOverflowHiddenElements` boolean option #225

Adds `skipOverflowHiddenElements` boolean option #225

icd2k3 commented May 7, 2018 •

edited

Loading

stipsan commented May 7, 2018

icd2k3 commented May 7, 2018

stipsan commented May 7, 2018

danbrianwhite commented May 8, 2018

icd2k3 commented May 8, 2018

stipsan commented May 8, 2018 •

edited

Loading

icd2k3 commented May 9, 2018

stipsan commented May 9, 2018

stipsan left a comment

stipsan May 9, 2018

stipsan May 9, 2018

stipsan May 9, 2018

icd2k3 commented May 10, 2018

stipsan commented May 10, 2018 •

edited

Loading

icd2k3 commented May 10, 2018

icd2k3 commented May 11, 2018

stipsan commented May 11, 2018

stipsan commented May 11, 2018


		By default the [spec](https://drafts.csswg.org/cssom-view/#scrolling-box) states that `overflow: hidden` elements should be scrollable because it has [been used to allow programatic scrolling](https://drafts.csswg.org/css-overflow-3/#valdef-overflow-hidden). This behavior can sometimes lead to [scrolling issues](https://github.com/stipsan/scroll-into-view-if-needed/pull/225#issue-186419520) when you have a node that is a child of an `overflow: hidden` node.

		This package follows the convention [adopted by Firefox](https://hg.mozilla.org/integration/fx-team/rev/c48c3ec05012#l7.18) of setting a boolean option to _not_ scroll all nodes with `overflow: hidden` set.

Adds skipOverflowHiddenElements boolean option #225

Adds skipOverflowHiddenElements boolean option #225

Conversation

icd2k3 commented May 7, 2018 • edited Loading

stipsan commented May 7, 2018

Time for some back story!

And I must confess

We're between a rock and a hard place

The ideal solution

icd2k3 commented May 7, 2018

stipsan commented May 7, 2018

danbrianwhite commented May 8, 2018

icd2k3 commented May 8, 2018

stipsan commented May 8, 2018 • edited Loading

icd2k3 commented May 9, 2018

stipsan commented May 9, 2018

stipsan left a comment

Choose a reason for hiding this comment

stipsan May 9, 2018

Choose a reason for hiding this comment

stipsan May 9, 2018

Choose a reason for hiding this comment

stipsan May 9, 2018

Choose a reason for hiding this comment

icd2k3 commented May 10, 2018

stipsan commented May 10, 2018 • edited Loading

icd2k3 commented May 10, 2018

icd2k3 commented May 11, 2018

stipsan commented May 11, 2018

stipsan commented May 11, 2018

Adds `skipOverflowHiddenElements` boolean option #225

Adds `skipOverflowHiddenElements` boolean option #225

icd2k3 commented May 7, 2018 •

edited

Loading

stipsan commented May 8, 2018 •

edited

Loading

stipsan commented May 10, 2018 •

edited

Loading