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

[Shadow] activeElement behavior seems to break encapsulation #358

Closed
bicknellr opened this Issue Dec 21, 2015 · 46 comments

Comments

Projects
None yet
5 participants
@bicknellr

bicknellr commented Dec 21, 2015

It seems strange for a ShadowRoot's activeElement to point to elements outside of that ShadowRoot. In particular, the spec currently seems to allow a ShadowRoot's activeElement to point not only to elements within the ShadowRoot but also elements in the tree that the ShadowRoot's host participates in (and possibly even further outwards).

I think it would be more reasonable if activeElement of a ShadowRoot could only point to an element contained within that ShadowRoot (or be null if none was focused). Additionally, if the 'deepest' focused node (i.e. the focused, UA-implemented element without a ShadowRoot) was nested further within descendant ShadowRoots, any given ShadowRoot's activeElement along that chain would point to the most shallow host of that chain contained within that ShadowRoot.

One implication of this approach is that determining focused descendants of the ShadowRoot's host means checking activeElement of the Document / ShadowRoot containing that host. This situation seems to break encapsulation by implying that the code controlling the ShadowRoot / component needs to look at its surrounding tree. However, I would argue that it's preferable as descendants of the ShadowRoot's host (i.e. those outside of the ShadowRoot) should be the responsibility of the code controlling the tree in which those descendants (and the host) participate. Also, there's precedent for using a tree's root to determine focus within that tree given Document's activeElement property already works this way.

An option for providing the ShadowRoot with some insight into what non-shadow descendants of its host might be focused would be to have activeElement point at a slot when an element distributed to that slot (or descendant of) is focused. (Maybe even give slots activeElement that point to the focused non-shadow descendant? They seem a lot like ShadowRoot hosts with no descendants anyways.) I don't think this would really be necessary though, given that you could just check if the activeElement of the host's containing Document / ShadowRoot is contained within the host, but it might make working with focus of non-shadow tree descendants within a component easier.

In general, I think this would make the scope of responsibility of code watching focus of a component's nodes more closely mirror shadow tree boundaries.

(Also, I get the feeling that a ShadowRoot's encapsulation mode would somehow be relevant to this but I haven't been able to figure out anything about encapsulation modes from the spec other than that a ShadowRoot has one and it's either 'open' or 'closed'.)

@hayatoito

This comment has been minimized.

Member

hayatoito commented Jan 5, 2016

Thanks. Let me add an example, which came from our previous internal discussion, slightly modified so that it uses slot elements:

Example

document tree: (Updated: I fixed all wrong indentation and missing closing tags)

<body>
    <focusable1></focusable1>
    <x-foo>
        <focusable2 slot="slot1"></focusable2>   // This is assigned to a slot
        <focusable3></focusable3>   // This is not assigned to any slots
    </x-foo>
</body>

x-foo's shadow tree:

  <slot name="slot1"></slot>  // <- focusable2 is assigned to this slot
  <focusable4></focusable4>

A) The behavior what the current spec says:

  1. When focusable1 is focused:
    • a) document.activeElement -> focusable1
    • b) x-foo.shadowRoot.activeElement -> focusable1
      (This spec bug was introduced when I refactored the spec.. My bad. My original intention was null in this case. Blink returns null in this case)
  2. When focusable2 is focused:
    • a) document.activeElement -> focusable2
    • b) x-foo.shadowRoot.activeElement -> focusable2
  3. focusable3 can not be focused. Thus, let's skip this case.
  4. When focusable4 is focused:
    • a) document.activeElement -> x-foo
    • b) x-foo.shadowRoot.activeElement -> focusable4

B) The proposed change, as far as I understand correctly:

  1. When focusable1 is focused:
    • a) document.activeElement -> focusable1
    • b) x-foo.shadowRoot.activeElement -> null
  2. When focusable2 is focused:
    • a) document.activeElement -> focusable2
    • b) x-foo.shadowRoot.activeElement -> null
  3. focusable3 can not be focused. Let's skip this case.
  4. When focusable4 is focused:
    • a) document.activeElement -> x-foo
    • b) x-foo.shadowRoot.activeElement -> focusable4

I do not have a strong opinion. I'm fine with B. It looks very simple.
In a world of B, we do not need to use a composed tree to determine shadowRoot.activeElement. We can tell activeElement only by looking at a static structure of tree of trees.

