Joanmarie Diggs Reporter
	Description • 7 years ago

Steps to reproduce:
1. Launch Firefox
2. In the location bar type: javascript:alert("alert");
3. Press Return
4. Use an accessibility inspector like Accerciser to view the accessibility tree

Expected results: There would not be any object with ROLE_UNKNOWN in the tree.

Actual results: The child of the alert/dialog has ROLE_UNKNOWN. That unknown object  contains the dialog box contents.

Impact: Screen readers don't know what the "unknown" object is and may not present it as a result. While we could presumably hack around it, it would be far preferable to either prune this object from the tree or give it a role that indicates what its purpose is.

BTW: I am pretty sure this is a regression. However, I'm afraid I don't have time to track down when it was introduced. Sorry!

	alexander :surkov (:asurkov)
	Comment 1 • 7 years ago

<vbox anonid="mainContainer"> should have role="alert" I suppose, here's the widget itself [2] if I'm right. Gijs, may you take a look at the findings?

[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/prompts/content/tabprompts.xml#22

	:Gijs (he/him)
	Comment 2 • 7 years ago

(In reply to alexander :surkov from comment #1)
> <vbox anonid="mainContainer"> should have role="alert" I suppose, here's the
> widget itself [2] if I'm right. Gijs, may you take a look at the findings?
> 
> [2]
> https://dxr.mozilla.org/mozilla-central/source/toolkit/components/prompts/
> content/tabprompts.xml#22

This doesn't really make sense to me. Why does the <vbox> need a role but not the containing <hbox> ?

They're both basically visual-styling-only boxes. They shouldn't show up in the a11y tree at all, I think. Why do they?

	alexander :surkov (:asurkov)
	Comment 3 • 7 years ago

(In reply to :Gijs from comment #2)
> (In reply to alexander :surkov from comment #1)
> > <vbox anonid="mainContainer"> should have role="alert" I suppose, here's the
> > widget itself [2] if I'm right. Gijs, may you take a look at the findings?
> > 
> > [2]
> > https://dxr.mozilla.org/mozilla-central/source/toolkit/components/prompts/
> > content/tabprompts.xml#22
> 
> This doesn't really make sense to me. Why does the <vbox> need a role but
> not the containing <hbox> ?
> They're both basically visual-styling-only boxes. They shouldn't show up in
> the a11y tree at all, I think. Why do they?

From what I can tell the <vbox anonid="mainContainer"> corresponds to visual alert dialog boundaries; the container that corresponds to alert dialog inside tabpromptbox widget should have alertdialog role I think.

	:Gijs (he/him)
	Comment 4 • 7 years ago

(In reply to alexander :surkov from comment #3)
> (In reply to :Gijs from comment #2)
> > (In reply to alexander :surkov from comment #1)
> > > <vbox anonid="mainContainer"> should have role="alert" I suppose, here's the
> > > widget itself [2] if I'm right. Gijs, may you take a look at the findings?
> > > 
> > > [2]
> > > https://dxr.mozilla.org/mozilla-central/source/toolkit/components/prompts/
> > > content/tabprompts.xml#22
> > 
> > This doesn't really make sense to me. Why does the <vbox> need a role but
> > not the containing <hbox> ?
> > They're both basically visual-styling-only boxes. They shouldn't show up in
> > the a11y tree at all, I think. Why do they?
> 
> From what I can tell the <vbox anonid="mainContainer"> corresponds to visual
> alert dialog boundaries; the container that corresponds to alert dialog
> inside tabpromptbox widget should have alertdialog role I think.

I'm still confused why the vbox/hbox are in the tree right now. Can you clarify this?

As for the alertdialog role, the binding already has:

    <xbl:content xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
                 role="dialog"
                 aria-describedby="infoBody">

so it already has role=dialog. We can make that role=alertdialog instead, but without more info I don't think we should set role=dialog/alertdialog twice.

	Joanmarie Diggs Reporter
	Comment 5 • 7 years ago

Personally, I think it should be pruned from the tree.

That said, if for some reason there is any hbox or vbox which must be included in any tree, please give it ROLE_PANEL (the generic widget-container role). I'm pretty sure that's what GTK+ does.

If you decide to go that route, I think it would make sense to do it in a way that didn't require an explicit ARIA role, but instead JustWorked(tm). After all, ROLE_UNKNOWN is always wrong and having to explicitly take an action on a regular basis to prevent that problem is inefficient and doomed to fail. Extra panels are noise, but noise we're used to.

	alexander :surkov (:asurkov)
	Comment 6 • 7 years ago

I'd say we should get rid of role="dialog" from tabmodalprompt binding content as it's a background element, and put role=alertdialog it to the actual dialog which is present by vbox.mainContainer, which is focusable and thus has unknown role accessible.

Joanie, do you agree that we should do role=alertdialog and put it on an focusable element that represents a dialog itself?

Gijs, does it make sense with you?

	:Gijs (he/him)
	Comment 7 • 7 years ago

(In reply to alexander :surkov from comment #6)
> I'd say we should get rid of role="dialog" from tabmodalprompt binding
> content as it's a background element, and put role=alertdialog it to the
> actual dialog which is present by vbox.mainContainer, which is focusable and
> thus has unknown role accessible.
> 
> Joanie, do you agree that we should do role=alertdialog and put it on an
> focusable element that represents a dialog itself?
> 
> Gijs, does it make sense with you?

WFM, but we may need to update commonDialog.xul as well.

	Joanmarie Diggs Reporter
	Comment 8 • 7 years ago

(In reply to alexander :surkov from comment #6)
> I'd say we should get rid of role="dialog" from tabmodalprompt binding
> content as it's a background element,

Will this cause the above object to be pruned from the accessibility tree? Or to put it another way, will this change result in a new (and unexpected) parent for the following object:

> and put role=alertdialog it to the
> actual dialog which is present by vbox.mainContainer, which is focusable and
> thus has unknown role accessible.

?

	alexander :surkov (:asurkov)
	Comment 9 • 7 years ago

yeah, I think that should remove tabmodalprompt element form the hierarchy, but we should double check that of course

	alexander :surkov (:asurkov)
	Comment 10 • 7 years ago

Yura, could you give an advice how to cover javascript:alert('ho-ho') by browsers tests with some links if possible?

	James Teh [:Jamie]
	Comment 11 • 7 years ago

I question why the dialog root itself is focusable at all. That's non-standard for a dialog. Right now, there are three focusable things for alert("alert"):
1. When it first opens, the OK button has focus.
2. If you shift+tab, you get to the "alert" label. This too is non-standard, but I can see how some might like this.
3. If you shift+tab again, focus hits the vbox, which according to comment 3 should be the dialog itself semantically speaking (currently role unknown, proposed role alertdialog).

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 12 • 7 years ago

(In reply to alexander :surkov from comment #10)
> Yura, could you give an advice how to cover javascript:alert('ho-ho') by
> browsers tests with some links if possible?

I would look at accessible/tests/browser/events/browser_test_focus_dialog.js test. It does not have an actual alert but I think it would be similar where we would wait for a show or focused accessible event on the alert (granted it will need a custom target matcher).