Closed Bug 1349223 Opened 8 years ago Closed 6 years ago

cut aria-hidden tree

Categories

(Core :: Disability Access APIs, enhancement, P3)

Product:
Core
Component:
Disability Access APIs
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 18 obsolete files)

patch
777 bytes, patch
Jamie
: review+
Details | Diff | Splinter Review
aria-hidden patch (Yura's comments addressed)
31.61 KB, patch
Details | Diff | Splinter Review
1349223 marionette patch
4.21 KB, patch
ato
: review+
Details | Diff | Splinter Review
One more reincarnation of bug 780888 and bug 972845, hopefully a final one. We had proposed/tried more correct/flexible implementations for aria-hidden than cut-the-tree approach, which was taken by majority of the browsers, but neither of the approaches was a perfect implementation. The implementations (specifically bug 786143 - hidden object attribute propagation and 1123360 - disconnected trees) are somewhat clumsy and have perfromance/stability concerns. Also there's no good mapping on OS X a11y API for them, and thus no VoiceOver support for aria-hidden in Firefox (bug 948540). Cut-the-tree is also known the real web problems, because the lost focus issue, but its simplicity probably overtakes its disadvantage. After all it's the authors responsibility to keep web sites accessible. Also the UAIG spec stays in support of cut-the-tree approach [1] user agents SHOULD NOT include the following elements in the accessibility tree: Elements, including their descendents, that have a WAI-ARIA global attribute of aria-hidden="true" It's probably time to sync up the browsers implementations. [1] https://www.w3.org/TR/wai-aria-implementation/#exclude_elements2
Attached patch part1: cut-the-tree (obsolete) — — Splinter Review
Assignee: nobody → surkov.alexander
Attachment #8850778 - Flags: review?(yzenevich)
Attached patch part2: accessfu (obsolete) — — Splinter Review
Yura, could you please take a look at tests? It seems the problem there is simpleMovePrevious is running before the tree is updated caused by ariaShowBack call. It seems there's no proper event hook in jsat infrastructure to catch this case.
Attachment #8850804 - Flags: feedback?(yzenevich)
Attached patch part3: dump prev impl (obsolete) — — Splinter Review
Attachment #8850820 - Flags: review?(yzenevich)
Comment on attachment 8850778 [details] [diff] [review] part1: cut-the-tree Review of attachment 8850778 [details] [diff] [review]: ----------------------------------------------------------------- quick not (not a review yet) test_ariahidden.html seems to be not included here though it's in the a11y.ini . Also it would be awesome if we would start considering browser-chrome tests for our tests at some point :)
Attached patch part1: cut-the-tree v2 (obsolete) — — Splinter Review
added missed file
Attachment #8850778 - Attachment is obsolete: true
Attachment #8850778 - Flags: review?(yzenevich)
Attachment #8850826 - Flags: review?(yzenevich)
(In reply to Yura Zenevich [:yzen] from comment #4) > Also it would be awesome if we would > start considering browser-chrome tests for our tests at some point :) I'm flexible, but I'm not yet clear on when we should use mochitest and when we should stick with browser-chrome tests, let's have a chat on it next week or so.
Comment on attachment 8850826 [details] [diff] [review] part1: cut-the-tree v2 Review of attachment 8850826 [details] [diff] [review]: ----------------------------------------------------------------- looks good with questions answered. thanks ::: accessible/generic/DocAccessible.cpp @@ +773,5 @@ > return; > > + // Update the accessible tree on aria-hidden change. Make sure to not create > + // a tree under aria-hidden='true'. > + if (aAttribute == nsGkAtoms::aria_hidden) { Do we not want to check that it's different from previous aria-hidden value? Just making sure that this will not re-trigger contentRemoved[Inserted] processing on same value change. ::: accessible/tests/mochitest/attributes/test_obj.html @@ -147,5 @@ > addA11yLoadEvent(doTest); > </script> > </head> > <body> > - <a target="_blank" What's the reason for removing all these notes? ::: accessible/tests/mochitest/events/test_aria_objattr.html @@ -92,5 @@ > - </a> > - > - <a target="_blank" > - href="https://bugzilla.mozilla.org/show_bug.cgi?id=640707" > - title="Add event support for aria-sort"> is aria-sort bug note removal related? same for attribute change? ::: accessible/tests/mochitest/treeupdate/test_ariahidden.html @@ +86,5 @@ > + > + function t2_insertUnderARIAHidden() > + { > + this.eventSeq = [ > + new unexpectedInvokerChecker(EVENT_REORDER, 't2') Wouldn't it be reoder on t2_child ?
Attachment #8850826 - Flags: review?(yzenevich) → review+
Attachment #8850820 - Flags: review?(yzenevich) → review+
(In reply to Yura Zenevich [:yzen] from comment #8) > > + // Update the accessible tree on aria-hidden change. Make sure to not create > > + // a tree under aria-hidden='true'. > > + if (aAttribute == nsGkAtoms::aria_hidden) { > > Do we not want to check that it's different from previous aria-hidden value? we should, not yet sure how that important is, since it's an author error to use other values than 'true' > Just making sure that this will not re-trigger contentRemoved[Inserted] > processing on same value change. I'm not 100% sure but I think the callback shouldn't trigger on same value changes. > > - <a target="_blank" > > What's the reason for removing all these notes? we don't use them anymore, the lists are getting too big and it's easier to check hg logs for those. > > ::: accessible/tests/mochitest/events/test_aria_objattr.html > @@ -92,5 @@ > > - </a> > > - > > - <a target="_blank" > > - href="https://bugzilla.mozilla.org/show_bug.cgi?id=640707" > > - title="Add event support for aria-sort"> > > is aria-sort bug note removal related? same for attribute change? only removing all links here. I have these options 1) continue to add new links making the list bigger, 2) make the list out of sync 3) remove the list, 1st and 2nd don't make much sense. > ::: accessible/tests/mochitest/treeupdate/test_ariahidden.html > @@ +86,5 @@ > > + > > + function t2_insertUnderARIAHidden() > > + { > > + this.eventSeq = [ > > + new unexpectedInvokerChecker(EVENT_REORDER, 't2') > > Wouldn't it be reoder on t2_child ? no, because the change is under aria-hidden subtree, no subtree => no mutations
Hi Alex, I've been doing some digging regarding the test. There might be some places where we can see a failure because the test does not wait for the hide/show event associated with the aria-hidden attribute being set. However I had a follow up question, this code in jsat: https://dxr.mozilla.org/mozilla-central/source/accessible/jsat/EventManager.jsm#397-413 We make a decision when current focused vc element is going away where to focus again. So my question is: can this patch somehow introduce changes our ability to retrieve hide event's targetPrevSibling or targetParent? and also event's accessible's previousSibling or parent? Thanks
Flags: needinfo?(surkov.alexander)
(In reply to Yura Zenevich [:yzen] from comment #10) > We make a decision when current focused vc element is going away where to > focus again. So my question is: can this patch somehow introduce changes our > ability to retrieve hide event's targetPrevSibling or targetParent? and also > event's accessible's previousSibling or parent? none, I can think of. these are ordinal insertions/removals and the standard code path of events dispatching is applied. what does it make you to suspect it?
Flags: needinfo?(surkov.alexander) → needinfo?(yzenevich)
VC seems to automatically switch to parent, not a sibling that is expected (on hide)..
Attached patch 1349223 test patch (obsolete) — — Splinter Review
Hi Alex, OK I managed to fix the test however I see this assertion quite often (<50%): GECKO(77363) | [Parent 77363] ###!!! ASSERTION: No text container for focus while there's one for common ancestor?!: 'caretCntr', file /Users/yzenevich/Developer/gecko/accessible/base/SelectionManager.cpp, line 144 GECKO(77363) | #01: mozilla::a11y::NotificationController::WillRefresh(mozilla::TimeStamp)[/Users/yzenevich/Developer/gecko/obj-x86_64-apple-darwin16.4.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3b25c50] GECKO(77363) | #02: nsRefreshDriver::Tick(long long, mozilla::TimeStamp)[/Users/yzenevich/Developer/gecko/obj-x86_64-apple-darwin16.4.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x2d1710a] GECKO(77363) | #03: mozilla::RefreshDriverTimer::TickRefreshDrivers(long long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&)[/Users/yzenevich/Developer/gecko/obj-x86_64-apple-darwin16.4.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x2d1ca44] GECKO(77363) | #04: mozilla::RefreshDriverTimer::Tick(long long, mozilla::TimeStamp)[/Users/yzenevich/Developer/gecko/obj-x86_64-apple-darwin16.4.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x2d1c82f] GECKO(77363) | #05: mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp)[/Users/yzenevich/Developer/gecko/obj-x86_64-apple-darwin16.4.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x2d1d8c4] GECKO(77363) | #06: mozilla::detail::RunnableMethodImpl<mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver*, void (mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::*)(mozilla::TimeStamp), true, false, mozilla::TimeStamp>::Run()[/Users/yzenevich/Developer/gecko/obj-x86_64-apple-darwin16.4.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x2d1dbbb] GECKO(77363) | #07: nsThread::ProcessNextEvent(bool, bool*)[/Users/yzenevich/Developer/gecko/obj-x86_64-apple-darwin16.4.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x120f0b] GECKO(77363) | #08: NS_ProcessPendingEvents(nsIThread*, unsigned int)[/Users/yzenevich/Developer/gecko/obj-x86_64-apple-darwin16.4.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x11e423] GECKO(77363) | #09: nsBaseAppShell::NativeEventCallback()[/Users/yzenevich/Developer/gecko/obj-x86_64-apple-darwin16.4.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x2a0cc01] GECKO(77363) | #10: nsAppShell::ProcessGeckoEvents(void*)[/Users/yzenevich/Developer/gecko/obj-x86_64-apple-darwin16.4.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x2a72f0e] GECKO(77363) | #11: __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__[/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation +0xa7981] GECKO(77363) | #12: __CFRunLoopDoSources0[/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation +0x88a7d] GECKO(77363) | #13: __CFRunLoopRun[/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation +0x87f76] GECKO(77363) | #14: CFRunLoopRunSpecific[/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation +0x87974] GECKO(77363) | #15: RunCurrentEventLoopInMode[/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox +0x30a5c] GECKO(77363) | #16: ReceiveNextEventCommon[/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox +0x30891] GECKO(77363) | #17: _BlockUntilNextEventMatchingListInModeWithFilter[/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox +0x306c6] GECKO(77363) | #18: _DPSNextEvent[/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit +0x475b4] GECKO(77363) | #19: -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:][/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit +0x7c1d6b] GECKO(77363) | #20: -[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:][/Users/yzenevich/Developer/gecko/obj-x86_64-apple-darwin16.4.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x2a722b6] GECKO(77363) | #21: -[NSApplication run][/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit +0x3bf35] GECKO(77363) | #22: nsAppShell::Run()[/Users/yzenevich/Developer/gecko/obj-x86_64-apple-darwin16.4.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x2a7361b] GECKO(77363) | #23: nsAppStartup::Run()[/Users/yzenevich/Developer/gecko/obj-x86_64-apple-darwin16.4.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3e07977] GECKO(77363) | #24: XREMain::XRE_mainRun()[/Users/yzenevich/Developer/gecko/obj-x86_64-apple-darwin16.4.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3eb7c18] GECKO(77363) | #25: XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&)[/Users/yzenevich/Developer/gecko/obj-x86_64-apple-darwin16.4.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3eb89ca] GECKO(77363) | #26: XRE_main(int, char**, mozilla::BootstrapConfig const&)[/Users/yzenevich/Developer/gecko/obj-x86_64-apple-darwin16.4.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3eb92c9] GECKO(77363) | #27: main[/Users/yzenevich/Developer/gecko/obj-x86_64-apple-darwin16.4.0/dist/NightlyDebug.app/Contents/MacOS/firefox +0xd1a]
Flags: needinfo?(yzenevich)
Basically I make sure the test waits for hide/show on aria-hidden change, because sometimes it did not. Also I think some of the Utils.jsm stuff is still needed (like isVisible utility, should at least check state). If I update it however, I get failures in test_output.html
Comment on attachment 8850804 [details] [diff] [review] part2: accessfu removing f? since I added some patches to address the tests
Attachment #8850804 - Flags: feedback?(yzenevich)
Attached patch part1: cut-the-tree v3 (obsolete) — — Splinter Review
Attachment #8850804 - Attachment is obsolete: true
Attachment #8850820 - Attachment is obsolete: true
Attachment #8850826 - Attachment is obsolete: true
Attachment #8854142 - Attachment is obsolete: true
Attachment #8854143 - Attachment is obsolete: true
Attached patch part2: accessfu v3 (obsolete) — — Splinter Review
Yura, mind to look at the pivot tests, test_virtualcursor.html halts, you might have good ideas, what's wrong there
Attached patch part3: dump prev impl v3 (obsolete) — — Splinter Review
Attached patch 1349223 fixed vc test (obsolete) — — Splinter Review
So the issue is that we still relied on aria-hidden nodes to have an accessible. We needed to remove them from traversal rules and also get rid of places where we tried getting an accessible for them.
This is something we should triage and fix but isn't among our most urgent bugs.(Contributions welcome as always!)
Priority: -- → P3
Please see bug 1123248.
Attached patch part1: cut the tree (obsolete) — — Splinter Review
Attachment #8855434 - Attachment is obsolete: true
Attachment #8855435 - Attachment is obsolete: true
Attachment #8855437 - Attachment is obsolete: true
Attachment #8855881 - Attachment is obsolete: true
Attached patch part2: accessfu (obsolete) — — Splinter Review
Attached patch part3: rm leftovers (obsolete) — — Splinter Review
Yura, I updated the patches to trunk. Could you please take a look again: there's some jsat failures?
Flags: needinfo?(yzenevich)
(In reply to alexander :surkov from comment #26) > Yura, I updated the patches to trunk. Could you please take a look again: > there's some jsat failures? As discussed in person, hide/show events are absent when aria-hidden is set, removed. This is why the tests are stalling
Flags: needinfo?(yzenevich)
Attached patch patch (obsolete) — — Splinter Review
Attachment #8972964 - Attachment is obsolete: true
Attachment #8972965 - Attachment is obsolete: true
Attachment #8972966 - Attachment is obsolete: true
It seems the observed problem is fixed now, but there's still something preventing them to work. I saw that you and Eitan pushed a number of changes to jsat testing, so I guess it's something related. Yura, could you take a look at jsat tests one more time please?
Flags: needinfo?(yzenevich)
(In reply to alexander :surkov from comment #30) > It seems the observed problem is fixed now, but there's still something > preventing them to work. I saw that you and Eitan pushed a number of changes > to jsat testing, so I guess it's something related. Yura, could you take a > look at jsat tests one more time please? I did some digging, though it looks like HIDE event fires correctly when aria-hidden is set to true, when it is set back false though, it looks like SHOW event is fired after virtual cursor change event. I think here we need to ensure that the SHOW event fires first before trying to move vc to previous position.
Flags: needinfo?(yzenevich) → needinfo?(surkov.alexander)
(In reply to Yura Zenevich [:yzen] from comment #32) > (In reply to alexander :surkov from comment #30) > > It seems the observed problem is fixed now, but there's still something > > preventing them to work. I saw that you and Eitan pushed a number of changes > > to jsat testing, so I guess it's something related. Yura, could you take a > > look at jsat tests one more time please? > > I did some digging, though it looks like HIDE event fires correctly when > aria-hidden is set to true, when it is set back false though, it looks like > SHOW event is fired after virtual cursor change event. I think here we need > to ensure that the SHOW event fires first before trying to move vc to > previous position. It appears that virtual cursor is not directly related with mutation events. Is there a chance that the test is doing something wrong? For example, if the tests moves VC before show event :) Yura, any advise where to look?
Flags: needinfo?(surkov.alexander) → needinfo?(yzenevich)
Attached patch patch (obsolete) — — Splinter Review
jsat test_content_integration fixed
Attachment #8982618 - Attachment is obsolete: true
Attached patch working content integration test (obsolete) — — Splinter Review
Ill take a look at output on monday, hope that's ok
OK I did more digging into the why output test fails: * In the tests as is, combobox is hidden (possible because it's scrolled off screen) as well as the combobox option. This is why we ignore a text child of a hidden combobox option. * I even tried running the test with combobox being visible (not off screen) but the combobox option is still hidden so the ignore part remains the same. This all seems to go away if I revert changes to isHidden method in Utils.jsm also I'm not sure now about the other changes related to visibility checks.
Flags: needinfo?(yzenevich) → needinfo?(surkov.alexander)
Attached patch patch (obsolete) — — Splinter Review
* working output test with suggested IsHidden() fix * managed to move through the first test_content_integration.html failure, but stuck with the next one: FAIL Event text matches. Got ["Traversal Rule test document","Home","button"], expected ["Home","button"]. - Structures begin differing at: got[0] = "Traversal Rule test document" expected[0] = "Home" Yura, any ideas please?
Attachment #8984528 - Attachment is obsolete: true
Attachment #8984563 - Attachment is obsolete: true
Flags: needinfo?(surkov.alexander) → needinfo?(yzenevich)
Flags: needinfo?(yzenevich)
test_content_integration fails on debug build (https://treeherder.mozilla.org/logviewer.html#?job_id=183267711&repo=try&lineNumber=4577): 01:02:21 INFO - GECKO(5284) | [AccessFu] WARNING AccessibilityEventObserver.observe: no window for accessible document: text selection changed accessible: [ document | such app ] 01:02:21 INFO - GECKO(5284) | [Parent 5284, Main Thread] ###!!! ASSERTION: no outer doc for accessible remote tab!: 'outerDoc', file z:/build/build/src/accessible/windows/msaa/AccessibleWrap.cpp, line 1341 01:02:21 INFO - GECKO(5284) | #01: mozilla::a11y::AccessibleWrap::FireWinEvent(mozilla::a11y::Accessible *,unsigned int) [accessible/windows/msaa/AccessibleWrap.cpp:1213] 01:02:21 INFO - GECKO(5284) | #02: mozilla::a11y::ProxyCaretMoveEvent(mozilla::a11y::ProxyAccessible *,mozilla::gfx::IntRectTyped<mozilla::LayoutDevicePixel> const &) [accessible/windows/msaa/Platform.cpp:144] 01:02:21 INFO - GECKO(5284) | #03: mozilla::a11y::DocAccessibleParent::RecvCaretMoveEvent(unsigned __int64 const &,mozilla::gfx::IntRectTyped<mozilla::LayoutDevicePixel> const &,int const &) [accessible/ipc/DocAccessibleParent.cpp:289] Yura, could you detect which part of the test fails? Need to figure out if aria-hidden reveals somehow a bug.
Flags: needinfo?(yzenevich)
(In reply to alexander :surkov from comment #38) > test_content_integration fails on debug build > (https://treeherder.mozilla.org/logviewer. > html#?job_id=183267711&repo=try&lineNumber=4577): > > 01:02:21 INFO - GECKO(5284) | [AccessFu] WARNING > AccessibilityEventObserver.observe: no window for accessible document: text > selection changed accessible: [ document | such app ] > 01:02:21 INFO - GECKO(5284) | [Parent 5284, Main Thread] ###!!! > ASSERTION: no outer doc for accessible remote tab!: 'outerDoc', file > z:/build/build/src/accessible/windows/msaa/AccessibleWrap.cpp, line 1341 > 01:02:21 INFO - GECKO(5284) | #01: > mozilla::a11y::AccessibleWrap::FireWinEvent(mozilla::a11y::Accessible > *,unsigned int) [accessible/windows/msaa/AccessibleWrap.cpp:1213] > 01:02:21 INFO - GECKO(5284) | #02: > mozilla::a11y::ProxyCaretMoveEvent(mozilla::a11y::ProxyAccessible > *,mozilla::gfx::IntRectTyped<mozilla::LayoutDevicePixel> const &) > [accessible/windows/msaa/Platform.cpp:144] > 01:02:21 INFO - GECKO(5284) | #03: > mozilla::a11y::DocAccessibleParent::RecvCaretMoveEvent(unsigned __int64 > const &,mozilla::gfx::IntRectTyped<mozilla::LayoutDevicePixel> const &,int > const &) [accessible/ipc/DocAccessibleParent.cpp:289] > > Yura, could you detect which part of the test fails? Need to figure out if > aria-hidden reveals somehow a bug. Alex could you rebase, squash the patches they do not apply any longer
Flags: needinfo?(surkov.alexander)
Attached patch patch (obsolete) — — Splinter Review
sure, here you are :)
Attachment #8985409 - Attachment is obsolete: true
Flags: needinfo?(surkov.alexander)
Attachment #8985614 - Flags: review?(yzenevich)
It might be not a bug actually, we fail, when we attempt to deliver an event in parent process. Since this is async operation, then there's no guarantee that a target document/window is in good shape. It might the case, when we want just throw out an event. Jamie, what is your thinking on this?
Flags: needinfo?(jteh)
Attached patch patch — — Splinter Review
Here's a thing: * focus is moved within a document, which results in caret move event * aria-hidden="true" is placed on iframe element, which makes iframe accessible unattached from the document * focus/caret move events arrive to parent process, there's no iframe accessible, thus we assert. It all looks like a legal situation
Flags: needinfo?(yzenevich)
Flags: needinfo?(jteh)
Attachment #8986559 - Flags: review?(jteh)
Attachment #8986559 - Attachment filename: patch → no assert for no outerdoc case
Comment on attachment 8985614 [details] [diff] [review] patch Review of attachment 8985614 [details] [diff] [review]: ----------------------------------------------------------------- Please see comments that need to be addressed. Also I suggest running mach eslint accessible/ before pushing to inbound whenever js changes are made. ::: accessible/generic/DocAccessible.cpp @@ +1798,5 @@ > // what means there's no container. Ignore the insertion too. > nsIContent* prevNode = mNodes->SafeElementAt(mNodesIdx - 1); > nsIContent* node = mNodes->ElementAt(mNodesIdx++); > + Accessible* container = Document()-> > + AccessibleOrTrueContainer(node, DocAccessible::eNoContainerIfARIAHidden); I'm guessing this is intentional that you're not forwarding aARIAHidden? ::: accessible/jsat/EventManager.jsm @@ +296,4 @@ > _handleShow: function _handleShow(aEvent) { > let {liveRegion, isPolite} = this._handleLiveRegion(aEvent, > ["additions", "all"]); > + if (this.inTest) { Not used, lets remove this. @@ +318,4 @@ > let {liveRegion, isPolite} = this._handleLiveRegion( > aEvent, ["removals", "all"]); > let acc = aEvent.accessible; > + if (this.inTest) { Same. Not used, lets remove this. @@ +627,5 @@ > return; > } > + let content; > + try { > + content = event.accessibleDocument.window; This will still happen? If not lets remove try catch @@ +629,5 @@ > + let content; > + try { > + content = event.accessibleDocument.window; > + } catch (e) { > + Logger.warning( I would prefer Logger.logException here ::: accessible/jsat/Utils.jsm @@ +278,4 @@ > }, > > isHidden: function isHidden(aAccessible) { > + return false; Lets remove this function completely. Where it is used in visible child count simply check: this.getState(aAccessible).contains(states.INVISIBLE) In _traverse, we dont need the whole include logic then, so remove it. This means we also need to get rid of _includeInvisible property in Pivot as well as clean up aIncludeInvisible wherever it is passed to new PivotContext in AccessFu. @@ +524,5 @@ > } > > if (aEvent.eventType == Events.VIRTUALCURSOR_CHANGED) { > + try { > + let event = aEvent.QueryInterface( See below, ideally we would not need to change anything here. @@ +528,5 @@ > + let event = aEvent.QueryInterface( > + Ci.nsIAccessibleVirtualCursorChangeEvent); > + let pivot = aEvent.accessible.QueryInterface( > + Ci.nsIAccessibleDocument).virtualCursor; > + str += ` (${this.accessibleToString(event.oldAccessible)} -> ${this.accessibleToString(pivot.position)})`; nit: indentation, this will add \n after -> so I would keep this whole thing unchanged. @@ +530,5 @@ > + let pivot = aEvent.accessible.QueryInterface( > + Ci.nsIAccessibleDocument).virtualCursor; > + str += ` (${this.accessibleToString(event.oldAccessible)} -> ${this.accessibleToString(pivot.position)})`; > + } catch (x) { > + // Likely can't query nsIAccessibleVirtualCursorChangeEvent in parent I think Eitan is adding support for this via IPC in his android patches, should we wait for those to land? ::: accessible/tests/mochitest/events.js @@ +119,5 @@ > +function waitForEventPromise(eventType, target) { > + return new Promise(resolve => { > + let eventObserver = { > + observe(subject, topic, data) { > + if (topic !== "accessible-event") { No need for this check really. @@ +127,5 @@ > + let event = subject.QueryInterface(nsIAccessibleEvent); > + if (event.eventType !== eventType) { > + return; > + } > + nit: extra space ::: accessible/tests/mochitest/treeupdate/test_ariahidden.html @@ +16,5 @@ > + <script type="application/javascript" > + src="../events.js"></script> > + > + <script type="application/javascript"> > + function t1_setARIAHidden() { nit: indentation @@ +52,5 @@ > + return "remove aria-hidden"; > + }; > + } > + > + function t2_setARIAHidden() { All 3 functions kind of look the same, i would parametrize them and just have 1 that sets the aria-hidden value and checks if there is/is not accessible.
Attachment #8985614 - Flags: review?(yzenevich) → review+
(In reply to Yura Zenevich [:yzen] from comment #43) > Please see comments that need to be addressed. Also I suggest running mach > eslint accessible/ before pushing to inbound whenever js changes are made. right, iirc it is a standard test for try fuzzy a11y one I usually do. > ::: accessible/generic/DocAccessible.cpp > @@ +1798,5 @@ > > // what means there's no container. Ignore the insertion too. > > nsIContent* prevNode = mNodes->SafeElementAt(mNodesIdx - 1); > > nsIContent* node = mNodes->ElementAt(mNodesIdx++); > > + Accessible* container = Document()-> > > + AccessibleOrTrueContainer(node, DocAccessible::eNoContainerIfARIAHidden); > > I'm guessing this is intentional that you're not forwarding aARIAHidden? not sure I caught about eARIAHidden forwarding. This bit is about we should ignore content insertion if it happens under aria-hidden element. > Lets remove this function completely. Where it is used in visible child > count simply check: > > this.getState(aAccessible).contains(states.INVISIBLE) > > In _traverse, we dont need the whole include logic then, so remove it. ok > This means we also need to get rid of _includeInvisible property in Pivot as > well as clean up aIncludeInvisible wherever it is passed to new PivotContext > in AccessFu. are you sure you want to make this going into aria-hidden patch? > @@ +528,5 @@ > > + let event = aEvent.QueryInterface( > > + Ci.nsIAccessibleVirtualCursorChangeEvent); > > + let pivot = aEvent.accessible.QueryInterface( > > + Ci.nsIAccessibleDocument).virtualCursor; > > + str += ` (${this.accessibleToString(event.oldAccessible)} -> ${this.accessibleToString(pivot.position)})`; > > nit: indentation, this will add \n after -> so I would keep this whole thing > unchanged. it probably wasn't my change, but ok I will change that :) > @@ +530,5 @@ > > + let pivot = aEvent.accessible.QueryInterface( > > + Ci.nsIAccessibleDocument).virtualCursor; > > + str += ` (${this.accessibleToString(event.oldAccessible)} -> ${this.accessibleToString(pivot.position)})`; > > + } catch (x) { > > + // Likely can't query nsIAccessibleVirtualCursorChangeEvent in parent > > I think Eitan is adding support for this via IPC in his android patches, > should we wait for those to land? not sure I can see the connection between all parts, but I wouldn't make aria-hidden feature blocked by accessfu, if possible. > ::: accessible/tests/mochitest/events.js > @@ +119,5 @@ > > +function waitForEventPromise(eventType, target) { > > + return new Promise(resolve => { > > + let eventObserver = { > > + observe(subject, topic, data) { > > + if (topic !== "accessible-event") { > > No need for this check really. you mean to use QueryInterface(nsIAccessibleEvent) as a check? > @@ +52,5 @@ > > + return "remove aria-hidden"; > > + }; > > + } > > + > > + function t2_setARIAHidden() { > > All 3 functions kind of look the same, i would parametrize them and just > have 1 that sets the aria-hidden value and checks if there is/is not > accessible. Not sure, that would require all three methods to have if/else checks, and plus something special for t2_setARIAHidden since it checks a tree. I'd say there's no big overhead with the methods as is, and new utility functions don't necessary make the code more readable. So I wouldn't change it until you feel strong on it.
(In reply to alexander :surkov from comment #44) > (In reply to Yura Zenevich [:yzen] from comment #43) > > > Please see comments that need to be addressed. Also I suggest running mach > > eslint accessible/ before pushing to inbound whenever js changes are made. > > right, iirc it is a standard test for try fuzzy a11y one I usually do. > > > ::: accessible/generic/DocAccessible.cpp > > @@ +1798,5 @@ > > > // what means there's no container. Ignore the insertion too. > > > nsIContent* prevNode = mNodes->SafeElementAt(mNodesIdx - 1); > > > nsIContent* node = mNodes->ElementAt(mNodesIdx++); > > > + Accessible* container = Document()-> > > > + AccessibleOrTrueContainer(node, DocAccessible::eNoContainerIfARIAHidden); > > > > I'm guessing this is intentional that you're not forwarding aARIAHidden? > > not sure I caught about eARIAHidden forwarding. This bit is about we should > ignore content insertion if it happens under aria-hidden element. > > > Lets remove this function completely. Where it is used in visible child > > count simply check: > > > > this.getState(aAccessible).contains(states.INVISIBLE) > > > > In _traverse, we dont need the whole include logic then, so remove it. > > ok > > > This means we also need to get rid of _includeInvisible property in Pivot as > > well as clean up aIncludeInvisible wherever it is passed to new PivotContext > > in AccessFu. > > are you sure you want to make this going into aria-hidden patch? It could be a follow up bug but the part where we check visible children has to be updated to be correct, e.g. we need to check the INVISIBLE state there. > > > @@ +528,5 @@ > > > + let event = aEvent.QueryInterface( > > > + Ci.nsIAccessibleVirtualCursorChangeEvent); > > > + let pivot = aEvent.accessible.QueryInterface( > > > + Ci.nsIAccessibleDocument).virtualCursor; > > > + str += ` (${this.accessibleToString(event.oldAccessible)} -> ${this.accessibleToString(pivot.position)})`; > > > > nit: indentation, this will add \n after -> so I would keep this whole thing > > unchanged. > > it probably wasn't my change, but ok I will change that :) Thanks. > > > @@ +530,5 @@ > > > + let pivot = aEvent.accessible.QueryInterface( > > > + Ci.nsIAccessibleDocument).virtualCursor; > > > + str += ` (${this.accessibleToString(event.oldAccessible)} -> ${this.accessibleToString(pivot.position)})`; > > > + } catch (x) { > > > + // Likely can't query nsIAccessibleVirtualCursorChangeEvent in parent > > > > I think Eitan is adding support for this via IPC in his android patches, > > should we wait for those to land? > > not sure I can see the connection between all parts, but I wouldn't make > aria-hidden feature blocked by accessfu, if possible. Fair. > > > ::: accessible/tests/mochitest/events.js > > @@ +119,5 @@ > > > +function waitForEventPromise(eventType, target) { > > > + return new Promise(resolve => { > > > + let eventObserver = { > > > + observe(subject, topic, data) { > > > + if (topic !== "accessible-event") { > > > > No need for this check really. > > you mean to use QueryInterface(nsIAccessibleEvent) as a check? No need to check at all, there's no way this will get any other events. > > > @@ +52,5 @@ > > > + return "remove aria-hidden"; > > > + }; > > > + } > > > + > > > + function t2_setARIAHidden() { > > > > All 3 functions kind of look the same, i would parametrize them and just > > have 1 that sets the aria-hidden value and checks if there is/is not > > accessible. > > Not sure, that would require all three methods to have if/else checks, and > plus something special for t2_setARIAHidden since it checks a tree. I'd say > there's no big overhead with the methods as is, and new utility functions > don't necessary make the code more readable. So I wouldn't change it until > you feel strong on it. Fair.
(In reply to Yura Zenevich [:yzen] from comment #45) > > > Lets remove this function completely. Where it is used in visible child > > > count simply check: > > > > > > this.getState(aAccessible).contains(states.INVISIBLE) > > > > > > In _traverse, we dont need the whole include logic then, so remove it. > > > > ok > > > > > This means we also need to get rid of _includeInvisible property in Pivot as > > > well as clean up aIncludeInvisible wherever it is passed to new PivotContext > > > in AccessFu. > > > > are you sure you want to make this going into aria-hidden patch? > > It could be a follow up bug but the part where we check visible children has > to be updated to be correct, e.g. we need to check the INVISIBLE state there. Wait. The patch doesn't change anything about INVISIBLE state, neither in core nor in accessfu. Previosly, isHidden() was used to check aria-hidden stuff only. Since no aria-hidden now, then agreed no need in isHidden(), but honestly I don't follow INVISIBLE part. > > > > + if (topic !== "accessible-event") { > > > > > > No need for this check really. > > > > you mean to use QueryInterface(nsIAccessibleEvent) as a check? > > No need to check at all, there's no way this will get any other events. I see now
Comment on attachment 8986559 [details] [diff] [review] patch Seems reasonable. However, I don't think this can happen except for aria-hidden being set on an iframe in a different process. I guess normally, the document should only be destroyed from content, in which case there would be no pending async events and this shouldn't happen. If I'm correct, it might be good to specifically provide this aria-hidden on iframe example in the comment so we don't question this logic in a few years time. :)
Attachment #8986559 - Flags: review?(jteh) → review+
(In reply to James Teh [:Jamie] from comment #48) > Seems reasonable. However, I don't think this can happen except for > aria-hidden being set on an iframe in a different process. I guess normally, > the document should only be destroyed from content, in which case there > would be no pending async events and this shouldn't happen. If I'm correct, > it might be good to specifically provide this aria-hidden on iframe example > in the comment so we don't question this logic in a few years time. :) agreed, makes sense.
Pushed by surkov.alexander@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e6878a9478df do not assert if a document is unattached from tree when seding the events, r=jamie
Backout by csabou@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d5905334d10d Backed out changeset b2df5de436bd for permafailling marionette harness on test_accessibility.py. CLOSED TREE
It claims [1] that test_element_visible_but_not_visible_to_accessbility fails, which are about aria-hidden buttons [2]. The patch makes these button not accessible, and I assume this is unexpected result by the test. Henric, do you have suggestions how to fix the issue? Should I simply remove 'displayed_but_a11y_hidden_elementIDs' test or modify it somehow? [1] https://treeherder.mozilla.org/logviewer.html#?job_id=184747365&repo=mozilla-inbound&lineNumber=1406 [2] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette_harness/tests/unit/test_accessibility.py?q=test_accessibility.py&redirect_type=direct#84
Flags: needinfo?(hskupin)
(In reply to alexander :surkov from comment #53) > Henric, do you have suggestions how to fix the issue? Should I simply remove > 'displayed_but_a11y_hidden_elementIDs' test or modify it somehow? Yura created all those tests AFAIR so it would be better if he could have a look at those test failures.
Flags: needinfo?(hskupin) → needinfo?(yzenevich)
https://hg.mozilla.org/mozilla-central/rev/e6878a9478df
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Only the changeset from comment 50 stuck -> reopening.
Status: RESOLVED → VERIFIED
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Hi all, Sorry, I am a bit late to the discussion here, having only recently found this thread whilst searching for aria-hidden related bugs. From a screen reader/magnifier vendors point of view, I don't understand why you are trying to eliminate aria-hidden content from the accessibility tree instead of just marking it as hidden as an attribute and leaving the decision to ignore it up to the screen reader or magnifier. What problem are you trying to solve? This seems like a step backwards in overall accessibility because you are making assumptions about the user requirements rather than leaving it up to the AT software to decide what to do. aria-hidden makes a lot of sense for a blind screen reader user, providing the web author has used it correctly. For a magnifier and speech user, who can see the screen but are also using speech, things are not so straightforward. There are common websites which have marked visible objects as aria-hidden and put in hidden objects with different text content. The problem with this is that what is visible on the screen and what the TTS speaks are different causing user confusion. Many magnifier users don't just move around with the mouse and visually read the text on the screen. We have a lot of users (more than those who are solely speech users) who use magnification but also use speech to help read the screen, both by using a "read under mouse" feature, or a virtual cursor. What I have realized is that I need to selectively ignore aria-hidden, depending on the context and whether the user is using magnification. When I started porting my Chrome code to work with Firefox, it was a relief to find that you have exposed aria-hidden as an attribute instead of pruning the tree. If this change goes in then you will be making the overall accessibility of Firefox worse and I will be back with the same problems I have with Chrome. Please reconsider why you are making this change. Regards, Mike.
Hi Mike, Thanks for your input. We've gone back and forth on this many times over the years. The unfortunate reality is that (at least IMO) aria-hidden was very poorly specified in the first place, failing to consider exactly the kinds of use cases you describe. Originally, it was only intended to hide content that was truly visually hidden (e.g. because it was completely visually obscured by other content but in such a way that it was difficult for browser accessibility layers to detect). Unfortunately, the definition was then broadened somewhat, and now it's used for all sorts of things with no ability to distinguish between them. I made all of the same arguments in bug 780888. Unfortunately, as noted in comment 0, there's been a fair bit of pressure over the years to prune the tree instead: 1. The spec states that we should exclude aria-hidden from the tree, so our current behaviour is a spec violation. See: https://www.w3.org/TR/core-aam-1.1/#include_elements https://www.w3.org/TR/core-aam-1.1/#mapping_state-property_table 2. There's no mapping for Mac, Android and UIA to support this approach. Thus, the way we're doing it now means we can't support aria-hidden at all on those platforms. Pruning the tree is not really something we can do purely in the platform specific code. 3. Authors (and others) see that things behave one way in Firefox and another everywhere else. Because Firefox is the one which doesn't follow the spec, we're the ones left with the blame. Unfortunately, saying "we think the spec is wrong so we won't follow it " just isn't good enough and generally results in cries of poor behaviour. See also bug 581096, which includes discussion about attempts to have the spec changed. There are also AT bugs like this NVDA bug, where web authors reason that mouse tracking should not report aria-hidden elements: https://github.com/nvaccess/nvda/issues/4322 at the end of the day, the inconsistency between implementations doesn't do anyone any good. At least with a consistent approach, the responsible use of aria-hidden is left with the spec and the authors. IMO, aria-hidden should never have made it into the spec the way it is, but unfortunately, it did, despite objections. The only way a reasonable change can be made here is to get this changed in the spec. I'd strongly encourage you to file an issue against the spec explaining use cases and scenarios that are broken by this behaviour.
Hi James, Thanks for taking time to explain your reasoning. I agree that it makes sense for all implementations to be consistent both across browsers and platforms. I also understand why you would want to follow the spec, even if it's not ideal. I don't believe I should need to make any changes to Supernova to support the change in behavior. I look forward to testing it shortly. Regards, Mike.
Flags: needinfo?(yzenevich)
Attachment #8989758 - Flags: review?(ato)
Attachment #8989758 - Flags: review?(ato) → review+
Pushed by yura.zenevich@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2ef78c362877 cut aria-hidden tree, r=yzen https://hg.mozilla.org/integration/mozilla-inbound/rev/6f9c801d8433 update marionette a11y test based on the new handling of aria-hidden. r=ato
https://hg.mozilla.org/mozilla-central/rev/2ef78c362877 https://hg.mozilla.org/mozilla-central/rev/6f9c801d8433
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Resolution: --- → FIXED
Blocks: 1330278
Came here to say that in Chrome we're soon going to be adding back focusable aria-hidden elements into the tree so that they can fire events. There are ways of marking elements as hidden in UIA, Android and on Mac. I don't think the original spec accounted for that. If this is something Mozilla would like to do as well I'll file a bug.
(In reply to Aaron Leventhal from comment #64) > Came here to say that in Chrome we're soon going to be adding back focusable > aria-hidden elements into the tree so that they can fire events. > > If this is something Mozilla would like to do as well I'll file a bug. I think we should have a bug on this at least to keep the issue on track, so please file one if you don't mind. I always argued that we should never hide the focus within an aria-hidden tree, but I gave up someday, sometimes it's better to stick with working thing than trying to make them perfect. > There are > ways of marking elements as hidden in UIA, Android and on Mac. I don't think > the original spec accounted for that. It will probably require AT to adopt the change (at least in case of ATK/IA2), and also it can be a challenging to implement. Do you solve some use cases that depend on the behavior change?
@askurkov, I filed bug 1510347. Turns out this breaks tabbing in Twitch.tv dialogs.
Blocks: 1371024
Blocks: 1275989
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: