-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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:
|
data-x="concept-select-pick">picking</span> an <code>option</code> will fire a corresponding <code | ||
data-x="event-keydown">keydown</code> or <code data-x="event-mouseup">mouseup</code> event. | ||
Calling <code data-x="dom-Event-preventDefault">preventDefault()</code> on either of these events | ||
will prevent the <span>pick an option</span> algorithm from running.</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 think it would be good to flush this out more as an actual algorithm. If we have to make changes here that would be quite hard as it stands today.
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 tried moving more of the paragraph into the algorithm. How does it look?
corresponding <code data-x="event-mousedown">mousedown</code> or <code | ||
data-x="event-keydown">keydown</code> event on the <code>select</code> element. Calling | ||
<code data-x="dom-Event-preventDefault">preventDefault()</code> on either of these events will | ||
prevent the picker from opening. Otherwise, the picker is opened.</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 too seems awfully informal. We generally don't talk about public APIs in the actual processing models.
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 preventDefault() with the canceled flag and I tried moving parts of it into an algorithm
source
Outdated
<p>If the <code>select</code> is being rendered as a <span>drop-down box</span> with <span>base | ||
appearance</span>, then the user agent should allow the user to <dfn | ||
data-x="select-open-picker">open the picker</dfn> given a corresponding <code>select</code> | ||
element <var>select</var> and a corresponding <code data-x="event-mousedown">mousedown</code> or | ||
<code data-x="event-keydown">keydown</code> event <var>event</var>: |
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.
The timing of this is pretty curious to me. We're checking the event's <span>canceled flag</span>
, which means we're expecting this to run after event listeners have run. But the hook during which the below algorithm runs is not really clear. Am I right in thinking it is just after event listeners are expected to fire? In that case, it sounds a lot like activation behavior, but not limited to click events. But from reading HTML, I guess that's how activation behavior is setup, since it allows for "the user to manually trigger elements that have activation behavior".
I haven't looked too too closely but I'm wondering if there is an opportunity to use activation behavior here and just filter out untrusted non-"mousedown"-or-"keydown" events... what is the Chromium implementation doing?
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're correct, this runs after like activation behavior after author event listeners are run. The way this differs from activation behavior is that this algorithm cannot be triggered by the author firing the relevant event, since @domenic suggested to do so here: #10762 (comment)
I haven't looked too too closely but I'm wondering if there is an opportunity to use activation behavior here and just filter out untrusted non-"mousedown"-or-"keydown" events... what is the Chromium implementation doing?
If we changed the DOM spec to run activation behavior for events besides click (I think the algorithm which runs activation behavior in the DOM spec is only running it for click), and at some point checked the trusted bit on the event as you alluded to, then yeah we could use activation behavior. It would certainly be a more specific way of specifying this behavior. Maybe it should be a new thing with a name other than "activation behavior" though, since we are listening to all kinds of events...? In chromium we call it "DefaultEventHandler". I'm also not sure how to suggest that user agents should fire a mousedown when opening the picker via a user gesture, or if that's important.
Would you be in favor of making this change to the DOM spec? How about you @domenic?
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's weird because the way https://html.spec.whatwg.org/C#activation reads almost kinda works for other events—given the whole "manual activation" paragraph—but I guess that's not actually what we want since it fires a synthetic click event and doesn't let you discriminate between the actual user actions that "triggered" activation. For example, if we wanted to activate <select>
with mousedown
but not "voice input" (??), we wouldn't be able to tell which user action got us to the <select>
's activation behavior by the time we wound there, I think. So yeah, maybe activation behavior isn't ideal here.
Separately, it'd allow user-fired events to trigger activation behavior, which I personally don't see a problem with. I understand it breaks "action vs. occurrence", and I was thinking about that when I wrote my original comment, but I kinda find that confusing anyways. I get it in principle. But the distinction between "it's bad to fire a click event and perform things from its listener" and "it's OK to call a method like click()
which happens to fire click
events, and perform things from its listener" seems arbitrary. You're always going to perform actions from event listeners, so the distinction relies on which came first: (a) the event, or (b) an occurrence that happens to fire events.
Anyways, it's good to know that "DefaultEventHandler" exists in Chromium, and that's essentially the timing your implementation would use that we can't quite express in specs yet. Because it is still performing actions in response to an event, it feels like it can break "action vs. occurrence" (nothing is stopping the event from being user-dispatched, and even then, user-dispatched events can still be in response to some higher-level "occurrence"), but I'm pretty sure I think that's fine here. It seems like "DefaultEventHandler" is what I'm asking to spec here, and since we don't have a way of expressing it in specs it feels like a blocker to me.
Either way, I think we have to go with activation behavior or some new timing hook like "DefaultEventHandler"; what we currently have I don't think guarantees interoperability, given the timing: we essentially want an internal event handler that runs after all user event handlers, but we're not saying so rigorously right now.
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.
If activation behavior works for other events besides click (is there an example of that anywhere?), and it doesn't make any distinctions between trusted events or not, then I guess we could do it and then check for the trusted flag on the event in our algorithm to make it not break action vs occurrence.
DefaultEventHandler in chromium is only called for trusted events except for the click event: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/events/event_dispatcher.cc;l=391-393;drc=c49c7cd1195fa960412b2863a92983a396045ff3
Not sure if that's actually written in the spec or not?
|
||
<ol> | ||
<li><p>A <dfn>select button slot</dfn>, which is a <code>slot</code> element. It is appended to | ||
the <code>select</code>'s <span>shadow root</span> as the first child. It is expected to take 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.
What does "take" mean 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.
"take" in this sentence means that the first child button element is slotted into the select button slot.
I originally had an algorithm that was more specific, but in response to anne's comment I made this use the same prose as the existing details element, which uses the word "take" 8e17138#r1834329824
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 )
/references.html ( diff )
/rendering.html ( diff )