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

CSS ::marker does not support defining CSS variables #7376

Merged
merged 1 commit into from Dec 12, 2022

Conversation

karlcow
Copy link
Member

@karlcow karlcow commented Dec 9, 2022

52052a2

CSS `::marker` does not support defining CSS variables
https://bugs.webkit.org/show_bug.cgi?id=241566
rdar://problem/95551387

Reviewed by Tim Nguyen and Antti Koivisto.

This aligns WebKit with Firefox and Chrome. It adds
CSSPropertyCustom into isValidMarkerStyleProperty
and isValidCueStyleProperty. This also adds two new
Web Platform Tests for css marker.

* LayoutTests/imported/w3c/web-platform-tests/css/css-pseudo/marker-variable-computed-style-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-pseudo/marker-variable-computed-style.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-pseudo/marker-variable-expected.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-pseudo/marker-variable-ref.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-pseudo/marker-variable.html: Added.
* Source/WebCore/style/PropertyAllowlist.cpp:
(WebCore::Style::isValidMarkerStyleProperty):
(WebCore::Style::isValidCueStyleProperty):

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

eab5e35

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  πŸ§ͺ win
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug βœ… πŸ›  gtk βœ… πŸ›  wincairo
βœ… πŸ§ͺ webkitperl   πŸ§ͺ ios-wk2   πŸ§ͺ api-mac   πŸ§ͺ gtk-wk2
⏳ πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk1 ⏳ πŸ§ͺ api-gtk
βœ… πŸ›  tv βœ… πŸ§ͺ mac-wk2
βœ… πŸ›  tv-sim βœ… πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch βœ… πŸ§ͺ mac-wk2-stress
βœ… πŸ›  watch-sim

@karlcow karlcow self-assigned this Dec 9, 2022
@karlcow karlcow added the CSS Cascading Style Sheets implementation label Dec 9, 2022
Copy link
Member

@nt1m nt1m left a comment

Choose a reason for hiding this comment

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

Please add a reftest (WPT would be the preference).

@anttijk
Copy link
Contributor

anttijk commented Dec 9, 2022

A test would be cool.

@karlcow
Copy link
Member Author

karlcow commented Dec 9, 2022

Please add a reftest (WPT would be the preference).

@nt1m https://github.com/web-platform-tests/wpt/pull/37427/files
would that work.

I tried an automated test, but I can't extract the color with window.getComputedStyle (maybe anti-fingerprinting?)

@brandonmcconnell
Copy link

@karlcow I'm able to extract the colors for ::marker using window.getComputedStyle's second arg. Both should work in Safari as well once the fix has been added. Currently, a color value is extracted in both browsers, butβ€”due to this bugβ€”Safari shows rgb(0, 0, 0) for the second test, which should change once this fix is in place:

Hope this helps πŸ™‚

for (const li of document.querySelectorAll('li')) {
  const markerStyles = window.getComputedStyle(li, 'marker');
  const markerColor = markerStyles.color; // <-- the extracted color
}
Test Chrome Safari
1 Screenshot 2022-12-09 at 11 40 34 AM Screenshot 2022-12-09 at 11 43 23 AM
2 Screenshot 2022-12-09 at 11 40 43 AM Screenshot 2022-12-09 at 11 43 31 AM

@karlcow
Copy link
Member Author

karlcow commented Dec 10, 2022

@karlcow I'm able to extract the colors for ::marker using window.getComputedStyle's second arg.

doh! I realized my mistake this morning. OK. I have an automated test.
conundrum. Firefox fails the automated test, while still doing the right thing for the color.

Copy link
Member

@nt1m nt1m left a comment

Choose a reason for hiding this comment

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

Can you import the 2 tests you've added as part of this patch?

@karlcow karlcow changed the title CSS ::marker does not support defining CSS variables CSS ::marker/::cue does not support defining CSS variables Dec 11, 2022
@karlcow karlcow changed the title CSS ::marker/::cue does not support defining CSS variables CSS ::marker/::cue do not support defining CSS variables Dec 11, 2022
@karlcow karlcow changed the title CSS ::marker/::cue do not support defining CSS variables CSS ::marker does not support defining CSS variables Dec 12, 2022
@karlcow
Copy link
Member Author

karlcow commented Dec 12, 2022

Can you import the 2 tests you've added as part of this patch?

@nt1m DONE!

@nt1m nt1m added the merge-queue Applied to send a pull request to merge-queue label Dec 12, 2022
https://bugs.webkit.org/show_bug.cgi?id=241566
rdar://problem/95551387

Reviewed by Tim Nguyen and Antti Koivisto.

This aligns WebKit with Firefox and Chrome. It adds
CSSPropertyCustom into isValidMarkerStyleProperty
and isValidCueStyleProperty. This also adds two new
Web Platform Tests for css marker.

* LayoutTests/imported/w3c/web-platform-tests/css/css-pseudo/marker-variable-computed-style-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-pseudo/marker-variable-computed-style.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-pseudo/marker-variable-expected.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-pseudo/marker-variable-ref.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-pseudo/marker-variable.html: Added.
* Source/WebCore/style/PropertyAllowlist.cpp:
(WebCore::Style::isValidMarkerStyleProperty):
(WebCore::Style::isValidCueStyleProperty):

Canonical link: https://commits.webkit.org/257711@main
@webkit-commit-queue
Copy link
Collaborator

Committed 257711@main (52052a2): https://commits.webkit.org/257711@main

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

@webkit-early-warning-system webkit-early-warning-system merged commit 52052a2 into WebKit:main Dec 12, 2022
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Dec 12, 2022
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
6 participants