Closed
Bug 682811
Opened 13 years ago
Closed 23 days ago
menupopup_start/menupopup_end/alert events should be fired for accessibles inside the shown/hide subtree
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
136 Branch
| Tracking | Status | |
|---|---|---|
| firefox136 | --- | fixed |
People
(Reporter: surkov, Assigned: eeejay)
References
(Blocks 5 open bugs)
Details
(Keywords: papercut, Whiteboard: [bk1])
Attachments
(2 files, 3 obsolete files)
|
5.24 KB,
patch
|
Details | Diff | Splinter Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review |
These events are issued for root of changed subtree, if menupopup/alert accessibles are inside of the subtree then these events aren't emitted. We did that for menupopup_end events prior bug 570275, this behavior should be returned back.
Example for missed alert event for ARIA alerts is http://archive.dojotoolkit.org/nightly/dojotoolkit/dijit/tests/test_TooltipDialog.html
|
||
Comment 2•13 years ago
|
||
We're not working around this bug at present; no one has reported this with NVDA.
This test case works fine with NVDA because a descendant of the dialogs gets focus. (Imo, a dialog shouldn't fire an alert event if it gets focus anyway, but that's a different issue.)
|
||
Updated•13 years ago
|
Assignee: nobody → Jennyherrera.19
|
Reporter | |
Comment 4•13 years ago
|
||
(In reply to Jenny from comment #3)
> what files should be firing the alert events?
what do you mean by 'files'?
|
||
Comment 5•13 years ago
|
||
(In reply to alexander :surkov from comment #4)
> (In reply to Jenny from comment #3)
> > what files should be firing the alert events?
>
> what do you mean by 'files'?
I believe shes asking what classes should be firing the events.
|
|
Reporter | |
Comment 6•13 years ago
|
||
(In reply to Daniel Goodrich from comment #5)
> (In reply to alexander :surkov from comment #4)
> > (In reply to Jenny from comment #3)
> > > what files should be firing the alert events?
> >
> > what do you mean by 'files'?
>
> I believe shes asking what classes should be firing the events.
simple search shows that EVENT_MENUPOPUP_END for example is fired by base/nsDocAccessible and generic/RootAccessible, see http://mxr.mozilla.org/mozilla-central/ident?i=EVENT_MENUPOPUP_END&filter=
|
|
||
Comment 7•13 years ago
|
||
Can you fully explain what this bug is not doing but should be doing?
Looking into it, we have an idea that adds the firing of events to the starting / closing of the subtree, but
(from comment #2)
> This test case works fine with NVDA because a descendant of the dialogs gets
> focus. (Imo, a dialog shouldn't fire an alert event if it gets focus anyway,
> but that's a different issue.)
so we do not know if this should be taken out and replaced. As it sits right now, the DOMi addon shows everything being fired correctly.
|
|
Reporter | |
Comment 8•13 years ago
|
||
(In reply to Daniel Goodrich from comment #7)
> Can you fully explain what this bug is not doing but should be doing?
real example:
you have the page like:
<html>
<script>
function onload()
{
var div = document.createElement("div");
div.setAttribute("role", "alert");
div.textContent = "This is alert";
document.body.appendChild(div);
}
</script>
<body>
my document
</body>
</html>
when the page is loaded then alert appears. In this case we should fire alert event, we don't and that's a bug. We don't fire alert event because we construct accessible tree after div is inserted into DOM tree (i.e. we don't process content insertion notification for inserted div where we used to fire alert event).
demo example:
<script>
function addContent()
{
var div = document.createElement("div");
var alertDiv = document.createElement("div");
alertDiv.setAttribute("role", "alert");
alertDiv.textContent = "Hello";
div.appendChild(alertDiv);
document.body.appendChild(div);
}
</script>
in this case the alert is part of inserted subtree so we don't fire alert event because we don't look deep into inserted subtree. It's a bug as well.
> Looking into it, we have an idea that adds the firing of events to the
> starting / closing of the subtree, but
yes
> (from comment #2)
> > This test case works fine with NVDA because a descendant of the dialogs gets
> > focus. (Imo, a dialog shouldn't fire an alert event if it gets focus anyway,
> > but that's a different issue.)
>
> so we do not know if this should be taken out and replaced. As it sits
> right now, the DOMi addon shows everything being fired correctly.
dialogs don't have alert role (which is used to detect whether we should fire alert event or shouldn't) so we don't touch this behavior here.
|
|
||
Comment 9•13 years ago
|
||
If we add an iterative loop inside if (aChild->ARIARole() == roles::MENUPOPUP)(line 1911 of nsDocAcessible.cpp) to check each child and call nsDocAccessible::UpdateTreeInternal for each child passing aIsInsert as false each time, that would fire the end and hide events for each child. Is this correct and would it work for showing all of the menu_popup_end events?
|
|
||
Comment 10•13 years ago
|
||
(In reply to Daniel Goodrich from comment #9)
> If we add an iterative loop inside if (aChild->ARIARole() ==
> roles::MENUPOPUP)(line 1911 of nsDocAcessible.cpp) to check each child and
> call nsDocAccessible::UpdateTreeInternal for each child passing aIsInsert as
> false each time, that would fire the end and hide events for each child. Is
> this correct and would it work for showing all of the menu_popup_end events?
in general the approach is fine, but you'd be better off moving the code to fire the events to CacheChildrenInSubTree() / UnCacheChildrenInSubTree() which are the next methods in the file, because they already recurse through the subtree.
|
|
||
Comment 11•13 years ago
|
||
|
|
Reporter | |
Comment 12•13 years ago
|
||
Comment on attachment 625439 [details] [diff] [review]
patch
Review of attachment 625439 [details] [diff] [review]:
-----------------------------------------------------------------
::: accessible/src/base/nsDocAccessible.cpp
@@ +1972,5 @@
> + } else if (ariaRole == roles::ALERT) {
> + // Fire EVENT_ALERT if ARIA alert appears.
> + FireDelayedAccessibleEvent(nsIAccessibleEvent::EVENT_ALERT, child->GetNode(),
> + AccEvent::eRemoveDupes);
> + }
this makes event fired before show event what is not right (show event means for AT that accessible is created)
@@ +1995,5 @@
> +
> + if (event)
> + FireDelayedAccessibleEvent(event);
> + }
> + UncacheChildrenInSubtree(child);
this makes it fire after hide event (hide event means for AT accessible is destroyed)
|
|
||
Comment 13•13 years ago
|
||
Attachment #625439 -
Attachment is obsolete: true
|
|
Reporter | |
Comment 14•13 years ago
|
||
Comment on attachment 626071 [details] [diff] [review]
Patch 2
Review of attachment 626071 [details] [diff] [review]:
-----------------------------------------------------------------
btw, could you fix your editor to not use tabs and use 2 spaces for indentation please? and it's better to ask somebody for feedback explicitly (use feedback flag) to make sure your patch isn't lost in bug spam.
|
|
||
Comment 15•13 years ago
|
||
Comment on attachment 626071 [details] [diff] [review]
Patch 2
>diff --git a/accessible/src/base/nsDocAccessible.cpp b/accessible/src/base/nsDocAccessible.cpp
>--- a/accessible/src/base/nsDocAccessible.cpp
>+++ b/accessible/src/base/nsDocAccessible.cpp
>@@ -1881,48 +1881,34 @@ nsDocAccessible::UpdateTree(nsAccessible
>
> PRUint32
> nsDocAccessible::UpdateTreeInternal(nsAccessible* aChild, bool aIsInsert)
> {
> PRUint32 updateFlags = eAccessible;
>
> nsINode* node = aChild->GetNode();
> if (aIsInsert) {
>+ // Fire show event.
>+ nsRefPtr<AccEvent> event = new AccShowEvent(aChild, node);
>+ if (event)
>+ FireDelayedAccessibleEvent(event);
> // Create accessible tree for shown accessible.
> CacheChildrenInSubtree(aChild);
reorganizing this method a little might be nice, but you should spend a little more timing sure that you do the same things in the case of insert and removal as happened before. Order also matters here, you should fire this event after you call CacheSubTreeInternal()
>+ nsRefPtr<AccEvent> event = new AccHideEvent(aChild, node);
>+ if (event)
>+ FireDelayedAccessibleEvent(event);
a general comment, we made new be infalible a while ago, but haven't touched this code since, so you should remove nolonger needed null checks when you touch code anyway.
> nsDocAccessible::CacheChildrenInSubtree(nsAccessible* aRoot)
> {
> aRoot->EnsureChildren();
>
> // Make sure we create accessible tree defined in DOM only, i.e. if accessible
> // provides specific tree (like XUL trees) then tree creation is handled by
> // this accessible.
> PRUint32 count = aRoot->ContentChildCount();
> for (PRUint32 idx = 0; idx < count; idx++) {
>- nsAccessible* child = aRoot->ContentChildAt(idx);
>+ nsAccessible* child = aRoot->ContentChildAt(idx);
> NS_ASSERTION(child, "Illicit tree change while tree is created!");
> // Don't cross document boundaries.
>- if (child && child->IsContent())
>+ if (child && child->IsContent()){
>+ roles::Role ariaRole = child->ARIARole();
>+ if (ariaRole == roles::MENUPOPUP) {
>+ // Fire EVENT_MENUPOPUP_START if ARIA menu appears.
>+ FireDelayedAccessibleEvent(nsIAccessibleEvent::EVENT_MENUPOPUP_START,
>+ child->GetNode(), AccEvent::eRemoveDupes);
I'd suggest handling this after the loop, and letting recursion take care of visiting all the nodes of the tree, so the root isn't a special case.
>- for (PRUint32 idx = 0; idx < count; idx++)
>- UncacheChildrenInSubtree(aRoot->ContentChildAt(idx));
>+ for (PRUint32 idx = 0; idx < count; idx++) {
>+ nsAccessible* child = aRoot->ContentChildAt(idx);
>+ // Fire EVENT_MENUPOPUP_END
>+ if (child->ARIARole() == roles::MENUPOPUP) {
>+ nsRefPtr<AccEvent> event =
>+ new AccEvent(nsIAccessibleEvent::EVENT_MENUPOPUP_END, child);
similar here.
|
|
||
Comment 16•13 years ago
|
||
Removed firing of events in updateTreeInternal and moved them to cache and uncache subtree methods. cleaned up code to remove creation checks (i.e. if(event) after event was created by new).
Attachment #626071 -
Attachment is obsolete: true
Attachment #626264 -
Flags: feedback?(trev.saunders)
|
|
||
Comment 17•13 years ago
|
||
Comment on attachment 626264 [details] [diff] [review]
Patch 3
>diff --git a/accessible/src/base/nsDocAccessible.cpp b/accessible/src/base/nsDocAccessible.cpp
>--- a/accessible/src/base/nsDocAccessible.cpp
>+++ b/accessible/src/base/nsDocAccessible.cpp
>@@ -1881,61 +1881,25 @@ nsDocAccessible::UpdateTree(nsAccessible
>
> PRUint32
> nsDocAccessible::UpdateTreeInternal(nsAccessible* aChild, bool aIsInsert)
> {
> PRUint32 updateFlags = eAccessible;
>
> nsINode* node = aChild->GetNode();
> if (aIsInsert) {
>+ // Fire show event.
>+ nsRefPtr<AccEvent> event = new AccShowEvent(aChild, node);
>+ FireDelayedAccessibleEvent(event);
>+
> // Create accessible tree for shown accessible.
> CacheChildrenInSubtree(aChild);
I think you shouldn't change behavior here by moving the event firing to before caching the subtree.
> UncacheChildrenInSubtree(aChild);
>+
>+ // Fire hide event.
>+ nsRefPtr<AccEvent> event = new AccHideEvent(aChild, node);
>+ FireDelayedAccessibleEvent(event);
similarly its a change to fire hide event after uncacing, not sure if this is good or bad, but I'd tend to say not to change it here.
>+ // Fire EVENT for root then children
comment is sort of funny, you only fire event for what actually is a menupopup
>+ if (aRoot->ARIARole() == roles::MENUPOPUP) {
>+ // Fire EVENT_MENUPOPUP_START if ARIA menu appears.
>+ FireDelayedAccessibleEvent(nsIAccessibleEvent::EVENT_MENUPOPUP_START,
>+ aRoot->GetNode(), AccEvent::eRemoveDupes);
>+
>+ } else if (aRoot->ARIARole() == roles::ALERT) {
not sure you should change alert logic too
I noticed you introduced some whitespace at the end of lines, please configure editor to not do tht.
otherwise this seems fine, please have surkov review next version.
Attachment #626264 -
Flags: feedback?(trev.saunders) → feedback+
|
|
Reporter | |
Comment 18•13 years ago
|
||
Comment on attachment 626264 [details] [diff] [review]
Patch 3
Review of attachment 626264 [details] [diff] [review]:
-----------------------------------------------------------------
1) please fix Trevor's comment
2) add mochitests for examples I described in comment #8 (replace example 1 on loading a document containing ARIA alert into iframe), you can extend events/test_aria_alert.html and events/test_aria_menu.html to test new logic
3) upload the patch and ask for review
::: accessible/src/base/nsDocAccessible.cpp
@@ +1885,5 @@
> PRUint32 updateFlags = eAccessible;
>
> nsINode* node = aChild->GetNode();
> if (aIsInsert) {
> + // Fire show event.
nit: wrong indentation
|
|
||
Comment 19•13 years ago
|
||
(In reply to Trevor Saunders (:tbsaunde) from comment #17)
> Comment on attachment 626264 [details] [diff] [review]
> Patch 3
>
> >diff --git a/accessible/src/base/nsDocAccessible.cpp b/accessible/src/base/nsDocAccessible.cpp
> >--- a/accessible/src/base/nsDocAccessible.cpp
> >+++ b/accessible/src/base/nsDocAccessible.cpp
> >@@ -1881,61 +1881,25 @@ nsDocAccessible::UpdateTree(nsAccessible
> >
> > PRUint32
> > nsDocAccessible::UpdateTreeInternal(nsAccessible* aChild, bool aIsInsert)
> > {
> > PRUint32 updateFlags = eAccessible;
> >
> > nsINode* node = aChild->GetNode();
> > if (aIsInsert) {
> >+ // Fire show event.
> >+ nsRefPtr<AccEvent> event = new AccShowEvent(aChild, node);
> >+ FireDelayedAccessibleEvent(event);
> >+
> > // Create accessible tree for shown accessible.
> > CacheChildrenInSubtree(aChild);
>
> I think you shouldn't change behavior here by moving the event firing to
> before caching the subtree.
This was done to accommodate:
(In reply to alexander :surkov from comment #12)
> Comment on attachment 625439 [details] [diff] [review]
> patch
>
> Review of attachment 625439 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: accessible/src/base/nsDocAccessible.cpp
> @@ +1972,5 @@
> > + } else if (ariaRole == roles::ALERT) {
> > + // Fire EVENT_ALERT if ARIA alert appears.
> > + FireDelayedAccessibleEvent(nsIAccessibleEvent::EVENT_ALERT, child->GetNode(),
> > + AccEvent::eRemoveDupes);
> > + }
>
> this makes event fired before show event what is not right (show event means
> for AT that accessible is created)
>
> @@ +1995,5 @@
> > +
> > + if (event)
> > + FireDelayedAccessibleEvent(event);
> > + }
> > + UncacheChildrenInSubtree(child);
>
> this makes it fire after hide event (hide event means for AT accessible is
> destroyed)
If there is another way that I should do it, please let me know otherwise the firing order will not be changed from what it currently is at.
|
|
||
Comment 20•13 years ago
|
||
(In reply to Trevor Saunders (:tbsaunde) from comment #17)
> Comment on attachment 626264 [details] [diff] [review]
> Patch 3
> >+ // Fire EVENT for root then children
>
> comment is sort of funny, you only fire event for what actually is a
> menupopup
>
Comment has been removed as the recursion is self explanitory
> >+ if (aRoot->ARIARole() == roles::MENUPOPUP) {
> >+ // Fire EVENT_MENUPOPUP_START if ARIA menu appears.
> >+ FireDelayedAccessibleEvent(nsIAccessibleEvent::EVENT_MENUPOPUP_START,
> >+ aRoot->GetNode(), AccEvent::eRemoveDupes);
> >+
> >+ } else if (aRoot->ARIARole() == roles::ALERT) {
>
> not sure you should change alert logic too
>
> I noticed you introduced some whitespace at the end of lines, please
> configure editor to not do tht.
>
Alert logic has been taken from the updateTreeInternal method before the changes.
|
|
||
Comment 21•13 years ago
|
||
No mochitests were added as the current mochitests seem to be sufficient.
Attachment #626264 -
Attachment is obsolete: true
Attachment #628574 -
Flags: feedback?(surkov.alexander)
|
|
Reporter | |
Comment 22•13 years ago
|
||
(In reply to Daniel Goodrich from comment #21)
> No mochitests were added as the current mochitests seem to be sufficient.
If they are sufficient then why they don't fail?
|
||
Comment 23•13 years ago
|
||
(In reply to alexander :surkov from comment #22)
> (In reply to Daniel Goodrich from comment #21)
>
> > No mochitests were added as the current mochitests seem to be sufficient.
>
> If they are sufficient then why they don't fail?
So they are suppose to fail?
|
|
||
Comment 24•13 years ago
|
||
(In reply to alexander :surkov from comment #22)
> (In reply to Daniel Goodrich from comment #21)
>
> > No mochitests were added as the current mochitests seem to be sufficient.
>
> If they are sufficient then why they don't fail?
So they are suppose to fail?
|
|
Reporter | |
Comment 25•13 years ago
|
||
say you have a bug which is covered by automated test. When you fix a bug then that test should fail, if it doesn't then bug is not covered by that test. Sounds correct?
|
|
||
Comment 26•13 years ago
|
||
(In reply to alexander :surkov from comment #25)
> say you have a bug which is covered by automated test. When you fix a bug
> then that test should fail, if it doesn't then bug is not covered by that
> test. Sounds correct?
Ok, I understand this but I have a question, I made the changes to the DocAccessible and added tests to events/test_aria_alert.html, which passes, but if I revert the changes of DocAccessible to the original from the repository and keep the added tests, should it fail?
|
|
Reporter | |
Comment 27•13 years ago
|
||
(In reply to Jenny from comment #26)
> (In reply to alexander :surkov from comment #25)
> Ok, I understand this but I have a question, I made the changes to the
> DocAccessible and added tests to events/test_aria_alert.html, which passes,
> but if I revert the changes of DocAccessible to the original from the
> repository and keep the added tests, should it fail?
if test is correct then yes
|
|
Reporter | |
Comment 28•13 years ago
|
||
Any success on mochitest? If you run into problems then you might want to show it for feedback.
|
|
||
Comment 29•13 years ago
|
||
It is finals week for us, so we are concentrating on that more than this bug.
|
|
Reporter | |
Comment 30•13 years ago
|
||
(In reply to Daniel Goodrich from comment #29)
> It is finals week for us, so we are concentrating on that more than this bug.
sure, please take your time
|
|
Reporter | |
Comment 31•13 years ago
|
||
Comment on attachment 628574 [details] [diff] [review]
Patch 4 - on new DocAccessible.cpp file - no mochitests
cancelling feedback until comments are addressed
Attachment #628574 -
Flags: feedback?(surkov.alexander)
|
||
Comment 32•3 years ago
|
||
The bug assignee didn't login in Bugzilla in the last 7 months.
:Jamie, could you have a look please?
For more information, please visit auto_nag documentation.
Assignee: Jennyherrera.19 → nobody
Flags: needinfo?(jteh)
|
|
||
Updated•3 years ago
|
|
|
||
Updated•3 years ago
|
Severity: S4 → S3
|
|
||
Updated•2 years ago
|
See Also: → https://github.com/nvaccess/nvda/issues/13108
|
||
Updated•9 months ago
|
|
|
||
Updated•2 months ago
|
|
|
||
Comment 33•2 months ago
|
||
Nathan discovered that even when we do fire a menu popup hide event, we fire it after the hide event, despite the contradictory code comment. For remote content documents (i.e. the web), this means we don't fire the menu popup end event at all because the hide event causes the RemoteAccessible to be destroyed, resulting in this error. So, we need to fix this as well, either as part of this work or separately in bug 1556047.
|
Assignee | |
Updated•26 days ago
|
Assignee: nobody → eitan
|
|
Assignee | |
Comment 34•25 days ago
|
||
I placed them in a way that will ensure the correct order of events and
not allow menu start or alert events to get fired before the show, and
not allow menu end events to fire after the hide.
|
||
Comment 35•24 days ago
|
||
Pushed by eisaacson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ad04f56318fe
Fire alert/menu events on subtree insertion/deletion. r=nlapre
|
||
Comment 36•23 days ago
|
||
| bugherder | ||
Status: NEW → RESOLVED
Closed: 23 days ago
status-firefox136:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 136 Branch
You need to log in
before you can comment on or make changes to this bug.
Description
•