Bug 1369185 - Don't allow <select> to aria-own, or <option> to be owned. r=surkov
authorEitan Isaacson <eitan@monotonous.org>
Wed, 13 Sep 2017 14:04:00 -0400
changeset 381339 a2e766e76dba72797a9408d816394b42dc7e1ec8
parent 381338 b3f6d72c338e5ac6af99a68dac7c7807b40e2dac
child 381340 f39e7ce332c0fd66c0c05e44684daa53f70bcaac
push id32518
push userryanvm@gmail.com
push dateSat, 16 Sep 2017 20:51:51 +0000
treeherdermozilla-central@45b63125a430 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssurkov
bugs1369185
milestone57.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1369185 - Don't allow <select> to aria-own, or <option> to be owned. r=surkov
accessible/generic/Accessible.h
accessible/generic/DocAccessible.cpp
accessible/html/HTMLSelectAccessible.cpp
accessible/html/HTMLSelectAccessible.h
accessible/tests/browser/tree/browser_aria_owns.js
--- a/accessible/generic/Accessible.h
+++ b/accessible/generic/Accessible.h
@@ -493,17 +493,17 @@ public:
    * platform specific event.
    */
   virtual nsresult HandleAccEvent(AccEvent* aAccEvent);
 
   /**
    * Return true if the accessible is an acceptable child.
    */
   virtual bool IsAcceptableChild(nsIContent* aEl) const
