Bug 90436 – Some events should be always stopped at shadow boundary.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Hayato Ito]

The spec is here: https://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#events-that-are-always-stopped

The following events should be always stopped at shadow boundary.

abort
select
change
reset
resize
scroll
selectstart

[Hayato Ito]

Created attachment 150565 [details]
stop events at shadow boundary

[Ryosuke Niwa]

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?

[Hayato Ito]

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.

[Hayato Ito]

Created attachment 152675 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 152675 [details]
Patch for landing

Clearing flags on attachment: 152675

Committed r122801: <http://trac.webkit.org/changeset/122801>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Allan Sandfeld Jensen]

(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

[Hayato Ito]

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

[Hayato Ito]

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

[Lucas Forschler]

Mass move bugs into the DOM component.