Bug 91821 – [Meta] Code calling shadowAncestorNode() does not seem to consider nested ShadowDOM

• Component: DOM
• Status: RESOLVED CONFIGURATION CHANGED

[Shinya Kawanaka]

We would like to remove Node::shadowAncestorNode(), since it is confusing due to returning itself if not in Shadow DOM.

[Kent Tamura]

FYI: I have no plan to replace the remaining.
Many instances of shadowAncestorNode() is in the editing area.

[Dominic Cooney]

If the remaining uses need to stay, should shadowAncestorNode be renamed to something more accurate?

Or should we just close this bug?

[Shinya Kawanaka]

Please don't close this.

I woud like to replace all, but the priority is low. Also, I have to read the code around shadowAncestorNode() when modifying shadowAncestorNode(), since it might contain a bug about nested Shadow DOM.

[Dominic Cooney]

@tkent: Is Comment 1 saying that _you_ don’t plan to replace calls to shadowAncestorNode in editing, or you don’t think _anyone_ should?

[Shinya Kawanaka]

I don't know why you think _anyone_ should not replace it...
Anyway, most of code having shadowAncestorNode() has some bug as far as I see. We have to fix them sooner or later. Now that shadowAncestorNode() is deprecated, we should use shadowHost() when we fix the bugs.

[Kent Tamura]

Anyone should, of course.

[Dominic Cooney]

I’m confused. Let‘s talk in person and update the bug after that.

[Hajime Morrita]

Just FYI, here is what I heard:

- Basically, shadowAncestorNode() is used in wrong way.
  We should replace/fix it correctly.
- Shinya noticed that the usage of shadowAncestorNode() is a 
  signal of unawareness of nested shadow trees.
  In many case, just replacing it to shadowHost() doesn't help much.
  We should inspect the code around and make it work properly instead of
  replacing it blindly. This is not so trivial.
- Kent doesn't have any plan to do it by himself since
  many usage appears outside his territory.
- Shinya has some idea about fixing them. He has no immediate plan to fix though
  since this issue is not yet prioritized well.

[Shinya Kawanaka]

(In reply to comment #8)
> Just FYI, here is what I heard:
> 
> - Basically, shadowAncestorNode() is used in wrong way.
>   We should replace/fix it correctly.
> - Shinya noticed that the usage of shadowAncestorNode() is a 
>   signal of unawareness of nested shadow trees.
>   In many case, just replacing it to shadowHost() doesn't help much.
>   We should inspect the code around and make it work properly instead of
>   replacing it blindly. This is not so trivial.
> - Kent doesn't have any plan to do it by himself since
>   many usage appears outside his territory.
> - Shinya has some idea about fixing them. He has no immediate plan to fix though
>   since this issue is not yet prioritized well.

The original motivation is Bug 87230 (merged to this already... but maybe this bug should have been merged to that.)

What morrita said is correct. Using shadowAncestorNode() is a very good signal to the unawareness of nested shadow trees. Sometimes it does not consider ShadowDOM itself well. As far as I see, most of code seems wrong. However, exposing a bug is not so trivial. shadowAncestorNode() is used in various code including editing, rendering, accessibility, event handler, etc..

Also, shadowAncestorNode() is deprecated, and we're using shadowHost() instead. So when we fix this kind of bugs, we have to consider using shadowHost() instead of shadowAncestorNode().

Anyway, I think the current bug title is confusing.

[Shinya Kawanaka]

*** Bug 91589 has been marked as a duplicate of this bug. ***

[Hajime Morrita]

(In reply to comment #9)
> Also, shadowAncestorNode() is deprecated, and we're using shadowHost() instead. So when we fix this kind of bugs, we have to consider using shadowHost() instead of shadowAncestorNode().

This is good point.

I don't think people outside Web Components team are aware of this.
We could prevent further usage of this API by renaming it to deprecatedSomething().
Such change will make the code looks ugly and motivate us to fix it.
http://www.joelonsoftware.com/articles/Wrong.html

[Shinya Kawanaka]

(In reply to comment #11)
> (In reply to comment #9)
> > Also, shadowAncestorNode() is deprecated, and we're using shadowHost() instead. So when we fix this kind of bugs, we have to consider using shadowHost() instead of shadowAncestorNode().
> 
> This is good point.
> 
> I don't think people outside Web Components team are aware of this.
> We could prevent further usage of this API by renaming it to deprecatedSomething().
> Such change will make the code looks ugly and motivate us to fix it.
> http://www.joelonsoftware.com/articles/Wrong.html

Yep. Let's do it first.

[Ryosuke Niwa]

This function longer exists.