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

Ignore ::-webkit-scrollbar styles when scrollbar-width is not auto #14440

Merged

Conversation

lukewarlow
Copy link
Member

@lukewarlow lukewarlow commented May 28, 2023

9649486

Ignore ::-webkit-scrollbar styles when scrollbar-width is not auto
https://bugs.webkit.org/show_bug.cgi?id=257052

Reviewed by Tim Nguyen and Simon Fraser.

RenderStyle::hasCustomScrollbarStyle() returns true if a custom style
for the scrollbar has been set via ::-webkit-scrollbar and the value of
scrollbar-width is auto (default). This replaces usages of
hasPseudoStyle(PseudoId::Scrollbar)
so the standard properties for styling scrollbars take precedence over
the non-standard.

WPT tests are included.

* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-color-001-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-color-001.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-color-002-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-color-002.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-color-003-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-color-003.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-color-004-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-color-004.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-color-005-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-color-005.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-width-010-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-width-010.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-width-011-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-width-011.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-width-012-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-width-012.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-width-013-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-width-013.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-width-014-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-width-014.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-width-015-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-width-015.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-width-016-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-width-016.html: Added.
* LayoutTests/platform/ios-wk2/TestExpectations:
* LayoutTests/platform/mac-wk1/TestExpectations:
* Source/WebCore/page/LocalFrameView.cpp:
(WebCore::LocalFrameView::rootElementForCustomScrollbarPartStyle const):
(WebCore::LocalFrameView::createScrollbar):
(WebCore::LocalFrameView::canShowNonOverlayScrollbars const):
(WebCore::LocalFrameView::styleHidesScrollbarWithOrientation const):
* Source/WebCore/page/LocalFrameView.h:
* Source/WebCore/rendering/RenderBox.cpp:
(WebCore::RenderBox::canUseOverlayScrollbars const):
* Source/WebCore/rendering/RenderLayerScrollableArea.cpp:
(WebCore::RenderLayerScrollableArea::createScrollbar):
* Source/WebCore/rendering/RenderListBox.cpp:
(WebCore::RenderListBox::createScrollbar):
* Source/WebCore/rendering/RenderMenuList.cpp:
(RenderMenuList::createScrollbar):
* Source/WebCore/rendering/RenderSearchField.cpp:
(WebCore::RenderSearchField::createScrollbar):
* Source/WebCore/rendering/style/RenderStyle.h:
* Source/WebCore/rendering/style/RenderStyleInlines.h:
(WebCore::RenderStyle::hasCustomScrollbarStyle const):

Canonical link: https://commits.webkit.org/265129@main

3475ee2

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv ❌ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@lukewarlow lukewarlow requested a review from cdumez as a code owner May 28, 2023 12:04
@lukewarlow lukewarlow self-assigned this May 28, 2023
@lukewarlow lukewarlow added the CSS Cascading Style Sheets implementation label May 28, 2023
@lukewarlow
Copy link
Member Author

WPT PR web-platform-tests/wpt#40276

@lukewarlow lukewarlow requested review from smfr and nt1m May 28, 2023 12:08
@lukewarlow lukewarlow force-pushed the scrollbar-width-override-pseudo branch from 41d5013 to d35e969 Compare May 28, 2023 12:16
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 28, 2023
@lukewarlow lukewarlow removed the merging-blocked Applied to prevent a change from being merged label May 28, 2023
@lukewarlow lukewarlow force-pushed the scrollbar-width-override-pseudo branch from d35e969 to 611dbab Compare May 28, 2023 15:15
@lukewarlow lukewarlow added the request-merge-queue Request a pull request to be added to merge-queue once ready label May 28, 2023
@webkit-ews-buildbot webkit-ews-buildbot added merging-blocked Applied to prevent a change from being merged and removed request-merge-queue Request a pull request to be added to merge-queue once ready labels May 28, 2023
@lukewarlow lukewarlow added request-merge-queue Request a pull request to be added to merge-queue once ready and removed merging-blocked Applied to prevent a change from being merged labels May 28, 2023
@webkit-ews-buildbot webkit-ews-buildbot added merging-blocked Applied to prevent a change from being merged and removed request-merge-queue Request a pull request to be added to merge-queue once ready labels May 28, 2023
@lukewarlow lukewarlow added request-merge-queue Request a pull request to be added to merge-queue once ready and removed merging-blocked Applied to prevent a change from being merged request-merge-queue Request a pull request to be added to merge-queue once ready labels May 29, 2023
@lukewarlow
Copy link
Member Author

Just putting what I put in slack here WRT this behaviour choice:

This is the behaviour Chrome has gone for in its experimental implementation so I've matched that for interop reasons.
I think also honouring the standard properties rather than non standard is better for Firefox compatability and more in keeping with how CSS generally works.

