Bug 91821 - [Meta] Code calling shadowAncestorNode() does not seem to consider nested ShadowDOM
Summary: [Meta] Code calling shadowAncestorNode() does not seem to consider nested Sha...
Status: RESOLVED CONFIGURATION CHANGED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Web Components Team
URL:
Keywords:
: 91589 (view as bug list)
Depends on: 87343 91589 91824 91966 92060 97869 97871 97872 97875 99544 106528 106533 106534 107220 107222 107624 108287 108302
Blocks: 97279
  Show dependency treegraph
 
Reported: 2012-07-19 23:02 PDT by Shinya Kawanaka
Modified: 2019-10-04 22:51 PDT (History)
6 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Shinya Kawanaka 2012-07-19 23:02:28 PDT
We would like to remove Node::shadowAncestorNode(), since it is confusing due to returning itself if not in Shadow DOM.
Comment 1 Kent Tamura 2012-07-24 23:46:18 PDT
FYI: I have no plan to replace the remaining.
Many instances of shadowAncestorNode() is in the editing area.
Comment 2 Dominic Cooney 2012-12-16 18:44:38 PST
If the remaining uses need to stay, should shadowAncestorNode be renamed to something more accurate?

Or should we just close this bug?
Comment 3 Shinya Kawanaka 2012-12-16 20:36:08 PST
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.
Comment 4 Dominic Cooney 2013-01-10 06:12:22 PST
@tkent: Is Comment 1 saying that _you_ don’t plan to replace calls to shadowAncestorNode in editing, or you don’t think _anyone_ should?
Comment 5 Shinya Kawanaka 2013-01-22 00:37:33 PST
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.
Comment 6 Kent Tamura 2013-01-22 00:44:03 PST
Anyone should, of course.
Comment 7 Dominic Cooney 2013-01-22 06:44:32 PST
I’m confused. Let‘s talk in person and update the bug after that.
Comment 8 Hajime Morrita 2013-01-22 17:16:50 PST
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.
Comment 9 Shinya Kawanaka 2013-01-22 18:37:18 PST
(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.
Comment 10 Shinya Kawanaka 2013-01-22 18:49:06 PST
*** Bug 91589 has been marked as a duplicate of this bug. ***
Comment 11 Hajime Morrita 2013-01-22 20:01:32 PST
(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
Comment 12 Shinya Kawanaka 2013-01-22 21:11:29 PST
(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.
Comment 13 Ryosuke Niwa 2019-10-04 22:51:42 PDT
This function longer exists.