Does someone have any use cases where web developers would have trouble with B??
If there is none, I'm okay to update the spec with B.

@hayatoito

This comment has been minimized.

Member

hayatoito commented Jan 29, 2016

I think we disused the desired behavior a couple of weeks ago with Polymer team.

@TakayoshiKochi
Could you summarize the result here?

@TakayoshiKochi

This comment has been minimized.

Member

TakayoshiKochi commented Feb 2, 2016

Here it is:

For activeElement, we should calculate as tree-of-trees basis, (the option "B" in @hayatoito 's comment above) instead of current evnet-path based calculation. Even when a distributed element is focused, shadow tree's shadow root should return null for acrtiveElement.

In the meantime, if any element in a shadow tree gets focused (from outside the shadow tree), its shadow host should get focus event rather than the event bubbling is scoped at its shadow root.

That way a "focus" event listener on a shadow host can notice when its shadow content gets focused and also dig into which element gets focused by using its shadow root's acitveElement.

@rniwa

This comment has been minimized.

Contributor

rniwa commented Feb 2, 2016

Why don't we make x-foo.shadowRoot.activeElement point to the slot to which focusable2 is assigned in case 2? That'd make it clear to the containing shadow root where the focus is without having to listen to each focus event.

@TakayoshiKochi

This comment has been minimized.

Member

TakayoshiKochi commented Feb 3, 2016

Yeah, x-foo.shadowRoot.activeElement could point to slot to which focusable2 is assigned, just like document.activeElement is adjusted to the shadow host which contains a real focused element in its shadow.

But the point is that we want "when .activeElement changes, please notify the pointed element via focus/blur event" behavior.

The second paragraph in my last comment was confusing. Let me explain more.
@bicknellr @sorvell @kevinpschaaf if you remember the rationale for the focus event behavior more,
feel free to chime in.

This behavior is for consistency between document.activeElement and ShadowRoot.activeElement about focus/blur event and activeElement value change

Without Shadow DOM, document.activeElement always points to a focused element if any,
and once the focus moves, a focus event is dispatched and document.activeElement changes
to point the new focused element.

If we put an element with shadow DOM, and some element in the shadow gets focus,
document.activeElement points to the shadow host, but no element outside the
shadow root gets focus event. - This is the motivation for the behavior, to get focus event
to shadow host. If the focus move happened within the same shadow tree, the host do not
have to get focus event.

@TakayoshiKochi

This comment has been minimized.

Member

TakayoshiKochi commented Feb 3, 2016

So if we allow x-foo.shadowRoot.activeElement to point to slot, then we might get 2 focus events, one for focusable2 and the other for x-foo - which we may not want.

@hayatoito

This comment has been minimized.

Member

hayatoito commented Feb 3, 2016

It can not be allowed. That's the bottom-line, I think.

The reason is simple: a slot is not a focusable element. It never participates in the document composed tree.

{Document|ShadowRoot}.activeElement should point to a real element which exits in the document composed tree.

@bicknellr

This comment has been minimized.

bicknellr commented Feb 3, 2016

I think what Hayato said - that only focusable elements can be assigned to a Document / ShadowRoot's activeElement - makes sense / was the original rationale for not focusing slots.

This is probably outside the scope of this particular bug but it might still be worth considering if there should be a simple(r) way to determine more information about if / where a focused element deeper in the composed tree exists from within a ShadowRoot (without giving away information external to it). To my understanding, the event propagation path makes it possible to capture focus / blur events anywhere deeper in the composed tree than the node you're listening on (even if those events' targets are reprojected from somewhere shallower in the tree-of-trees). Given that, it's kind of odd that this information is first made available within the ShadowRoot but then lost after the event has stopped propagating. But, maybe it should be lost for the same reason as this change to activeElement?

@rniwa

This comment has been minimized.

Contributor

rniwa commented Feb 3, 2016

The reason is simple: a slot is not a focusable element. It never participates in the document composed tree.

Why?

I think what Hayato said - that only focusable elements can be assigned to a Document / ShadowRoot's activeElement - makes sense / was the original rationale for not focusing slots.