Iirc the ::-webkit-scrollbar CSS is considered a mistake. So this approach could allow its removal (aside from maybe something like display: none working) in future.

Without scrollbar-color being implemented this would lead to scenarios where there's un-themed scrollbars where previously there were but I personally would consider that acceptable. And also I intend to Implement scrollbar-color as well

@lukewarlow lukewarlow force-pushed the scrollbar-width-override-pseudo branch from 8fa8319 to 40ac705 Compare June 6, 2023 13:24
@lukewarlow lukewarlow requested a review from nt1m June 6, 2023 13:27
@lukewarlow lukewarlow added the request-merge-queue Request a pull request to be added to merge-queue once ready label Jun 6, 2023
@webkit-ews-buildbot webkit-ews-buildbot added merging-blocked Applied to prevent a change from being merged and removed request-merge-queue Request a pull request to be added to merge-queue once ready labels Jun 6, 2023
@lukewarlow lukewarlow removed the merging-blocked Applied to prevent a change from being merged label Jun 6, 2023
@lukewarlow lukewarlow force-pushed the scrollbar-width-override-pseudo branch from 40ac705 to 3475ee2 Compare June 13, 2023 09:18
@lukewarlow lukewarlow added the request-merge-queue Request a pull request to be added to merge-queue once ready label Jun 13, 2023
@webkit-ews-buildbot webkit-ews-buildbot added merging-blocked Applied to prevent a change from being merged and removed request-merge-queue Request a pull request to be added to merge-queue once ready labels Jun 13, 2023
@lukewarlow lukewarlow added request-merge-queue Request a pull request to be added to merge-queue once ready and removed merging-blocked Applied to prevent a change from being merged labels Jun 13, 2023
@Ahmad-S792 Ahmad-S792 added merge-queue Applied to send a pull request to merge-queue and removed request-merge-queue Request a pull request to be added to merge-queue once ready labels Jun 13, 2023
https://bugs.webkit.org/show_bug.cgi?id=257052

Reviewed by Tim Nguyen and Simon Fraser.

RenderStyle::hasCustomScrollbarStyle() returns true if a custom style
for the scrollbar has been set via ::-webkit-scrollbar and the value of
scrollbar-width is auto (default). This replaces usages of
hasPseudoStyle(PseudoId::Scrollbar)
so the standard properties for styling scrollbars take precedence over
the non-standard.

WPT tests are included.

* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-color-001-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-color-001.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-color-002-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-color-002.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-color-003-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-color-003.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-color-004-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-color-004.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-color-005-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-color-005.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-width-010-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-width-010.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-width-011-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-width-011.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-width-012-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-width-012.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-width-013-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-width-013.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-width-014-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-width-014.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-width-015-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-width-015.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-width-016-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-scrollbars/scrollbar-width-016.html: Added.
* LayoutTests/platform/ios-wk2/TestExpectations:
* LayoutTests/platform/mac-wk1/TestExpectations:
* Source/WebCore/page/LocalFrameView.cpp:
(WebCore::LocalFrameView::rootElementForCustomScrollbarPartStyle const):
(WebCore::LocalFrameView::createScrollbar):
(WebCore::LocalFrameView::canShowNonOverlayScrollbars const):
(WebCore::LocalFrameView::styleHidesScrollbarWithOrientation const):
* Source/WebCore/page/LocalFrameView.h:
* Source/WebCore/rendering/RenderBox.cpp:
(WebCore::RenderBox::canUseOverlayScrollbars const):
* Source/WebCore/rendering/RenderLayerScrollableArea.cpp:
(WebCore::RenderLayerScrollableArea::createScrollbar):
* Source/WebCore/rendering/RenderListBox.cpp:
(WebCore::RenderListBox::createScrollbar):
* Source/WebCore/rendering/RenderMenuList.cpp:
(RenderMenuList::createScrollbar):
* Source/WebCore/rendering/RenderSearchField.cpp:
(WebCore::RenderSearchField::createScrollbar):
* Source/WebCore/rendering/style/RenderStyle.h:
* Source/WebCore/rendering/style/RenderStyleInlines.h:
(WebCore::RenderStyle::hasCustomScrollbarStyle const):

Canonical link: https://commits.webkit.org/265129@main
@webkit-commit-queue webkit-commit-queue force-pushed the scrollbar-width-override-pseudo branch from 3475ee2 to 9649486 Compare June 13, 2023 18:51
@webkit-commit-queue
Copy link
Collaborator

Committed 265129@main (9649486): https://commits.webkit.org/265129@main

Reviewed commits have been landed. Closing PR #14440 and removing active labels.

@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jun 13, 2023
@webkit-commit-queue webkit-commit-queue merged commit 9649486 into WebKit:main Jun 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CSS Cascading Style Sheets implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants