656225 - [A11Y]New profile item in firefox4.0 user profile dialog is not accessible.

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Tim Miao]

Steps to reproduce:
1. Enable a11y support and start firefox4.0 user profile creator with command: firefox -P
2. Start accerciser.
3. Create a new profile in firefox user profile dialog, follow the wizard to finish this setup and back to firefox user profile dialog.
4. Poke firefox user profile dialog in accerciser to the new created profile.

Expected result:
New profile table list item should be accessible to at-tools.

Actual result:
New profile table list item is not accessible to at-tools

Comment 1

[Marco Zehe (:MarcoZ)]

I can confirm this on Windows, too. Here are my observations:
1. After the new profile has been created, all new and old entries are navigable using the arrow keys.
2. The list itself gets the name of the currently selected profile.
3. The list items only reflect the profiles that were there before the new profile was created. The new profile does NOT show up as a list item.

This is still the case in Tuesday May 10th nightly build.

Comment 2

[Marco Zehe (:MarcoZ)]

This might impact extensions dynamically updating listboxes, requesting whether we want to track this and see if we can have a fix for FX6.

Comment 3

[Ginn Chen]

surkov, how do we watch child add/remove for xul list box?
For the profile manager dialog, we don't have a document, right?

I have noticed another problem here, if I have 10 profiles, I can only see 5 profiles in Accerciser, i.e. only profiles which are visible without moving the scrollbar have accessible objects.
If I scorll up/down and force UpdateChildren(), all profiles will be listed in Accerciser.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to comment #3)
> surkov, how do we watch child add/remove for xul list box?

we don't have special case for list box, layout notifies us the content is inserted, maybe something wrong with out tree update logic.

> For the profile manager dialog, we don't have a document, right?

we always should have a document

> I have noticed another problem here, if I have 10 profiles, I can only see 5
> profiles in Accerciser, i.e. only profiles which are visible without moving
> the scrollbar have accessible objects.
> If I scorll up/down and force UpdateChildren(), all profiles will be listed
> in Accerciser.

that's may be related that frames objects aren't created until they are visible.

Comment 5

[alexander :surkov (:asurkov)]

XUL listboxes have a special handling in nsCSSFrameConstructor::ContentRangeInserted by NotifyListBoxBody method (http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#6872), that's a reason why we don't get any notifications.

Boris, what's the best place to add a11y notification for this case?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Where do you handle a11y notifications right now when someone scrolls a listbox?

Comment 7

[Asa Dotzler [:asa]]

Is this a regression? If so, when did it break? Is it broken for all listboxes?

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #6)
> Where do you handle a11y notifications right now when someone scrolls a
> listbox?

Probably nowhere. We have scroll listener registered on root frame but nothing special on listbox. But I don't understand why you ask this.

If I get right there's two problems here:
1) when listitem is appended to listbox then a11y doesn't receive notification (comment #5)
2) when listbox has more items than rows count then not visible listitems aren't accessible, I think they should have accessible objects in this case (or at least create them when listbox is focused).

Comment 9

[Boris Zbarsky [:bzbarsky]]

> But I don't understand why you ask this.

Because listboxes only create frames for the items that are visible.  So depending on what you're trying to do with the non-visible items, exactly, the exact locations where you want to hook in the accessible creation might vary...

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> Is this a regression? If so, when did it break? Is it broken for all
> listboxes?

It is, from Firefox 3.6 I believe. For all listboxes.

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #9)
> > But I don't understand why you ask this.
> 
> Because listboxes only create frames for the items that are visible.  So
> depending on what you're trying to do with the non-visible items, exactly,
> the exact locations where you want to hook in the accessible creation might
> vary...

I think all the screen reader should know is the total number of list items in listbox. If accessible objects for list items will be created dynamically (for example, when user scrolls or arrows down and select them) then it should work too. If we would created accessible objects for all listitems (visible and scrolled off) then for sure we expose correct the number list items. But while they weren't created then we need to have a way to calculate it. It sounds it's not very right to rely on DOM here.

But here's as I said there's second problem. When I do
listbox.appendItem(name, '');
then it doesn't trigger a11y notification (details in comment 5). 

Note, the list item is visible.

