Closed Bug 1259023 Opened 8 years ago Closed 8 years ago

make more of nsIAccessible work with proxied accessibles

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: tbsaunde, Assigned: tbsaunde)

References

Details

Attachments

(6 files, 1 obsolete file)

      No description provided.
Attachment #8733850 - Flags: review?(amarchesini)
Attached patch 1259023 patchSplinter Review
Attachment #8737320 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8737320 [details] [diff] [review]
1259023 patch

>-    mBits(reinterpret_cast<uintptr_t>(aProxy) | IS_PROXY) {}
>+    mBits(aProxy == nullptr ? 0 : (reinterpret_cast<uintptr_t>(aProxy) | IS_PROXY)) {}

aProxy ? reinterpret_cast<>() : 0 seems more normal.

> NS_IMETHODIMP
> xpcAccessible::GetNextSibling(nsIAccessible** aNextSibling)
> {
>   NS_ENSURE_ARG_POINTER(aNextSibling);
>   *aNextSibling = nullptr;
>+  if (IntlGeneric().IsNull())
>+    return NS_ERROR_FAILURE;
>
>   if (IntlGeneric().IsAccessible()) {

given these conditions you can change the check proxy is not null to an assert here and below, but I guess its not really important.
Flags: needinfo?(tbsaunde+mozbugs)
Attachment #8737320 - Flags: review?(tbsaunde+mozbugs) → review+
Attachment #8737336 - Flags: review?(tbsaunde+mozbugs) → review+
Comment on attachment 8733850 [details] [diff] [review]
make nsIAccessible.parent work with proxies

Review of attachment 8733850 [details] [diff] [review]:
-----------------------------------------------------------------

Sealing review as per Trevor's request.

::: accessible/atk/AccessibleWrap.cpp
@@ +1089,5 @@
> +AtkObject*
> +GetWrapperFor(AccessibleOrProxy aObj)
> +{
> +  if (aObj.IsProxy()) {
> +      return GetWrapperFor(aObj.AsProxy());

nit: indentation here and below.
Attachment #8733850 - Flags: review?(amarchesini) → review+
Comment on attachment 8733851 [details] [diff] [review]
make nsIAccessible.indexInParent work on proxied accessibles

Review of attachment 8733851 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good with my follow up accounting for nullptr.
Attachment #8733851 - Flags: review?(amarchesini) → review+
Comment on attachment 8733853 [details] [diff] [review]
make nsIAccessible.{Next,Prev}Sibling work with proxied accessibles

Review of attachment 8733853 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good with my follow up handling nullptr
Attachment #8733853 - Flags: review?(amarchesini) → review+
https://hg.mozilla.org/mozilla-central/rev/b45ffb5760bc
https://hg.mozilla.org/mozilla-central/rev/e8c276c75965
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
I believe there are 3 more patches to land, I should've added leave-open for this bug.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch 1259023 patch 3 (obsolete) — Splinter Review
Don't check for Intl() since we have a check for isNull and isAccessible in next/previous sibling and index in parent methods.
Attachment #8738204 - Flags: review?(tbsaunde+mozbugs)
Note: without the change in accessible/tests/mochitest/selectable/a11y.ini the mochitests do not run locally after bug 1242051
(In reply to Yura Zenevich [:yzen] from comment #16)
> Note: without the change in accessible/tests/mochitest/selectable/a11y.ini
> the mochitests do not run locally after bug 1242051

Actually ignore the a11y.ini when reviewing, a follow up from the person updating the *.ini files is already in inbound.
Attached patch 1259023 patch 3Splinter Review
cleaned up version with not a11y.ini changes
Attachment #8738204 - Attachment is obsolete: true
Attachment #8738204 - Flags: review?(tbsaunde+mozbugs)
Attachment #8739015 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8739015 [details] [diff] [review]
1259023 patch 3

it'd really make more sense to squash this into the commits this effects.
Attachment #8739015 - Flags: review?(tbsaunde+mozbugs) → review+
Assignee: nobody → tbsaunde+mozbugs
You need to log in before you can comment on or make changes to this bug.