I don't see why that's any more rational than focusing the slot for the sake of developer ergonomics. Having to listen to focus/blur events to know where the focus resides is such an annoyance.

@bicknellr

This comment has been minimized.

bicknellr commented Feb 4, 2016

That sounds reasonable also. I don't have a strong opinion on this.

@bicknellr

This comment has been minimized.

bicknellr commented Feb 4, 2016

On a related note, can slots be styled? If so, would this have implications as to whether or not you could use :focus with it?

slot:focus {
  color: blue;
}
@rniwa

This comment has been minimized.

Contributor

rniwa commented Feb 4, 2016

You can't style by default because it's supposed to have the default style of display: contents. See the issue #308. You need to override display property.

@bicknellr

This comment has been minimized.

bicknellr commented Feb 4, 2016

Specifically, any CSS rules that would select a slot element act as if they do not?

@rniwa

This comment has been minimized.

Contributor

rniwa commented Feb 4, 2016

Well, there is no difference in the way CSS rules are applied so you should be able to get the computed style of a slot just like any other element.

@bicknellr

This comment has been minimized.

bicknellr commented Feb 4, 2016

Oh, I misunderstood your earlier comment / #308 then. So styling a slot is like any other element and the UA stylesheet in particular should contain slot { display: contents; } (i.e. no box is generated by default but display can be overridden)?

@hayatoito

This comment has been minimized.

Member

hayatoito commented Feb 4, 2016

In the current spec:

  • A slot does not participate in the composed tree.
  • Thus, slot can not be styled.

A good analogy for a slot is DocumentFragment, which will disappear once their role ended when the composition finished, in terms of rendering.

We discussed this topic in #308 and concluded we should not consider a slot being the composed tree until v2 because there is no browser support for {display: contents} and that will not happen soon.

@rniwa

This comment has been minimized.

Contributor

rniwa commented Feb 4, 2016

We're planning to add display: contents support as we speak. In fact, when that happens we may need to make #308 a v1 issue since we would be using that feature for slot elements.

@hayatoito

This comment has been minimized.

Member

hayatoito commented Feb 4, 2016

Can we continue the discussion in #308?
As #308 (comment) said, leaving slots in comopsed tree violates a fundamental assumption of Blink's engine.

@hayatoito

This comment has been minimized.

Member

hayatoito commented Feb 4, 2016

Even if a slot could be a focusable element, I prefer x-foo.shadowRoot.activeElement -> null to x-foo.shadowRoot.activeElement -> slot.

As @kochi said in #358 (comment), in terms of a document tree, that would cause two active elements in a document tree, focusable2 and x-foo, if we adapt what the following says. That will violate a lot of assumptions and what we would like to avoid, I think.

In the meantime, if any element in a shadow tree gets focused (from outside the shadow tree), its shadow host should get focus event rather than the event bubbling is scoped at its shadow root.

@bicknellr

This comment has been minimized.

bicknellr commented Feb 4, 2016

Thanks for the clarification on #308.

@rniwa

This comment has been minimized.

Contributor

rniwa commented Feb 4, 2016

As @kochi said in #358 (comment), in terms of a document tree, that would cause two active elements in a document tree, focusable2 and x-foo, if we adapt what the following says. That will violate a lot of assumptions and what we would like to avoid, I think.

Not at all. Just because ShadowRoot.activeElement point to a slot, it doesn't mean the document contains multiple active elements. There is a single focused/active element in the document, and each shadow root's activeElement just points to the slot containing that active/focused element.

That is, I'm not suggesting to focus the slot element when the slot contains a focused/active element. I'm simply suggesting to make ShadowRoot.activeElement return the slot that contained focused/active element without ever focusing the slot element. Whether the slot element should be focused (or should be focusable in the first place) or not is an orthogonal issue.

@TakayoshiKochi

This comment has been minimized.

Member

TakayoshiKochi commented Feb 4, 2016

@rniwa if a slot can be ShadowRoot.activeElement, what's the idiom to know the exact focused element?

// some pseudo code included
focused = root.activeElement;
if (focused.tagName == 'SLOT') {
    for (node in focused.getAssignedNodes()) {
         if (node is really focused)
             return node;
    }
}
return focused;

