[webkit-reviews] review denied: [Bug 27099] Elements with display none still gets focus and take part in the tab order : [Attachment 38648] This adds an assert that the layout is up to date.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Aug 27 18:30:23 PDT 2009

Eric Seidel <eric at webkit.org> has denied Erik Arvidsson <arv at chromium.org>'s
request for review:
Bug 27099: Elements with display none still gets focus and take part in the tab

Attachment 38648: This adds an assert that the layout is up to date.

------- Additional Comments from Eric Seidel <eric at webkit.org>
// All platforms actually want this code, but bug 12345 handles fixing that.
    if (isLink())
	return false;

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

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
 1228	  // we can set the the focused node to be handled later when we have
 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
 1683	      m_frame->document()->updateLayoutIgnorePendingStylesheets();

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

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

Are you aware of our JS-only/DOM-only tests?
They give you things like testPassed() for free.  These tests are also OK as

Need one more round.

More information about the webkit-reviews mailing list