Bug 596350 - Make object elements focusable when they contain a document and no longer by default. r=smaug,enndeakin a=blocking

content/base/public/nsContentUtils.h

@@ -1690,6 +1690,16 @@ public:
    */
   static PRBool IsFocusedContent(const nsIContent *aContent);
 
+  /**
+   * Returns if aContent has a tabbable subdocument.
+   * A sub document isn't tabbable when it's a zombie document.
+   *
+   * @param aElement element to test.
+   *
+   * @return Whether the subdocument is tabbable.
+   */
+  static bool IsSubDocumentTabbable(nsIContent* aContent);
+
 private:
 
   static PRBool InitializeEventTable();

content/base/src/nsContentUtils.cpp

@@ -6250,6 +6250,42 @@ nsContentUtils::IsFocusedContent(const nsIContent* aContent)
   return fm && fm->GetFocusedContent() == aContent;
 }
 
+bool
+nsContentUtils::IsSubDocumentTabbable(nsIContent* aContent)
+{
+  nsIDocument* doc = aContent->GetCurrentDoc();
+  if (!doc) {
+    return false;
+  }
+
+  // XXXbz should this use GetOwnerDoc() for GetSubDocumentFor?
+  // sXBL/XBL2 issue!
+  nsIDocument* subDoc = doc->GetSubDocumentFor(aContent);
+  if (!subDoc) {
+    return false;
+  }
+
+  nsCOMPtr<nsISupports> container = subDoc->GetContainer();
+  nsCOMPtr<nsIDocShell> docShell = do_QueryInterface(container);
+  if (!docShell) {
+    return false;
+  }
+
+  nsCOMPtr<nsIContentViewer> contentViewer;
+  docShell->GetContentViewer(getter_AddRefs(contentViewer));
+  if (!contentViewer) {
+    return false;
+  }
+
+  nsCOMPtr<nsIContentViewer> zombieViewer;
+  contentViewer->GetPreviousViewer(getter_AddRefs(zombieViewer));
+
+  // If there are 2 viewers for the current docshell, that
+  // means the current document is a zombie document.
+  // Only navigate into the subdocument if it's not a zombie.
+  return !zombieViewer;
+}
+
 void nsContentUtils::RemoveNewlines(nsString &aString)
 {
   // strip CR/LF and null

content/html/content/src/nsGenericHTMLElement.cpp

@@ -2425,36 +2425,9 @@ nsGenericHTMLFrameElement::IsHTMLFocusable(PRBool aWithMouse,
     return PR_TRUE;
   }
 
-  // If there is no subdocument, docshell or content viewer, it's not tabbable
-  PRBool isFocusable = PR_FALSE;
-  nsIDocument *doc = GetCurrentDoc();
-  if (doc) {
-    // XXXbz should this use GetOwnerDoc() for GetSubDocumentFor?
-    // sXBL/XBL2 issue!
-    nsIDocument *subDoc = doc->GetSubDocumentFor(this);
-    if (subDoc) {
-      nsCOMPtr<nsISupports> container = subDoc->GetContainer();
-      nsCOMPtr<nsIDocShell> docShell(do_QueryInterface(container));
-      if (docShell) {
-        nsCOMPtr<nsIContentViewer> contentViewer;
-        docShell->GetContentViewer(getter_AddRefs(contentViewer));
-        if (contentViewer) {
-          isFocusable = PR_TRUE;
-          nsCOMPtr<nsIContentViewer> zombieViewer;
-          contentViewer->GetPreviousViewer(getter_AddRefs(zombieViewer));
-          if (zombieViewer) {
-            // If there are 2 viewers for the current docshell, that 
-            // means the current document is a zombie document.
-            // Only navigate into the frame/iframe if it's not a zombie.
-            isFocusable = PR_FALSE;
-          }
-        }
-      }
-    }
-  }
+  *aIsFocusable = nsContentUtils::IsSubDocumentTabbable(this);
 
-  *aIsFocusable = isFocusable;
-  if (!isFocusable && aTabIndex) {
+  if (!*aIsFocusable && aTabIndex) {
     *aTabIndex = -1;
   }
 

content/html/content/src/nsGenericHTMLElement.h

@@ -774,7 +774,6 @@ protected:
   // Used by A, AREA, LINK, and STYLE.
   already_AddRefed<nsIURI> GetHrefURIForAnchors() const;
 
-private:
   /**
    * Returns whether this element is an editable root. There are two types of
    * editable roots:
@@ -787,6 +786,7 @@ private:
    */
   PRBool IsEditableRoot() const;
 
+private:
   void ChangeEditableState(PRInt32 aChange);
 };
 

content/html/content/src/nsHTMLObjectElement.cpp

@@ -144,6 +144,12 @@ private:
    */
   NS_HIDDEN_(void) StartObjectLoad(PRBool aNotify);
 
+  /**
+   * Returns if the element is currently focusable regardless of it's tabindex
+   * value. This is used to know the default tabindex value.
+   */
+  bool IsFocusableForTabIndex();
+
   PRPackedBool mIsDoneAddingChildren;
 };
 
@@ -297,23 +303,61 @@ nsHTMLObjectElement::UnsetAttr(PRInt32 aNameSpaceID, nsIAtom* aAttribute,
   return nsGenericHTMLFormElement::UnsetAttr(aNameSpaceID, aAttribute, aNotify);
 }
 
+bool
+nsHTMLObjectElement::IsFocusableForTabIndex()
+{
+  nsIDocument* doc = GetCurrentDoc();
+  if (!doc || doc->HasFlag(NODE_IS_EDITABLE)) {
+    return false;
+  }
+
+  return Type() == eType_Plugin || IsEditableRoot() ||
+         (Type() == eType_Document && nsContentUtils::IsSubDocumentTabbable(this));
+}
+
 PRBool
 nsHTMLObjectElement::IsHTMLFocusable(PRBool aWithMouse,
                                      PRBool *aIsFocusable, PRInt32 *aTabIndex)
 {
-  if (Type() == eType_Plugin) {
+  // TODO: this should probably be managed directly by IsHTMLFocusable.
+  // See bug 597242.
+  nsIDocument *doc = GetCurrentDoc();
+  if (!doc || doc->HasFlag(NODE_IS_EDITABLE)) {
+    if (aTabIndex) {
+      GetIntAttr(nsGkAtoms::tabindex, -1, aTabIndex);
+    }
+
+    *aIsFocusable = PR_FALSE;
+
+    return PR_FALSE;
+  }
+
+  // This method doesn't call nsGenericHTMLFormElement intentionally.
+  // TODO: It should probably be changed when bug 597242 will be fixed.
+  if (Type() == eType_Plugin || IsEditableRoot() ||
+      (Type() == eType_Document && nsContentUtils::IsSubDocumentTabbable(this))) {
     // Has plugin content: let the plugin decide what to do in terms of
     // internal focus from mouse clicks
     if (aTabIndex) {
-      GetTabIndex(aTabIndex);
+      GetIntAttr(nsGkAtoms::tabindex, 0, aTabIndex);
     }
-  
+
     *aIsFocusable = PR_TRUE;
 
     return PR_FALSE;
   }
 
-  return nsGenericHTMLFormElement::IsHTMLFocusable(aWithMouse, aIsFocusable, aTabIndex);
+  // TODO: this should probably be managed directly by IsHTMLFocusable.
+  // See bug 597242.
+  const nsAttrValue* attrVal = mAttrsAndChildren.GetAttr(nsGkAtoms::tabindex);
+
+  *aIsFocusable = attrVal && attrVal->Type() == nsAttrValue::eInteger;
+
+  if (aTabIndex && *aIsFocusable) {
+    *aTabIndex = attrVal->GetIntegerValue();
+  }
+
+  return PR_FALSE;
 }
 
 PRUint32
@@ -375,7 +419,8 @@ NS_IMPL_STRING_ATTR(nsHTMLObjectElement, Height, height)
 NS_IMPL_INT_ATTR(nsHTMLObjectElement, Hspace, hspace)
 NS_IMPL_STRING_ATTR(nsHTMLObjectElement, Name, name)
 NS_IMPL_STRING_ATTR(nsHTMLObjectElement, Standby, standby)
-NS_IMPL_INT_ATTR_DEFAULT_VALUE(nsHTMLObjectElement, TabIndex, tabindex, 0)
+NS_IMPL_INT_ATTR_DEFAULT_VALUE(nsHTMLObjectElement, TabIndex, tabindex,
+                               IsFocusableForTabIndex() ? 0 : -1)
 NS_IMPL_STRING_ATTR(nsHTMLObjectElement, Type, type)
 NS_IMPL_STRING_ATTR(nsHTMLObjectElement, UseMap, usemap)
 NS_IMPL_INT_ATTR(nsHTMLObjectElement, Vspace, vspace)

content/html/content/test/Makefile.in

@@ -232,6 +232,7 @@ _TEST_FILES = \
 		test_bug557087-6.html \
 		test_bug600155.html \
 		test_bug598643.html \
+		test_bug596350.html \
 		$(NULL)
 
 libs:: $(_TEST_FILES)

content/html/content/test/test_bug430351.html

@@ -148,7 +148,6 @@ var focusableElements = [
     "<input type=\"text\" tabindex=\"1\">",
     "<input type=\"text\" contenteditable=\"true\">",
 
-    "<object></object>",
     "<object tabindex=\"-1\"></object>",
     "<object tabindex=\"0\"></object>",
     "<object tabindex=\"1\"></object>",
@@ -241,6 +240,8 @@ var nonFocusableElements = [
     "<input type=\"text\" tabindex=\"0\" disabled>",
     "<input type=\"text\" disabled>",
 
+    "<object></object>",
+
     "<select tabindex=\"0\" disabled></select>",
     "<select disabled></select>"
 ];
@@ -352,11 +353,9 @@ var focusableInContentEditable = [
     "<input type=\"text\" tabindex=\"1\">",
     "<input type=\"text\" contenteditable=\"true\">",
 
-    "<object></object>",
     "<object tabindex=\"-1\"></object>",
     "<object tabindex=\"0\"></object>",
     "<object tabindex=\"1\"></object>",
-    "<object contenteditable=\"true\"></object>",
 
     // Disabled doesn't work for <object>.
     "<object tabindex=\"0\" disabled></object>",

content/html/content/test/test_bug596350.html

@@ -0,0 +1,65 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=596350
+-->
+<head>
+  <title>Test for Bug 596350</title>
+  <script type="application/javascript" src="/MochiKit/packed.js"></script>
+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+</head>
+<body onload="runTests();">
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=596350">Mozilla Bug 596350</a>
+<p id="display"></p>
+<div id="content" style="display: none">
+  <object></object>
+  <object data="iVBORw0KGgoAAAANSUhEUgAAAAoAAAAKCAYAAACNMsALGPC/xhBQAAAAlwSFlzAAALEwAACxMBAJqcGAAAAAd0SU1FB9YGARc5KB0XV+IAAAAddEVYdENvbW1lbnQAQ3JlYXRlZCB3aXRoIFRoZSBHSU1Q72QlbgAAAF1JREFUGNO9zL0NglAAxPEfdLTs4BZM4DIO4C7OwQg2JoQ9LE1exdlYvBBeZ7jqch9//q1uH4TLzw4d6+ErXMMcXuHWxId3KOETnnXXV6MJpcq2MLaI97CER3N0vr4MkhoXe0rZigAAAABJRU5ErkJggg=="></object>
+  <object data="data:text/html,foo"></object>
+</div>
+<pre id="test">
+<script type="application/javascript">
+
+/** Test for Bug 596350 **/
+
+SimpleTest.waitForExplicitFinish();
+
+var testData = [
+// Object 0
+  [ 0, null, "-1" ],
+  [ 0, "1", "1" ],
+  [ 0, "-1", "-1" ],
+  [ 0, "0", "0" ],
+  [ 0, "foo", "-1" ],
+// Object 1
+  [ 1, null, "-1" ],
+  [ 1, "1", "1" ],
+// Object 2
+  [ 2, null, "0" ],
+  [ 2, "1", "1" ],
+  [ 2, "-1", "-1" ],
+];
+
+var objects = document.getElementsByTagName("object");
+
+function runTests()
+{
+  for each(var data in testData) {
+    var obj = objects[data[0]];
+
+    if (data[1]) {
+      obj.setAttribute("tabindex", data[1]);
+    }
+
+    is(obj.tabIndex, data[2], "tabIndex value should be " + data[2]);
+
+    obj.removeAttribute("tabindex");
+  }
+
+  SimpleTest.finish();
+}
+
+</script>
+</pre>
+</body>
+</html>