-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Define base appearance for <select> #10629
base: main
Are you sure you want to change the base?
Conversation
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'm missing the base appearance style sheet?
source
Outdated
|
||
<p>Whenever an <code>option</code> <var>option</var>'s <span | ||
data-x="concept-option-selectedness">selectedness</span> is set to true, run <span>maybe clone | ||
option into select button</span> given <var>option</var>.</p> |
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 seems weird. Normally changing members doesn't have weird side effects like this.
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 this to the send select update notifications algorithm
source
Outdated
|
||
<p>When a <code>select</code> is being rendered as a <span>drop-down box</span> with a <span>base | ||
appearance</span>, it is expected to render as if it has the following <span>shadow | ||
root</span>:</p> |
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 can't define a shadow root in terms of an HTML syntax fragment.
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 rewrote it without using HTML. How does it look?
source
Outdated
HTML:</p> | ||
|
||
<!-- TODO is it OK for me to use <ul>, <li>, and <p> like this even though it isn't an | ||
algorithm? --> |
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 the problem is really with how you define the shadow root. If you do that differently this will be different too. But you can use lists outside of algorithms for sure.
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 rewrote this and used it to replace the HTML. How does it look?
source
Outdated
|
||
<p>The <span>implicit anchor element</span> of the <span>select popover</span> element is the | ||
<code>select</code> element shadow host of the shadow host in which <span>select popover</span> | ||
resides.</p> |
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 can't parse this.
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 that was not worded well. I rewrote it, how does it look now?
source
Outdated
|
||
<li><p>Set <var>select</var>'s <span>select popover slot</span>'s <span>manually assigned | ||
nodes</span> to <var>otherChildren</var>.</p></li> | ||
</ol> |
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.
Why can't we describe this more similarly to the details
element?
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 replaced this algorithm with some prose at the definition of the slot elements to look more like the details element.
Thanks for the review! I just pushed a lot of changes.
I have a separate PR for that here: #10670 I can get rid of that PR and include it in this PR if you'd prefer. |
source
Outdated
<li><p>Return null.</p></li> | ||
</ol> | ||
|
||
<p>To <dfn>clone selected option into select button</dfn>, given a <code>select</code> element |
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 has a similar problem.
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.
fixed, but I didn't put a <code>
around "button" because there's not actually a button element present. I'm also open to different naming idea to avoid the word "button"
source
Outdated
<var>text</var>.</p></li> | ||
</ol> | ||
|
||
<p>The <span>activation behavior</span> of an <code>option</code> <var>option</var> is to run the |
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 the current Canary, option element selection seems to happen on pointerdown (or is it mousedown, anyhow -down), not when activating
(which is why it took a bit time to figure out why an anchor element inside an option doesn't get activated when clicked).
But click event is still dispatched. It is just dispatched to the common ancestor of the option and whatever happens to be under the pointer when the popup has been closed and pointerup dispatched.
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, this doesn't match the chromium implementation right now. I'll update this and the chromium implementation after it looks like there is more consensus here: #10762
I added paragraphs for opening the picker and moving focus between options |
from whatnot:
|
focused.</p> | ||
|
||
<p class="note">Implementations commonly allow the user to choose the next or previous option via | ||
the arrow-up and arrow-down keys, or pick the first or last option via the Home or End keys.</p> |
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.
Might be worth to mention also PageDown/Up
This PR defines the base appearance except for the UA stylesheet for customizable select as proposed in #9799
Base appearance is being defined in CSS here: w3c/csswg-drafts#10691
(See WHATWG Working Mode: Changes for more details.)
/form-control-infrastructure.html ( diff )
/form-elements.html ( diff )
/infrastructure.html ( diff )
/rendering.html ( diff )