[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]

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
order
https://bugs.webkit.org/show_bug.cgi?id=27099

Attachment 38648: This adds an assert that the layout is up to date.
https://bugs.webkit.org/attachment.cgi?id=38648&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
// 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.