Bug 90436

Created attachment 150565 [details] stop events at shadow boundary

Comment on attachment 150565 [details] stop events at shadow boundary View in context: https://bugs.webkit.org/attachment.cgi?id=150565&action=review > Source/WebCore/dom/EventDispatcher.cpp:-367 > - // WebKit never allowed selectstart event to cross the the shadow DOM boundary. > - // Changing this breaks existing sites. > - // See https://bugs.webkit.org/show_bug.cgi?id=52195 for details. It seems valuable to document this information somewhere. I would just leave it here if I were you. > LayoutTests/fast/dom/shadow/events-stopped-at-shadow-boundary.html:24 > +function debugDispatchedEvent(eventType) "debug"DispatchedEvent sounds misleading. Maybe dumpDispatchedEvent?

Thank you for the review. Let me land after I address your comments. (In reply to comment #2) > (From update of attachment 150565 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=150565&action=review > > > Source/WebCore/dom/EventDispatcher.cpp:-367 > > - // WebKit never allowed selectstart event to cross the the shadow DOM boundary. > > - // Changing this breaks existing sites. > > - // See https://bugs.webkit.org/show_bug.cgi?id=52195 for details. > > It seems valuable to document this information somewhere. I would just leave it here if I were you. Okay. I'll leave it as is. > > > LayoutTests/fast/dom/shadow/events-stopped-at-shadow-boundary.html:24 > > +function debugDispatchedEvent(eventType) > > "debug"DispatchedEvent sounds misleading. Maybe dumpDispatchedEvent? Let me rename.

Created attachment 152675 [details] Patch for landing

Comment on attachment 152675 [details] Patch for landing Clearing flags on attachment: 152675 Committed r122801: <http://trac.webkit.org/changeset/122801>

All reviewed patches have been landed. Closing bug.

(In reply to comment #3) > Thank you for the review. Let me land after I address your comments. > > (In reply to comment #2) > > (From update of attachment 150565 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=150565&action=review > > > > > Source/WebCore/dom/EventDispatcher.cpp:-367 > > > - // WebKit never allowed selectstart event to cross the the shadow DOM boundary. > > > - // Changing this breaks existing sites. > > > - // See https://bugs.webkit.org/show_bug.cgi?id=52195 for details. > > > > It seems valuable to document this information somewhere. I would just leave it here if I were you. > > Okay. I'll leave it as is. > I think it would be better to update it, since the behaviour and comment is superseded by the spec and this bug. As it stands it doesn't explain the extra events you added, or that selectstart is now a part of a spec and no longer a quirk necessary for backward compatibility

As far as I remeber, there is a special reason for a 'selectsstart' event due to the implementation of <input> element in WebKit. If we don't stop 'selectstart' at the shadow boundary, an <input> element will break. I think this is not directly related to the Shadow DOM spec. The issue has been there before the current shadow DOM spec was defined, I guess. I think we should explain such behavior somewhere, or update the comment here. But I am not the right person to explain it. If I am wrong, please correct me. (In reply to comment #7) > (In reply to comment #3) > > Thank you for the review. Let me land after I address your comments. > > > > (In reply to comment #2) > > > (From update of attachment 150565 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=150565&action=review > > > > > > > Source/WebCore/dom/EventDispatcher.cpp:-367 > > > > - // WebKit never allowed selectstart event to cross the the shadow DOM boundary. > > > > - // Changing this breaks existing sites. > > > > - // See https://bugs.webkit.org/show_bug.cgi?id=52195 for details. > > > > > > It seems valuable to document this information somewhere. I would just leave it here if I were you. > > > > Okay. I'll leave it as is. > > > I think it would be better to update it, since the behaviour and comment is superseded by the spec and this bug. As it stands it doesn't explain the extra events you added, or that selectstart is now a part of a spec and no longer a quirk necessary for backward compatibility

I agree that the comment here should be updated somehow. Let me fix it in another patch.

Mass move bugs into the DOM component.