Bug 101369 - AX: Textfields don't get focus when navigated to from 'show all tabs' button
Summary: AX: Textfields don't get focus when navigated to from 'show all tabs' button
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified OS X 10.8
: P2 Normal
Assignee: chris fleizach
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-11-06 10:30 PST by chris fleizach
Modified: 2012-11-07 11:24 PST (History)
5 users (show)

See Also:


Attachments
patch (2.58 KB, patch)
2012-11-06 10:35 PST, chris fleizach
no flags Details | Formatted Diff | Diff
patch (2.62 KB, patch)
2012-11-06 11:39 PST, chris fleizach
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description chris fleizach 2012-11-06 10:30:23 PST
When going from the tabs to HTML content with a text field, the keyboard focus remains on a previous element. You can't type in the field when it's not focused.
Comment 1 chris fleizach 2012-11-06 10:30:39 PST
rdar://12263874
Comment 2 chris fleizach 2012-11-06 10:35:20 PST
Created attachment 172614 [details]
patch
Comment 3 Darin Adler 2012-11-06 10:50:39 PST
Comment on attachment 172614 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1506
> +    Document* document = this->document();
>      if (!on)
> -        m_renderer->document()->setFocusedNode(0);
> +        document->setFocusedNode(0);
>      else {
> -        if (m_renderer->node()->isElementNode())
> -            static_cast<Element*>(m_renderer->node())->focus();
> +        Node* node = this->node();
> +        // If this node is already the currently focused node, then calling focus() won't do anything.
> +        // That is a problem when focus is removed from the webpage to chrome, and then returns.
> +        // In these cases, we need to do what keyboard and mouse focus do, which is reset focus first.
> +        if (document->focusedNode() == node)
> +            document->setFocusedNode(0);
> +        
> +        if (node && node->isElementNode())
> +            toElement(node)->focus();
>          else
> -            m_renderer->document()->setFocusedNode(m_renderer->node());
> +            document->setFocusedNode(node);
>      }

Is there some way to share this code rather than repeating this logic here?
Comment 4 chris fleizach 2012-11-06 10:59:58 PST
(In reply to comment #3)
> (From update of attachment 172614 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172614&action=review
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1506
> > +    Document* document = this->document();
> >      if (!on)
> > -        m_renderer->document()->setFocusedNode(0);
> > +        document->setFocusedNode(0);
> >      else {
> > -        if (m_renderer->node()->isElementNode())
> > -            static_cast<Element*>(m_renderer->node())->focus();
> > +        Node* node = this->node();
> > +        // If this node is already the currently focused node, then calling focus() won't do anything.
> > +        // That is a problem when focus is removed from the webpage to chrome, and then returns.
> > +        // In these cases, we need to do what keyboard and mouse focus do, which is reset focus first.
> > +        if (document->focusedNode() == node)
> > +            document->setFocusedNode(0);
> > +        
> > +        if (node && node->isElementNode())
> > +            toElement(node)->focus();
> >          else
> > -            m_renderer->document()->setFocusedNode(m_renderer->node());
> > +            document->setFocusedNode(node);
> >      }
> 
> Is there some way to share this code rather than repeating this logic here?

I couldn't think of a way to do that easily without altering long-standing behavior. 

What I thought might be reasonable would be to call setFocusedNode(0) when the WebPage loses focus, but I wasn't sure of the implications of doing that. The document's notion of what is focused would be lost. Maybe that's not important, but it was hard to know.

The keyboard tabbing method always assumes that focus will be on the first focusable item when it sets focus. The mouse method is tied up heavily in normal mouse event handling.

The addition here (minus the refactoring) are just these two lines
>> +   if (document->focusedNode() == node)
> > +            document->setFocusedNode(0);

so it's not a lot of duplication
Comment 5 Darin Adler 2012-11-06 11:07:41 PST
Comment on attachment 172614 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1500
> +        if (document->focusedNode() == node)
> +            document->setFocusedNode(0);

Is this needed for the non-element case, or only for the element case?
Comment 6 chris fleizach 2012-11-06 11:33:57 PST
(In reply to comment #5)
> (From update of attachment 172614 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172614&action=review
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1500
> > +        if (document->focusedNode() == node)
> > +            document->setFocusedNode(0);
> 
> Is this needed for the non-element case, or only for the element case?

I think this would only ever be an issue in the element case. I can limit it to that
Comment 7 chris fleizach 2012-11-06 11:39:30 PST
Created attachment 172626 [details]
patch

patch that only applies this logic to Element types (instead of all Nodes)
Comment 8 Darin Adler 2012-11-07 10:04:41 PST
Comment on attachment 172626 [details]
patch

OK, I can live with this, but there’s something about this that seems wrong. I don’t see why there’s no function on node to do this whole dance.
Comment 9 WebKit Review Bot 2012-11-07 11:24:37 PST
Comment on attachment 172626 [details]
patch

Clearing flags on attachment: 172626

Committed r133785: <http://trac.webkit.org/changeset/133785>
Comment 10 WebKit Review Bot 2012-11-07 11:24:41 PST
All reviewed patches have been landed.  Closing bug.