Closed Bug 1187609 Opened 4 years ago Closed 4 years ago

Handle proxies in mozAccessible role, subrole

Categories

(Core :: Disability Access APIs, defect)

x86_64
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: lsocks, Assigned: lsocks)

Details

Attachments

(1 file, 3 obsolete files)

No description provided.
Attached patch role, subrole check for proxies (obsolete) — Splinter Review
The way this is set up isn't very clean, but I'm not sure if there's a better way to do it.
Assignee: nobody → lorien
Attachment #8638933 - Flags: feedback?(tbsaunde+mozbugs)
Attached patch proxies in role, subrole (obsolete) — Splinter Review
Attachment #8638933 - Attachment is obsolete: true
Attachment #8638933 - Flags: feedback?(tbsaunde+mozbugs)
Attachment #8639029 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8639029 [details] [diff] [review]
proxies in role, subrole

>+DocAccessibleChild::RecvLandmarkRole(const uint64_t& aID, nsString* aLandmark)
>+{
>+  Accessible* acc = IdToAccessible(aID);
>+  if (!acc)
>+    return true;
>+
>+  if (nsIAtom* roleAtom = (acc->LandmarkRole()))

isn't there some extra parens there?

>+{
>+  Accessible* acc = IdToAccessible(aID);
>+  if (!acc)
>+    return true;
>+
>+  if (nsIAtom* roleAtom = (*(acc->ARIARoleMap()->roleAtom)))

I think RoleMap() can return null

>+void
>+ProxyAccessible::LandmarkRole(nsIAtom* aLandmark) const
>+{
>+  nsString landmark;
>+  unused << mDoc->SendLandmarkRole(mID, &landmark);
>+  nsCOMPtr<nsIAtom> landmarkAtom = do_GetAtom(landmark);
>+  *aLandmark = *landmarkAtom.get();

how exactly do you expect that to work?  Why aren't you just returning a nsIAtom*?

>+}
>+
>+void ProxyAccessible::ARIARoleAtom(nsIAtom* aRole) const
>+{
>+  nsString role;
>+  unused << mDoc->SendARIARoleAtom(mID, &role);
>+  nsCOMPtr<nsIAtom> roleAtom = do_GetAtom(role);
>+  *aRole = *roleAtom.get();

same

>-  if (accWrap->HasARIARole()) {
>+  if (accWrap && accWrap->HasARIARole()) {
>     nsRoleMapEntry* roleMap = accWrap->ARIARoleMap();
>     if (roleMap->Is(nsGkAtoms::alert))
>       return @"AXApplicationAlert";
>     if (roleMap->Is(nsGkAtoms::alertdialog))
>       return @"AXApplicationAlertDialog";
>     if (roleMap->Is(nsGkAtoms::article))
>       return @"AXDocumentArticle";
>     if (roleMap->Is(nsGkAtoms::dialog))
>@@ -970,23 +975,58 @@ ConvertToNSArray(nsTArray<ProxyAccessibl
>       return @"AXApplicationStatus";
>     if (roleMap->Is(nsGkAtoms::tabpanel))
>       return @"AXTabPanel";
>     if (roleMap->Is(nsGkAtoms::timer))
>       return @"AXApplicationTimer";
>     if (roleMap->Is(nsGkAtoms::tooltip))
>       return @"AXUserInterfaceTooltip";
>   }
>+  if (proxy) {
>+    nsIAtom* roleAtom = nullptr;
>+    proxy->ARIARoleAtom(roleAtom);

so not only are you copying the atom, but you are trying to copy it to 0x0???

>+    if (roleAtom) {
>+      if (roleAtom == nsGkAtoms::alert)
>+        return @"AXApplicationAlert";
>+      if (roleAtom == nsGkAtoms::alertdialog)
>+        return @"AXApplicationAlertDialog";
>+      if (roleAtom == nsGkAtoms::article)
>+        return @"AXDocumentArticle";
>+      if (roleAtom == nsGkAtoms::dialog)
>+        return @"AXApplicationDialog";
>+      if (roleAtom == nsGkAtoms::document)
>+        return @"AXDocument";
>+      if (roleAtom == nsGkAtoms::log_)
>+        return @"AXApplicationLog";
>+      if (roleAtom == nsGkAtoms::marquee)
>+        return @"AXApplicationMarquee";
>+      if (roleAtom == nsGkAtoms::math)
>+        return @"AXDocumentMath";
>+      if (roleAtom == nsGkAtoms::note_)
>+        return @"AXDocumentNote";
>+      if (roleAtom == nsGkAtoms::region)
>+        return @"AXDocumentRegion";
>+      if (roleAtom == nsGkAtoms::status)
>+        return @"AXApplicationStatus";
>+      if (roleAtom == nsGkAtoms::tabpanel)
>+        return @"AXTabPanel";
>+      if (roleAtom == nsGkAtoms::timer)
>+        return @"AXApplicationTimer";
>+      if (roleAtom == nsGkAtoms::tooltip)
>+        return @"AXUserInterfaceTooltip";
>+    }

why can't you get the atom and share the rest of this?
Attachment #8639029 - Flags: review?(tbsaunde+mozbugs) → review-
Attached patch proxies in role, subrole (obsolete) — Splinter Review
Attachment #8639029 - Attachment is obsolete: true
Attachment #8641734 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8641734 [details] [diff] [review]
proxies in role, subrole

>+  if (nsIAtom* roleAtom = acc->LandmarkRole())
>+    roleAtom->ToString(*aLandmark);
>+  return true;

blank line before return please

>+  if (nsRoleMapEntry* roleMap = acc->ARIARoleMap())
>+    if (nsIAtom* roleAtom = *(roleMap->roleAtom))
>+      roleAtom->ToString(*aRole);
>+  return true;

same

>+  virtual bool RecvARIARoleAtom(const uint64_t& aID, nsString* aRole);

override

>+ProxyAccessible::LandmarkRole() const
>+{
>+  nsString landmark;
>+  unused << mDoc->SendLandmarkRole(mID, &landmark);
>+  nsCOMPtr<nsIAtom> landmarkAtom = do_GetAtom(landmark);
>+  return landmarkAtom.get();

just use NS_GetStaticAtom().   those methods on Accessible only ever return static atoms, so the child is broken if it sends us something else, and we don't want to create atoms we won't use for anything.

>+ProxyAccessible::ARIARoleAtom() const
>+{
>+  nsString role;
>+  unused << mDoc->SendARIARoleAtom(mID, &role);
>+  nsCOMPtr<nsIAtom> roleAtom = do_GetAtom(role);
>+  return roleAtom.get();

same
Attachment #8641734 - Flags: review?(tbsaunde+mozbugs) → review+
carry r=tbsaunde
Attachment #8641734 - Attachment is obsolete: true
Attachment #8643050 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/081d48ca7e97
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.