openui / open-ui

Maintain an open standard for UI and promote its adherence and adoption.
https://open-ui.org
Other
3.48k stars 186 forks source link

[SELECT] Should the search for parts pierce into shadow roots? #347

Open dandclark opened 3 years ago

dandclark commented 3 years ago

In #291 (and in the Chromium <selectmenu> prototype), I've used a flat tree traversal to describe how the custom <select> looks for its parts. This was useful because a search in the flat tree under the <selectmenu> includes both the <select>'s own shadow DOM, and the content slotted into its shadow from the light DOM.

But, using a flat tree traversal also means that content from nested shadows are included in the search for parts. For example:

<select>
    <div slot="button">
        <template shadowroot="open">
            <div part="button" id="is-this-a-select-part">
                Custom button?
            </div>
        </template>
    </div>
    <option>Thing 1</option>
    <option>Thing 2</option>
</select>

Should the <select> choose div#is-this-a-select-part as one of its parts? I think this is probably not what we want. Content in a nested shadow might be an internal part of some component that doesn't want to be exposed.

There is a related issue with nested custom <select>s:

<select>
    <select>
        <option>Thing 1</option>
        <option>Thing 2</option>
        <option>Thing 3</option>
    </select>
</select>

<select>
    <my-custom-element>
        <!-- These options are slotted into the default slot of <my-custom-element> -->
        <option>Thing 1</option>
        <option>Thing 2</option>
        <option>Thing 3</option>
    </my-custom-element>
</select>

A flat tree traversal would mean that both <select>s would assign the <option>s as parts, which is probably bad.

So I propose to avoid these issues by having <select> search for its parts in the following places:

  1. Elements in its own shadow tree.
  2. Content slotted into <slot> elements that are in its own shadow tree, except children of those elements that are slotted into another, nested shadow tree (avoiding the nested-<select> issue).

Maybe there's a spec term that encapsulates this sort of search, but I haven't found one yet.

Thoughts? Is there a use case that I'm missing for finding parts in nested shadow roots? Note, I've discussed <select> in this issue but it will apply to other customizable controls as well.

mfreed7 commented 3 years ago

Thanks for raising this issue. It is peripherally related to the "leakage" issue with the part attribute.

You mentioned in the Chromium issue that a proposed fix would be to simply skip over nested <selectmenu> or <select> elements in a "normal" flat tree traversal. That seems to me to solve this issue, and in a much simpler way than the proposal in the OP here. Defining a new tree traversal algorithm, which is effectively what you're proposing here, would be tricky and bug prone I think. But is there a problem you found with just skipping over nested <selectmenu>/<select> elements?

In your second example, using <my-custom-element> you didn't show what the shadow tree looked like. I can see two possibilities:

1. nested <selectmenu> inside shadow tree:

<selectmenu>
    <my-custom-element>
        <template shadowroot=open>
            <selectmenu><slot></slot></selectmenu>
        </template>
        <option>Thing 1</option>
        <option>Thing 2</option>
        <option>Thing 3</option>
    </my-custom-element>
</selectmenu>

2. something else

<selectmenu>
    <my-custom-element>
        <template shadowroot=open>
            <div><slot></slot></div>
        </template>
        <option>Thing 1</option>
        <option>Thing 2</option>
        <option>Thing 3</option>
    </my-custom-element>
</selectmenu>

Clearly, in case #1, the "Thing X" options should not be considered to be part of the outer <selectmenu>, because they're already part of the shadow tree's <selectmenu>. In case #2, I actually think the "Thing X" options should be considered as part of the outer <selectmenu>. Maybe you want a custom element that provides a customized <option> with an image and other markup, and you want to include that in "outer" <selectmenu>s.

In both of these cases, simply skipping over <selectmenu> or <select> in a flat tree traversal would seem to do the trick.

dandclark commented 3 years ago

I think I agree with what you've stated regarding the case of nested <selectmenu>/<select>. I'm still not sure about the issue of other nested shadow roots though. With a flat tree traversal, in this example the <selectmenu> reaches inside <my-custom-element>'s shadow and applies controller code to the <div part="button"> inside it.

<selectmenu>
    <my-custom-element slot="button">
        <template shadowroot=closed>
            <div part="button">My custom element's button</div>
            <div>Other stuff in my custom element</div>
        </template>
    </my-custom-element>
    <option>Thing 1</option>
    <option>Thing 2</option>
    <option>Thing 3</option>
</selectmenu>

This is a little different from your case #2 because the thing we're applying controller code is fully inside <my-custom-element>'s shadow, rather than being slotted in from outside. Applying controller code here seems suspicious to me because we're breaking <my-custom-element>'s usual expectation of encapsulation by reaching inside its shadow DOM and placing custom behaviors on one of its sub-elements.

But maybe that's actually OK, and we should just be clear that the part attribute (or whatever name we decide to use in https://github.com/openui/open-ui/issues/354) is one of those special attributes that can break shadow encapsulation for any element that it's applied to.

mfreed7 commented 3 years ago

Hmm, that's a good point. One (very slightly) mitigating factor would be the change of name from part to something more unique and descriptive. However, I do think it's not great to break <my-custom-element>'s expectations of encapsulation. I'm not sure how to achieve both things, though: expectations of encapsulation, but also the ability to provide a custom element that provides customized <option>s for an outer <selectmenu>. I hesitate to suggest this, but perhaps the shadow root needs to opt itself in to "exporting" parts? There must be a better way here, I'm just not sure what it is.

mfreed7 commented 3 years ago

So from today's OpenUI meeting, there was a desire to see what the flat tree representation looks like for some of the examples above, and a bit more about why I think we should use a flat tree traversal for locating custom control parts.

First, the examples:

1. nested <selectmenu> inside shadow tree:

<selectmenu>
    <option>This select menu contains a nested, custom select</option>
    <my-custom-select>
        <template shadowroot=open>
            <selectmenu><slot></slot></selectmenu>
        </template>
        <option>Thing 1</option>
        <option>Thing 2</option>
        <option>Thing 3</option>
    </my-custom-select>
</selectmenu>

The above DOM structure results in this flat tree structure:

<selectmenu>
    <option data-x=should_get_controller_code>This select menu contains a nested, custom select</option>
    <my-custom-select>
        <selectmenu>
            <slot>
                <option data-x=should_NOT_get_controller_code>Thing 1</option>
                <option data-x=should_NOT_get_controller_code>Thing 2</option>
                <option data-x=should_NOT_get_controller_code>Thing 3</option>
            </slot>
        </selectmenu>
    </my-custom-select>
</selectmenu>

I've annotated which <option> elements should or should not get controller code applied.

2. "fancy option" custom element

<selectmenu>
    <option>This is a "normal" option</option>
    <my-fancy-option>
        <template shadowroot=open>
            <style>option {} img {} etc {} </style>
            <option>
                <img src="fancy_option_icon.png">
                <slot></slot>
            </option>
        </template>
        This is a "fancy" option
    </my-fancy-option>
</selectmenu>

The above DOM structure results in this flat tree structure:

<selectmenu>
    <option data-x=should_get_controller_code>This is a "normal" option</option>
    <my-fancy-option>
        <style>option {} img {} etc {} </style>
        <option data-x=should_get_controller_code>
            <img src="fancy_option_icon.png">
            <slot>
                This is a "fancy" option
            </slot>
        </option>
    </my-fancy-option>
</selectmenu>

In both of these cases, I think it makes perfect sense to use the flat tree representation when trying to locate <option> elements or other parts for the custom control <selectmenu>, as long as we "stop" on other custom controls. That way the nested <selectmenu> in example #1 does not have its <option>'s connected to both <selectmenu>'s, even though it's a light-DOM child of the outer <selectmenu>. And in example #2, the inner <option> does get controller code, even though it is not a light-dom child of the outer <selectmenu>.

As mentioned in the meeting, where this will break is if a custom element uses an <option> element in its shadow dom, but doesn't expect that <option> to get special controller code when the custom element is nested inside a <selectmenu>. So at the meeting, I said I was concerned about compat, and that wasn't strictly right. As people pointed out, <selectmenu> is new, so there shouldn't be compat issues. But I'm concerned about composability with this new <selectmenu>. I want existing components to "just work" when nested inside a <selectmenu>. And that's why we need to think about what the controller code would do in this case.

dandclark commented 3 years ago

Thanks @mfreed7 for these examples, I'm definitely leaning towards flat tree traversal being the right way to go now.

The remaining case that I'm somewhat concerned about is one mentioned during the 6/17 meeting, involving a custom select that uses <option>. As you stated above, hopefully existing components would "just work" when nested in <selectmenu>, but here's a specific case where they wouldn't:

<selectmenu>
  <my-custom-select>
    <option>Thing 1</option>
    <option>Thing 2</option>
  </my-custom-select>
</selectmenu>

<my-custom-select> uses <option> just like the native <select>. This seems pretty reasonable so I suspect that there are some custom select implementations that do this. As we resolved during the meeting (notes), the <selectmenu> will apply controller code to <option> elements even when they are not labeled with an attribute. So in the example above, <selectmenu> will apply controller code to the <option>s used with <my-custom-select>.

As a specific example of how this would go wrong, consider the case:

  1. User opens the outer <selectmenu>'s dropdown.
  2. User clicks the <my-custom-select>'s button to open it.
  3. User clicks an <option> in the <my-custom-select>.

Expected: <my-custom-select>'s dropdown closes, and its value is set to the value of the clicked <option>. The <selectmenu> remains open. Actual: The outer <selectmenu>'s dropdown closes, and its value is set to the value of the clicked <option> (since the <option> got controller code from the <selectmenu>, including click handlers etc).

This seems kind of bad. But, nesting selects seems like a weird thing to do. So maybe it's fine if this doesn't work?

github-actions[bot] commented 2 years ago

There hasn't been any discussion on this issue for a while, so we're marking it as stale. If you choose to kick off the discussion again, we'll remove the 'stale' label.