-    { return true; }
+    { return aEl && !aEl->IsAnyOfHTMLElements(nsGkAtoms::option, nsGkAtoms::optgroup); }
 
   /**
    * Returns text of accessible if accessible has text role otherwise empty
    * string.
    *
    * @param aText         [in] returned text of the accessible
    * @param aStartOffset  [in, optional] start offset inside of the accessible,
    *                        if missed entire text is appended
--- a/accessible/generic/DocAccessible.cpp
+++ b/accessible/generic/DocAccessible.cpp
@@ -1306,19 +1306,19 @@ DocAccessible::BindToDocument(Accessible
   if (aAccessible->IsNodeMapEntry())
     mNodeToAccessibleMap.Put(aAccessible->GetNode(), aAccessible);
 
   // Put into unique ID cache.
   mAccessibleCache.Put(aAccessible->UniqueID(), aAccessible);
 
   aAccessible->SetRoleMapEntry(aRoleMapEntry);
 
-  AddDependentIDsFor(aAccessible);
+  if (aAccessible->HasOwnContent()) {
+    AddDependentIDsFor(aAccessible);
 
-  if (aAccessible->HasOwnContent()) {
     nsIContent* el = aAccessible->GetContent();
     if (el->HasAttr(kNameSpaceID_None, nsGkAtoms::aria_owns)) {
       mNotificationController->ScheduleRelocation(aAccessible);
     }
   }
 }
 
 void
@@ -1602,16 +1602,21 @@ DocAccessible::AddDependentIDsFor(Access
     }
 
     IDRefsIterator iter(this, relProviderEl, relAttr);
     while (true) {
       const nsDependentSubstring id = iter.NextID();
       if (id.IsEmpty())
         break;
 
+      nsIContent* dependentContent = iter.GetElem(id);
+      if (relAttr == nsGkAtoms::aria_owns &&
+          !aRelProvider->IsAcceptableChild(dependentContent))
+        continue;
+
       AttrRelProviderArray* providers = mDependentIDsHash.Get(id);
       if (!providers) {
         providers = new AttrRelProviderArray();
         if (providers) {
           mDependentIDsHash.Put(id, providers);
         }
       }
 
@@ -1620,17 +1625,16 @@ DocAccessible::AddDependentIDsFor(Access
           new AttrRelProvider(relAttr, relProviderEl);
         if (provider) {
           providers->AppendElement(provider);
 
           // We've got here during the children caching. If the referenced
           // content is not accessible then store it to pend its container
           // children invalidation (this happens immediately after the caching
           // is finished).
-          nsIContent* dependentContent = iter.GetElem(id);
           if (dependentContent) {
             if (!HasAccessible(dependentContent)) {
               mInvalidationList.AppendElement(dependentContent);
             }
           }
         }
       }
     }
--- a/accessible/html/HTMLSelectAccessible.cpp
+++ b/accessible/html/HTMLSelectAccessible.cpp
@@ -305,16 +305,23 @@ HTMLSelectOptGroupAccessible::NativeRole
 }
 
 uint64_t
 HTMLSelectOptGroupAccessible::NativeInteractiveState() const
 {
   return NativelyUnavailable() ? states::UNAVAILABLE : 0;
 }
 
+bool
+HTMLSelectOptGroupAccessible::IsAcceptableChild(nsIContent* aEl) const
+{
+  return aEl->IsNodeOfType(nsINode::eDATA_NODE) ||
+    aEl->IsHTMLElement(nsGkAtoms::option);
+}
+
 uint8_t
 HTMLSelectOptGroupAccessible::ActionCount()
 {
   return 0;
 }
 
 void
 HTMLSelectOptGroupAccessible::ActionNameAt(uint8_t aIndex, nsAString& aName)
@@ -453,16 +460,22 @@ HTMLComboboxAccessible::ActionNameAt(uin
     return;
 
   if (comboFrame->IsDroppedDown())
     aName.AssignLiteral("close");
   else
     aName.AssignLiteral("open");
 }
 
+bool
+HTMLComboboxAccessible::IsAcceptableChild(nsIContent* aEl) const
+{
+  return false;
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // HTMLComboboxAccessible: Widgets
 
 bool
 HTMLComboboxAccessible::IsWidget() const
 {
   return true;
 }
@@ -588,23 +601,28 @@ HTMLComboboxListAccessible::RelativeBoun
     *aBoundingFrame = nullptr;
     return nsRect();
   }
 
   *aBoundingFrame = frame->GetParent();
   return (*aBoundingFrame)->GetRect();
 }
 
+bool
+HTMLComboboxListAccessible::IsAcceptableChild(nsIContent* aEl) const
+{
+  return aEl->IsAnyOfHTMLElements(nsGkAtoms::option, nsGkAtoms::optgroup);
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // HTMLComboboxListAccessible: Widgets
 
 bool
 HTMLComboboxListAccessible::IsActiveWidget() const
 {
   return mParent && mParent->IsActiveWidget();
 }
 
 bool
 HTMLComboboxListAccessible::AreItemsOperable() const
 {
   return mParent && mParent->AreItemsOperable();
 }
-
--- a/accessible/html/HTMLSelectAccessible.h
+++ b/accessible/html/HTMLSelectAccessible.h
@@ -34,29 +34,28 @@ class HTMLSelectListAccessible : public 
 public:
 
   HTMLSelectListAccessible(nsIContent* aContent, DocAccessible* aDoc);
   virtual ~HTMLSelectListAccessible() {}
 
   // Accessible
   virtual a11y::role NativeRole() override;
   virtual uint64_t NativeState() override;
+  virtual bool IsAcceptableChild(nsIContent* aEl) const override;
 
   // SelectAccessible
   virtual bool SelectAll() override;
   virtual bool UnselectAll() override;
 
   // Widgets
   virtual bool IsWidget() const override;
   virtual bool IsActiveWidget() const override;
   virtual bool AreItemsOperable() const override;
   virtual Accessible* CurrentItem() override;
   virtual void SetCurrentItem(Accessible* aItem) override;
-
-  virtual bool IsAcceptableChild(nsIContent* aEl) const override;
 };
 
 /*
  * Options inside the select, contained within the list
  */
 class HTMLSelectOptionAccessible : public HyperTextAccessibleWrap
 {
 public:
@@ -133,16 +132,17 @@ public:
   HTMLSelectOptGroupAccessible(nsIContent* aContent, DocAccessible* aDoc) :
     HTMLSelectOptionAccessible(aContent, aDoc)
     { mType = eHTMLOptGroupType; }
   virtual ~HTMLSelectOptGroupAccessible() {}
 
   // Accessible
   virtual a11y::role NativeRole() override;
   virtual uint64_t NativeInteractiveState() const override;
+  virtual bool IsAcceptableChild(nsIContent* aEl) const override;
 
   // ActionAccessible
   virtual uint8_t ActionCount() override;
   virtual void ActionNameAt(uint8_t aIndex, nsAString& aName) override;
   virtual bool DoAction(uint8_t aIndex) override;
 };
 
 /** ------------------------------------------------------ */
@@ -164,16 +164,17 @@ public:
 
   // Accessible
   virtual void Shutdown() override;
   virtual void Description(nsString& aDescription) override;
   virtual void Value(nsString& aValue) override;
   virtual a11y::role NativeRole() override;
   virtual uint64_t NativeState() override;
   virtual bool RemoveChild(Accessible* aChild) override;
+  virtual bool IsAcceptableChild(nsIContent* aEl) const override;
 
   // ActionAccessible
   virtual uint8_t ActionCount() override;
   virtual void ActionNameAt(uint8_t aIndex, nsAString& aName) override;
   virtual bool DoAction(uint8_t aIndex) override;
 
   // Widgets
   virtual bool IsWidget() const override;
@@ -205,16 +206,17 @@ public:
                              DocAccessible* aDoc);
   virtual ~HTMLComboboxListAccessible() {}
 
   // Accessible
   virtual nsIFrame* GetFrame() const override;
   virtual a11y::role NativeRole() override;
   virtual uint64_t NativeState() override;
   virtual nsRect RelativeBounds(nsIFrame** aBoundingFrame) const override;
+  virtual bool IsAcceptableChild(nsIContent* aEl) const override;
 
   // Widgets
   virtual bool IsActiveWidget() const override;
   virtual bool AreItemsOperable() const override;
 };
 
 } // namespace a11y
 } // namespace mozilla
--- a/accessible/tests/browser/tree/browser_aria_owns.js
+++ b/accessible/tests/browser/tree/browser_aria_owns.js
@@ -81,41 +81,85 @@ addAccessibleTask(`
     await contentSpawnMutation(browser, NO_MOVE, function() {
       document.getElementById("container").setAttribute("aria-owns", "a");
     });
 
     testChildrenIds(containerAcc, []);
   }
 );
 
-// Correctly aria-own children of <select>
+// Don't aria-own children of <select>
 addAccessibleTask(`
   <div id="container" role="group" aria-owns="b"></div>
   <select id="select">
     <option id="a"></option>
     <option id="b"></option>
   </select>`,
   async function(browser, accDoc) {
     let containerAcc = findAccessibleChildByID(accDoc, "container");
     let selectAcc = findAccessibleChildByID(accDoc, "select");
 
-    testChildrenIds(containerAcc, ["b"]);
-    testChildrenIds(selectAcc.firstChild, ["a"]);
+    testChildrenIds(containerAcc, []);
+    testChildrenIds(selectAcc.firstChild, ["a", "b"]);
+  }
+);
+
+// Don't allow <select> to aria-own
+addAccessibleTask(`
+  <div id="container" role="group">
+    <div id="a"></div>
+    <div id="b"></div>
+  </div>
+  <select id="select" aria-owns="a">
+    <option id="c"></option>
+  </select>`,
+  async function(browser, accDoc) {
+    let containerAcc = findAccessibleChildByID(accDoc, "container");
+    let selectAcc = findAccessibleChildByID(accDoc, "select");
+
+    testChildrenIds(containerAcc, ["a", "b"]);
+    testChildrenIds(selectAcc.firstChild, ["c"]);
+  }
+);
 
-    let waitfor = { expected: [
-      [EVENT_REORDER, "container"],
-      [EVENT_REORDER,
-        evt => getAccessibleDOMNodeID(evt.accessible.parent) == "select"]] };
+// Don't allow one <select> to aria-own an <option> from another <select>.
+addAccessibleTask(`
+  <select id="select1" aria-owns="c">
+    <option id="a"></option>
+    <option id="b"></option>
+  </select>
+  <select id="select2">
+    <option id="c"></option>
+  </select>`,
+  async function(browser, accDoc) {
+    let selectAcc1 = findAccessibleChildByID(accDoc, "select1");
+    let selectAcc2 = findAccessibleChildByID(accDoc, "select2");
 
-    await contentSpawnMutation(browser, waitfor, function() {
-      document.getElementById("container").removeAttribute("aria-owns");
+    testChildrenIds(selectAcc1.firstChild, ["a", "b"]);
+    testChildrenIds(selectAcc2.firstChild, ["c"]);
+  }
+);
+
+// Don't allow a <select> to reorder its children with aria-owns.
+addAccessibleTask(`
+  <select id="container" aria-owns="c b a">
+    <option id="a"></option>
+    <option id="b"></option>
+    <option id="c"></option>
+  </select>`,
+  async function(browser, accDoc) {
+    let containerAcc = findAccessibleChildByID(accDoc, "container");
+
+    testChildrenIds(containerAcc.firstChild, ["a", "b", "c"]);
+
+    await contentSpawnMutation(browser, NO_MOVE, function() {
+      document.getElementById("container").setAttribute("aria-owns", "a c b");
     });
 
-    testChildrenIds(containerAcc, []);
-    testChildrenIds(selectAcc.firstChild, ["a", "b"]);
+    testChildrenIds(containerAcc.firstChild, ["a", "b", "c"]);
   }
 );
 
 addAccessibleTask(`
   <ul id="one">
     <li id="a">Test</li>
     <li id="b">Test 2</li>
     <li id="c">Test 3</li>