If I do 
var l = listbox.appendChild(document.createElement("listbox"));
l.setAttribute("label", name);
then it works and accessible is created.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> var l = listbox.appendChild(document.createElement("listbox"));
> l.setAttribute("label", name);

Did you really append a listbox to a listbox there?

Comment 13

[alexander :surkov (:asurkov)]

(In reply to comment #12)
> > var l = listbox.appendChild(document.createElement("listbox"));
> > l.setAttribute("label", name);
> 
> Did you really append a listbox to a listbox there?

of course no, listitem.

Comment 14

[alexander :surkov (:asurkov)]

(In reply to comment #11)

> If I do 
> var l = listbox.appendChild(document.createElement("listitem"));
> l.setAttribute("label", name);
> then it works and accessible is created.

this works because presumably we get notification about frame changes for label. AppendChild doesn't trigger a11y notifications (details in comment 5), that's why listbox.appendItem fails - http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/listbox.xml#750

Boris, how can I proceed with this?

Comment 15

[Boris Zbarsky [:bzbarsky]]

I think it still really depends on what you want the behavior to be for out-of-view listitems.

Most simply, if you just want to be notified when listitem frames are created and destroyed, you want to hook into nsCSSFrameConstructor::CreateListBoxContent and nsListBoxBodyFrame::RemoveChildFrame respectively.

But if you want something else, then you'll need to tell me what that something else is.

Comment 16

[alexander :surkov (:asurkov)]

(In reply to comment #15)
> I think it still really depends on what you want the behavior to be for
> out-of-view listitems.
> 
> Most simply, if you just want to be notified when listitem frames are
> created and destroyed, you want to hook into
> nsCSSFrameConstructor::CreateListBoxContent and
> nsListBoxBodyFrame::RemoveChildFrame respectively.

ok, thanks, that's for first part of bug.

> But if you want something else, then you'll need to tell me what that
> something else is.

Is there a reliable way to know how may items in the listbox (when frames for out-of-view items aren't created yet)? If we can know this then it should be a fix for the second part of bug.

Comment 17

[Boris Zbarsky [:bzbarsky]]

The nsIListBoxObject for a listbox has GetRowCount on it.  So does nsListBoxBodyFrame if you have one directly.

Comment 18

[alexander :surkov (:asurkov)]

(In reply to comment #15)
> I think it still really depends on what you want the behavior to be for
> out-of-view listitems.
> 
> Most simply, if you just want to be notified when listitem frames are
> created and destroyed, you want to hook into
> nsCSSFrameConstructor::CreateListBoxContent and

Boris, is CreateListBoxContent called for every listitem frame? For example, if I add dynamically listbox with 3 listitems then does ContentRangeInserted trigger for listbox and then CreateListBoxContent trigger for each listitems?

var lb = document.createElement("listbox");
for (var i = 0; i < 3; i++)
  lb.appendChild(document.createElement("listitem"));
document.documentElement.appendChild(lb);

Comment 19

[Boris Zbarsky [:bzbarsky]]

CreateListBoxContent will trigger for each listitem separately.

It will happen sometime after ContentRangeInserted returns (after the next reflow, iirc).

It will not necessarily happen for all the items you insert, of course.

Comment 20

[alexander :surkov (:asurkov)]

Attachment: patch

it seems I won't hit bug 690417 any time soon to work out the performant solution. At least Firefox UI doesn't have examples with tons of listitems so it's not going to hit us. If we get a case when this is a problem for add-ons then let's bump the priority.

Nowdays it looks like frames for scrolled out listitems are created so we don't need to care about group attributes for now. If one day list items frames will be created on demand (in case if it was a regression for example) then added mochitest will fail.

Comment 21

[Trevor Saunders (:tbsaunde)]

Comment on attachment 608601 [details] [diff] [review]
patch

>+      <a target="_blank"
>+         href="https://bugzilla.mozilla.org/show_bug.cgi?id=656225"
>+         title="XUL listbox accessible tree doens't get updated">

typo?

>-    if (NS_SUCCEEDED(rv) && (nsnull != newFrame)) {
>+    if (newFrame) {
>       // Notify the parent frame
>       if (aIsAppend)
>         rv = ((nsListBoxBodyFrame*)aParentFrame)->ListBoxAppendFrames(frameItems);
>       else
>         rv = ((nsListBoxBodyFrame*)aParentFrame)->ListBoxInsertFrames(aPrevFrame, frameItems);
>     }
> 
>     EndUpdate();
>+
>+#ifdef ACCESSIBILITY
>+    if (newFrame) {
>+      nsAccessibilityService* accService = nsIPresShell::AccService();
>+      if (accService) {
>+        accService->ContentRangeInserted(mPresShell, aChild->GetParent(),
>+                                         aChild, aChild->GetNextSibling());

this organization feels a little  weird to me, but I don't know the requirements here well enough to know if they'res a better option.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 608601 [details] [diff] [review]
patch

r=me

Comment 23

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d7b5a5d1b8c

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7d7b5a5d1b8c

Comment 25

[Marco Zehe (:MarcoZ)]

Verified fixed in Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20120329 Firefox/14.0a1.

Note that this bug also fixes high-profile and high-user-visible Thunderbird bug 673860.

Comment 26

[alexander :surkov (:asurkov)]

Comment on attachment 608601 [details] [diff] [review]
patch

[Approval Request Comment]
Regression caused by (bug #): unknown
User impact if declined: dynamic XUL listboxes are inaccessible (hits Firefox UI, add-ons, Thunderbird)
Testing completed (on m-c, etc.): yes
Risk to taking this patch (and alternatives if risky): possible perf problems in add-ons if somebody uses XUL listboxes a lot (since a11y community is not large then we won't figure it out until Firefox release).
String changes made by this patch: none

Comment 27

[Alex Keybl [:akeybl]]

(In reply to alexander :surkov from comment #26)
> User impact if declined: dynamic XUL listboxes are inaccessible (hits
> Firefox UI, add-ons, Thunderbird)

Is there any reason to believe that this bug is worse today than in May 2011? I haven't seen any dupes, so my default position would be to let this ride the trains.

Comment 28

[alexander :surkov (:asurkov)]

(In reply to Alex Keybl [:akeybl] from comment #27)
> (In reply to alexander :surkov from comment #26)
> > User impact if declined: dynamic XUL listboxes are inaccessible (hits
> > Firefox UI, add-ons, Thunderbird)
> 
> Is there any reason to believe that this bug is worse today than in May
> 2011? 

No, just keeping the users unhappy a little bit longer.

>I haven't seen any dupes, so my default position would be to let this
> ride the trains.

there's one dupe, bug 673860. but a11y community work differently, many people comes to irc channel and ask if some bug is known.

Comment 29

[Alex Keybl [:akeybl]]

(In reply to alexander :surkov from comment #28)
> (In reply to Alex Keybl [:akeybl] from comment #27)
> > (In reply to alexander :surkov from comment #26)
> > > User impact if declined: dynamic XUL listboxes are inaccessible (hits
> > > Firefox UI, add-ons, Thunderbird)
> > 
> > Is there any reason to believe that this bug is worse today than in May
> > 2011? 
> 
> No, just keeping the users unhappy a little bit longer.
> 
> >I haven't seen any dupes, so my default position would be to let this
> > ride the trains.
> 
> there's one dupe, bug 673860. but a11y community work differently, many
> people comes to irc channel and ask if some bug is known.

Thanks for the background - one last question. Is the perf risk here only for a11y users?

Comment 30

[alexander :surkov (:asurkov)]

(In reply to Alex Keybl [:akeybl] from comment #29)

> Thanks for the background - one last question. Is the perf risk here only
> for a11y users?

yes

Comment 31

[Alex Keybl [:akeybl]]

Comment on attachment 608601 [details] [diff] [review]
patch

(In reply to alexander :surkov from comment #30)
> (In reply to Alex Keybl [:akeybl] from comment #29)
> 
> > Thanks for the background - one last question. Is the perf risk here only
> > for a11y users?
> 
> yes

OK - given that, approving for Aurora 13. If we don't expect to see fallout until late in testing (or post-release) regardless, cutting 3 weeks off of the bake time does not introduce significant risk.

Comment 32

[Marco Zehe (:MarcoZ)]

Landed on Alexander's behalf on Aurora: http://hg.mozilla.org/releases/mozilla-aurora/rev/c77d34d9dc8c. Note a slight merge in the Makefile.in file, rest applied cleanly.