-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Correct assorted issues with standalone pseudo elements #43784
Conversation
I discovered these issues while writing WebKit/WebKit#22231. Standalone pseudo-elements still need to be properly formatted. And ::placeholder is a valid pseudo-element. Closes w3c/csswg-drafts#9751.
The negative case of the |
let style = getComputedStyle( | ||
document.documentElement, ":view-transition"); | ||
document.documentElement, "::view-transition"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have a test that this is invalid, I know @karlcow had created a test about getComputedStyle + pseudo elements, so it could be added there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wpt/css/cssom/getComputedStyle-pseudo.html
Lines 161 to 175 in ee42dc1
test(function() { | |
let li = document.querySelector('li'); | |
assert_true( | |
getComputedStyle(li, ':marker').length == 0, | |
"Should return an empty style for :marker"); | |
assert_true( | |
getComputedStyle(li, 'marker').length != 0, | |
"Should return the element style for marker"); | |
assert_equals( | |
getComputedStyle(li, 'marker').color, "rgb(255, 0, 0)", | |
"Should return the element style for marker (ex: color is rgb(255, 0, 0), not rgb(0, 128, 0)"); | |
assert_equals( | |
getComputedStyle(li, '::marker').color, "rgb(0, 128, 0)", | |
"Should return the element style for marker (ex: color is rgb(0, 128, 0)"); | |
}, "Unknown pseudo-element with a known string (ex: marker)"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I also pointed to this above. I think that's okay for now, but we could add more coverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could change that test to loop over different pseudo elements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could probably cover all of them. That would make sure nothing bad is happening. With the loop being outside of the test. So we know which pseudo-element fails instead of failing everything if one is missing.
@@ -253,7 +253,6 @@ | |||
'before', | |||
':abc', | |||
'::abc', | |||
'::placeholder', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why this was considered invalid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think they previously prohibited -webkit-prefixed pseudo-elements and maybe this is a leftover of that, but not sure.
I discovered these issues while writing WebKit/WebKit#22231. Standalone pseudo-elements still need to be properly formatted.
And ::placeholder is a valid pseudo-element.
Closes w3c/csswg-drafts#9751.