Bug 67096

Summary:

Make sure accesskey works for elements in shadow DOM

Product:

WebKit

Reporter:

Hayato Ito <hayato>

Component:

DOM

Assignee:

Hayato Ito <hayato>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, dglazkov, dominicc, morrita, rolandsteiner, sam, shinyak, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

66669

Attachments:

Description	Flags
Add minimum tests.	none
WIP, scoped accesskey map.	none
only layouttest. No functional change	none

Hayato Ito

Reported 2011-08-28 19:26:07 PDT

We have to define the behavior in the case that an element in shadow DOM has a 'accesskey' attribute. I guess there are some cases we have to consider. I'll post LayoutTests for each later.

Attachments
Add minimum tests. (4.48 KB, patch) 2011-08-29 01:25 PDT, Hayato Ito	no flags	Details Formatted Diff Diff
WIP, scoped accesskey map. (14.23 KB, patch) 2011-08-30 23:39 PDT, Hayato Ito	no flags	Details Formatted Diff Diff
only layouttest. No functional change (4.94 KB, patch) 2011-09-07 02:56 PDT, Hayato Ito	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Hayato Ito

Comment 1 2011-08-28 21:25:08 PDT

At first, let me post a question. I'd like to collect opinions. Suppose the following dom tree, including one shadow host. <html> <body> <input id="inputA" /> <shadow-host> -- <shadow-root> <input id="inputB" accesskey="g" /> When 'inputA' is focused and a user presses 'g' key with proper modifiers, should we focus on 'inputB'?

Alexey Proskuryakov

Comment 2 2011-08-28 23:08:33 PDT

By the nature of shadow DOM, it seems that accesskey should be ignored in it, shouldn't it?

Hayato Ito

Comment 3 2011-08-29 00:21:15 PDT

Hi Alexey, Thank you for the comment. (In reply to comment #2) > By the nature of shadow DOM, it seems that accesskey should be ignored in it, shouldn't it? I agree. Such internal accesskeys should be hidden. What about the reverse case? Suppose the following dom tree. When 'inputB' is focused and a user presses 'f' key with proper modifier keys, should we focus on 'inputA'? In this case, we should support accesskey, looking up the TreeScope, shoudn't we? <html> <body> <input id="inputA" accesskey="f" /> <shadow-host> -- <shadow-root> <input id="inputB" accesskey="g" />

Alexey Proskuryakov

Comment 4 2011-08-29 00:41:17 PDT

The reverse seems certain to me, yes. Doesn't it work now?

Hayato Ito

Comment 5 2011-08-29 00:46:24 PDT

(In reply to comment #4) > The reverse seems certain to me, yes. Doesn't it work now? Yeah, I agree again. I'm now writing LayoutTest to make sure that. Let me update this bugzilla later.

Hayato Ito

Comment 6 2011-08-29 01:25:39 PDT

Created attachment 105474 [details] Add minimum tests.

Hayato Ito

Comment 7 2011-08-29 01:29:36 PDT

I've posted a LayoutTest. It seems that the current implementation doesn't ignore accesskey in shadow DOM. I think this should be fixed. Let me continue investigations further.

Hayato Ito

Comment 8 2011-08-29 01:39:05 PDT

I've found this change. https://trac.webkit.org/changeset/85680 This change traverses into shadow trees when building the access key map. So accesskey in shadow trees isn't ignored now. I think each access key map should be stored in each TreeScope, not in Document. Any opinions?

Alexey Proskuryakov

Comment 9 2011-08-29 23:20:08 PDT

> I've found this change. > https://trac.webkit.org/changeset/85680 I think that the change was wrong. Shadow DOM is about reusable components - but how can you put two components with accesskey on a single page?

Dimitri Glazkov (Google)

Comment 10 2011-08-30 07:53:59 PDT

(In reply to comment #9) > > I've found this change. > > https://trac.webkit.org/changeset/85680 > > I think that the change was wrong. Shadow DOM is about reusable components - but how can you put two components with accesskey on a single page? I agree. Hayato-san, I think your notion of "propagates in, but not out" is the right thing to do. Also, this needs to be spec'd properly.

Dominic Cooney

Comment 11 2011-08-30 08:08:59 PDT

Why shouldn’t accesskey propagate *in* to shadow DOM? Yes, two inputs should not have the same access key on a given page. That is a separate issue that exists regardless of components or not. My $0.0.2: Someone building a component should be able to set up key shortcuts to controls in their component. Why not let accesskey propagate into shadow DOM? At worst, if there are multiple instances of the component or if the containing page uses the same accesskey, then the accesskey will not work. But this seems better than what is proposed here, which ensures that accesskey will *never* work in a component. What about this shadow DOM: <html> <body> <input id="inputA" /> <shadow-host> -- <shadow-root> <input id="inputB" /> <input id="inputC" accesskey="g" /> If inputB has the focus, does modifier-g focus inputC?

Hayato Ito

Comment 12 2011-08-30 23:39:39 PDT

Created attachment 105745 [details] WIP, scoped accesskey map.

Hayato Ito

Comment 13 2011-08-30 23:42:54 PDT

Thank you for the comments, guys. Okay. I've just implemented the idea of 'scoped' accesskey as an experiment. If a component doesn't have elements with the accesskey, the search continues to its parent component until an element with the accesskey is found See the layouttest in the patch for the details. One disadvantage of this approach is that we cannot rescue the following case as Dominic worried about: - Dom tree is: <html> <body> <input id="inputA"/> <shadow-host> -- <shadow-root> <input id="inputB" accesskey="g" /> - InputA has the focus and modifier-g is pressed. In this case, inputB won't get focused in my experimental implementation. I'd like to loosen up this restriction.

Hayato Ito

Comment 14 2011-08-31 00:09:04 PDT

Let me try to 'propagate-in' somehow, reusing scoped accesskey concept. (In reply to comment #13) > Thank you for the comments, guys. > > Okay. I've just implemented the idea of 'scoped' accesskey as an experiment. > If a component doesn't have elements with the accesskey, the search continues to its parent component until an element with the accesskey is found > > See the layouttest in the patch for the details. > > One disadvantage of this approach is that we cannot rescue the following case as Dominic worried about: > > - Dom tree is: > <html> > <body> > <input id="inputA"/> > <shadow-host> > -- <shadow-root> > <input id="inputB" accesskey="g" /> > > - InputA has the focus and modifier-g is pressed. > > In this case, inputB won't get focused in my experimental implementation. I'd like to loosen up this restriction.

Hayato Ito

Comment 15 2011-08-31 02:03:22 PDT

I am now thinking the following simple model. 1. Local elements in the TreeScope of 'focused' component should be honored at first priority, which means a access key could not be overridden by other component as long as a user stay within the component. 2. If there is no such a local element, simply return the 'first' (or last?) element with the accesskey in the overall 'Document', as current implementation does so. Any opinions?

Dominic Cooney

Comment 16 2011-08-31 09:08:35 PDT

(In reply to comment #15) > I am now thinking the following simple model. > > 1. Local elements in the TreeScope of 'focused' component should be honored at first priority, which means a access key could not be overridden by other component as long as a user stay within the component. > 2. If there is no such a local element, simply return the 'first' (or last?) element with the accesskey in the overall 'Document', as current implementation does so. Doesn’t it seem odd that the same access key does something different depending on where you are in the document? Could we do something simpler that operates over all of the accesskeys in the flattened DOM? eg markup with children and markup with shadow DOM that results in the same presentation should have the same accesskey behavior? I just played around with accesskey in Firefox Nightly and when multiple buttons have the same accesskey, it just focuses the first one but doesn’t activate it. If you press the access key again, the focus traverses elements with that accesskey. Whether WebKit should do this or not is a separate issue, but I think just making shadow DOM with access keys work like ordinary DOM with access keys, and then solving the duplicate accesskey problem separately, would be better.

Hajime Morrita

Comment 17 2011-09-01 01:48:07 PDT

We can think how shadow DOM works with flattened DOM in mind, On the other hand, we can think it as if shadow DOM were iframe. In flattened-DOM viewpoint, @accesskey inside DOM should be exposed. In iframe viewpoint, @accesskey should be hidden inside the tree. What does it happen if CSS contains a selector which matches elements with @accesskey?

Hayato Ito

Comment 18 2011-09-01 22:16:16 PDT

(In reply to comment #16) > (In reply to comment #15) > > I am now thinking the following simple model. > > > > 1. Local elements in the TreeScope of 'focused' component should be honored at first priority, which means a access key could not be overridden by other component as long as a user stay within the component. > > 2. If there is no such a local element, simply return the 'first' (or last?) element with the accesskey in the overall 'Document', as current implementation does so. > > Doesn’t it seem odd that the same access key does something different depending on where you are in the document? > > Could we do something simpler that operates over all of the accesskeys in the flattened DOM? eg markup with children and markup with shadow DOM that results in the same presentation should have the same accesskey behavior? I think that's exactly same behavior as the current implementations does. If that's a desired behavior, there is no much things we have to do with accesskeys in Shadow DOM. That's already implemented. > > I just played around with accesskey in Firefox Nightly and when multiple buttons have the same accesskey, it just focuses the first one but doesn’t activate it. If you press the access key again, the focus traverses elements with that accesskey. Whether WebKit should do this or not is a separate issue, but I think just making shadow DOM with access keys work like ordinary DOM with access keys, and then solving the duplicate accesskey problem separately, would be better. Yeah, that's totally different issue. I am not sure whether we should do the same thing in WebKit. We need to investigate other browsers' behavior as well.

Dominic Cooney

Comment 19 2011-09-02 06:02:10 PDT

FWIW, here is what Firefox Nightly does with accesskeys in frames: - Activate unique access key, focus outside frame => run command **even if button is inside the iframe** - Non-unique access key, focus outside frame => tab between elements with that access key in outer doc - Non-unique access key, but unique per frame, focus outside frame => run command in outer doc - Non-unique access key, focus inside frame => tab between elements with that access key in frame - Non-unique access key, but unique per frame, focus inside frame => run command in frame Chrome Canary: - Activate unique access key, focus outside frame => run command **if in outside frame** (otherwise do nothing) - Non-unique access key, focus outside frame => run last matching command in outer doc - Non-unique access key, but unique per frame, focus outside frame => run command in outer doc - Non-unique access key, focus inside frame => run last matching command in frame - Non-unique access key, but unique per frame, focus inside frame => run command in frame In general WebKit’s handling of accesskey seems less sophisticated, but maybe more consistent, than Firefox’s. Not arguing that we should fix or change WebKit’s accesskey behavior in this bug though. Just pointing out that things may work differently in future, so we might need to be prepared to revisit this. So if we want to use iframes as the model, then this change sounds like it makes shadow DOM match iframes. It might be nice if there was a test that set up shadow DOM and an analogous situation in iframes (same arrangement, different accesskeys) and verified that their behavior matched. That way if someone improves accesskey for iframes we can catch it and update shadow DOM handling too.

Hayato Ito

Comment 20 2011-09-05 19:09:32 PDT

Thank you for the further investigations. (In reply to comment #19) > FWIW, here is what Firefox Nightly does with accesskeys in frames: > > - Activate unique access key, focus outside frame => run command **even if button is inside the iframe** > - Non-unique access key, focus outside frame => tab between elements with that access key in outer doc > - Non-unique access key, but unique per frame, focus outside frame => run command in outer doc > - Non-unique access key, focus inside frame => tab between elements with that access key in frame > - Non-unique access key, but unique per frame, focus inside frame => run command in frame > > Chrome Canary: > > - Activate unique access key, focus outside frame => run command **if in outside frame** (otherwise do nothing) > - Non-unique access key, focus outside frame => run last matching command in outer doc > - Non-unique access key, but unique per frame, focus outside frame => run command in outer doc > - Non-unique access key, focus inside frame => run last matching command in frame > - Non-unique access key, but unique per frame, focus inside frame => run command in frame > > In general WebKit’s handling of accesskey seems less sophisticated, but maybe more consistent, than Firefox’s. Not arguing that we should fix or change WebKit’s accesskey behavior in this bug though. Just pointing out that things may work differently in future, so we might need to be prepared to revisit this. > > So if we want to use iframes as the model, then this change sounds like it makes shadow DOM match iframes. It might be nice if there was a test that set up shadow DOM and an analogous situation in iframes (same arrangement, different accesskeys) and verified that their behavior matched. That way if someone improves accesskey for iframes we can catch it and update shadow DOM handling too. I am not 100% sure that we should use iframe as the model. We should be flexible as per each feature. As for accesskey, I don't have a strong opinion yet, but it would be good to have a LayoutTest to ensure that behavior matches to iframe. Let me add tests for accesskey for iframes.

Hayato Ito

Comment 21 2011-09-06 07:09:09 PDT

I am adding a LayoutTest for accesskey in regard to iframes in: https://bugs.webkit.org/show_bug.cgi?id=67642

Hayato Ito

Comment 22 2011-09-07 01:27:15 PDT

Let me summarize how WebKit handles accesskey at this point: - There is one accesskey map per each Document (iframe). You can think Accesskey as a HashMap<Key, Element>. - Only accesskey map in a focused frame is used to look up the accesskey. Aaccesskey maps in other iframes are not used at all. - If there is a duplication of the same accesskey, the latter (in Document order) wins. Therefore, when one iframe has some shadow DOM components which define accesskeys inside of them, they share one access key map. Simply the latter wins in case of duplication. There is no mechanism to hide. They are simply 'flattened'. I thought we had to hide accesskeys inside of shadow DOM from an outer Document, but now I am not sure whether this is a good idea or not. I'd like to mark this bug as 'WONT FIX', only adding a layout test to verify the current behavior. If we encounter the significant use case that must be handled, this bug entry should be opened again.

Hayato Ito

Comment 23 2011-09-07 02:56:12 PDT

Created attachment 106562 [details] only layouttest. No functional change

Hayato Ito

Comment 24 2011-09-07 02:57:50 PDT

I've uploaded a patch which contains only a LayoutTest to verify the current implementation. So instead of marking bug as 'WONTFIX', I'll use this bug for this LayoutTest. (In reply to comment #23) > Created an attachment (id=106562) [details] > only layouttest. No functional change

WebKit Review Bot

Comment 25 2011-09-08 11:14:06 PDT

Comment on attachment 106562 [details] only layouttest. No functional change Clearing flags on attachment: 106562 Committed r94768: <http://trac.webkit.org/changeset/94768>

WebKit Review Bot

Comment 26 2011-09-08 11:14:11 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.