(Probably there's a better way than mine :)

Or is it enough that a component knows some element under <slot> is focused, and it doesn't care which element it is? If you have any good use case scenario, could you share?

@rniwa

This comment has been minimized.

Contributor

rniwa commented Feb 4, 2016

s/focused.tagName == 'SLOT'/focused instance HTMLSlotElement/ but that's the idea. We can also add Node.isFocused for better ergonomics.

@TakayoshiKochi

This comment has been minimized.

Member

TakayoshiKochi commented Feb 5, 2016

Hmm, without Node.isFocused, if we don't allow slot to be an active element,
we can know the real focused element by document.activeElement and recursive
host->shadowRoot.activeElement.

I don't still understand what's the point of allowing a slot to be an active element.
@rniwa, why it's a good idea?

@rniwa

This comment has been minimized.

Contributor

rniwa commented Feb 6, 2016

I don't still understand what's the point of allowing a slot to be an active element.
@rniwa, why it's a good idea?

In many UI components, it's important to know whether something inside the component is focused or not. e.g. toolbar-pane component may need to style itself differently when a text field inside is focused.

@TakayoshiKochi

This comment has been minimized.

Member

TakayoshiKochi commented Feb 8, 2016

{Document,ShadowRoot}.activeElement is meant to know which element has the focus, thus having to search through elements using isFocused sounds weird.

For styling in the toolbar-pane example, maybe using :focus pseudo class is not enough because you want to style non-focused parts differently when a specific part gets focus?

In that case you need to listen to focus event for the specific part then handle the styling in the listener.

<toolbar-pane>
  <#shadow>
    <button id="button">
    <slot name="inputs"></slot>
  </#shadow>
  <input id="x" slot="inputs" button-color="#333">
  <input id="y" slot="inputs" button-color="#ff0">
  <input id="z" slot="inputs" button-color="#0cc">
</toolbar-pane>

Let's say #button changes its color according to the focused node's attribute.
To do that synchronously when any of the <input>s above gets newly focused, you need to have focus event listener for these, because <slot> never gets focused. And that's enough and you don't need ShadowRoot.activeElement point to <slot>.

If you are saying that you need an API to determine if focus is inside the component, including distributed nodes in O(1) way, it might be okay. But then what is the notification mechanism that a distributed node gets focused for the component?

@hayatoito

This comment has been minimized.

Member

hayatoito commented Feb 8, 2016

My preference is option B in #358 (comment). I prefer keeping activeElement simple and understandable. I am not a fan of making it too magical one.

If we find a use case where we need an intelligent magical API, such as x-foo.shadowRoot.magicalActiveElement -> slot, we might want to provide such a magical API as another API, if it's difficult to implement in JS. However, developers still need a plain old activeElement anyway, I think. That's an orthogonal issue. Both cover different use cases. Thus, just making activeElement act a plain simple role could be a good starting point, I think.

I am afraid that it might be too early optimization to make activeElement too magical.

TakayoshiKochi added a commit to TakayoshiKochi/webcomponents that referenced this issue Feb 9, 2016

@rniwa

This comment has been minimized.

Contributor

rniwa commented Feb 9, 2016

@annevk @travisleithead : what are you opinions here?

@annevk

This comment has been minimized.

Member

annevk commented Feb 11, 2016

It seems hard to decide this if we don't have agreement on #308. This fundamentally depends on that I think. My impression was always that slots would not be part of the composed tree.

@TakayoshiKochi

This comment has been minimized.

Member

TakayoshiKochi commented Mar 25, 2016

After thinking of #358 (comment) more, I come to think if we have HTMLSlotElement.activeElement (new!) then it might be okay for ShadowRoot.activeElement to a slot within its shadow DOM.

I was imagining the following case, and the code is listening mosuedown event on shadow root, then uses ShadowRoot.activeElement to get where the focus is. If it returns null, the code has to check the exact focused element via document.activeElement and recursive ShadowRoot.activeElement, then check if the element is under its own slot.

<my-menu>
  #shadow-root
    <my-item>...</my-item>
    <my-item>...</my-item>
    <!-- optional items go here -->
    <slot></slot>
  #shadow-root-end
  <my-optional-item>...</my-optional-item>
  <my-optional-item>...</my-optional-item>
  <my-optional-item>...</my-optional-item>
</my-menu>

The concern is that to implement ShadowRoot.activeElement that way (returns slot), the UA native code has to do the similar search which may be a bit costly.

What do you think?

@rniwa

This comment has been minimized.

Contributor

rniwa commented Mar 26, 2016

This is why I think it's better for activeElement to point to the slot element. I don't think UA code matters much but I'd imagine some components care whether its slot contains a focused element or not.

@hayatoito

This comment has been minimized.

Member

hayatoito commented Mar 26, 2016

I was imagining the following case, and the code is listening mosuedown event on shadow root, then uses ShadowRoot.activeElement to get where the focus is.

This does not sound a good use case because:

  • mousedown event is inappropriate to know the change of the focus. You should use focusin event.
  • You can use focusin event's target property to know which element is focused. That should work in the given example, returning one of <my-optional-item>.

BTW, my preference is still option B. If we want to give developers an ability such as ShadowRoot.magicalActiveElement => slot, we can give it as another API, so that we do not break any assumption on existing APIs.

Remember that a slot is now always a focusable element, given that it does not generate CSS Box in default. We should honor that the return value from DocumetnOrShadowRoot.activeElement is a focusable element.

Please give us a concrete proposal how activeElement should be if a slot does not generate CSS BOX.

In addition that,
#358 (comment)

In the meantime, if any element in a shadow tree gets focused (from outside the shadow tree), its shadow host should get focus event rather than the event bubbling is scoped at its shadow root.

What happened to this?

Please give us a concrete proposal how ':focus' pseudo class and 'focus event' work and the relationships between activeElement.

@rniwa

This comment has been minimized.

Contributor

rniwa commented Mar 26, 2016

Since ShadowRoot is a new API, it should be fine to return a slot element in ShadowRoot.activeElement. I don't see why activeElement always need to return a focusable element either. The primary reason an author uses activeElement is to determine where the focus is. For that scenario, it's a lot more useful for activeElement to return the slot than null.

Furthermore, whether an element generates a CSS box or not is nothing to do with whether an element is focusable or not. For example, elements in the fallback contents of a canvas element can be focusable but they never generate CSS boxes.

@hayatoito

This comment has been minimized.

Member

hayatoito commented Mar 28, 2016

My point is that we should be extremely careful to give the existing API yet another meaning.

  • If there is a necessary reason, we should do it.
    • A good instance is: document.activeElement -> x-foo in 4-a in #358 (comment)
      This is necessary to explain a Web Platform's built-in element which may or may not use a shadow tree , or an element which uses an user-created closed shadow tree.
  • If the only reason for the change is The Change Looks Useful, be careful. We should refrain from changing the meaning of the existing API carelessly. The change might not be a local effect as we thought at first. That's the reason I asked a concrete proposal to evaluate the effects of a proposal.
    • A good instance is "in a document" concept. There had been a temptation to extend the meaning of "in a document", but I opposed to it.
    • Another good instance is "Node.contains". I remember that I opposed to an idea of making Node.contains() consider a shadow tree too. It sounds useful at first, but this would cause unpredictable side effects and breaks the assumption.

Okay, I've added how :focus pseudo class works in option B).

  1. When focusable1 is focused:
    • a) document.activeElement -> focusable1
    • b) x-foo.shadowRoot.activeElement -> null
    • c) :focus pseudo class can be applied to:
      • in a document tree: focusable1
      • in a shadow tree: none
  2. When focusable2 is focused:
    • a) document.activeElement -> focusable2
    • b) x-foo.shadowRoot.activeElement -> null
    • c) :focus pseudo class can be applied to:
      • in a document tree: focusable2
      • in a shadow tree: none
  3. focusable3 can not be focused. Let's skip this case.
  4. When focusable4 is focused:
    • a) document.activeElement -> x-foo
    • b) x-foo.shadowRoot.activeElement -> focusable4
    • c) :focus pseudo class can be applied to:
      • in a document tree: x-foo
      • in a shadow tree: focusable4

My proposal can satisfy the following expectations:

  • If documentOrShadowRoot.activeElement is null, ':focus' is never applied to its shadow host (if it exists). In other words, if documentOrShadowRoot.activeElement is non-null, ':focus' is always applied to the shadow host.
  • There is no divergence between a document tree and a shadow tree. DocumentOrShadowRoot.activeElement is always guaranteed to be a focusable element. Thus,
const previousFocusedElement = documentOrShadowRoot.activeElement;
....
previousFocusedElement.focus();  // This makes sure that previousFocusedElement gets focused again.

I did not realize that the fallback contents of a canvas is an exception, but I do not think we should spread out such a bad citizen any more.

I appreciate if someone give us a concrete proposal other than an option B, so that we can compare proposals in a broader perspective. Please include the formal definition or an algorithm for:

  • Input: the focused element and documentOrShadowRoot
  • Output what documentOrShadowRoot.activeElement returns
@rniwa

This comment has been minimized.

Contributor

rniwa commented Mar 28, 2016

If the only reason for the change is The Change Looks Useful, be careful. We should refrain from changing the meaning of the existing API carelessly.

There is no meaning or existing API since this is about ShadowRoot.

I did not realize that the fallback contents of a canvas is an exception, but I do not think we should spread out such a bad citizen any more.

It's not bad at all. It's HIGHLY desirable that fallback contents of a canvas element is focusable for accessibility purposes. In general, focusability of an element should be independent of whether it generates a CSS box or not for accessibility purposes.

The alternative proposal is as follows:

  • activeElement on ShadowRoot would return the focused element in the shadow tree that contains ShadowRoot or the slot through which a focused element is distributed (i.e. a focused element is in the result of the get distributed nodes algorithm.

In the sample,

  • When focusable1 is focused:
    • document.activeElement -> focusable1
    • x-foo.shadowRoot.activeElement -> null
    • :focus pseudo class can be applied to:
      • in a document tree: focusable1
      • in a shadow tree: none
  • When focusable2 is focused:
    • document.activeElement -> focusable2
    • x-foo.shadowRoot.activeElement -> slot1
    • :focus pseudo class can be applied to:
      • in a document tree: focusable2
      • in a shadow tree: none
  • When focusable4 is focused:
    • document.activeElement -> x-foo
    • x-foo.shadowRoot.activeElement -> focusable4
    • :focus pseudo class can be applied to:
      • in a document tree: x-foo
      • in a shadow tree: focusable4
@hayatoito

This comment has been minimized.

Member

hayatoito commented Mar 28, 2016

I guess such a definition is not what you want. Your definition is missing cases, I guess.

Suppose focusableX is focused in the following example, could you clarify what happens, and update the definition?

<body>
    <focusable1></focusable1>
    <x-foo>
      <focusable2 slot="slot1">
        <focusableX></focusableX> 
      </focusable2>
      <focusable3></focusable3>
    </x-foo>
</body>

x-foo's shadow tree: (updated after I got the reply :( )

  <x-bar>
    <div slot="slot2">
       <slot name="slot1"></slot>
    </div>
  </x-bar>
  <focusable4></focusable4>

x-bar's shadow tree:

   <slot name="slot2"></slot>
@rniwa

This comment has been minimized.

Contributor

rniwa commented Mar 28, 2016

In that example, x-foo.shadowRoot.activeElement would still point to slot with name=slot1. x-bar.shadowRoot.activeElement would also point to slot with name=slot2.

@hayatoito

This comment has been minimized.

Member

hayatoito commented Mar 28, 2016

Thanks. Now I understand your intention clearly, which matches what I expected. We need a formal definition for that, which is not difficult to define, I think.

BTW, I still prefer [(option B) + (New API)].

New API would tell us the exact focused element, instead of a slot.
e.g. When focusable2 is focused:
x-foo.shadowRoot.newMagicalActiveElement -> focusable2 (instead of a slot1)

@rniwa

This comment has been minimized.

Contributor

rniwa commented Mar 28, 2016

By "exact focused element", you mean the lowest shadow-inclusive ancestor of a focused element, which is unclosed to the shadow root?

If so, we should just make activeElement do that since the semantics of activeElement can be defined to whatever we'd like. If you're concerned about your v0 API in Blink, you can probably just have a differently behavior based on whether shadow root is created by v0 API or v1 API.

@hayatoito

This comment has been minimized.

Member

hayatoito commented Mar 28, 2016

By "exact focused element", you mean the lowest shadow-inclusive ancestor of a focused element, which is unclosed to the shadow root?

Yeah, that's the option A, I think. But #358 (comment) has a concern about activeElement having such a behavior. That's the reason why I am proposing option B (+ new APIs).

@hayatoito

This comment has been minimized.

Member

hayatoito commented Mar 28, 2016

@bicknellr, do you have any updates on this? Any thought?
I am afraid we are diverging from what you thought at first...

@bicknellr

This comment has been minimized.

bicknellr commented Mar 29, 2016

I still think that shadowRoot.activeElement shouldn't point to something outside that ShadowRoot: shadowRoot.contains(shadowRoot.activeElement) should be true unless shadowRoot.activeElement is null. Also, I now agree that changing the API of activeElement to include slots (even though this is a new context) probably isn't ideal because it seems natural to assume that activeElement always returns a focused element with a useful focus() method, if any. A new property sounds like a better way to handle finding the 'deep' (non-slot) active element. For the same reason I opened this issue, I think it this new property still shouldn't directly reference a node in an arbitrary ancestor tree of the ShadowRoot, but should work like the alternative proposal here instead.

Maybe this property could be called activePath (or activeDescendant)? Given that a slot acts as a 'Shadow DOM boundary' and can reference elements distributed within it, I think it makes sense to put a property of the same name on slot. Then, you could still follow this property from any ShadowRoot or slot and eventually reach the lowest focused shadow-inclusive ancestor that is contained within that ShadowRoot or slot's composed tree. (Assuming I've got the right understanding of 'lowest shadow-inclusive ancestor'..) For example:

function getDeepActiveElement(start) {
  let end = start.activePath;
  while (end && end.activePath) {
    end = end.activePath;
  }
  return end;
}

getDeepActiveElement would take a ShadowRoot or slot (start) and end up at the 'deep' focused element (end). end would always be a non-slot, focused descendant of start's composed tree, unless start.activePath was null. Additionally, if start is a slot, there's a nice similarity with activeElement where there's always 0 or 1 elements x of start.getAssignedNodes() for which x.contains(start.activePath) and 0 or 1 elements x of start.getAssignedNodes({flatten: true}) for which x.contains(end). However, I don't think it would be appropriate for anything other than Document, ShadowRoot, or slot (i.e. anything that could host a ShadowRoot) to have this property because getDeepActiveElement would then take you inside those hosts' ShadowRoots.

IMHO, ShadowRoot should only provide access to things within itself, with the notable exception of the reference back to its host. Slots seem like a better place to access things distributed into the ShadowRoot from outside, but these accesses should be restricted to the portion of the ancestor tree that was assigned to the slot. If someone wants build a component that reaches elements outside of a ShadowRoot's composed tree, it should be up to them to get there by going up through the component's host through something like parentNode and not down through the ShadowRoot or its slots. These APIs should lead you to the boundaries provided by ShadowRoot and slot, but then require you to explicitly step across those boundaries.

@TakayoshiKochi

This comment has been minimized.

Member

TakayoshiKochi commented Mar 31, 2016

Thanks @bicknellr for the updates in detail!

@rniwa, I don't buy the "ShadowRoot.activeElement is a new API and we can do anything
as we like" argument much. What @haytatoito put is not a concern about compatibility
for Blink's legacy implementation, but I understand that Document and ShadowRoot should
keep consistent API semantics as much as possible if they share the same interface, and
if they diverge, it gets confusing to users, error prone to implement etc. So the option B.

On the other hand, I understand it would be nice for a developer to have a way to know
if a shadow root has focus within or its slots' assigned descendants flat tree-wise.
How about having one in a different name for #358 (comment) or #358 (comment) ?

@rniwa

This comment has been minimized.

Contributor

rniwa commented Mar 31, 2016

I also don't buy that returning a slot would be violating the "API semantics".

@hayatoito

This comment has been minimized.

Member

hayatoito commented Mar 31, 2016

@bicknellr. Thank you. The idea looks promising. I like it.

I prefer:

  • For DocumentOrShadowRoot.activeElement: Option B.
  • Add new API: {DocumentOrShadowRoot or Slot}.activePath (A tentative name)

If we can agree on this idea, let me update the spec.

@hayatoito

This comment has been minimized.

Member

hayatoito commented Apr 5, 2016

Let me close this with the conclusion of Option B.
Regarding activePath, I have filed another issue, #479.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment