| Summary: | [Meta] Code calling shadowAncestorNode() does not seem to consider nested ShadowDOM | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Shinya Kawanaka <shinyak> |
| Component: | DOM | Assignee: | Web Components Team <webcomponents-bugzilla> |
| Status: | RESOLVED CONFIGURATION CHANGED | ||
| Severity: | Normal | CC: | dominicc, morrita, rniwa, shinyak, tasak, tkent |
| Priority: | P2 | ||
| Version: | 528+ (Nightly build) | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| Bug Depends on: | 87343, 91589, 91824, 91966, 92060, 97869, 97871, 97872, 97875, 99544, 106528, 106533, 106534, 107220, 107222, 107624, 108287, 108302 | ||
| Bug Blocks: | 97279 | ||
|
Description
Shinya Kawanaka
2012-07-19 23:02:28 PDT
FYI: I have no plan to replace the remaining. Many instances of shadowAncestorNode() is in the editing area. If the remaining uses need to stay, should shadowAncestorNode be renamed to something more accurate? Or should we just close this bug? 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. @tkent: Is Comment 1 saying that _you_ don’t plan to replace calls to shadowAncestorNode in editing, or you don’t think _anyone_ should? 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. Anyone should, of course. I’m confused. Let‘s talk in person and update the bug after that. 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. (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. *** Bug 91589 has been marked as a duplicate of this bug. *** (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 (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. This function longer exists. |