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

Add hidden=until-found HTML attribute and beforematch event #7475

Merged
merged 68 commits into from
Mar 23, 2022

Conversation

@josepharhar
Copy link
Contributor Author

@emilio do you have any thoughts on this?
@domenic advice for how to get this to build would be greatly appreciated

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

This isn't the right structure. It's not a new attribute; it's a new value for an existing attribute.

So, you need to modify the https://html.spec.whatwg.org/#the-hidden-attribute section.

Importantly, you need to modify the definition of the attribute from being a boolean attribute to an enumerated attribute, as discussed in #6040.

Also, please make sure the spec builds correctly before sending the PR for review.

@josepharhar
Copy link
Contributor Author

This isn't the right structure. It's not a new attribute; it's a new value for an existing attribute.

Ok, I tried putting it within the attr-hidden section.

Importantly, you need to modify the definition of the attribute from being a boolean attribute to an enumerated attribute, as discussed in #6040.

I removed the sentence that says its a boolean attribute, and I didn't notice any other text that seemed contradictory to having 3 states. How does it look?

Also, please make sure the spec builds correctly before sending the PR for review.

My bad, I was having trouble with the definitions. It now builds locally on my machine, but on github it's saying that I'm doing something wrong with nested <ol> tags, but I don't really understand because I already made a very similar section for auto-expanding details: https://html.spec.whatwg.org/multipage/interactive-elements.html#ancestor-details-revealing-algorithm

source Outdated Show resolved Hide resolved
@mtrootyy
Copy link

Because "hidden" attribute at present is boolean attribute, "hidden" attribute can have a value of empty or "hidden".
Does this PR mean that the "hidden" attribute is no longer boolean attribute?
Does this PR mean that the "hidden" attribute can have a value of empty, "hidden" or "until-found"?

@domenic
Copy link
Member

domenic commented Jan 14, 2022

Yes, this is no longer a boolean attribute. It will become an enumerated attribute with three states, per #6040 (comment). ("hidden" is not a valid value, though the empty string is.)

source Outdated
indicates that the element is not yet, or is no longer, directly relevant to the page's current
state, or that it is being used to declare content to be reused by other parts of the page as
opposed to being directly accessed by the user. <span w-nodev>User agents should not render
elements that have the <code data-x="attr-hidden">hidden</code> attribute specified. This
requirement may be implemented indirectly through the style layer. For example, an HTML+CSS user
agent could implement these requirements <a href="#hiddenCSS">using the rules suggested in the
Rendering section</a>.</span></p>
Copy link
Member

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/ .

Copy link
Contributor Author

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:

  • the attribute is not set
  • the attribute is set to "until-found"
  • the attribute is set to anything else

Should I still say that it's an enumerated attribute anyway?

Copy link
Member

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.

Copy link
Contributor Author

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?

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated
noframes, param, rp, script, style, template, title {
display: none;
}

<span id="hiddenCSS" data-x="">[hidden]:not([hidden=until-found])</span> {
Copy link
Member

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] { ... } or el[hidden] { ... } in their stylesheets?

Copy link
Contributor Author

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.

Copy link
Member

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 at html.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.

Copy link
Contributor Author

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"...

Copy link
Member

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.

Copy link
Contributor

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-712

Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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)

Copy link
Member

Choose a reason for hiding this comment

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

Filed #7650

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@zcorpan
Copy link
Member

zcorpan commented Jan 20, 2022

@domenic

Yes, this is no longer a boolean attribute. It will become an enumerated attribute with three states, per #6040 (comment). ("hidden" is not a valid value, though the empty string is.)

That would make currently conforming documents using hidden="hidden" invalid. That seems unnecessary, hidden can continue to be an allowed value, no?

edit: from the spec

If the attribute is present, its value must either be the empty string or a value that is an ASCII case-insensitive match for the attribute's canonical name, with no leading or trailing whitespace.

https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#boolean-attributes

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated
[hidden=until-found] {
content-visibility: hidden;
}

embed[hidden] { display: inline; height: 0; width: 0; } <!-- because for legacy reasons it still needs to instantiate the plugin -->
Copy link
Member

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 (uses visibility: collapse; instead of display: none)

Copy link
Contributor Author

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 to height: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=efb59b0cefb13faf55cce3066809f11f99ab9ab8

