Skip to content

Rely on scrollIntoView() when doing feature detection of RTL behavior#188

Closed
fred-wang wants to merge 1 commit into
KingSora:masterfrom
fred-wang:issue-187
Closed

Rely on scrollIntoView() when doing feature detection of RTL behavior#188
fred-wang wants to merge 1 commit into
KingSora:masterfrom
fred-wang:issue-187

Conversation

@fred-wang

Copy link
Copy Markdown

The Chromium community is moving to standard behavior from the CSSOM
specification [1]. In order to launch the feature and analyze potential
broken URLs, they are counting pages setting the scrollLeft of a RTL
scroller to a positive value. Websites using OverlayScrollbars appear
in the top results and cause a lot of false positives [2]. In order to
workaround that issue, rely on Element.scrollIntoView() instead.

[1] https://www.chromestatus.com/feature/5759578031521792
[2] #187

The Chromium community is moving to standard behavior from the CSSOM
specification [1]. In order to launch the feature and analyze potential
broken URLs, they are counting pages setting the scrollLeft of a RTL
scroller to a positive value. Websites using OverlayScrollbars appear
in the top results and cause a lot of false positives [2]. In order to
workaround that issue, rely on Element.scrollIntoView() instead.

[1] https://www.chromestatus.com/feature/5759578031521792
[2] KingSora#187
@fred-wang

Copy link
Copy Markdown
Author

The non-jquery version has been tested with
https://people.igalia.com/fwang/OverlayScrollbars/old.html
https://people.igalia.com/fwang/OverlayScrollbars/new.html

On the following platforms:

  • Gecko (Firefox Linux)
  • WebKit (Epiphany Linux)
  • Blink (Chromium Linux)
  • EdgeHTML (Edge Windows)
  • Trident (Internet Explorer Windows)

PS: I'm not sure whether the post-cleanup is actually needed.

@fred-wang

Copy link
Copy Markdown
Author

I noticed that scrollIntoView has the side effect of scrolling the whole viewport and to make things safer, it is better to make the scroller at position: fixed; top: 0; left: 0.

@KingSora

Copy link
Copy Markdown
Owner

@fred-wang Thanks for your PR. Before I merge it, I'll also think of a suitable solution. May I ask you a question.. would it be allowed to set the scrollLeft value to something negative, without triggering a false positive?

@fred-wang

fred-wang commented Feb 17, 2020

Copy link
Copy Markdown
Author

@KingSora For an RTL scroller, the CSSOM standard says the scrollLeft coordinate must indeed be negative, so that's perfectly valid and would indeed not trigger Chromium's use counter. If you can find a feature detection relying on this, please let me know as I think that's an interesting alternative.

In any case, I believe this PR shouldn't be merged without adding the suggestion of #188 (comment)

@KingSora

KingSora commented Feb 17, 2020

Copy link
Copy Markdown
Owner

@fred-wang Yes, if I merge it, I would definitely add the CSS rules, to prevent of scrolling parent containers. My alternative solution would be: https://jsfiddle.net/o8pscgex/ can you please check if out and tell me if thats valid (I think it should be). The detectBehavior1 function is the old soluton and the detectBehavior2 is the new one.

@fred-wang

Copy link
Copy Markdown
Author

@KingSora Yes, detectBehavior2() looks correct to me. It's setting to a negative value out of the scroller boundary but just to be clear, that won't be counted by Chromium and so won't generate false positive. Thanks.

@KingSora

Copy link
Copy Markdown
Owner

@fred-wang Nice! Thank you very much for all your efforts and for contributing to this and the chromium project to make the web a better place! I'll close this PR and your issue for now. I'll implement this solution in the next patch version.

@fred-wang

Copy link
Copy Markdown
Author

@KingSora OK sounds good. Thanks for the kind words and for your help on this. Please keep me updated when your fix lands and your library is released again.

@KingSora KingSora closed this Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants