Last Comment Bug 809338 - make HTML select hierarchical
: make HTML select hierarchical
Status: RESOLVED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla23
Assigned To: Trevor Saunders (:tbsaunde)
:
Mentors:
Depends on:
Blocks: treea11y
  Show dependency treegraph
 
Reported: 2012-11-06 23:04 PST by alexander :surkov
Modified: 2013-04-23 04:12 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
wip (10.48 KB, patch)
2012-11-10 01:44 PST, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Review
wip (11.31 KB, patch)
2012-12-03 15:06 PST, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Review
My test case (489 bytes, text/html)
2012-12-07 01:17 PST, Marco Zehe (:MarcoZ)
no flags Details
bug 809338 - don't flatten optgroups (12.49 KB, patch)
2013-03-11 19:51 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Review
bug 809338 - don't flatten optgroups (14.74 KB, patch)
2013-04-10 23:08 PDT, Trevor Saunders (:tbsaunde)
surkov.alexander: review+
Details | Diff | Review

Description alexander :surkov 2012-11-06 23:04:51 PST
1) We were asked about this by AT developer
2) We do extra job on flattering HTML selects tree (and we didn't managed to make it correctly for all cases).

Jamie, do you have concerns?
Comment 1 James Teh [:Jamie] 2012-11-06 23:32:27 PST
Can you give an example of what the behaviour is currently and what it would be with this change? I'm not sure I understand.
Comment 2 alexander :surkov 2012-11-06 23:52:07 PST
It's all about optgroups

<select>
  <optgroup>
    <option><option>
  </optgroup>
  <option></option>
</select>

Now we have

combobox
  heading
  option
  option

We want

combobox
  heading
    option
  option
Comment 3 James Teh [:Jamie] 2012-11-07 01:31:43 PST
Ah. Got it. Actually, this is probably better for NVDA, since this way, we'll read the names of the groups as the user moves through the list.

Note that this may break screen readers that render the name of the currently selected item for select with @size > 1. Unless I'm missing something, they have to crawl into the list to get the selected item; I don't think there's any other way. NVDA doesn't currently do this, though arguably it should. However, I believe JAWS and probably other screen readers do. These screen readers may not expect to have to crawl more than one level deep.
Comment 4 alexander :surkov 2012-11-07 15:01:03 PST
I see.

Trev, would you like to sketch it up?
Comment 5 Trevor Saunders (:tbsaunde) 2012-11-10 01:44:00 PST
Created attachment 680335 [details] [diff] [review]
wip

this passes all tests locally, and seems about correct.  Yes I know CacheOptSiblings can just be inlined now ;)

Marco could you give remote:   https://tbpl.mozilla.org/?tree=Try&rev=b9dc8e850127 a try?
Comment 6 alexander :surkov 2012-11-10 02:11:24 PST
(In reply to Trevor Saunders (:tbsaunde) from comment #5)
> Yes I know
> CacheOptSiblings can just be inlined now ;)

I'd guess we can use accTreeWalker to create a tree.
Comment 7 Trevor Saunders (:tbsaunde) 2012-11-10 02:24:50 PST
(In reply to alexander :surkov from comment #6)
> (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> > Yes I know
> > CacheOptSiblings can just be inlined now ;)
> 
> I'd guess we can use accTreeWalker to create a tree.

well, I'm assuming we don't want to change that html select can only have option / optgroup accessibles for children atleast for now.  Of course giving GetOrCreateAccessible() context / parent accessible would take care of this, but it seems the easiest path forward is to just inline CacheOptSiblings()
Comment 8 alexander :surkov 2012-11-10 02:29:01 PST
(In reply to Trevor Saunders (:tbsaunde) from comment #7)
> (In reply to alexander :surkov from comment #6)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #5)

> well, I'm assuming we don't want to change that html select can only have
> option / optgroup accessibles for children atleast for now.

I'd guess DOM takes care about this.
Comment 9 Trevor Saunders (:tbsaunde) 2012-11-10 02:35:01 PST
(In reply to alexander :surkov from comment #8)
> (In reply to Trevor Saunders (:tbsaunde) from comment #7)
> > (In reply to alexander :surkov from comment #6)
> > > (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> 
> > well, I'm assuming we don't want to change that html select can only have
> > option / optgroup accessibles for children atleast for now.
> 
> I'd guess DOM takes care about this.

why?  I'd like that to be true, but I wouldn't expect it.  Do you have some evidence?  If not would you have objection to follow up for using nsAccTreeWalker to get kids for select / optgroup, I'd like to minimize the amount of change we do at once / number of things that could possibly slow this down.
Comment 10 alexander :surkov 2012-11-10 02:50:58 PST
(In reply to Trevor Saunders (:tbsaunde) from comment #9)
> (In reply to alexander :surkov from comment #8)

> > I'd guess DOM takes care about this.
> 
> why?  I'd like that to be true, but I wouldn't expect it.

somebody should take care about this: either DOM or layout. Usually DOM takes care. For example, DOM automatically generates tbody for HTML tables.

>  Do you have some
> evidence?

I didn't try but you can play trying on different options.

>  If not would you have objection to follow up for using
> nsAccTreeWalker to get kids for select / optgroup, I'd like to minimize the
> amount of change we do at once / number of things that could possibly slow
> this down.

come on, you changed tree hierarchy, nothing can be worst :) really just try to put randoms there and check out DOM.
Comment 11 Trevor Saunders (:tbsaunde) 2012-11-10 03:21:43 PST
(In reply to alexander :surkov from comment #10)
> (In reply to Trevor Saunders (:tbsaunde) from comment #9)
> > (In reply to alexander :surkov from comment #8)
> 
> > > I'd guess DOM takes care about this.
> > 
> > why?  I'd like that to be true, but I wouldn't expect it.
> 
> somebody should take care about this: either DOM or layout. Usually DOM
> takes care. For example, DOM automatically generates tbody for HTML tables.

ok, what I worry more about is things like optgroup.appendChild(form) when I was reducing another test case (you can maybe guess which) I noticed that <legend><fieldset></fieldset></legend> didn't get the legend inserted, but appendChild() could stick it there.

> >  Do you have some
> > evidence?
> 
> I didn't try but you can play trying on different options.

I was hoping for a link to some code in the DOM that did this, but sure I guess I'll play with it.

> >  If not would you have objection to follow up for using
> > nsAccTreeWalker to get kids for select / optgroup, I'd like to minimize the
> > amount of change we do at once / number of things that could possibly slow
> > this down.
> 
> come on, you changed tree hierarchy, nothing can be worst :) really just try
> to put randoms there and check out DOM.

heh, ok, though I'll wait for Marco before worrying about polish.
Comment 12 Marco Zehe (:MarcoZ) 2012-11-12 09:12:43 PST
Trev, I cannot run this build. Gives me some sort of funky errors that debug builds usually do. Could you kick off a win32 release build so I can try that with NVDA?
Comment 13 Trevor Saunders (:tbsaunde) 2012-11-12 09:28:31 PST
Marco remote:   https://tbpl.mozilla.org/?tree=Try&rev=8934aa733cf0
Comment 14 Marco Zehe (:MarcoZ) 2012-11-13 00:49:57 PST
I don't see a difference between a regular nightly and the try build. First I have the inner option in focus, then an empty line, then the outer option. This is according to the example given in comment #3, augmented with some actual text. So, NVDA gets the items, but doesn't do anything special with the item in the optgroup.
Comment 15 alexander :surkov 2012-11-13 00:52:35 PST
Marco, what about other screen readers?

(to be on safe side) did you tried both select (combobox) and select@size>1 (listbox) having optgroups?
Comment 16 James Teh [:Jamie] 2012-11-13 02:31:15 PST
(In reply to Marco Zehe (:MarcoZ) from comment #14)
> So, NVDA gets the items, but doesn't do anything special with
> the item in the optgroup.
I find this surprising. It should speak the group when you enter it with this change. What role does the group get? It should have a role of grouping imo.

I'm at a conference at present, so don't have time to test the try build, but I can give it a spin when I get back next week.
Comment 17 Marco Zehe (:MarcoZ) 2012-11-13 02:48:12 PST
With NVDA, a select with size > 1 is half broken. When initially entering, NVDA announces the optgroup text and the option, it even announces that this is on level 2. So unlike the size = 1 combo box, here the header is picked up correctly and spoken.
However, if I navigate out of this and onto an option on the same level as the opt group, and navigate back into the inner option, the header and inner option are no longer spoken. Once on the outer option, focus gets stuck there, for NVDA, only the selection is added and removed. But the inner option, so it seems, never gets the focus any more.
Comment 18 Marco Zehe (:MarcoZ) 2012-11-13 02:54:25 PST
JAWS 13 is largely broken with comboboxes and list boxes in that try build. It only gets the focus properly once after initially focusing the combobox or listbox, but after that, arrowing does not speak the focus in either case. When tabbing out and back in, the newly focused item is seen once, but arrowing will yield silence again.
Comment 19 Trevor Saunders (:tbsaunde) 2012-11-13 03:21:53 PST
(In reply to James Teh [:Jamie] from comment #16)
> (In reply to Marco Zehe (:MarcoZ) from comment #14)
> > So, NVDA gets the items, but doesn't do anything special with
> > the item in the optgroup.
> I find this surprising. It should speak the group when you enter it with
> this change. What role does the group get? It should have a role of grouping
> imo.

I'd tend to agree, but in that build it should have heading role as we've been doing for optgroups for a while
Comment 20 alexander :surkov 2012-11-16 22:30:48 PST
I tried the build http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/trev.saunders@gmail.com-8934aa733cf0/try-win32/ and I don't see hierarchical comboboxes. Where can I get working one?
Comment 21 alexander :surkov 2012-11-16 22:42:35 PST
(In reply to alexander :surkov from comment #20)
> I tried the build
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/trev.
> saunders@gmail.com-8934aa733cf0/try-win32/ and I don't see hierarchical
> comboboxes. Where can I get working one?

ignore it that seems to be right build
Comment 22 alexander :surkov 2012-11-19 16:47:45 PST
Trev, btw, I was reported that group position is not correct (it doesn't take into account optgroups).

Let's try compatibility mode (default is hierarchical select, compatibility is flat)? And let's make sure that Firefox 19/20 is not releasing before next NVDA having a fix. Everything else let's run under compatibility mode.

Btw, we don't have any data about Orca, if it's broken then we should contact to Joanie.
Comment 23 alexander :surkov 2012-11-19 16:47:46 PST
Trev, btw, I was reported that group position is not correct (it doesn't take into account optgroups).

Let's try compatibility mode (default is hierarchical select, compatibility is flat)? And let's make sure that Firefox 19/20 is not releasing before next NVDA having a fix. Everything else let's run under compatibility mode.

Btw, we don't have any data about Orca, if it's broken then we should contact to Joanie.
Comment 24 Trevor Saunders (:tbsaunde) 2012-11-20 07:17:33 PST
(In reply to alexander :surkov from comment #23)
> Trev, btw, I was reported that group position is not correct (it doesn't
> take into account optgroups).

ok,  changing the role of the optgroup accessible from heading to grouping should probably fix that and be done anyway see above.

> Let's try compatibility mode (default is hierarchical select, compatibility
> is flat)? And let's make sure that Firefox 19/20 is not releasing before
> next NVDA having a fix. Everything else let's run under compatibility mode.

everything else == known screen readers on windows?  What about those dictionary things and the uia dll's?

> Btw, we don't have any data about Orca, if it's broken then we should
> contact to Joanie.

I tested quickly, it seemd to work fine.
Comment 25 alexander :surkov 2012-11-20 07:28:58 PST
(In reply to Trevor Saunders (:tbsaunde) from comment #24)
> (In reply to alexander :surkov from comment #23)
> > Trev, btw, I was reported that group position is not correct (it doesn't
> > take into account optgroups).
> 
> ok,  changing the role of the optgroup accessible from heading to grouping
> should probably fix that and be done anyway see above.

I wouldn't change roles. Perhaps we need to put a fix into AccGroupInfo.

> > Let's try compatibility mode (default is hierarchical select, compatibility
> > is flat)? And let's make sure that Firefox 19/20 is not releasing before
> > next NVDA having a fix. Everything else let's run under compatibility mode.
> 
> everything else == known screen readers on windows?

yes, AT we can reach.

>  What about those
> dictionary things and the uia dll's?

Dictionary things don't fall into category "AT we can reach" so ignore them. UIA means Narrator and other MS things, we don't have much info about it. We could run them under compatibility mode if we were required but in that case having running both JAWS and some MS piece may put us into collisions.

> > Btw, we don't have any data about Orca, if it's broken then we should
> > contact to Joanie.
> 
> I tested quickly, it seemd to work fine.

nice
Comment 26 James Teh [:Jamie] 2012-11-20 16:50:06 PST
This build is very broken for me. When I move to any option inside an optgroup, I don't get a focus event, nor is the focused state set, though the selected state is set (confirmed with AccProbe). Interestingly, if I bounce focus out and back in (e.g. by pressing alt twice), a focus event is correctly fired and the focused state is set. The offscreen state also seems to be exposed on options where it shouldn't be, but perhaps this is a different bug.

(In reply to alexander :surkov from comment #25)
> I wouldn't change roles.
Why not? Having options inside a role of heading really makes no sense. If you're concerned about breaking clients, making the select hierarchical is much more likely to cause breakage than changing the role.
Comment 27 alexander :surkov 2012-11-20 19:33:15 PST
(In reply to James Teh [:Jamie] from comment #26)
> This build is very broken for me. When I move to any option inside an
> optgroup, I don't get a focus event, nor is the focused state set, though
> the selected state is set (confirmed with AccProbe). Interestingly, if I
> bounce focus out and back in (e.g. by pressing alt twice), a focus event is
> correctly fired and the focused state is set. The offscreen state also seems
> to be exposed on options where it shouldn't be, but perhaps this is a
> different bug.

I don't spot anything evident, our widget interface should work since it doesn't depend on hierarchy. So I leave the question to Trev.

> (In reply to alexander :surkov from comment #25)
> > I wouldn't change roles.
> Why not? Having options inside a role of heading really makes no sense. If
> you're concerned about breaking clients, making the select hierarchical is
> much more likely to cause breakage than changing the role.

agree.
Comment 28 Trevor Saunders (:tbsaunde) 2012-12-03 14:58:45 PST
(In reply to alexander :surkov from comment #27)
> (In reply to James Teh [:Jamie] from comment #26)
> > This build is very broken for me. When I move to any option inside an
> > optgroup, I don't get a focus event, nor is the focused state set, though
> > the selected state is set (confirmed with AccProbe). Interestingly, if I
> > bounce focus out and back in (e.g. by pressing alt twice), a focus event is
> > correctly fired and the focused state is set. The offscreen state also seems
> > to be exposed on options where it shouldn't be, but perhaps this is a
> > different bug.
> 
> I don't spot anything evident, our widget interface should work since it
> doesn't depend on hierarchy. So I leave the question to Trev.

actually, the hirarchy change broke the implementtion of the widget interface on options.  Particularly getting the widget for an option in an optgroup didn't work because it expected the parent of an option was the select not the optgroup.
> > (In reply to alexander :surkov from comment #25)
> > > I wouldn't change roles.
> > Why not? Having options inside a role of heading really makes no sense. If
> > you're concerned about breaking clients, making the select hierarchical is
> > much more likely to cause breakage than changing the role.
> 
> agree.
Comment 29 Trevor Saunders (:tbsaunde) 2012-12-03 15:06:15 PST
Created attachment 687994 [details] [diff] [review]
wip

focus should work with options in optgroups this time, someone on windows want to try it?
https://tbpl.mozilla.org/?tree=Try&rev=9c7a3ee171d9
Comment 30 alexander :surkov 2012-12-03 17:36:13 PST
(In reply to Trevor Saunders (:tbsaunde) from comment #29)
> Created attachment 687994 [details] [diff] [review]
> wip
> 
> focus should work with options in optgroups this time, someone on windows
> want to try it?
> https://tbpl.mozilla.org/?tree=Try&rev=9c7a3ee171d9

it'd be good if you provided a link to the build
Comment 32 Marco Zehe (:MarcoZ) 2012-12-04 02:21:18 PST
With this latest try build, I get:
1. In a combobox (select @size="1":
a) proper speaking of the actual element. It's the first item that gets focus.
b) Focus on an empty item that's called "2 of 2", but which has no text.
c) Proper announcement of the outer item, the one that's not inside an optgroup.
2. In a list box select @size > 1:
a) Announcement that there's a header grouping, and the inner actual item spoken.
b) A second empty item which NVDA announces as "list item" and which has a group position of 2 of 2.
c) The outer element as usual.

So, in the optgroups, the actual header is only spoken in the list box, not the combo box. There is also an empty list item in either one.
Comment 33 Trevor Saunders (:tbsaunde) 2012-12-04 20:56:25 PST
> So, in the optgroups, the actual header is only spoken in the list box, not

I don't have ideas here, Jamie?

> the combo box. There is also an empty list item in either one.

what markup are you testing with?  I'm pretty sure this passes tests thatthings like

<select>
<optgroup>
<option>1</option>
<option>2</option>
</optgroup>
</select>

create the right number of option accessibles.
Comment 34 James Teh [:Jamie] 2012-12-05 00:19:08 PST
(In reply to Marco Zehe (:MarcoZ) from comment #32)
> So, in the optgroups, the actual header is only spoken in the list box, not
> the combo box.
I suspect you didn't expand the combo box for this bit. When collapsed, we only know that the value changed; there is no hierarchy. I think the same is true for sighted users. You won't get position info when it's collapsed either. If i expand the combo box, the header is spoken for me just like it is for size > 1.

> There is also an empty list item in either one.
I don't see this. I too am curious as to what markup you're using.

This build fixes the issues i mentioned earlier. However, I did notice something about position info which, while not a regression, seems strange to me now. A level >= 2 is reported for the inner items, but a level of 1 isn't reported for the outer items. I guess it'd be odd to report a level for a select without optgroups, though.
2. The
Comment 35 Trevor Saunders (:tbsaunde) 2012-12-06 21:16:15 PST
(In reply to James Teh [:Jamie] from comment #34)
> (In reply to Marco Zehe (:MarcoZ) from comment #32)
> > So, in the optgroups, the actual header is only spoken in the list box, not
> > the combo box.
> I suspect you didn't expand the combo box for this bit. When collapsed, we
> only know that the value changed; there is no hierarchy. I think the same is
> true for sighted users. You won't get position info when it's collapsed
> either. If i expand the combo box, the header is spoken for me just like it
> is for size > 1.
> 
> > There is also an empty list item in either one.
> I don't see this. I too am curious as to what markup you're using.

Marco, ping?

> This build fixes the issues i mentioned earlier. However, I did notice

ok, great

> something about position info which, while not a regression, seems strange
> to me now. A level >= 2 is reported for the inner items, but a level of 1
> isn't reported for the outer items. I guess it'd be odd to report a level
> for a select without optgroups, though.
> 2. The

yeah, I'd guess that the reason we don't report anything for options directly under a select is that we think select isn't a  grouping parent where we should report position.  I don't think it would be too terrible to always expose it or expose it  conditioned on there being an optgroup, but probably  best left to its own bug.
Comment 36 alexander :surkov 2012-12-06 21:30:10 PST
(In reply to Trevor Saunders (:tbsaunde) from comment #35)

> > something about position info which, while not a regression, seems strange
> > to me now. A level >= 2 is reported for the inner items, but a level of 1
> > isn't reported for the outer items. I guess it'd be odd to report a level
> > for a select without optgroups, though.
> > 2. The

if select is hierarchical (i.e. has optgroups) then we should expose level on each item, otherwise we shouldn't expose it. Agree?

> yeah, I'd guess that the reason we don't report anything for options
> directly under a select is that we think select isn't a  grouping parent
> where we should report position.  I don't think it would be too terrible to
> always expose it or expose it  conditioned on there being an optgroup, but
> probably  best left to its own bug.

if own bug then it should go immediately after this one (in the same Firefox release), we shouldn't practice a half baked solutions.
Comment 37 Trevor Saunders (:tbsaunde) 2012-12-06 22:23:08 PST
(In reply to alexander :surkov from comment #36)
> (In reply to Trevor Saunders (:tbsaunde) from comment #35)
> 
> > > something about position info which, while not a regression, seems strange
> > > to me now. A level >= 2 is reported for the inner items, but a level of 1
> > > isn't reported for the outer items. I guess it'd be odd to report a level
> > > for a select without optgroups, though.
> > > 2. The
> 
> if select is hierarchical (i.e. has optgroups) then we should expose level
> on each item, otherwise we shouldn't expose it. Agree?

exposing it on select with optgroup doesn't seem particularly bad, and it should be faster.

> > yeah, I'd guess that the reason we don't report anything for options
> > directly under a select is that we think select isn't a  grouping parent
> > where we should report position.  I don't think it would be too terrible to
> > always expose it or expose it  conditioned on there being an optgroup, but
> > probably  best left to its own bug.
> 
> if own bug then it should go immediately after this one (in the same Firefox
> release), we shouldn't practice a half baked solutions.

I don't particularly care much one way or another, it just sort of seems like a seperate issue to me, and I'd like to avoid this being the solve all the worlds problems bug :)
Comment 38 alexander :surkov 2012-12-06 22:38:35 PST
(In reply to Trevor Saunders (:tbsaunde) from comment #37)

> I don't particularly care much one way or another, it just sort of seems
> like a seperate issue to me, and I'd like to avoid this being the solve all
> the worlds problems bug :)

It can be separate from this bug but it's a part of the same work (make selects hierarchical). Don't afraid to being Atlant :) Users will appreciate results of your work.
Comment 39 Marco Zehe (:MarcoZ) 2012-12-07 01:17:07 PST
Created attachment 689615 [details]
My test case
Comment 40 James Teh [:Jamie] 2012-12-07 02:48:36 PST
(In reply to Marco Zehe (:MarcoZ) from comment #39)
> Created attachment 689615 [details]
> My test case
Your test case is broken. :) The correct usage for optgroup is:
<optgroup label="Header">
...
</optgroup>
Comment 41 Trevor Saunders (:tbsaunde) 2012-12-17 07:24:05 PST
(In reply to alexander :surkov from comment #38)
> (In reply to Trevor Saunders (:tbsaunde) from comment #37)
> 
> > I don't particularly care much one way or another, it just sort of seems
> > like a seperate issue to me, and I'd like to avoid this being the solve all
> > the worlds problems bug :)
> 
> It can be separate from this bug but it's a part of the same work (make
> selects hierarchical). Don't afraid to being Atlant :) Users will appreciate
> results of your work.

ideas on how can do it without being slow?

is there other issues we need to deal with here before we consider landing this?
Comment 42 alexander :surkov 2012-12-17 18:13:37 PST
1) We need to figure out a missed features that make users sad and schedule them into the same release this bug goes into. This doesn't necessary mean that you should fix all of them on your own but we should be on the same page about this.
2) We need to know which screen readers need a compatibility mode. For that we need to have a link to a try build and have a list of missed features so that we can send a link to this bug to AT developers asking them for testing.
Comment 43 Trevor Saunders (:tbsaunde) 2013-03-04 13:54:11 PST
Marco could you try the build for https://tbpl.mozilla.org/?tree=Try&rev=9ba4c599751b with various screen readers to see if any of them have problems when we do opt groups this way?
Comment 44 Marco Zehe (:MarcoZ) 2013-03-05 05:41:54 PST
NVDA:
1. Combobox (select @size=1), options inside and outside of optgroup read when focus changes, header not announced.
2. Listbox (select size greater than 1): options inside and outside read when focus changes, header announced when entering optgroup.

2. JAWS
Header when entering optgroup read in both cases, focus changes announced properly.

3. Window-Eyes
Headers never announced, items read at focus change as if header wasn't there.
Comment 45 Trevor Saunders (:tbsaunde) 2013-03-05 08:49:01 PST
ok, thanks!


(In reply to Marco Zehe (:MarcoZ) from comment #44)
> NVDA:
> 1. Combobox (select @size=1), options inside and outside of optgroup read
> when focus changes, header not announced.

did that used to happen before?

> 3. Window-Eyes
> Headers never announced, items read at focus change as if header wasn't
> there.

same?

So it looks like not flat tree is fine though changing role of opt group accessible from heading to group maybe somewhat problematic.
Comment 46 Marco Zehe (:MarcoZ) 2013-03-05 08:51:50 PST
Neither announced the optgroup heading before. I kinda was expecting for NVDA to pick this up now, but with a collapsed combobox, things seem to always be a bit different anyway. So while NVDA and Window-Eyes don't regress, JAWS even gets a bit of a new feature in that it announces the opgroup heading now.
Comment 47 Trevor Saunders (:tbsaunde) 2013-03-10 16:33:57 PDT
(In reply to alexander :surkov from comment #6)
> (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> > Yes I know
> > CacheOptSiblings can just be inlined now ;)
> 
> I'd guess we can use accTreeWalker to create a tree.

it seems select gets some xbl stuff so if we just let normal CacheChildren() do its thing then combobox list accessible also has text leaf child with value like "value:  I sort of suspect we don't wnat to expose that.option0" and a html button.
Comment 48 Trevor Saunders (:tbsaunde) 2013-03-11 19:51:22 PDT
Created attachment 723778 [details] [diff] [review]
bug 809338 - don't flatten optgroups

I kept the CacheChildren() stuff to avoid getting accessibles for the xbl stuff.
Comment 49 alexander :surkov 2013-03-11 22:24:41 PDT
Comment on attachment 723778 [details] [diff] [review]
bug 809338 - don't flatten optgroups

Review of attachment 723778 [details] [diff] [review]:
-----------------------------------------------------------------

interesting comment in bug 370163 comment #10:
"Oh shoot, that's why we have to keep the list flat -- the selection interface requires child offsets."

::: accessible/src/generic/Accessible.h
@@ +489,5 @@
>  
>    bool IsHTMLTable() const { return mType == eHTMLTableType; }
>    bool IsHTMLTableRow() const { return mType == eHTMLTableRowType; }
>  
> +  inline bool IsHTMLOptGroup() const { return mType == eHTMLOptGroupType; }

you don't need inline

@@ +893,5 @@
>    int32_t mIndexInParent;
>  
>    static const uint8_t kChildrenFlagsBits = 2;
>    static const uint8_t kStateFlagsBits = 5;
> +  static const uint8_t kTypeBits = 6;

I think you didn't update your sources for a while

::: accessible/src/html/HTMLSelectAccessible.cpp
@@ -156,1 @@
>      }

you wanted to inline it iirc

@@ +395,5 @@
> +    nsRefPtr<Accessible> accessible =
> +      GetAccService()->GetOrCreateAccessible(childContent, mDoc);
> +    if (accessible)
> +      AppendChild(accessible);
> +  }

this one can be processed by TreeWalker, correct?

::: accessible/src/html/HTMLSelectAccessible.h
@@ +125,5 @@
>    Accessible* GetCombobox() const
>    {
> +  Accessible* parent = mParent;
> +  while (parent && parent->IsHTMLOptGroup())
> +    parent = parent->Parent();

nit: wrong indent

you don't really need to use a cycle here because optgroup can't be nested

@@ +145,5 @@
>  public:
>  
> +  HTMLSelectOptGroupAccessible(nsIContent* aContent, DocAccessible* aDoc) :
> +    HTMLSelectOptionAccessible(aContent, aDoc)
> +  { mType = eHTMLOptGroupType; }

nit: extra two spaces indent for { }
Comment 50 Trevor Saunders (:tbsaunde) 2013-03-13 09:11:45 PDT
> @@ +893,5 @@
> >    int32_t mIndexInParent;
> >  
> >    static const uint8_t kChildrenFlagsBits = 2;
> >    static const uint8_t kStateFlagsBits = 5;
> > +  static const uint8_t kTypeBits = 6;
> 
> I think you didn't update your sources for a while

true

> ::: accessible/src/html/HTMLSelectAccessible.cpp
> @@ -156,1 @@
> >      }
> 
> you wanted to inline it iirc

not sure what your refering to?  The constructor? sure I guess I can do that.

> @@ +395,5 @@
> > +    nsRefPtr<Accessible> accessible =
> > +      GetAccService()->GetOrCreateAccessible(childContent, mDoc);
> > +    if (accessible)
> > +      AppendChild(accessible);
> > +  }
> 
> this one can be processed by TreeWalker, correct?

you mean children of optgroup? that depends how much you mind have a text node with the label attribute's value.
Comment 51 alexander :surkov 2013-03-13 09:22:46 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #50)

> > ::: accessible/src/html/HTMLSelectAccessible.cpp
> > @@ -156,1 @@
> > >      }
> > 
> > you wanted to inline it iirc
> 
> not sure what your refering to?  The constructor? sure I guess I can do that.

it was CacheOptSiblings actually

> > this one can be processed by TreeWalker, correct?
> 
> you mean children of optgroup? that depends how much you mind have a text
> node with the label attribute's value.

Yeah. It grows from native anonymous content? This is related with bug 378612 if it's still valid after this bug.
Comment 52 Trevor Saunders (:tbsaunde) 2013-03-13 09:42:27 PDT
(In reply to alexander :surkov from comment #51)
> (In reply to Trevor Saunders (:tbsaunde) from comment #50)
> 
> > > ::: accessible/src/html/HTMLSelectAccessible.cpp
> > > @@ -156,1 @@
> > > >      }
> > > 
> > > you wanted to inline it iirc
> > 
> > not sure what your refering to?  The constructor? sure I guess I can do that.
> 
> it was CacheOptSiblings actually
> 
> > > this one can be processed by TreeWalker, correct?
> > 
> > you mean children of optgroup? that depends how much you mind have a text
> > node with the label attribute's value.
> 
> Yeah. It grows from native anonymous content? This is related with bug
> 378612 if it's still valid after this bug.

I did check, but that's what I'd assume.
Comment 53 alexander :surkov 2013-03-13 20:23:25 PDT
So basically I have two issue as far:
1) comment #49 (atk selection interface)
2) backward compatibility (what versions of what AT gets broken by new hierarchy)
Comment 54 Trevor Saunders (:tbsaunde) 2013-03-13 20:30:13 PDT
(In reply to alexander :surkov from comment #53)
> So basically I have two issue as far:
> 1) comment #49 (atk selection interface)

well, so isn't that already pretty busted (xul trees and aria)

> 2) backward compatibility (what versions of what AT gets broken by new
> hierarchy)

So, its worked fine with everything Marco's tried  but I guess if your really worried older version will break we could ask him to test those.
Comment 55 alexander :surkov 2013-03-13 20:47:16 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #54)
> (In reply to alexander :surkov from comment #53)
> > So basically I have two issue as far:
> > 1) comment #49 (atk selection interface)
> 
> well, so isn't that already pretty busted (xul trees and aria)

xul trees are flat, ARIA trees can be flat as well. Also HTML select might be handled differently from trees and in either case HTML select is more popular than ARIA in the web. Did you/Marco tried ATK AT's?

Also it'd be good to get feedback from Orca developers.

> > 2) backward compatibility (what versions of what AT gets broken by new
> > hierarchy)
> 
> So, its worked fine with everything Marco's tried  but I guess if your
> really worried older version will break we could ask him to test those.

I'd be good to check previous versions of JAWS and WE.
Comment 56 Trevor Saunders (:tbsaunde) 2013-03-14 17:21:36 PDT
(In reply to alexander :surkov from comment #55)
> (In reply to Trevor Saunders (:tbsaunde) from comment #54)
> > (In reply to alexander :surkov from comment #53)
> > > So basically I have two issue as far:
> > > 1) comment #49 (atk selection interface)
> > 
> > well, so isn't that already pretty busted (xul trees and aria)
> 
> xul trees are flat, ARIA trees can be flat as well. Also HTML select might
> be handled differently from trees and in either case HTML select is more

I suppose, what do you want to do about it?

> popular than ARIA in the web. Did you/Marco tried ATK AT's?

I tried orca, it seemed fine.

> Also it'd be good to get feedback from Orca developers.

cc += Joanie then

> > > 2) backward compatibility (what versions of what AT gets broken by new
> > > hierarchy)
> > 
> > So, its worked fine with everything Marco's tried  but I guess if your
> > really worried older version will break we could ask him to test those.
> 
> I'd be good to check previous versions of JAWS and WE.

Marco can you check this?
Comment 57 alexander :surkov 2013-03-14 19:40:44 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #56)
> (In reply to alexander :surkov from comment #55)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #54)
> > > (In reply to alexander :surkov from comment #53)
> > > > So basically I have two issue as far:
> > > > 1) comment #49 (atk selection interface)
> > > 
> > > well, so isn't that already pretty busted (xul trees and aria)
> > 
> > xul trees are flat, ARIA trees can be flat as well. Also HTML select might
> > be handled differently from trees and in either case HTML select is more
> 
> I suppose, what do you want to do about it?

either keep hierarchy flat on linux or workaround the selection interface issue like we did similar thing for tables after we introduced rows in hierarchy (we provided table-cell-index object attribute which was supposed to be used instead indexInParent).
Comment 58 alexander :surkov 2013-04-05 00:21:02 PDT
I'm curious if there are other Orca developers we can reach if Joanie is busy these days?
Comment 59 Joanmarie Diggs 2013-04-05 06:21:35 PDT
Sorry Alex. Don't let me block you on this. Just do what you think needs doing and I'll then deal with it on the Orca side.
Comment 60 Trevor Saunders (:tbsaunde) 2013-04-08 14:29:11 PDT
(In reply to alexander :surkov from comment #58)
> I'm curious if there are other Orca developers we can reach if Joanie is
> busy these days?

I talked to Joanie on friday on IRC, and I want to test with orca again but I think the out come of that was that if that's fine she's happy and we'll sort out the atk spec later.  So I filed https://bugzilla.gnome.org/post_bug.cgi to sort out atkselection.
Comment 61 alexander :surkov 2013-04-08 19:11:47 PDT
Ok, cool. Then could you update the patch to trunk and address comments pls? (btw you need to remove the tree update logic for selects)

Marco, btw, did you try old proprietary screen readers?
Comment 62 Trevor Saunders (:tbsaunde) 2013-04-09 14:40:35 PDT
Alex, should we create text leaf child of optgroup when it has label=""?
Comment 63 alexander :surkov 2013-04-09 18:50:17 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #62)
> Alex, should we create text leaf child of optgroup when it has label=""?

we need to expose text interface on it, see bug 378612 (however exposing a text leaf child to AT might be confusing, they are quite tolerant usually though). I think we can do either way.
Comment 64 Trevor Saunders (:tbsaunde) 2013-04-10 23:08:31 PDT
Created attachment 736146 [details] [diff] [review]
bug 809338 - don't flatten optgroups
Comment 65 alexander :surkov 2013-04-11 05:24:39 PDT
Marco, your approval for previous screen reader versions.
Comment 66 Marco Zehe (:MarcoZ) 2013-04-11 08:38:36 PDT
Yes, should be fine. I tested with both WE 7.5 and 8, and to the best of my capabilities given that JAWS multiple versions is a mess for proper clean testing, JAWS 11, 13, and 14.
Comment 67 alexander :surkov 2013-04-14 22:03:52 PDT
Comment on attachment 736146 [details] [diff] [review]
bug 809338 - don't flatten optgroups

Review of attachment 736146 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/base/AccTypes.h
@@ +32,5 @@
>    eHTMLLabelType,
>    eHTMLLiType,
>    eHTMLSelectListType,
>    eHTMLMediaType,
> +  eHTMLOptGroupType,

you should move it down into the second group, these constants are used in one switch

::: accessible/src/generic/Accessible.h
@@ +490,5 @@
>  
>    bool IsHTMLTable() const { return mType == eHTMLTableType; }
>    bool IsHTMLTableRow() const { return mType == eHTMLTableRowType; }
>  
> +  inline bool IsHTMLOptGroup() const { return mType == eHTMLOptGroupType; }

no inline needed
btw, keep it in alphabetical order (move it above IsHTMLTable)

::: accessible/src/html/HTMLSelectAccessible.cpp
@@ +132,5 @@
>      }
>  
>      nsIAtom* tag = childContent->Tag();
>      if (tag == nsGkAtoms::option ||
>          tag == nsGkAtoms::optgroup) {

what else can we pick up it here other than option and optgroup? or why don't you use here AccTreeWalker (I'm pretty sure we talked about this but I don't recall the summary)

::: accessible/tests/mochitest/focus/test_takeFocus.html
@@ +56,5 @@
>        gQueue.push(new takeFocusInvoker("item2"));
>        gQueue.push(new takeFocusInvoker("plugin"));
>        gQueue.push(new takeFocusInvoker(document));
>        gQueue.push(new takeFocusInvoker("lb_item2"));
> +      gQueue.push(new takeFocusInvoker(document));

what is idea of focusing the document between options?
Comment 68 Trevor Saunders (:tbsaunde) 2013-04-15 09:54:04 PDT
> ::: accessible/src/html/HTMLSelectAccessible.cpp
> @@ +132,5 @@
> >      }
> >  
> >      nsIAtom* tag = childContent->Tag();
> >      if (tag == nsGkAtoms::option ||
> >          tag == nsGkAtoms::optgroup) {
> 
> what else can we pick up it here other than option and optgroup? or why
> don't you use here AccTreeWalker (I'm pretty sure we talked about this but I
> don't recall the summary)

there's some anon content generated 9a button and maybe something else).  I'm pretty sure discussion is in this bug :)

> ::: accessible/tests/mochitest/focus/test_takeFocus.html
> @@ +56,5 @@
> >        gQueue.push(new takeFocusInvoker("item2"));
> >        gQueue.push(new takeFocusInvoker("plugin"));
> >        gQueue.push(new takeFocusInvoker(document));
> >        gQueue.push(new takeFocusInvoker("lb_item2"));
> > +      gQueue.push(new takeFocusInvoker(document));
> 
> what is idea of focusing the document between options?

I think it may have been a debug thing otherwise not sure
Comment 69 alexander :surkov 2013-04-15 22:08:45 PDT
Comment on attachment 736146 [details] [diff] [review]
bug 809338 - don't flatten optgroups

Review of attachment 736146 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments fixed/addressed
Comment 70 Trevor Saunders (:tbsaunde) 2013-04-16 22:34:10 PDT
(In reply to alexander :surkov from comment #69)
> Comment on attachment 736146 [details] [diff] [review]
> bug 809338 - don't flatten optgroups
> 
> Review of attachment 736146 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comments fixed/addressed

should I remove the document focusing thing in the test?
Comment 71 alexander :surkov 2013-04-17 00:42:17 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #70)

> > r=me with comments fixed/addressed
> 
> should I remove the document focusing thing in the test?

yes please
Comment 72 Trevor Saunders (:tbsaunde) 2013-04-22 23:02:56 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/7da9a210f365
and because I was trying without the document focusing stuff fix up
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fe2fb8ee8780

Note You need to log in before you can comment on or make changes to this bug.