Closed Bug 1322532 Opened 8 years ago Closed 8 years ago

[e10s a11y] Live regions broken on Windows

Categories

(Core :: Disability Access APIs, defect)

Product:
Core
Component:
Disability Access APIs
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox-esr52 --- disabled
firefox53 --- disabled
firefox54 --- disabled
firefox55 --- fixed

People

(Reporter: Jamie, Assigned: bugzilla)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: aes+)

Attachments

(4 files)

Part 1: Add live region support to handler dll
22.97 KB, patch
tbsaunde
: review+
Details | Diff | Splinter Review
Part 2: Move HWND resolution into DocAccessibleChild
3.23 KB, patch
yzen
: review+
Details | Diff | Splinter Review
Part 3: Add SyncTextChangeEvent
1.89 KB, patch
tbsaunde
: review+
billm
: review+
Details | Diff | Splinter Review
Part 4: Platform a11y changes to dispatch text change events to the handler
8.62 KB, patch
tbsaunde
: review+
Details | Diff | Splinter Review
E10s breaks ARIA live regions in Windows if a live region update occurs at the same time as other updates on the page. This is so common that in real world terms, e10s breaks live regions on Windows altogether. :) For IA2, live regions for text changes are implemented using textInserted/textRemoved events and the IAccessibleText::get_newText/get_oldText methods. Because win events can't provide data beyond the object they target, get_newText/get_oldText must be called synchronously within an in-context event handler to retrieve information about the changed text (the text and offsets). This breaks with e10s because events are now fired from the parent process but we communicate with the accessibles in the child process, so when a client calls get_newText/get_oldText, the child process has almost certainly handled another update by then. This is bad enough when there are two updates in quick succession in the same accessible, but this is made even worse by the fact that Gecko implements get_newText/get_oldText using process globals. I don't know that it's going to be possible to fix this in the child. However, one idea I had was to handle this using the COM smart proxy. The COM smart proxy would be entirely responsible for implementing IAccessibleText::get_newText/get_oldText. My thinking is as follows: 1. The child sends a text change event to the parent including information about the changed text. I believe this already happens. 2. The parent saves the information for the last text change. 3. The parent fires an appropriate text change win event. 4. An in-process client win event callback gets called (e.g. NVDA's live region handler). 5. The client callback gets the object for the event, thus creating a COM smart proxy. 6. The client callback calls IAccessibleText::get_newText/get_oldText. 7. The COM smart proxy's implementation of this method detects that it's running in the Firefox parent process and returns the information saved in step 2 above. Aaron, Trev, do you think this sounds reasonable?
Blocks: 1258839
Putting in flag and keyword since this regresses a major pillar of WAI-ARIA.
Keywords: regression
Whiteboard: aes+
I've been considering how the COM smart proxy dll will know it's in the Firefox parent process and how it will get the text change info. Here's my suggestion (though please disregard if you have a better one): 1. The proxy dll would have a private function to set the last text change info. This would set process globals in the dll. 2. Firefox parent would load the proxy dll and call this function each time the last text change info needs to be updated. 3. When IAccessibleText::get_newText/get_oldText is called on the proxy, it would check the global set by Firefox (point 2). If the globals are set, it would return appropriate info. Otherwise, it would return nothing. 4. In processes other than the Firefox parent process, the function (point 1) would never be called and so the text change methods would always return nothing.
Depends on: 1322593
No longer depends on: 1322593
Assignee: nobody → aklotz
Depends on: 1303060
We need to add live region support to the handler. Since a text change event might be raised by a content accessible, we need the handler to retrieve that info *before* the call reaches the proxy, otherwise it would just query the content process. We do this by implementing a tearoff for IAccessibleHypertext that passes through everything except for get_oldText and get_newText. The text changes are managed by AccessibleHandlerControl.
Attachment #8854621 - Flags: review?(tbsaunde+mozbugs)
Because of the way that live region events work, under certain circumstances we must use a sync message to dispatch the text change event. This is because the client is expecting to be able to reenter content to query information from child nodes of the node who fired the event. FWIW use the async variant whenever we can, but if we encounter a text change event that contains a sentinel character, we know that the client needs to call back into content and use the sync variant in that case.
Attachment #8854623 - Flags: review?(wmccloskey)
Attachment #8854623 - Flags: review?(tbsaunde+mozbugs)
If the handler is enabled, we invoke IHandlerControl::OnTextChange to fire the text change event instead of using ia2AccessibleText. We use mscom's ASYNC_INVOKER_FOR and ASYNC_INVOKE macros so that the code is flexible enough to be able to dispatch either to an in-proc handler (most likely, at least at this time) or an out-of-prof handler, depending on what is present.
Attachment #8854624 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8854622 [details] [diff] [review] Part 2: Move HWND resolution into DocAccessibleChild Review of attachment 8854622 [details] [diff] [review]: ----------------------------------------------------------------- looks hood thanks
Attachment #8854622 - Flags: review?(yzenevich) → review+
(In reply to Aaron Klotz [:aklotz] from comment #7) > Created attachment 8854623 [details] [diff] [review] > Part 3: Add SyncTextChangeEvent > > Because of the way that live region events work, under certain circumstances > we must use a sync message to dispatch the text change event. This is > because the client is expecting to be able to reenter content to query > information from child nodes of the node who fired the event. > > FWIW use the async variant whenever we can, but if we encounter a text > change event that contains a sentinel character, we know that the client > needs to call back into content and use the sync variant in that case. How commonly will we use the sync variant? Does it only happen when a screen reader is installed?
Flags: needinfo?(aklotz)
Comment on attachment 8854621 [details] [diff] [review] Part 1: Add live region support to handler dll >+TextChange& >+TextChange::operator=(TextChange&& aOther) >+{ >+ mIA2UniqueId = aOther.mIA2UniqueId; >+ mIsInsert = aOther.mIsInsert; >+ aOther.mIA2UniqueId = 0; >+ mText = aOther.mText; >+ aOther.mText.text = nullptr; you need to free the old text if there was any right? which means the move ctor needs to make sure the field is initialized first right? >+TextChange& >+TextChange::operator=(const TextChange& aOther) >+{ >+ mIA2UniqueId = aOther.mIA2UniqueId; >+ mIsInsert = aOther.mIsInsert; >+ mText = {BSTRCopy(aOther.mText.text), aOther.mText.start, aOther.mText.end}; same with s/move/copy/ >+TextChange::GetOld(long aIA2UniqueId, IA2TextSegment* aOutOldSegment) >+{ >+ if (!aOutOldSegment) { >+ return E_INVALIDARG; the caller already null checked didn't they? >+TextChange::GetNew(long aIA2UniqueId, IA2TextSegment* aOutNewSegment) >+{ >+ if (!aOutNewSegment) { >+ return E_INVALIDARG; same >+AccessibleHandlerControl::OnTextChange(long aHwnd, long aIA2UniqueId, >+ VARIANT_BOOL aIsInsert, >+ IA2TextSegment* aText) >+{ >+ // OnTextChange implies invalidate >+ Invalidate(); hrm, should it? its not clear to me text changing forces us to invalidate anything else. >+ >+ mTextChange = detail::TextChange(aIA2UniqueId, aIsInsert, aText); >+ NotifyWinEvent(aIsInsert ? IA2_EVENT_TEXT_INSERTED : IA2_EVENT_TEXT_REMOVED, >+ reinterpret_cast<HWND>(static_cast<uintptr_t>(aHwnd)), >+ OBJID_CLIENT, aIA2UniqueId); I thought we thought this should be done from the thread owning the HWND which this might not be? That said I guess this maybe kind of makes it work for out of process clients? >+AccessibleHandlerControl::GetNewText(long aIA2UniqueId, >+ IA2TextSegment* aOutNewText) >+{ >+ if (!aOutNewText) { >+ return E_INVALIDARG; and the caller of this already nul checked right? >+AccessibleHandlerControl::GetOldText(long aIA2UniqueId, >+ IA2TextSegment* aOutOldText) >+{ >+ if (!aOutOldText) { >+ return E_INVALIDARG; same >+ >+class TextChange might as well make it final >+{ >+public: >+ TextChange(); >+ TextChange(long aIA2UniqueId, bool aIsInsert, IA2TextSegment* aText); >+ TextChange(TextChange&& aOther); >+ TextChange(const TextChange& aOther); >+ >+ TextChange& operator=(TextChange&& aOther); >+ TextChange& operator=(const TextChange& aOther); do we really need all of these? seems like we could make some of them deleted. >+ HRESULT GetNewText(long aIA2UniqueId, IA2TextSegment* aOutNewText); >+ HRESULT GetOldText(long aIA2UniqueId, IA2TextSegment* aOutOldText); having three levels of methods to get this data seems kind of over abstracted, but I guess its good enough for now. >+HRESULT >+AccessibleTextTearoff::QueryInterface(REFIID aIid, void** aOutInterface) any problem with using IUnknownImpl.h here? its single threaded right? >+class AccessibleTextTearoff : public IAccessibleHypertext final? why are we implementing IAccessibleHyperText in addition to IAccessibleText? >+ ULONG mRefCnt; >+ RefPtr<AccessibleHandler> mHandler; >+ RefPtr<IAccessibleText> mAccTextProxy; >+ RefPtr<IAccessibleHypertext> mAccHypertextProxy; these refs are fine because they point at the handler which we already own a ref to?
Attachment #8854621 - Flags: review?(tbsaunde+mozbugs) → review+
Comment on attachment 8854623 [details] [diff] [review] Part 3: Add SyncTextChangeEvent as bad as it is I really don't see an alternative. This old and new text business can't die soon enough though.
Attachment #8854623 - Flags: review?(tbsaunde+mozbugs) → review+
(In reply to Bill McCloskey (:billm) from comment #10) > (In reply to Aaron Klotz [:aklotz] from comment #7) > How commonly will we use the sync variant? Does it only happen when a screen > reader is installed? The existence of that sentinel character that triggers the sync variant is highly dependent on the nature of the web content itself, however this only happens with screen readers.
Flags: needinfo?(aklotz)
Touchscreens will not trigger this, to be clear.
Comment on attachment 8854624 [details] [diff] [review] Part 4: Platform a11y changes to dispatch text change events to the handler > DocAccessibleChild::SendTextChangeEvent(const uint64_t& aID, > const nsString& aStr, > const int32_t& aStart, > const uint32_t& aLen, > const bool& aIsInsert, > const bool& aFromUser) > { > if (IsConstructedInParentProcess()) { so they are just out of luck for events fired before the top level doc is constructed in the parent process? >+ if (aStr.Contains(L'\xfffc')) { >+ // The AT is going to need to reenter content while the event is being >+ // dispatched synchronously. hm, it might be that's only true for insertions, but maybe we should add that optimization later. >+bool >+AccessibleWrap::DispatchTextChangeToHandler(bool aIsInsert, >+ const nsString& aText, >+ int32_t aStart, uint32_t aLen) hrm, I'd rather keep this stuff out of AccessibleWrap so we can stop creating the silly ones around proxies in the main process, but I guess this may be simpler for now. >+ nsTArray<HandlerControllerData>::index_type ctrlIndex = >+ sHandlerControllers->IndexOf(ourPid); I'd just make it size_t, but whatever.
Attachment #8854624 - Flags: review?(tbsaunde+mozbugs) → review+
Attachment #8854623 - Flags: review?(wmccloskey) → review+
(In reply to Trevor Saunders (:tbsaunde) from comment #11) > Comment on attachment 8854621 [details] [diff] [review] > Part 1: Add live region support to handler dll > > >+TextChange& > >+TextChange::operator=(TextChange&& aOther) > >+{ > >+ mIA2UniqueId = aOther.mIA2UniqueId; > >+ mIsInsert = aOther.mIsInsert; > >+ aOther.mIA2UniqueId = 0; > >+ mText = aOther.mText; > >+ aOther.mText.text = nullptr; > > you need to free the old text if there was any right? which means the move > ctor needs to make sure the field is initialized first right? Fixed > > >+TextChange& > >+TextChange::operator=(const TextChange& aOther) > >+{ > >+ mIA2UniqueId = aOther.mIA2UniqueId; > >+ mIsInsert = aOther.mIsInsert; > >+ mText = {BSTRCopy(aOther.mText.text), aOther.mText.start, aOther.mText.end}; > > same with s/move/copy/ Fixed > > >+TextChange::GetOld(long aIA2UniqueId, IA2TextSegment* aOutOldSegment) > >+{ > >+ if (!aOutOldSegment) { > >+ return E_INVALIDARG; > > the caller already null checked didn't they? Replaced with NotNull > > >+TextChange::GetNew(long aIA2UniqueId, IA2TextSegment* aOutNewSegment) > >+{ > >+ if (!aOutNewSegment) { > >+ return E_INVALIDARG; > > same Ditto > > >+AccessibleHandlerControl::OnTextChange(long aHwnd, long aIA2UniqueId, > >+ VARIANT_BOOL aIsInsert, > >+ IA2TextSegment* aText) > >+{ > >+ // OnTextChange implies invalidate > >+ Invalidate(); > > hrm, should it? its not clear to me text changing forces us to invalidate > anything else. Removed > > >+ > >+ mTextChange = detail::TextChange(aIA2UniqueId, aIsInsert, aText); > >+ NotifyWinEvent(aIsInsert ? IA2_EVENT_TEXT_INSERTED : IA2_EVENT_TEXT_REMOVED, > >+ reinterpret_cast<HWND>(static_cast<uintptr_t>(aHwnd)), > >+ OBJID_CLIENT, aIA2UniqueId); > > I thought we thought this should be done from the thread owning the HWND > which this might not be? > > That said I guess this maybe kind of makes it work for out of process > clients? Yeah, this is a kind-of thing. I don't think it needs to be done from the thread owning the HWND so much as it needs to be done on the thread who has registered WinEvent hooks. More something to explore in future work. > > >+AccessibleHandlerControl::GetNewText(long aIA2UniqueId, > >+ IA2TextSegment* aOutNewText) > >+{ > >+ if (!aOutNewText) { > >+ return E_INVALIDARG; > > and the caller of this already nul checked right? Replaced with NotNull > > >+AccessibleHandlerControl::GetOldText(long aIA2UniqueId, > >+ IA2TextSegment* aOutOldText) > >+{ > >+ if (!aOutOldText) { > >+ return E_INVALIDARG; > > same Ditto > > >+ > >+class TextChange > > might as well make it final Fixed > > >+HRESULT > >+AccessibleTextTearoff::QueryInterface(REFIID aIid, void** aOutInterface) > > any problem with using IUnknownImpl.h here? its single threaded right? Sure, done. > > >+class AccessibleTextTearoff : public IAccessibleHypertext > > final? Fixed. > why are we implementing IAccessibleHyperText in addition to IAccessibleText? Inheritance. > > >+ ULONG mRefCnt; > >+ RefPtr<AccessibleHandler> mHandler; > >+ RefPtr<IAccessibleText> mAccTextProxy; > >+ RefPtr<IAccessibleHypertext> mAccHypertextProxy; > > these refs are fine because they point at the handler which we already own a > ref to? Yeah, since we're a tearoff it's safe to hold strong refs here. (In reply to Trevor Saunders (:tbsaunde) from comment #15) > Comment on attachment 8854624 [details] [diff] [review] > Part 4: Platform a11y changes to dispatch text change events to the handler > > > DocAccessibleChild::SendTextChangeEvent(const uint64_t& aID, > > const nsString& aStr, > > const int32_t& aStart, > > const uint32_t& aLen, > > const bool& aIsInsert, > > const bool& aFromUser) > > { > > if (IsConstructedInParentProcess()) { > > so they are just out of luck for events fired before the top level doc is > constructed in the parent process? That event is enqueued and then played back later. I guess that could be a problem in the case where the event callback needs to be synchronous, but I'm not sure what we can do there atm.
Flags: needinfo?(aklotz)
status-firefox53: --- → wontfix
status-firefox54: --- → wontfix
status-firefox-esr52: --- → unaffected
Flags: needinfo?(aklotz)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: