Closed
Bug 1361879
Opened 7 years ago
Closed 7 years ago
sdnAccessible tearoff breaks COM object identity
Categories
(Core :: Disability Access APIs, enhancement)
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: bugzilla, Assigned: bugzilla)
References
Details
(Whiteboard: [aes+][JAWS])
Attachments
(1 file, 1 obsolete file)
4.00 KB,
patch
|
tbsaunde
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•7 years ago
|
Whiteboard: aes+ → [aes+][JAWS]
Assignee | ||
Comment 1•7 years ago
|
||
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.
Comment 2•7 years ago
|
||
(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.
Assignee | ||
Comment 3•7 years ago
|
||
Sounds reasonable.
Assignee | ||
Comment 4•7 years ago
|
||
Assignee: nobody → aklotz
Status: NEW → ASSIGNED
Attachment #8867932 -
Flags: review?(tbsaunde+mozbugs)
Assignee | ||
Comment 5•7 years ago
|
||
Attachment #8867932 -
Attachment is obsolete: true
Attachment #8867932 -
Flags: review?(tbsaunde+mozbugs)
Attachment #8868063 -
Flags: review?(tbsaunde+mozbugs)
Comment 6•7 years ago
|
||
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+
Assignee | ||
Comment 7•7 years ago
|
||
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
Comment 8•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d7c33d5c6270
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•