Define optgroup base appearance rendering#12201
Define optgroup base appearance rendering#12201josepharhar wants to merge 9 commits intowhatwg:mainfrom
Conversation
The old text didn't explain that there is a shadow tree with an element which renders the label attribute, and that it goes away when the author provides a legend element. The UA styles for rendering the optgroup element with base appearance are still valid. Fixes whatwg#12199
annevk
left a comment
There was a problem hiding this comment.
I think we should also change <optgroup>.label so it actually reflects the legend element when there is one. Seems quite strange the API would not reflect all of this. But that's probably a separate change at this point with its own tests.
| <span>expected</span> to contain a copy of the text in the <code>optgroup</code>'s <code | ||
| data-x="attr-optgroup-label">label</code> attribute. If the <code>optgroup</code> has a child | ||
| <code>legend</code> element, then the <span>optgroup label element</span> is not | ||
| rendered.</p></li> |
There was a problem hiding this comment.
Why not use a slot with fallback for this? Does the legend element intentionally have different requirements than the button element. It can appear anywhere?
There was a problem hiding this comment.
I also found out today that this optgroup label element is expected to have padding?
There was a problem hiding this comment.
And in Chrome min-height is set to 0 but that should probably be auto? cc @nt1m
There was a problem hiding this comment.
Why not use a slot with fallback for this? Does the
legendelement intentionally have different requirements than thebuttonelement. It can appear anywhere?
It is supposed to be first, so yeah we could do slotting instead: https://html.spec.whatwg.org/multipage/form-elements.html#the-optgroup-element
I'll go ahead and make that change here, in chromium, and WPT.
I also found out today that this optgroup label element is expected to have padding?
Yes, I made it have the same style as the legend element when rendered with base appearance. If we made it a web exposed part-like pseudo-element, which would be awesome, then we could define that in the same style rule as the legend element. Since it isn't, at least not yet, I'll add a paragraph there which says what the expected styles are.
And in Chrome
min-heightis set to 0 but that should probably be auto? cc @nt1m
Ah, my bad I thought that 0 was the default value. I'll change it to something better like unset.
There was a problem hiding this comment.
Maybe we can first agree on the details? Since we’d have to change styles as well presumably. I’m not a 100% sold on either approach yet.
There was a problem hiding this comment.
Sure, how would you like to go about agreeing on the details?
| <code>option</code> <span data-x="option-base-appearance">is being rendered with base | ||
| appearance</span> and the <code>option</code>'s <code data-x="attr-option-label">label</code> | ||
| attribute is not set, then the <code>option</code> is <span>expected</span> to render all of its | ||
| children rather than by displaying its <span data-x="concept-option-label">label</span>.</p> |
There was a problem hiding this comment.
Ok, I replaced this with an internal shadow tree definition
Apparently min-height:0 is not the initial/default value that we should be using, so I am changing it to unset. Context: whatwg/html#12201 (comment) Change-Id: Id3af6326646851d31c614b0290f9feca3db5e91e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7630018 Commit-Queue: Joey Arhar <jarhar@chromium.org> Reviewed-by: Joey Arhar <jarhar@chromium.org> Reviewed-by: David Baron <dbaron@chromium.org> Cr-Commit-Position: refs/heads/main@{#1608241}
|
I think we want to make it so only the first legend of an optgroup renders right? Currently chromium renders all legends, and Safari Tech Preview renders only the first. Making the rest display: none seems reasonable (not sure if that is what Safari has done). Though saying that fieldset currently renders all legends you give it, it just only treats the first one specially. Maybe we should still render the legends but just not give them the special styles, specifically I don't htink they should get the optgroups font-weight bolder. |
Considering that we still render everything else that doesn't fit in the content model, I don't think we should add behavior to not render this case in particular - assuming that you're talking about base appearance, not native appearance.
The screenshot in your second comment seems to show this, and yeah it looks to me like firefox just hasn't gotten around to implementing this yet. |
|
It feels weird for the second one to be bold to me given it's nothing special it's just text at that point. But perhaps if it's unbold it'll look too much like an option. Idk the safari option feels better at least for auto appearance. But I'll defer to others. |
|
Listbox base appearance is not yet finalized, right? For listbox auto appearance I would expect "get an optgroup element's label" to determine the label. And only the For dropdown base appearance I think it's fine if multiple |
|
Listbox base appearance is in the spec fwiw. |
|
Should I add any text to this PR which says that in native/primtive appearance only one legend or label should be rendered? Or anything else from #12199 like explicitly saying that when options and optgroups are placed outside of a select element they render their contents like a normal element? |
|
If the aim is to fix that issue just defining the base appearance is insufficient. As customizable select introduced new features, we need to update the rendering text to account for those, including for native appearance. I would be okay with addressing the issue with several PRs, but then OP should not claim it fixes that issue. |
|
Ok, I changed the op to say that the issue is not fixed and filed bugs. |
| <p>An <code>optgroup</code> element is <dfn data-x="optgroup-base-appearance">rendered with base | ||
| appearance</dfn> if it has an <span>ancestor</span> <code>select</code> element, and the nearest | ||
| <span>ancestor</span> <span><code>select</code>'s <code>option</code>s are being rendered with | ||
| base appearance</span>.</p> |
There was a problem hiding this comment.
Why are we talking about option elements here? An optgroup can't have option element ancestors, can it? Can we reference the existing definition for nearest ancestor select element?
There was a problem hiding this comment.
I did this in order to reuse the algorithm which determines when the elements inside a select besides the option are being rendered with base appearance: https://html.spec.whatwg.org/multipage/rendering.html#select's-options-are-being-rendered-with-base-appearance
I removed the word option from the algorithm to make that more clear. How does it look now?
| <p>If an <code>optgroup</code> element is not being <span | ||
| data-x="optgroup-base-appearance">rendered with base appearance</span>, then it is | ||
| <span>expected</span> to be rendered by displaying the element's <span | ||
| data-x="concept-optgroup-label">label</span>.</p> |
There was a problem hiding this comment.
I think if we're not fixing this here we should add an XXX marker as this is clearly broken.
There was a problem hiding this comment.
ok, i added an XXX
| displaying its <span data-x="concept-option-label">label</span>.</p> | ||
| <p>If an <code>option</code> element is not being <span data-x="option-base-appearance">rendered | ||
| with base appearance</span>, then it is <span>expected</span> to be rendered by displaying the | ||
| result of <span>collect option text</span> given the <code>option</code> and true.</p> |
There was a problem hiding this comment.
I think we first have to define what it means for an option element to be rendered with base appearance as base/native appearance isn't a thing for option elements (aiui). So the same order as above for optgroup with probably the same set of comments.
If we also tackle option we need to update the PR to make that clear.
There was a problem hiding this comment.
So should I add a <p class="XXX"> saying that option elements need their native and primitive appearance to be specified?
There was a problem hiding this comment.
That probably has to happen as well then, yes.
My preferred solution would be that we just define it as part of select's native appearance, but it seems that's not possible given how Chromium does things here and maybe Gecko?
There was a problem hiding this comment.
I took another look and I'm not sure what the option element is missing that the optgroup element has with regards to base/native appearance in this PR. Could you elaborate on what is missing?
There was a problem hiding this comment.
For optgroup we start out with defining what it means to be rendered with base appearance. For instance, we require an ancestor select (though we need to make that more precise so it deals with intermediate hr elements and the like I think).
I'm not sure that is sufficient however as these don't really have base appearance through appearance: base. It's a result of nesting, right? So I'm not sure that makes them compatible with the appearance base infra.
| not have the <code data-x="attr-option-label">label</code> attribute, or the <code | ||
| data-x="attr-option-label">label</code> is set to an empty string, then the <span>option label | ||
| element</span> is <span>expected</span> to have its <span>'display'</span> property set to | ||
| 'none'.</p></li> |
There was a problem hiding this comment.
So why not have this as a child of the slot element and then determine whether we slot things into the slot or not based on the condition of the label attribute? I thought that was the direction we were heading in.
There was a problem hiding this comment.
Ok, I rewrote it to use slot assignment and a fallback instead
|
|
||
| <p>The following styles are <span>expected</span> to apply to the <span>optgroup label | ||
| element</span> when its parent <code>optgroup</code> element is being <span | ||
| data-x="optgroup-base-appearance">rendered with base appearance</span>:</p> |
There was a problem hiding this comment.
Should we just make these unconditional based on select optgroup?
There was a problem hiding this comment.
I'd prefer not to, in chromium we have different styles for this element in appearance:auto: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/resources/html.css;l=2307-2309;drc=fc92cc6dc0a63cab87ad5e4ff9d3b17cdd7df198



The old text didn't explain that there is a shadow tree with an element which renders the label attribute, and that it goes away when the author provides a legend element. The UA styles for rendering the optgroup element with base appearance are still valid.
Context: #12199
(See WHATWG Working Mode: Changes for more details.)
/rendering.html ( diff )