bug 809338 - don't flatten optgroups r=surkov

accessible/src/base/AccTypes.h

@@ -48,6 +48,7 @@ enum AccType {
    * Other accessible types.
    */
   eApplicationType,
+  eHTMLOptGroupType,
   eImageMapType,
   eMenuPopupType,
   eProgressType,

accessible/src/generic/Accessible.h

@@ -488,6 +488,8 @@ public:
   bool IsHTMLListItem() const { return mType == eHTMLLiType; }
   HTMLLIAccessible* AsHTMLListItem();
 
+  bool IsHTMLOptGroup() const { return mType == eHTMLOptGroupType; }
+
   bool IsHTMLTable() const { return mType == eHTMLTableType; }
   bool IsHTMLTableRow() const { return mType == eHTMLTableRowType; }
 

accessible/src/generic/DocAccessible.cpp

@@ -1748,25 +1748,6 @@ DocAccessible::UpdateTree(Accessible* aContainer, nsIContent* aChildNode,
 
   if (child) {
     updateFlags |= UpdateTreeInternal(child, aIsInsert, reorderEvent);
-
-    // XXX: since select change insertion point of option contained by optgroup
-    // then we need to have special processing for them (bug 690417).
-    if (!aIsInsert && aChildNode->IsHTML(nsGkAtoms::optgroup) &&
-        aContainer->GetContent() &&
-        aContainer->GetContent()->IsHTML(nsGkAtoms::select)) {
-      for (nsIContent* optContent = aChildNode->GetFirstChild(); optContent;
-           optContent = optContent->GetNextSibling()) {
-        if (optContent->IsHTML(nsGkAtoms::option)) {
-          Accessible* option = GetAccessible(optContent);
-          if (option) {
-            NS_ASSERTION(option->Parent() == aContainer,
-                         "Not expected hierarchy on HTML select!");
-            if (option->Parent() == aContainer)
-              updateFlags |= UpdateTreeInternal(option, aIsInsert, reorderEvent);
-          }
-        }
-      }
-    }
   } else {
     TreeWalker walker(aContainer, aChildNode, true);
 

accessible/src/html/HTMLSelectAccessible.cpp

@@ -125,16 +125,7 @@ HTMLSelectListAccessible::CacheChildren()
   // as well as the accessibles for them. Avoid whitespace text nodes. We want
   // to count all the <optgroup>s and <option>s as children because we want
   // a flat tree under the Select List.
-  CacheOptSiblings(mContent);
+  for (nsIContent* childContent = mContent->GetFirstChild(); childContent;
-}
-
-////////////////////////////////////////////////////////////////////////////////
-// HTMLSelectListAccessible protected
-
-void
-HTMLSelectListAccessible::CacheOptSiblings(nsIContent* aParentContent)
-{
-  for (nsIContent* childContent = aParentContent->GetFirstChild(); childContent;
        childContent = childContent->GetNextSibling()) {
     if (!childContent->IsHTML()) {
       continue;
@@ -149,10 +140,6 @@ HTMLSelectListAccessible::CacheOptSiblings(nsIContent* aParentContent)
         GetAccService()->GetOrCreateAccessible(childContent, this);
       if (accessible)
         AppendChild(accessible);
-
-      // Deep down into optgroup element.
-      if (tag == nsGkAtoms::optgroup)
-        CacheOptSiblings(childContent);
     }
   }
 }
@@ -174,7 +161,7 @@ HTMLSelectOptionAccessible::
 role
 HTMLSelectOptionAccessible::NativeRole()
 {
-  if (mParent && mParent->Role() == roles::COMBOBOX_LIST)
+  if (GetCombobox())
     return roles::COMBOBOX_OPTION;
 
   return roles::OPTION;
@@ -333,23 +320,21 @@ HTMLSelectOptionAccessible::SetSelected(bool aSelect)
 Accessible*
 HTMLSelectOptionAccessible::ContainerWidget() const
 {
-  return mParent && mParent->IsListControl() ? mParent : nullptr;
+  Accessible* parent = Parent();
+  if (parent && parent->IsHTMLOptGroup())
+    parent = parent->Parent();
+
+  return parent && parent->IsListControl() ? parent : nullptr;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // HTMLSelectOptGroupAccessible
 ////////////////////////////////////////////////////////////////////////////////
 
-HTMLSelectOptGroupAccessible::
-  HTMLSelectOptGroupAccessible(nsIContent* aContent, DocAccessible* aDoc) :
-  HTMLSelectOptionAccessible(aContent, aDoc)
-{
-}
-
 role
 HTMLSelectOptGroupAccessible::NativeRole()
 {
-  return roles::HEADING;
+  return roles::GROUPING;
 }
 
 uint64_t
@@ -376,20 +361,6 @@ HTMLSelectOptGroupAccessible::ActionCount()
   return 0;
 }
 
-////////////////////////////////////////////////////////////////////////////////
-// HTMLSelectOptGroupAccessible: Accessible protected
-
-void
-HTMLSelectOptGroupAccessible::CacheChildren()
-{
-  // XXX To do (bug 378612) - create text child for the anonymous attribute
-  // content, so that nsIAccessibleText is supported for the <optgroup> as it is
-  // for an <option>. Attribute content is what layout creates for
-  // the label="foo" on the <optgroup>. See eStyleContentType_Attr and
-  // CreateAttributeContent() in nsCSSFrameConstructor
-}
-
-
 ////////////////////////////////////////////////////////////////////////////////
 // HTMLComboboxAccessible
 ////////////////////////////////////////////////////////////////////////////////

accessible/src/html/HTMLSelectAccessible.h

@@ -57,13 +57,6 @@ protected:
 
   // Accessible
   virtual void CacheChildren();
-
-  // HTMLSelectListAccessible
-
-  /**
-   * Recursive helper for CacheChildren().
-   */
-  void CacheOptSiblings(nsIContent* aParentContent);
 };
 
 /*
@@ -107,8 +100,12 @@ private:
    */
   Accessible* GetSelect() const
   {
-    if (mParent && mParent->IsListControl()) {
+    Accessible* parent = mParent;
-      Accessible* combobox = mParent->Parent();
+    if (parent && parent->IsHTMLOptGroup())
+      parent = parent->Parent();
+
+    if (parent && parent->IsListControl()) {
+      Accessible* combobox = parent->Parent();
       return combobox && combobox->IsCombobox() ? combobox : mParent.get();
     }
 
@@ -120,8 +117,12 @@ private:
    */
   Accessible* GetCombobox() const
   {
-    if (mParent && mParent->IsListControl()) {
+    Accessible* parent = mParent;
-      Accessible* combobox = mParent->Parent();
+    if (parent && parent->IsHTMLOptGroup())
+      parent = parent->Parent();
+
+    if (parent && parent->IsListControl()) {
+      Accessible* combobox = parent->Parent();
       return combobox && combobox->IsCombobox() ? combobox : nullptr;
     }
 
@@ -136,7 +137,9 @@ class HTMLSelectOptGroupAccessible : public HTMLSelectOptionAccessible
 {
 public:
 
-  HTMLSelectOptGroupAccessible(nsIContent* aContent, DocAccessible* aDoc);
+  HTMLSelectOptGroupAccessible(nsIContent* aContent, DocAccessible* aDoc) :
+    HTMLSelectOptionAccessible(aContent, aDoc)
+    { mType = eHTMLOptGroupType; }
   virtual ~HTMLSelectOptGroupAccessible() {}
 
   // nsIAccessible
@@ -149,10 +152,6 @@ public:
 
   // ActionAccessible
   virtual uint8_t ActionCount();
-
-protected:
-  // Accessible
-  virtual void CacheChildren();
 };
 
 /** ------------------------------------------------------ */

accessible/tests/mochitest/focus/test_takeFocus.html

@@ -57,6 +57,9 @@
       gQueue.push(new takeFocusInvoker("plugin"));
       gQueue.push(new takeFocusInvoker(document));
       gQueue.push(new takeFocusInvoker("lb_item2"));
+      gQueue.push(new takeFocusInvoker(document));
+      gQueue.push(new takeFocusInvoker("lb_item3.2"));
+      gQueue.push(new takeFocusInvoker("lb_item3.1"));
 
       gQueue.invoke(); // Will call SimpleTest.finish();
     }
@@ -114,6 +117,10 @@
   <select id="listbox" size="5">
     <option id="lb_item1">item1</option>
     <option id="lb_item2">item2</option>
+    <optgroup>
+      <option id="lb_item3.1">item 3.1</option>
+      <option id="lb_item3.2">item 3.2</option>
+    </optgroup>
   </select>
 </body>
 </html>

accessible/tests/mochitest/tree/test_select.html

@@ -20,21 +20,27 @@
         role: ROLE_LISTBOX,
         children: [
           {
-            role: ROLE_HEADING
+            role: ROLE_GROUPING,
-          },
-          {
-            role: ROLE_OPTION,
             children: [
               {
-                role: ROLE_TEXT_LEAF
+                role: ROLE_STATICTEXT,
-              }
+                children: [ ]
-            ]
+              },
-          },
-          {
-            role: ROLE_OPTION,
-            children: [
               {
-                role: ROLE_TEXT_LEAF
+                role: ROLE_OPTION,
+                children: [
+                  {
+                    role: ROLE_TEXT_LEAF
+                  }
+                ]
+              },
+              {
+                role: ROLE_OPTION,
+                children: [
+                  {
+                    role: ROLE_TEXT_LEAF
+                  }
+                ]
               }
             ]
           },
@@ -57,24 +63,30 @@
             role: ROLE_COMBOBOX_LIST,
             children: [
               {
-                role: ROLE_HEADING
+                role: ROLE_GROUPING,
-              },
-              {
-                role: ROLE_COMBOBOX_OPTION,
                 children: [
                   {
-                    role: ROLE_TEXT_LEAF
+                    role: ROLE_STATICTEXT,
-                  }
+                    children: [ ]
-                ]
+                  },
-              },
-              {
-                role: ROLE_COMBOBOX_OPTION,
-                children: [
                   {
-                    role: ROLE_TEXT_LEAF
+                    role: ROLE_COMBOBOX_OPTION,
-                  }
+                    children: [
+                      {
+                        role: ROLE_TEXT_LEAF
+                      }
+                    ]
+                  },
+                  {
+                    role: ROLE_COMBOBOX_OPTION,
+                    children: [
+                      {
+                        role: ROLE_TEXT_LEAF
+                      }
+                    ]
+                  },
                 ]
-              },
+},
               {
                 role: ROLE_COMBOBOX_OPTION,
                 children: [