Bug 95169 - [ShadowDOM] Shadow elements in the input element should be focusable.
Summary: [ShadowDOM] Shadow elements in the input element should be focusable.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: yosin
URL:
Keywords:
Depends on:
Blocks: 95168
  Show dependency treegraph
 
Reported: 2012-08-27 21:35 PDT by yosin
Modified: 2012-08-30 05:56 PDT (History)
6 users (show)

See Also:


Attachments
Patch 1 (10.48 KB, patch)
2012-08-27 22:17 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 2 (8.99 KB, patch)
2012-08-27 22:34 PDT, yosin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description yosin 2012-08-27 21:35:44 PDT
The implementation of multiple fields time input UI wants to have focusable shadow elements for focus navigation.
For this purpose, FocusController should walk through shadow elements for "time" input type. 

Although, current implementation of focus navigation in FocusController class introduced by r112511 treats all
input types in same way.
Comment 1 yosin 2012-08-27 22:17:19 PDT
Created attachment 160898 [details]
Patch 1
Comment 2 yosin 2012-08-27 22:18:03 PDT
Comment on attachment 160898 [details]
Patch 1

Could you review this patch?
Thanks in advance.
Comment 3 Kent Tamura 2012-08-27 22:28:23 PDT
Comment on attachment 160898 [details]
Patch 1

View in context: https://bugs.webkit.org/attachment.cgi?id=160898&action=review

> Source/WebCore/html/HTMLAudioElement.h:51
> +    virtual bool hasCustomFocusLogic() const OVERRIDE;

This should be defined at HTMLMediaElement instead of HTMLAudioElement and HTMLVideoElement because HTMLMediaElement is responsible to build shadow sub tree for media control UI.
Comment 4 yosin 2012-08-27 22:34:47 PDT
Created attachment 160901 [details]
Patch 2
Comment 5 yosin 2012-08-27 22:36:19 PDT
Comment on attachment 160901 [details]
Patch 2

Could you review this patch?
Thanks in advance.

= Changes since the last review =
* Add hasCustomFocusLogic() to HTMLMediaElement instead of HTMLAudioElement and HTMLVideoElement
Comment 6 Kent Tamura 2012-08-27 22:40:11 PDT
Comment on attachment 160901 [details]
Patch 2

ok
Comment 7 yosin 2012-08-27 22:41:23 PDT
Comment on attachment 160901 [details]
Patch 2

Clearing flags on attachment: 160901

Committed r126842: <http://trac.webkit.org/changeset/126842>
Comment 8 yosin 2012-08-27 22:41:27 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Eric Carlson 2012-08-28 09:18:00 PDT
(In reply to comment #5)
> (From update of attachment 160901 [details])
> * Add hasCustomFocusLogic() to HTMLMediaElement instead of HTMLAudioElement and HTMLVideoElement

What is "multiple fields time input UI" and what does it have to do with HTMLMediaElement?
Comment 10 yosin 2012-08-28 18:21:33 PDT
(In reply to comment #9)
> (In reply to comment #5)
> > (From update of attachment 160901 [details] [details])
> > * Add hasCustomFocusLogic() to HTMLMediaElement instead of HTMLAudioElement and HTMLVideoElement
> 
> What is "multiple fields time input UI"?
Multiple fields time input UI is introduced by bug 88970.
It is text field like UI but it has fields for hour, minute and second. It is similar to platform provided DateTime control.

>what does it have to do with HTMLMediaElement?
"audio" and "video" elements uses shadow DOM for implementing their UI, focus controller hasn't supported their detail of focus navigation yet. "input" element is same situation. But, I would like to override this focus navigation behavior based on input type.
Comment 11 Eric Carlson 2012-08-29 16:57:25 PDT
(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #5)
> > 
> > What is "multiple fields time input UI"?
> Multiple fields time input UI is introduced by bug 88970.
> It is text field like UI but it has fields for hour, minute and second. It is similar to platform provided DateTime control.
> 
> >what does it have to do with HTMLMediaElement?
> "audio" and "video" elements uses shadow DOM for implementing their UI, focus controller hasn't supported their detail of focus navigation yet. "input" element is same situation. But, I would like to override this focus navigation behavior based on input type.

I am sorry, but I still don't understand - maybe because I don't know what the focus controller or focus navigation are. Can you please explain in more detail or point me to something I can read to educate myself?
Comment 12 yosin 2012-08-30 01:38:25 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > (In reply to comment #5)
> > > 
> > > What is "multiple fields time input UI"?
> > Multiple fields time input UI is introduced by bug 88970.
> > It is text field like UI but it has fields for hour, minute and second. It is similar to platform provided DateTime control.
> > 
> > >what does it have to do with HTMLMediaElement?
> > "audio" and "video" elements uses shadow DOM for implementing their UI, focus controller hasn't supported their detail of focus navigation yet. "input" element is same situation. But, I would like to override this focus navigation behavior based on input type.
> 
> I am sorry, but I still don't understand - maybe because I don't know what the focus controller or focus navigation are. Can you please explain in more detail or point me to something I can read to educate myself?

Actually, I'm not familiar with FocusController. When I study how Shift+Tab handled, I saw page/EventHandler.cpp, then I reached page/FocusController.cpp

My trouble was FocusController didn't walk into shadow DOM tree hosted by "input" element. My experiment proofs FocusController walks into the "span" element. Simple search "input" gave me what I need to change.
Comment 13 Hayato Ito 2012-08-30 05:56:52 PDT
Let me comment briefly. I'll take a look at the patch tomorrow.

When I supported Shadow DOM in regard to focus navigations, I found that input element has a special focus logic. That caused me to exclude input elements from general focus navigation. So we cannot enter a shadow DOM of input elements in focus navigations.

I think we need to update input elements somehow before we get rid of such special treatment. Let me take a look at the patch tomorrow.

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > (In reply to comment #9)
> > > > (In reply to comment #5)
> > > > 
> > > > What is "multiple fields time input UI"?
> > > Multiple fields time input UI is introduced by bug 88970.
> > > It is text field like UI but it has fields for hour, minute and second. It is similar to platform provided DateTime control.
> > > 
> > > >what does it have to do with HTMLMediaElement?
> > > "audio" and "video" elements uses shadow DOM for implementing their UI, focus controller hasn't supported their detail of focus navigation yet. "input" element is same situation. But, I would like to override this focus navigation behavior based on input type.
> > 
> > I am sorry, but I still don't understand - maybe because I don't know what the focus controller or focus navigation are. Can you please explain in more detail or point me to something I can read to educate myself?
> 
> Actually, I'm not familiar with FocusController. When I study how Shift+Tab handled, I saw page/EventHandler.cpp, then I reached page/FocusController.cpp
> 
> My trouble was FocusController didn't walk into shadow DOM tree hosted by "input" element. My experiment proofs FocusController walks into the "span" element. Simple search "input" gave me what I need to change.