Bug 27099 – Elements with display none still gets focus and take part in the tab order

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

[Erik Arvidsson]

Given an element with tabIndex and style.display set to none the user can still focus that element by tabbing to it. This leads to the focus outline getting lost and there seems to be gaps in the focus order. The expected behavior is for non displayed elements to not take part in the tab order and not be focusable.

[Erik Arvidsson]

Created attachment 32481 [details]
Test case - tabbing should never focus "Div 1" which is hidden.

[Erik Arvidsson]

It seems like the following change works:

bool Node::isFocusable() const
{
    return renderer() && hasRareData() && rareData()->tabIndexSetExplicitly();
}

It checks if the Node has a renderer before it claims to be focusable. This works because display: none has no renderer. I'm not sure that this is sufficient or if it might have other side effects?

[Erik Arvidsson]

Created attachment 32584 [details]
Test case

I improved on the test case by including a 0x0 sized div and a visibility hidden div.

IE does not allow focus on the visibility hidden div but I think we should allow that since content might be visible in it.

[Erik Arvidsson]

Created attachment 33305 [details]
Make isKeyboardFocusable return false when there is no renderer.

[Adele Peterson]

Comment on attachment 33305 [details]
Make isKeyboardFocusable return false when there is no renderer.

should isFocusable check for the renderer?  If not, why not?

[Darin Adler]

Comment on attachment 33305 [details]
Make isKeyboardFocusable return false when there is no renderer.

Thanks for tackling this. It looks like you are on the right track.

Unfortunately this is not quite right.

Calling renderer() inside a Node function can give you a false negative. The object might have a different display type and layout just hasn't run yet. It's an error to ever call renderer() unless you know that layout is up to date.

Instead, this check needs to be something done by callers that are at a higher level; they are in a position to know that layout is updated and if renderer() is 0 it actually means something.

[Erik Arvidsson]

(In reply to comment #6)
> (From update of attachment 33305 [details])
> Thanks for tackling this. It looks like you are on the right track.
> 
> Unfortunately this is not quite right.
> 
> Calling renderer() inside a Node function can give you a false negative. The
> object might have a different display type and layout just hasn't run yet. It's
> an error to ever call renderer() unless you know that layout is up to date.
> 
> Instead, this check needs to be something done by callers that are at a higher
> level; they are in a position to know that layout is updated and if renderer()
> is 0 it actually means something.

Is it OK to check the renderer in HTMLElement and its subclasses? 

Another option is to get the computed style and check the display property but that seems less than optimal.

[Erik Arvidsson]

Created attachment 34159 [details]
Use renderStyle display in Element::isFocusable instead

[Darin Adler]

Comment on attachment 34159 [details]
Use renderStyle display in Element::isFocusable instead

This doesn't look right to me. One of the best ways to "check" for display: none is to check if there is a renderer, and that can be done at the Node level by checking renderer().

[Erik Arvidsson]

(In reply to comment #9)
> (From update of attachment 34159 [details])
> This doesn't look right to me. One of the best ways to "check" for display:
> none is to check if there is a renderer, and that can be done at the Node level
> by checking renderer().

I'm confused. Earlier you said that "Calling renderer() inside a Node function can give you a false negative"

The main problem I encountered is with option and optgroup elements (as well as with map elements but those are just plain broken). These do not have a renderer but they have display() != NONE. These have a nonRendererRenderStyle so I guess I could use that.

I'll see what I can come up with

[Erik Arvidsson]

Created attachment 34174 [details]
Checking renderer() first

This now checks renderer() in Element::isFocusable and handles the false negatives.

[Darin Adler]

Comment on attachment 34174 [details]
Checking renderer() first

No, in those cases we should check renderer() after calling updateLayoutIgnorePendingStylesheets().

I still don't understand why Element does this work and Node does not -- it makes sense for other nodes too.

But also, I'm a little worried about such a low level function calling updateLayout. We'll need to examine all calls to isFocusable and make sure they can handle a layout happening in the middle of the function right where they call updateLayout. A better design might be to do the layout at a higher level and have an assertion in isFocusable instead. Another possibility would be to move isFocusable to the render tree which would implicitly do the same thing in a clearer way.

[Erik Arvidsson]

(In reply to comment #12)
> (From update of attachment 34174 [details])
> No, in those cases we should check renderer() after calling
> updateLayoutIgnorePendingStylesheets().
> 
> I still don't understand why Element does this work and Node does not -- it
> makes sense for other nodes too.

I didn't think Node had any style info

> But also, I'm a little worried about such a low level function calling
> updateLayout. We'll need to examine all calls to isFocusable and make sure they
> can handle a layout happening in the middle of the function right where they
> call updateLayout. A better design might be to do the layout at a higher level
> and have an assertion in isFocusable instead.

I was also concerned about introducing a layout call in isFocusable.

What would that assertion be?

There is already a few overrides of isFocusable that check renderer() without any call to updateLayoutIgnorePendingStylesheets in them. All code paths that call isFocusable also seems to update the layout. The only exception I found is Element::attach but that one calls createRendererIfNeeded

> Another possibility would be to
> move isFocusable to the render tree which would implicitly do the same thing in
> a clearer way.

isFocusable depends on attributes like tabindex and href for links.

[Erik Arvidsson]

Created attachment 38561 [details]
Handle the basic hidden case in Node::isFocusable

This cleans up the mess of supportsFocus and isFocusable. supportsFocus now means that the node should support focus if layout is ignored. For example a link should support focus and a span with tabIndex >= 0 and a contentEditabl element as well. isFocusable checks that the element has a renderer and that it is visible. Some derived classes of Node also check that the box for the element is non empty (but I didn't change those cases since there were rdar bugs for those).

[Erik Arvidsson]

Comment on attachment 38561 [details]
Handle the basic hidden case in Node::isFocusable

I got some more fixes to a elements without href attributes and I'm adding more tests.

[Erik Arvidsson]

Created attachment 38569 [details]
Also handles a elements correctly.

[Erik Arvidsson]

Created attachment 38648 [details]
This adds an assert that the layout is up to date.

The assert found a code path where the layout was not up to date. When the user clicks somewhere we need to ensure layout is up to date before checking isFocusable as well.

[Eric Seidel (no email)]

Comment on attachment 38648 [details]
This adds an assert that the layout is up to date.

// All platforms actually want this code, but bug 12345 handles fixing that.
#if !PLATFORM(GTK)
    if (isLink())
        return false;
#endif

   // Allow tab index etc to control focus.
   return HTMLElement::isMouseFocusable();


Seems:
7474 bool WMLAElement::supportsFocus() const

should be:
return isLink() || WMLElement::supportsFocus();


Needs new comment:
7     // If it supports focus but the stylesheets have not loaded we continue so
 1228     // we can set the the focused node to be handled later when we have the
 1229     // stylehseets loaded.
 1230     if (doc->haveStylesheetsLoaded()) {
 1231         doc->updateLayoutIgnorePendingStylesheets();
 1232         if (!isFocusable())
 1233             return;
 1234     }

Something along the lines of:
// If we have already loaded stylesheets, we can reliably check isFocusable()
// If not, then we just continue and focus will update when the stylesheet loads finish.


 1239     // Setting the focused node above might have caused side effects like
 1240     // requiring the layout to update or made the node to no longer be
 1241     // focusable.
 1242     doc->updateLayoutIgnorePendingStylesheets();

// Setting the focused node may have invalidated layout.
why is that true?

 848     // Dave wants to fix that some day with a "has visible content" flag or the like.

we have like 5 daves now. :)  "Hyatt" is a better indicator these days.

I would have expected this to be closer to an isFocusable call?
2         // The layout needs to be up to date to determine if an element is focusable.
 1683         m_frame->document()->updateLayoutIgnorePendingStylesheets();

No need to depend on onload, we can just inline the script after the content, no?

 window.onload = function()
 12 {
 13     if (!window.layoutTestController)
 14         return;
 15 
 16     var aElement = document.getElementById('t');
 17     eventSender.mouseMoveTo(aElement.offsetLeft + 2, aElement.offsetTop + 2);
 18     eventSender.mouseDown();    
 19 };

Are you aware of our JS-only/DOM-only tests?
http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree
They give you things like testPassed() for free.  These tests are also OK as is.

Need one more round.

[Erik Arvidsson]

Created attachment 38708 [details]
Eric's comments taken care of

[Eric Seidel (no email)]

Comment on attachment 38708 [details]
Eric's comments taken care of

This looks fine to me.

I'm assuming the blur event count change is intentional:
-335 focus / 335 blur events disatched, and should be 335 / 335 PASSED
+329 focus / 329 blur events dispatched, and should be 335 / 335 PASSED

A comment here would have problem helped:
-bool HTMLFrameElementBase::isFocusable() const
+bool HTMLFrameElementBase::supportsFocus() const
 {
-    return renderer();
+    return true;
 }

This is OK as is.

[Eric Seidel (no email)]

Comment on attachment 38708 [details]
Eric's comments taken care of

Rejecting patch 38708 from commit-queue.  This patch will require manual commit.

Failed to run "['git', 'svn', 'dcommit']"  exit_code: 1  cwd: None

[Eric Seidel (no email)]

Comment on attachment 38708 [details]
Eric's comments taken care of

Race condition during commit.  adding back to commit-queue.

[Eric Seidel (no email)]

Comment on attachment 38708 [details]
Eric's comments taken care of

Rejecting patch 38708 from commit-queue.  This patch will require manual commit.

Failed to run "['git', 'svn', 'dcommit']"  exit_code: 1  cwd: None

[Erik Arvidsson]

(In reply to comment #23)
> (From update of attachment 38708 [details])
> Rejecting patch 38708 from commit-queue.  This patch will require manual
> commit.
> 
> Failed to run "['git', 'svn', 'dcommit']"  exit_code: 1  cwd: None

Is there anything I need to do to get this committed?

[Adam Barth]

Comment on attachment 38708 [details]
Eric's comments taken care of

Clearing flags on attachment: 38708

Committed r48106: <http://trac.webkit.org/changeset/48106>

[Adam Barth]

All reviewed patches have been landed.  Closing bug.