Currently Position and Range are not allowed to have ShadowRoot as a container. However, this makes it difficult to point direct children of ShadowRoot. Position and Range try pointing direct children of ShadowRoot, but since it's not allowed, they become null. Since a lot of code does not expect Position and Range become null, a lot of crashes are caused when doing something on elements in shadow subtrees. We should try to allow Position and Range to have ShadowRoot as a container. Note that we should be careful not to expose a ShadowRoot in JS.
This is going to be a really patch to review. You basically need to manually audit the entire editing code to make sure nothing depends on the current behavior. When text from controls transitioned to use new shadow DOM code, there were quite few places where we had to make changes.
Created attachment 133987 [details] Patch
I support this approach. From a higher viewpoint, it's the only path to allow position on shadow root if we want to define certain types of positions. And we want to do it. Otherwise we'd have no way to define any position between shadow children. Veteran built-in elements avoid this ambiguousness by forming its shadow tree carefully and making tricky position impossible by giving -webkit-user-modify:none. But such an approach is fragile and can cause problems like http://wkb.ug/65738
Could someone review this?
Comment on attachment 133987 [details] Patch The patch makes sense to me, but I defer to Ryosuke, because he knows this area a lot better. Niwa-san, review? review? review? :)
Comment on attachment 133987 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133987&action=review How about all the places in VisibleSelection that call shadowAncestorNode and nonBoundaryShadowTreeRootNode? Editor::nextVisibleRange, Editor::rangeOfString, etc... call call some shadow-related functions. In short, I don't know how to review this patch. Unfortunately, having passed all existing tests is not a good enough indication that a given change is correct in editing. > Source/WebCore/dom/Position.cpp:94 > - ASSERT(!((anchorType == PositionIsBeforeChildren || anchorType == PositionIsAfterChildren) > - && (m_anchorNode->isTextNode() || editingIgnoresContent(m_anchorNode.get())))); > + ASSERT(!m_anchorNode.get() > + || (!((anchorType == PositionIsBeforeChildren || anchorType == PositionIsAfterChildren) > + && (m_anchorNode->isTextNode() || editingIgnoresContent(m_anchorNode.get()))))); Why are we making this change? As far as I know, we should never be making a position before/after children with a null container. > LayoutTests/ChangeLog:22 > +2012-03-26 Shinya Kawanaka <shinyak@chromium.org> > + > + Position should be able to have ShadowRoot as a container. > + https://bugs.webkit.org/show_bug.cgi?id=82021 > + > + Reviewed by NOBODY (OOPS!). > + > + * fast/dom/shadow/doubleclick-on-meter-in-shadow-crash.html: Added. > + * fast/dom/shadow/rightclick-on-meter-in-shadow-crash.html: Added. > + Duplicate entry.
Created attachment 134720 [details] Patch
(In reply to comment #3) > I support this approach. > > From a higher viewpoint, it's the only path to allow position on shadow root > if we want to define certain types of positions. And we want to do it. > Otherwise we'd have no way to define any position between shadow children. > > Veteran built-in elements avoid this ambiguousness by forming its shadow tree carefully > and making tricky position impossible by giving -webkit-user-modify:none. > But such an approach is fragile and can cause problems like http://wkb.ug/65738 Such a bug is caused by another issue. The reason of the bug is UA shadow elements is selectable. But the reason of this bug is elements are not selectable correctly. # I confirmed my patch doesn't fix http://wkb.ug/65738
I've extracted all code where Position::containerNode() is used. https://docs.google.com/a/chromium.org/spreadsheet/ccc?key=0Aty2DXLelNGhdFhuaWliQWFnNEY0blNtX2lLUTRDZHc&pli=1#gid=0 Maybe we should create explicit tests for them to prove this patch does not introduce vulnerability...
I've talked with rniwa and morrita in IRC, we have decided to add this patch behind the SHADOW_DOM flag. Fixing editing bugs in Shadow DOM is really necessary this patch. Most of them depends on this. So when this patch is landed, we can proceed fixing editing bugs one by one. This could be a great step.
Created attachment 140698 [details] Patch
Comment on attachment 140698 [details] Patch Attachment 140698 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12642640
Comment on attachment 140698 [details] Patch Attachment 140698 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12648292
Comment on attachment 140698 [details] Patch Attachment 140698 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12643642
Comment on attachment 140698 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140698&action=review > Source/WebCore/ChangeLog:16 > + Currently this change is only enabled if shadow dom flag is enabled, since we cannot > + prove this change does not destroy the existing behavior. However, this change is really required > + to fix other editing bugs in Shadow DOM. A bunch of patches and tests will be added to I am ok with this as a temporary measure, but we need to have a plan to move forward. At some point, we will turn Shadow DOM on by default :) > Source/WebCore/dom/Position.h:221 > +inline ContainerNode* findParent(const Node* node) A free-standing function with a generic name like this seems like a bad thing. Can it be a static member of Position?
Comment on attachment 140698 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140698&action=review Please fix all the nits below and builds before you land. > Source/WebCore/dom/Position.cpp:114 > + ASSERT(!m_anchorNode || !editingIgnoresContent(m_anchorNode.get())); If RuntimeEnabledFeatures::shadowDOMEnabled() is false, then m_anchorNode should not be a shadow root. >> Source/WebCore/dom/Position.h:221 >> +inline ContainerNode* findParent(const Node* node) > > A free-standing function with a generic name like this seems like a bad thing. Can it be a static member of Position? Yes, please make this a private static member of Position. I would also rename it to something like parent(). I don't think we want to prefix it with "find" because it doesn't really search for the parent. > Source/WebCore/dom/Position.h:227 > + // FIXME: Since enabling Position to have ShadowRoot as a container is not proved not to > + // break editing code. However, this is really required for editing in Shadow DOM. > + // So we implement this behind the flag. After writing a lot of tests for them to confirm > + // this is safe, we will remove the flag for this purpose. > + // See also this umbrella bugs. http://wkb.ug/82697 This comment just repeats what the code says. You should just say "See http://webkit.org/b/82697" IMO.
Comment on attachment 140698 [details] Patch Ugh... I meant r+, not r-.
Created attachment 140843 [details] Build test
Created attachment 140844 [details] Build test
Thank you for the reviewing! I've updated the patch to fix what you mentioned. After all the bots are green, I'll land this patch. (In reply to comment #16) > (From update of attachment 140698 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=140698&action=review > > Please fix all the nits below and builds before you land. > > > Source/WebCore/dom/Position.cpp:114 > > + ASSERT(!m_anchorNode || !editingIgnoresContent(m_anchorNode.get())); > > If RuntimeEnabledFeatures::shadowDOMEnabled() is false, then m_anchorNode should not be a shadow root. > > >> Source/WebCore/dom/Position.h:221 > >> +inline ContainerNode* findParent(const Node* node) > > > > A free-standing function with a generic name like this seems like a bad thing. Can it be a static member of Position? > > Yes, please make this a private static member of Position. I would also rename it to something like parent(). I don't think we want to prefix it with "find" because it doesn't really search for the parent. > > > Source/WebCore/dom/Position.h:227 > > + // FIXME: Since enabling Position to have ShadowRoot as a container is not proved not to > > + // break editing code. However, this is really required for editing in Shadow DOM. > > + // So we implement this behind the flag. After writing a lot of tests for them to confirm > > + // this is safe, we will remove the flag for this purpose. > > + // See also this umbrella bugs. http://wkb.ug/82697 > > This comment just repeats what the code says. You should just say "See http://webkit.org/b/82697" IMO.
Comment on attachment 140844 [details] Build test Attachment 140844 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12645546
Comment on attachment 140844 [details] Build test Attachment 140844 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12650498
Created attachment 140856 [details] Build test
Created attachment 140869 [details] Patch for landing
Comment on attachment 140869 [details] Patch for landing Clearing flags on attachment: 140869 Committed r116508: <http://trac.webkit.org/changeset/116508>
All reviewed patches have been landed. Closing bug.