Closed Bug 1361879 Opened 3 years ago Closed 3 years ago

sdnAccessible tearoff breaks COM object identity

Categories

(Core :: Disability Access APIs, enhancement)

Unspecified
Windows
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: aklotz, Assigned: aklotz)

References

Details

(Whiteboard: [aes+][JAWS])

Attachments

(1 file, 1 obsolete file)

sdnAccessible::QueryInterface(IID_IUnknown) needs to return the IUnknown of the original AccessibleWrap.

This is not a problem if one goes through IServiceProvider, but if they just QI directly for IID_ISimpleDOMNode then things break.
Whiteboard: aes+ → [aes+][JAWS]
It turns out that this kinda works (we query that stuff from the node) but we revert back to using our own unknown if we can't resolve an accessible from that node.

As a tearoff we should keep a strong reference to the original AccessibleWrap so that this association never breaks.
(In reply to Aaron Klotz [:aklotz] (a11y work receiving priority right now) from comment #1)
> It turns out that this kinda works (we query that stuff from the node) but
> we revert back to using our own unknown if we can't resolve an accessible
> from that node.
> 
> As a tearoff we should keep a strong reference to the original
> AccessibleWrap so that this association never breaks.

I believe these objects are sometimes created on there own as non tearoffs, so that is correct in some cases, but we may need a strong ref to make the tear off case correct.
Sounds reasonable.
Assignee: nobody → aklotz
Status: NEW → ASSIGNED
Attachment #8867932 - Flags: review?(tbsaunde+mozbugs)
Attachment #8867932 - Attachment is obsolete: true
Attachment #8867932 - Flags: review?(tbsaunde+mozbugs)
Attachment #8868063 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8868063 [details] [diff] [review]
Hold a strong reference to associated AccessibleWrap

>   DocAccessible* document = GetDocument();
>-  return document ? document->GetAccessibleEvenIfNotInMap(mNode) : nullptr;
>+  if (!document) {
>+    return nullptr;
>+  }
>+
>+  // Once we have an accessible, we should hold a reference to it so that we
>+  // may preserve object identity.
>+  mWrap =
>+    static_cast<AccessibleWrap*>(document->GetAccessibleEvenIfNotInMap(mNode));

this will have interesting behavior when you hold on to a sdnAccessible accross an accessible being recreated for something like a layout change, but you choose to use this interface you get to keep the pieces :p

>+  explicit sdnAccessible(NotNull<AccessibleWrap*> aAccWrap)
>+    : mNode(aAccWrap->GetNode())
>+    , mWrap(aAccWrap)
>+  {
>+  }

I'd be fine with compressing the lines here a bit.

>-  Accessible* GetAccessible() const;
>+  Accessible* GetAccessible();

this could be AccessibleWrap* now and we could drop some casts right?
Attachment #8868063 - Flags: review?(tbsaunde+mozbugs) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7c33d5c6270bbe2ad93f21fd0e7347debb788de
Bug 1361879: Ensure that sdnAccessible holds a strong reference to its creating AccessibleWrap if it was instantiated as a tearoff; r=tbsaunde
https://hg.mozilla.org/mozilla-central/rev/d7c33d5c6270
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1367715
You need to log in before you can comment on or make changes to this bug.