Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add hidden=until-found HTML attribute and beforematch event #7475
Add hidden=until-found HTML attribute and beforematch event #7475
Changes from 8 commits
30b0552
61a3a41
387c131
9d25907
d04e4d6
93e1841
8089440
12cbbda
ca8eb40
0d6b2eb
b411034
7aace23
65448e1
ca6d534
e725820
09fdbde
3dc6833
206a802
f7a1366
08adbe7
16eaebe
08635ea
e8d2b9a
e701e98
90e266c
81c9644
d18bd7b
9321fb7
63060cf
1cb11b2
05c2dbf
ec99d06
8fe8e25
642c996
732f694
ab5485b
0bb2751
be0e978
a065268
064105d
1466e3c
e380a50
a5f07b8
ad370de
bb761d0
95042f1
377a2d5
3b7158c
d043b77
5d51c96
24a2b8f
e0726f9
c955cf3
bfcb672
cb88ab9
39db0dc
ecd4a32
c173bd4
282af51
75621bd
f3456c2
ec6f978
16ceccc
383e976
291e33c
fa64e8c
c9bdf96
c885c5e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
You need to specify that the element is an enumerated attribute, per the comment in #6040 (comment). See other examples of enumerated attribute, and how they specify keywords and states, by searching for "enumerated attribute" in https://html.spec.whatwg.org/ .
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.
This doesn't seem like an enumerated attribute because instead of having a bunch of different relevant string values, it has these relevant states:
Should I still say that it's an enumerated attribute anyway?
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.
Enumerated attributes are exactly attributes with multiple states. "not set" = missing valid default state. anything else = invalid value default state.
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 see, thanks!
I made it an enumerated attribute with a table like the other enumerated attributes.
How does it look?
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.
Nit: put the
<p>
above in here? And maybe the<p>
before that as well? Doesn't seem to contain a requirement.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 moved the first
<p>
above into this<div>
.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.
It looks like this will change the specificity of the selector. I wonder if that will have bad consequences for pages that use selectors like
[hidden] { ... }
orel[hidden] { ... }
in their stylesheets?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.
In chromium, this is implemented as a presentational style, which means it doesn't have a CSS selector and is no problem in chromium. I believe that presentational styles have ultra low specificity/precedence. I'm not sure how hidden is implemented in other browsers.
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.
Oh, hmm. Is that true for
[hidden]
as well? I.e. Chrome doesn't respect the current spec which uses the user-agent stylesheet, instead it uses a presentational style? Looking athtml.css
I see that might be the case.Maybe we should try to straighten that out before building a new feature on top of it... Is there a way to distinguish presentational style vs. UA stylesheet for the selector
[hidden]
? I'm not sure I understand the differences enough to write a test... Is the answer any different for[hidden]:not([hidden=until-found])
?Looking at https://drafts.csswg.org/css-cascade-4/#cascade-origin I can't really tell in which situations it would matter whether something is a presentational hint vs. in the UA stylesheet... Maybe @zcorpan or @tabatkins could help out here.
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.
Rune implied in this comment that we can't put attribute selectors in html.css due to performance concerns. What I'm calling a "presentational style" based on the code is actually called an "Attribute style" in DevTools when you look at a
hidden
element.I guess there isn't a perfect way to represent this in html.css, but I think this is the closest we can get. Isn't this CSS just a suggestion anyway? The current spec text says that "This requirement may be implemented indirectly through the style layer"...
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.
Well, the distinction is potentially important. If there are testable differences between using presentational hints (spec concept)/presentation styles (Chromium concept) vs. the user agent stylesheet (spec concept)/html.css (Chromium concept), then that could lead to interop issues, where other browsers implement the spec, but Chromium does not, and then developers get different results in different browsers.
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.
Firefox also implements
hidden
as a presentational attribute in Firefox, see: https://searchfox.org/mozilla-central/rev/b0c5c3b9821c2f22193fd6e1e9f66032639da1a1/layout/style/res/html.css#711-712There 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.
@josepharhar it does exist in the spec, click the definition of "presentational hint" to see all uses of it:
https://html.spec.whatwg.org/multipage/rendering.html#presentational-hints
Changing
hidden
to a preshint in the spec can be changed separately. Whether that is with a selector or with prose is entirely editorial.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.
Oh, awesome! I think we should definitely change
hidden
to a presentational hint in a separate PR.After looking at the text right above the hidden CSS, it says "Some rules are intended for the author-level zero-specificity presentational hints part of the CSS cascade; these are explicitly called out as presentational hints."
Does this mean that we can just add a sentence above the hidden CSS that says the hidden CSS is a presentational hint?
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.
Yes. See examples linked in #7475 (comment)
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.
Filed #7650
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.
Does this need changes for
until-found
?Also see the Tables section in the rendering section for more special styling for
hidden
(usesvisibility: collapse;
instead ofdisplay: none
)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.
Interesting, it looks like chromium's implementation doesn't do
display:inline
but does toheight:0; width:0
: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_embed_element.cc;l=81-90;drc=efb59b0cefb13faf55cce3066809f11f99ab9ab8The only reason I'd want to change this
embed[hidden]
rule is becausedisplay:inline
doesn't work well withcontent-visibility:hidden
, but I really don't think anyone is going to be usinghidden=until-found
directly on an embed element anyway. I'm ok keeping this as is.Looking at the tables section, it's similar: the hidden attribute there sets
display
to things other thannone
, which is also the goal of the fancy selector I have here. I'm ok leaving it as is.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.
Should
<embed hidden="until-found">
be disallowed (for web content)? Or woulddisplay: inline-block
work better?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.
ok, I added a
:not(hidden=until-found)
to embed here.Tables still looks ok to me
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 see now that I should have done
:not(embed)
on[hidden=until-found]
instead: #7475 (comment)