The only reason I'd want to change this embed[hidden] rule is because display:inline doesn't work well with content-visibility:hidden, but I really don't think anyone is going to be using hidden=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 than none, which is also the goal of the fancy selector I have here. I'm ok leaving it as is.

Copy link
Member

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 would display: inline-block work better?

Copy link
Contributor Author

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

Copy link
Contributor Author

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)

@josepharhar
Copy link
Contributor Author

That would make currently conforming documents using hidden="hidden" invalid. That seems unnecessary, hidden can continue to be an allowed value, no?

My goal is to keep all of the same behavior with regards to the hidden attribute and element.hidden, except when you assign 'until-found' into it.

source Outdated Show resolved Hide resolved
@zcorpan
Copy link
Member

zcorpan commented Jan 25, 2022

That would make currently conforming documents using hidden="hidden" invalid. That seems unnecessary, hidden can continue to be an allowed value, no?

My goal is to keep all of the same behavior with regards to the hidden attribute and element.hidden, except when you assign 'until-found' into it.

Then the value "hidden" should be a valid keyword, in addition to the empty string, both mapping to the hidden state.

(See https://html.spec.whatwg.org/multipage/interaction.html#attr-autocapitalize for an example of an enumerated attribute with multiple values mapping to the same state.)

Maybe the prose could be something like this:

The hidden content attribute is an enumerated attribute which (does something). The keywords for this attribute and their state mappings are as follows:

Keyword State
The empty string hidden
hidden (same cell as above)
until-found hidden-until-found

The invalid value default is the hidden state. The missing value default is the not-hidden state.

@bathos
Copy link

bathos commented Mar 16, 2022

Thanks, missed that it was already nullable. And including a numeric type is (almost) like adjusting the order of steps 14 and 17 of union conversion - makes sense.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

LGTM with nits. Will merge tomorrow-ish unless @zcorpan spots something about the embed case which I misunderstood.

@@ -10405,7 +10407,7 @@ interface <dfn interface>HTMLElement</dfn> : <span>Element</span> {
[<span>CEReactions</span>] attribute DOMString <span data-x="dom-dir">dir</span>;

// <span>user interaction</span>
[<span>CEReactions</span>] attribute boolean <span data-x="dom-hidden">hidden</span>;
[<span>CEReactions</span>] attribute (boolean or unrestricted double or DOMString?) <span data-x="dom-hidden">hidden</span>;
Copy link
Member

Choose a reason for hiding this comment

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

I guess this would be slightly cleaner as (boolean or unrestricted double or DOMString)?, i.e. move the ? outside. It shouldn't have any normative impact.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried making this change to the IDL in chromium, but it actually ended up removing the null/undefined type from the type union... do you think the IDL really should be the same either way? Is there something wrong in chrome's IDL implementation?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I guess that is a bug in Chromium! Nullable unions are definitely supported in IDL... we should change the spec, IMO, and file a low-priority Chromium bug. In the meantime we can keep using the DOMString? version in the implementation since it won't be observably different.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

crbug filed: https://bugs.chromium.org/p/chromium/issues/detail?id=1307051
And I just pushed a commit to this PR which moves the question mark outside the parenthesis

source Show resolved Hide resolved
source Show resolved Hide resolved
content-visibility: hidden;
}

embed[hidden]:not([hidden=until-found i]) { display: inline; height: 0; width: 0; } <!-- because for legacy reasons it still needs to instantiate the plugin -->
Copy link
Member

Choose a reason for hiding this comment

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

This looks good! We should be sure this is tested. I'd also appreciate if @zcorpan had time to weigh in since I believe he was discussing hidden embeds earlier in the PR, but we don't need to block on that if he's busy.

@domenic
Copy link
Member

domenic commented Mar 16, 2022

Tests are written and can be reviewed and commented upon at:

We should be sure there are tests for the UA stylesheet. web-platform-tests/wpt#5625 discusses the general issue but we could instead do targeted tests. Basically:

  • Test that hidden="" and hidden=asdf on a div give display: none and leave content-visibility at its default value.
  • Test that hidden=until-found and hidden=UNTIL-foUNd on a div give content-visibility: hidden and leave display at its default value.
  • Test that hidden="" and hidden=asdf and hidden=until-found give display: inline; width: 0; height: 0; on an embed and leave content-visibility at its default value.
  • Test that hidden="" and hidden=asdf and hidden=until-found give the appropriate values for all special table elements: https://whatpr.org/html/7475/rendering.html#tables-2 and don't touch content-visibility.

@josepharhar
Copy link
Contributor Author

We should be sure there are tests for the UA stylesheet

I started a test here: https://chromium-review.googlesource.com/c/chromium/src/+/3529819

Unfortunately, I think I found some issues with hidden attributes on embeds and tables.

For embeds, it looks like the display:inline works as it says in the spec, but the width:0px and height:0px only apply if hidden is "yes" or "true", and only in webkit and chromium: https://jsfiddle.net/jarhar/a17exs2g/

For tables, I tried the first element in the user agent stylesheet from the spec, the colgroup element. I found that instead of applying display:table-column-group when hidden like the spec says, it does display:none in all browsers: https://jsfiddle.net/jarhar/74Lqx1uf/
Maybe the [hidden] should be removed from these table elements in the spec?

@zcorpan
Copy link
Member

zcorpan commented Mar 17, 2022

embed[hidden] style was specced in b0e1a3e to fix https://www.w3.org/Bugs/Public/show_bug.cgi?id=15133 (browsers impl predated the spec)

colgroup[hidden] style was specced in 27f2a83 - I can't find any relevant discussion for this change.

@pjetrucha
Copy link

Are you aware that normalize.css has declarations for [hidden] already:
https://github.com/necolas/normalize.css/blob/fc091cce1534909334c1911709a39c22d406977b/normalize.css#L347-L349

I have seen similar declarations in many projects and I wonder if this will not result in an unconscious opt-out?

@josepharhar
Copy link
Contributor Author

I have seen similar declarations in many projects and I wonder if this will not result in an unconscious opt-out?

Yes, that will result in an opt-out. Auto-expanding details has a similar issue where sites make details:not([open]) > * display:none as well, but that ended up actually being a useful way to opt-out for websites that didn't want the auto-expanding behavior. For this feature, hopefully developers would notice that hidden=until-found isn't working in this case, but I'm not sure how I would detect this scenario and add a console message for it or otherwise help developers with this. Do you have any suggestions?

@zcorpan
Copy link
Member

zcorpan commented Mar 17, 2022

Maybe you can identify the most common sources (like normalize.css) and file issues suggesting to replace with

[hidden=""], [hidden="hidden"] { display: none }

This would not hide elements with hidden=until-found in really old browsers that don't support the hidden attribute, but, maybe that's not an issue? Maybe the suggestion should be to remove the rule altogether, instead.

aarongable pushed a commit to chromium/chromium that referenced this pull request Mar 18, 2022
whatwg/html#7475 (comment)

Bug: 1307051
Change-Id: I458f0c74ac24a663a6504746e7bac15218a9a8ac
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3529820
Auto-Submit: Joey Arhar <[email protected]>
Reviewed-by: Domenic Denicola <[email protected]>
Commit-Queue: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#982876}
aarongable pushed a commit to chromium/chromium that referenced this pull request Mar 21, 2022
Most event handlers like this should be in GlobalEventHandlers:
whatwg/html#7475 (comment)

[email protected]

Bug: 1055002
Change-Id: I30fbf99b17db3703d8fb133bb3c581de2fa02141
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3530434
Reviewed-by: Vladimir Levin <[email protected]>
Commit-Queue: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#983452}
@josepharhar
Copy link
Contributor Author

[hidden=""], [hidden="hidden"] { display: none }

Wouldn't this be better?

[hidden]:not([hidden=until-found]) {
  display:none;
}

Then again, I just realized that now that we made "until-found" case insensitive, this selector wouldn't catch hidden=UNTIL-FOUND... is there a selector I can make that would capture all cases? Should we change this feature to be only lowercase "until-found" to address this?

@bathos
Copy link

bathos commented Mar 22, 2022

Then again, I just realized that now that we made "until-found" case insensitive, this selector wouldn't catch hidden=UNTIL-FOUND... is there a selector I can make that would capture all cases? Should we change this feature to be only lowercase "until-found" to address this?

Could "hidden" be safely added to the list of HTML attributes whose values CSS selectors must match case-insensitively? It seems like this list is meant for stuff like this, though I’m only inferring that and don’t know that for sure.

(It’s possible that web content could exist that’s relying on its sensitivity today, but this seems pretty unlikely given it was boolean before + the same could be said about other aspects of this change anyway.)

@domenic
Copy link
Member

domenic commented Mar 22, 2022

We're already using [hidden=until-found i] in the style sheet. The i makes it case-insensitive.

@zcorpan
Copy link
Member

zcorpan commented Mar 23, 2022

Could "hidden" be safely added to the list of HTML attributes whose values CSS selectors must match case-insensitively?

No, that list is not intended to have full coverage of attributes that are case-insensitive in HTML, it's intended to be web compatible. So new case-insensitive attributes should not be added. Also, hidden was already case-insensitive before this change, since it was a boolean attribute (hidden="HIDDEN" is valid).

@zcorpan
Copy link
Member

zcorpan commented Mar 23, 2022

Wouldn't this be better?

[hidden]:not([hidden=until-found]) {
  display:none;
}

If we were to ever extend the hidden attribute again with a new value, we'd have the same problem again.

As for case-insensitive value, the i flag can be used in author CSS as well, but in legacy browsers that don't support it the whole ruleset will be dropped. Since the rule is intended for old browsers (that don't support the hidden attribute), this is relevant.

