Closed Bug 809338 Opened 12 years ago Closed 11 years ago

make HTML select hierarchical

Categories

(Core :: Disability Access APIs, defect)

Product:
Core
Component:
Disability Access APIs
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla23

People

(Reporter: surkov, Assigned: tbsaunde)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(4 files, 1 obsolete file)

wip
10.48 KB, patch
Details | Diff | Splinter Review
wip
11.31 KB, patch
Details | Diff | Splinter Review
My test case
489 bytes, text/html
Details
bug 809338 - don't flatten optgroups
14.74 KB, patch
surkov
: review+
Details | Diff | Splinter Review 
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?
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.
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
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.
I see.

Trev, would you like to sketch it up?
Attached patch wip — — Splinter Review 
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?
(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.
(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()
(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.
(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.
(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.
(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.
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?
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.
Marco, what about other screen readers?

(to be on safe side) did you tried both select (combobox) and select@size>1 (listbox) having optgroups?
(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.
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.
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.
(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
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?
(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
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.
Flags: needinfo?(jamie)
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.
(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.
(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
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.
Flags: needinfo?(jamie)
(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.
(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.
Attached patch wip — — Splinter Review 
focus should work with options in optgroups this time, someone on windows want to try it?
https://tbpl.mozilla.org/?tree=Try&rev=9c7a3ee171d9
(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
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.
> 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.
(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
(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.
(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.
(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 :)
(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.
Attached file My test case — 

    
    

    
  
(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>

    
    

    
  
(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?

    
    

    
  
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.

    
    

    
  
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?

    
  
Flags: needinfo?(marco.zehe)

    
    

    
  
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.
Flags: needinfo?(marco.zehe)

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
(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.

    
    

    
  


  
I kept the CacheChildren() stuff to avoid getting accessibles for the xbl stuff.

        Attachment #723778 -
        Flags: review?(surkov.alexander)

    
    

    
  
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 { }

    
  
Assignee: nobody → trev.saunders

    
    

    
  
> @@ +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.

    
    

    
  
(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.

    
    

    
  
(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.

    
    

    
  
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)

    
    

    
  
(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.

    
    

    
  
(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.

    
    

    
  
(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?

    
  
Flags: needinfo?(jdiggs)

    
    

    
  
(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).

    
    

    
  
I'm curious if there are other Orca developers we can reach if Joanie is busy these days?

    
    

    
  
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.
Flags: needinfo?(jdiggs)

    
    

    
  
(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.

    
    

    
  
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?

    
    

    
  
Alex, should we create text leaf child of optgroup when it has label=""?

    
    

    
  
(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.

    
    

    
  


  

        Attachment #736146 -
        Flags: review?(surkov.alexander)

    
  

        Attachment #723778 -
        Attachment is obsolete: true

        Attachment #723778 -
        Flags: review?(surkov.alexander)

    
    

    
  
Marco, your approval for previous screen reader versions.
Flags: needinfo?(marco.zehe)

    
    

    
  
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.
Flags: needinfo?(marco.zehe)

    
    

    
  
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?

    
    

    
  
> ::: 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 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

        Attachment #736146 -
        Flags: review?(surkov.alexander) → review+

    
    

    
  
(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?

    
    

    
  
(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

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/7da9a210f365
https://hg.mozilla.org/mozilla-central/rev/fe2fb8ee8780
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: