bug 809338 - don't flatten optgroups r=surkov
authorTrevor Saunders <trev.saunders@gmail.com>
Sat, 10 Nov 2012 04:41:40 -0500
changeset 129559 7da9a210f36594d1459d48d1edd7b79a8aa7a2ef
parent 129558 a6639d6743db4bfbb88e25214e466243bd8ad64a
child 129560 fe2fb8ee87801c32ab9a41c98a0cedf70a488d3e
push id1
push userroot
push dateMon, 20 Oct 2014 17:29:22 +0000
reviewerssurkov
bugs809338
milestone23.0a1
bug 809338 - don't flatten optgroups r=surkov
accessible/src/base/AccTypes.h
accessible/src/generic/Accessible.h
accessible/src/generic/DocAccessible.cpp
accessible/src/html/HTMLSelectAccessible.cpp
accessible/src/html/HTMLSelectAccessible.h
accessible/tests/mochitest/focus/test_takeFocus.html
accessible/tests/mochitest/tree/test_select.html
--- a/accessible/src/base/AccTypes.h
+++ b/accessible/src/base/AccTypes.h
@@ -43,16 +43,17 @@ enum AccType {
   eOuterDocType,
   ePluginType,
   eTextLeafType,
 
   /**
    * Other accessible types.
    */
   eApplicationType,
+  eHTMLOptGroupType,
   eImageMapType,
   eMenuPopupType,
   eProgressType,
   eRootType,
   eXULLabelType,
   eXULTabpanelsType,
   eXULTreeType,
 
--- a/accessible/src/generic/Accessible.h
+++ b/accessible/src/generic/Accessible.h
@@ -483,16 +483,18 @@ public:
   bool IsHyperText() const { return HasGenericType(eHyperText); }
   HyperTextAccessible* AsHyperText();
 
   bool IsHTMLFileInput() const { return mType == eHTMLFileInputType; }
 
   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; }
 
   bool IsImage() const { return mType == eImageType; }
   ImageAccessible* AsImage();
 
   bool IsImageMap() const { return mType == eImageMapType; }
   HTMLImageMapAccessible* AsImageMap();
--- a/accessible/src/generic/DocAccessible.cpp
+++ b/accessible/src/generic/DocAccessible.cpp
@@ -1743,35 +1743,16 @@ DocAccessible::UpdateTree(Accessible* aC
     logging::MsgEnd();
   }
 #endif
 
   nsRefPtr<AccReorderEvent> reorderEvent = new AccReorderEvent(aContainer);
 
   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);
 
     while ((child = walker.NextChild()))
       updateFlags |= UpdateTreeInternal(child, aIsInsert, reorderEvent);
   }
 
   // Content insertion/removal is not cause of accessible tree change.
--- a/accessible/src/html/HTMLSelectAccessible.cpp
+++ b/accessible/src/html/HTMLSelectAccessible.cpp
@@ -120,44 +120,31 @@ HTMLSelectListAccessible::SetCurrentItem
 
 void
 HTMLSelectListAccessible::CacheChildren()
 {
   // Cache accessibles for <optgroup> and <option> DOM decendents as children,
   // 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);
-}
-
-////////////////////////////////////////////////////////////////////////////////
-// HTMLSelectListAccessible protected
-
-void
-HTMLSelectListAccessible::CacheOptSiblings(nsIContent* aParentContent)
-{
-  for (nsIContent* childContent = aParentContent->GetFirstChild(); childContent;
+  for (nsIContent* childContent = mContent->GetFirstChild(); childContent;
        childContent = childContent->GetNextSibling()) {
     if (!childContent->IsHTML()) {
       continue;
     }
 
     nsIAtom* tag = childContent->Tag();
     if (tag == nsGkAtoms::option ||
         tag == nsGkAtoms::optgroup) {
 
       // Get an accessible for option or optgroup and cache it.
       nsRefPtr<Accessible> accessible =
         GetAccService()->GetOrCreateAccessible(childContent, this);
       if (accessible)
         AppendChild(accessible);
-
-      // Deep down into optgroup element.
-      if (tag == nsGkAtoms::optgroup)
-        CacheOptSiblings(childContent);
     }
   }
 }
 
 
 ////////////////////////////////////////////////////////////////////////////////
 // HTMLSelectOptionAccessible
 ////////////////////////////////////////////////////////////////////////////////
@@ -169,17 +156,17 @@ HTMLSelectOptionAccessible::
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // HTMLSelectOptionAccessible: Accessible public
 
 role
 HTMLSelectOptionAccessible::NativeRole()
 {
-  if (mParent && mParent->Role() == roles::COMBOBOX_LIST)
+  if (GetCombobox())
     return roles::COMBOBOX_OPTION;
 
   return roles::OPTION;
 }
 
 ENameValueFlag
 HTMLSelectOptionAccessible::NativeName(nsString& aName)
 {
@@ -328,33 +315,31 @@ HTMLSelectOptionAccessible::SetSelected(
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // HTMLSelectOptionAccessible: Widgets
 
 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
 HTMLSelectOptGroupAccessible::NativeInteractiveState() const
 {
   return NativelyUnavailable() ? states::UNAVAILABLE : 0;
 }
 
@@ -372,30 +357,16 @@ HTMLSelectOptGroupAccessible::GetActionN
 
 uint8_t
 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
 ////////////////////////////////////////////////////////////////////////////////
 
 HTMLComboboxAccessible::
   HTMLComboboxAccessible(nsIContent* aContent, DocAccessible* aDoc) :
   AccessibleWrap(aContent, aDoc)
 {
   mGenericTypes |= eCombobox;
--- a/accessible/src/html/HTMLSelectAccessible.h
+++ b/accessible/src/html/HTMLSelectAccessible.h
@@ -52,23 +52,16 @@ public:
   virtual bool AreItemsOperable() const;
   virtual Accessible* CurrentItem();
   virtual void SetCurrentItem(Accessible* aItem);
 
 protected:
 
   // Accessible
   virtual void CacheChildren();
-
-  // HTMLSelectListAccessible
-
-  /**
-   * Recursive helper for CacheChildren().
-   */
-  void CacheOptSiblings(nsIContent* aParentContent);
 };
 
 /*
  * Options inside the select, contained within the list
  */
 class HTMLSelectOptionAccessible : public HyperTextAccessibleWrap
 {
 public:
@@ -102,62 +95,68 @@ protected:
 
 private:
 
   /**
    * Return a select accessible the option belongs to if any.
    */
   Accessible* GetSelect() const
   {
-    if (mParent && mParent->IsListControl()) {
-      Accessible* combobox = mParent->Parent();
+    Accessible* parent = mParent;
+    if (parent && parent->IsHTMLOptGroup())
+      parent = parent->Parent();
+
+    if (parent && parent->IsListControl()) {
+      Accessible* combobox = parent->Parent();
       return combobox && combobox->IsCombobox() ? combobox : mParent.get();
     }
 
     return nullptr;
   }
 
   /**
    * Return a combobox accessible the option belongs to if any.
    */
   Accessible* GetCombobox() const
   {
-    if (mParent && mParent->IsListControl()) {
-      Accessible* combobox = mParent->Parent();
+    Accessible* parent = mParent;
+    if (parent && parent->IsHTMLOptGroup())
+      parent = parent->Parent();
+
+    if (parent && parent->IsListControl()) {
+      Accessible* combobox = parent->Parent();
       return combobox && combobox->IsCombobox() ? combobox : nullptr;
     }
 
     return nullptr;
   }
 };
 
 /*
  * Opt Groups inside the select, contained within the list
  */
 class HTMLSelectOptGroupAccessible : public HTMLSelectOptionAccessible
 {
 public:
 
-  HTMLSelectOptGroupAccessible(nsIContent* aContent, DocAccessible* aDoc);
+  HTMLSelectOptGroupAccessible(nsIContent* aContent, DocAccessible* aDoc) :
+    HTMLSelectOptionAccessible(aContent, aDoc)
+    { mType = eHTMLOptGroupType; }
   virtual ~HTMLSelectOptGroupAccessible() {}
 
   // nsIAccessible
   NS_IMETHOD DoAction(uint8_t index);
   NS_IMETHOD GetActionName(uint8_t aIndex, nsAString& aName);
 
   // Accessible
   virtual a11y::role NativeRole();
   virtual uint64_t NativeInteractiveState() const;
 
   // ActionAccessible
   virtual uint8_t ActionCount();
-
-protected:
-  // Accessible
-  virtual void CacheChildren();
 };
 
 /** ------------------------------------------------------ */
 /**  Finally, the Combobox widgets                         */
 /** ------------------------------------------------------ */
 
 class HTMLComboboxListAccessible;
 
--- a/accessible/tests/mochitest/focus/test_takeFocus.html
+++ b/accessible/tests/mochitest/focus/test_takeFocus.html
@@ -52,16 +52,19 @@
 
       gQueue.push(new takeFocusInvoker("aria-link"));
       gQueue.push(new takeFocusInvoker("aria-link2"));
       gQueue.push(new takeFocusInvoker("link"));
       gQueue.push(new takeFocusInvoker("item2"));
       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();
     }
 
     function waitForPlugin()
     {
       window.setTimeout((isAccessible("plugin") ? doTest : waitForPlugin), 0);
     }
@@ -109,11 +112,15 @@
     <div role="option" id="item3">item3</div>
   </div>
 
   <embed id="plugin" type="application/x-test" width="200" height="200" wmode="window"></embed>
 
   <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>
--- a/accessible/tests/mochitest/tree/test_select.html
+++ b/accessible/tests/mochitest/tree/test_select.html
@@ -15,31 +15,37 @@
 
   <script type="application/javascript">
     function doTest()
     {
       var accTree = {
         role: ROLE_LISTBOX,
         children: [
           {
-            role: ROLE_HEADING
-          },
-          {
-            role: ROLE_OPTION,
+            role: ROLE_GROUPING,
             children: [
               {
-                role: ROLE_TEXT_LEAF
-              }
-            ]
-          },
-          {
-            role: ROLE_OPTION,
-            children: [
+                role: ROLE_STATICTEXT,
+                children: [ ]
+              },
               {
-                role: ROLE_TEXT_LEAF
+                role: ROLE_OPTION,
+                children: [
+                  {
+                    role: ROLE_TEXT_LEAF
+                  }
+                ]
+              },
+              {
+                role: ROLE_OPTION,
+                children: [
+                  {
+                    role: ROLE_TEXT_LEAF
+                  }
+                ]
               }
             ]
           },
           {
             role: ROLE_OPTION,
             children: [
               {
                 role: ROLE_TEXT_LEAF
@@ -52,34 +58,40 @@
 
       accTree = {
         role: ROLE_COMBOBOX,
         children: [
           {
             role: ROLE_COMBOBOX_LIST,
             children: [
               {
-                role: ROLE_HEADING
-              },
-              {
-                role: ROLE_COMBOBOX_OPTION,
+                role: ROLE_GROUPING,
                 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: [
                   {
                     role: ROLE_TEXT_LEAF
                   }
                 ]
               }