@josepharhar
Copy link
Contributor Author

We're already using [hidden=until-found i] in the style sheet. The i makes it case-insensitive.

As for case-insensitive value, the i flag can be used in author CSS as well

I didn't know about this, thanks!

but in legacy browsers that don't support it the whole ruleset will be dropped. Since the rule is intended for old browsers (that don't support the hidden attribute), this is relevant

I see... it doesn't seem like there is a perfect option to update normalize.css with.

No, that list is not intended to have full coverage of attributes that are case-insensitive in HTML, it's intended to be web compatible. So new case-insensitive attributes should not be added. Also, hidden was already case-insensitive before this change, since it was a boolean attribute (hidden="HIDDEN" is valid)

Thanks for the info! It sounds like this PR doesn't need any updates. @domenic can we merge this?

@domenic
Copy link
Member

domenic commented Mar 23, 2022

We're waiting on tests for the UA stylesheet, per #7475 (comment), before we can merge.

@josepharhar
Copy link
Contributor Author

Tests for the UA stylesheet have been merged here: web-platform-tests/wpt#33221

@domenic domenic merged commit f5def65 into whatwg:main Mar 23, 2022
@domenic
Copy link
Member

domenic commented Mar 23, 2022

Awesome! I've merged the spec. Please file Gecko and WebKit bugs, and edit the original post with links to them.

@zcorpan
Copy link
Member

zcorpan commented Mar 23, 2022

I see... it doesn't seem like there is a perfect option to update normalize.css with.

Yeah. I think my proposal in #7475 (comment) strikes the right balance, as almost all cases will either use minimized syntax (<div hidden>) or the long form and lowercase (<div hidden="hidden">).

@josepharhar
Copy link
Contributor Author

Awesome! I've merged the spec. Please file Gecko and WebKit bugs, and edit the original post with links to them.

Done:

sideshowbarker added a commit to stevefaulkner/html-1 that referenced this pull request Apr 20, 2022
This error seems to have been introduced in f5def65,
whatwg#7475 — so it’s unclear why CI didn’t
catch it there.
sideshowbarker added a commit to stevefaulkner/html-1 that referenced this pull request Apr 20, 2022
This error seems to have been introduced in f5def65,
whatwg#7475 — so it’s unclear why CI didn’t
catch it there.
sideshowbarker added a commit to stevefaulkner/html-1 that referenced this pull request Apr 20, 2022
This error seems to have been introduced in f5def65,
whatwg#7475 — so it’s unclear why CI didn’t
catch it there.
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
whatwg/html#7475 (comment)

Bug: 1307051
Change-Id: I458f0c74ac24a663a6504746e7bac15218a9a8ac
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3529820
Auto-Submit: Joey Arhar <[email protected]>
Reviewed-by: Domenic Denicola <[email protected]>
Commit-Queue: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#982876}
NOKEYCHECK=True
GitOrigin-RevId: b7c96593694b8bef1d1da3bed54d377b60a47a53
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
Most event handlers like this should be in GlobalEventHandlers:
whatwg/html#7475 (comment)

[email protected]

Bug: 1055002
Change-Id: I30fbf99b17db3703d8fb133bb3c581de2fa02141
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3530434
Reviewed-by: Vladimir Levin <[email protected]>
Commit-Queue: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#983452}
NOKEYCHECK=True
GitOrigin-RevId: 695d94e6e2196d7fa86678a9a388fd679ef30fd4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements document conformance impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation
Development

Successfully merging this pull request may close these issues.