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

VERIFIED FIXED in Firefox 13

Status

()

Core
Disability Access APIs
--
major
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: Tim Miao, Assigned: surkov)

Tracking

(Blocks: 1 bug, {access})

Trunk
mozilla14
access
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox6-, firefox13 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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
(Reporter)

Updated

6 years ago
Keywords: access

Comment 1

6 years ago
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.
Component: Disability Access → Disability Access APIs
OS: Solaris → All
Product: Firefox → Core
QA Contact: disability.access → accessibility-apis
Hardware: x86 → All
Version: 4.0 Branch → Trunk

Comment 2

6 years ago
This might impact extensions dynamically updating listboxes, requesting whether we want to track this and see if we can have a fix for FX6.
tracking-firefox6: --- → ?

Comment 3

6 years ago
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.
(Assignee)

Comment 4

6 years ago
(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.
Blocks: 572951
(Assignee)

Comment 5

6 years ago
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?
Where do you handle a11y notifications right now when someone scrolls a listbox?

Comment 7

6 years ago
Is this a regression? If so, when did it break? Is it broken for all listboxes?
(Assignee)

Comment 8

6 years ago
(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).
> 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...
(Assignee)

Comment 10

6 years ago
(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.
(Assignee)

Comment 11

6 years ago
(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.
> var l = listbox.appendChild(document.createElement("listbox"));
> l.setAttribute("label", name);

Did you really append a listbox to a listbox there?

Updated

6 years ago
Whiteboard: marco nominated in comment 2

Updated

6 years ago
tracking-firefox6: ? → -
(Assignee)

Comment 13

6 years ago
(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.
(Assignee)

Comment 14

6 years ago
(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?
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.
(Assignee)

Comment 16

6 years ago
(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.
The nsIListBoxObject for a listbox has GetRowCount on it.  So does nsListBoxBodyFrame if you have one directly.
(Assignee)

Comment 18

6 years ago
(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);
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.

Updated

6 years ago
Whiteboard: marco nominated in comment 2 → [marco nominated in comment 2] [bk1]
(Assignee)

Comment 20

5 years ago
Created attachment 608601 [details] [diff] [review]
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.
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #608601 - Flags: review?(trev.saunders)
Attachment #608601 - Flags: review?(bzbarsky)
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.
Attachment #608601 - Flags: review?(trev.saunders) → review+
Comment on attachment 608601 [details] [diff] [review]
patch

r=me
Attachment #608601 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 23

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d7b5a5d1b8c
(Assignee)

Updated

5 years ago
Flags: in-testsuite+
Whiteboard: [marco nominated in comment 2] [bk1]
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/7d7b5a5d1b8c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
(Assignee)

Comment 26

5 years ago
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
Attachment #608601 - Flags: approval-mozilla-aurora?
(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.
(Assignee)

Comment 28

5 years ago
(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.

Updated

5 years ago
Blocks: 674462
(Assignee)

Updated

5 years ago
Blocks: 673860
(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?
(Assignee)

Comment 30

5 years ago
(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 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.
Attachment #608601 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
status-firefox13: --- → fixed
You need to log in before you can comment on or make changes to this bug.