Bug 101369 – AX: Textfields don't get focus when navigated to from 'show all tabs' button

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[chris fleizach]

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.

[chris fleizach]

rdar://12263874

[chris fleizach]

Created attachment 172614 [details]
patch

[Darin Adler]

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?

[chris fleizach]

(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

[Darin Adler]

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?

[chris fleizach]

(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

[chris fleizach]

Created attachment 172626 [details]
patch

patch that only applies this logic to Element types (instead of all Nodes)

[Darin Adler]

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.

[WebKit Review Bot]

Comment on attachment 172626 [details]
patch

Clearing flags on attachment: 172626

Committed r133785: <http://trac.webkit.org/changeset/133785>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.