357969 - container xul element which doesn't have a xbl def under a deck frame has no accessible object

Status: RESOLVED FIXED

(Firefox :: Disability Access, defect)

Description

[Nian Liu(n/a in a long time)]

tabpanel, hbox/vbox has no accessible object 
1.use at-poke to poke edit->preferences->advanced or tools->page info
2.all contents belonging to different tab shows under tabbox

solution can be 1)buidl accessible object for container xul element or
2)only expose the current tab contents

i prefer solution 2

Comment 1

[Aaron Leventhal]

vbox/hbox have no semantic value, so don't need to be exposed.

tabpanel should be exposed w/ nsIAccessible::ROLE_PROPERTYPAGE. I guess you'll need to create an XBL definition for it just so you can expose the accessible type.

So for MSAA it will be exposed as ROLE_SYSTEM_PROPERTYPAGE, for ATK it will become ATK_ROLE_SCROLL_PANE and for UA it will become NSAccessibilityGroupRole.

Comment 2

[Nian Liu(n/a in a long time)]

(In reply to comment #1)
> vbox/hbox have no semantic value, so don't need to be exposed.
> 
vbox/hbox has semantic value when it's used in tabpanels. tabpanel is nothing less/more than a vbox/hbox
> tabpanel should be exposed w/ nsIAccessible::ROLE_PROPERTYPAGE. I guess you'll
> need to create an XBL definition for it just so you can expose the accessible
> type.
> 
> So for MSAA it will be exposed as ROLE_SYSTEM_PROPERTYPAGE, for ATK it will
> become ATK_ROLE_SCROLL_PANE and for UA it will become NSAccessibilityGroupRole.
> 

Comment 3

[Nian Liu(n/a in a long time)]

Attachment: patch

solution 1

Comment 4

[Nian Liu(n/a in a long time)]

seems children includes="" doesn't work. if necessary, i'll file a bug to clear tabpanels with hbox/vbox issue as in page info.

Comment 5

[Aaron Leventhal]

(In reply to comment #2)
> (In reply to comment #1)
> vbox/hbox has semantic value when it's used in tabpanels. tabpanel is nothing
> less/more than a vbox/hbox
It currently is nothing more, but you can make it more.
If <tabpanel> is the element, the CSS in xul.css should point to a binding that creates an accessibility object via nsIAccessibleProvider.
Not the case for vbox or hbox.

If something in a .xul file is using <hbox> or <vbox> when they should say <tabpanel>, those need to be changed.

Comment 6

[Asaf Romano (gone)]

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

a new tab panel could be added way after the tabpanels elements is constructed, we better find a way to go back from the tabpanel to its corresponding tab.

With that said, what's the additional value here? XBL is expensive...

Comment 7

[Aaron Leventhal]

Nian, why the label property?

Comment 8

[Nian Liu(n/a in a long time)]

> 
> If something in a .xul file is using <hbox> or <vbox> when they should say
> <tabpanel>, those need to be changed.
> 
that's what i mean^_^

Comment 9

[Nian Liu(n/a in a long time)]

> 
> If something in a .xul file is using <hbox> or <vbox> when they should say
> <tabpanel>, those need to be changed.
> 
that's what i mean^_^

also label property is easy for at-poke to get the tabpanel's corresponding tab

Comment 10

[Nian Liu(n/a in a long time)]

> tabpanel should be exposed w/ nsIAccessible::ROLE_PROPERTYPAGE. I guess you'll
> need to create an XBL definition for it just so you can expose the accessible
> type.
currentlly tabpanels' role is ROLE_PROPERTYPAGE. i'll change it to ROLE_PANE and make tabpanel's role to ROLE_PROPERTYPAGE
> 
> So for MSAA it will be exposed as ROLE_SYSTEM_PROPERTYPAGE, for ATK it will
> become ATK_ROLE_SCROLL_PANE and for UA it will become NSAccessibilityGroupRole.
> 

Comment 11

[Aaron Leventhal]

Mano, we need it so that <tabpanel> can be exposed as ROLE_TABPANEL. Assistive technologies look at our accessibility object hierarchy and expect us to use the roles in the spec. We just need to follow the spec to avoid problems. Is it that much overhead? Tabpanel is only used in dialogs, so it's not like this will affect page load times. Once the dialog is closed the memory should be freed, correct? How much overhead is it, really?

Nian, you haven't answered comment 7 yet, have you?
Also, I don't understand your comment 10 and whether you still plan to do something for any <hbox> or <vbox> in our codebase. Can you be explicit on what you plan to do?

Comment 12

[Nian Liu(n/a in a long time)]

> Nian, you haven't answered comment 7 yet, have you?
yep, in comment 9. i try to build a relationship between tab and tabpanel with label property. or else, we don't have a relationship between them. any advice?
> Also, I don't understand your comment 10 and whether you still plan to do
> something for any <hbox> or <vbox> in our codebase. Can you be explicit on what
> you plan to do?
> 
let me describe it in another way. nsXULTabPanelsAccessible::GetRole returns ROLE_PROPERTYPAGE. i don't think that's correct. TabPanels should be just Role_Pane and TabPanel should be ROLE_PROPERTYPAGE. I'll change it if you agree with me.

for vbox/hbox, as i stated in comment 4, i plan to file another bug to remove all the vbox/hbox in tabpanels after this bug is fixed.

Comment 13

[Aaron Leventhal]

(In reply to comment #12)
> > Nian, you haven't answered comment 7 yet, have you?
> yep, in comment 9. i try to build a relationship between tab and tabpanel with
> label property. or else, we don't have a relationship between them. any advice?
Yes, use aaa:labelledby on the tabpanel and have it point to the tab. This will automatically do the right thing in terms of relationships etc.

> let me describe it in another way. nsXULTabPanelsAccessible::GetRole returns
> ROLE_PROPERTYPAGE. i don't think that's correct. TabPanels should be just
> Role_Pane and TabPanel should be ROLE_PROPERTYPAGE. I'll change it if you 
> agree with me.
Yes, agreed.

> for vbox/hbox, as i stated in comment 4, i plan to file another bug to remove
> all the vbox/hbox in tabpanels after this bug is fixed.
I'm still not clear on this, but we can work on that in the other bug. I think you mean there are some places where we use <vbox> or <hbox> but we should be using a more semantic element like <tabpanel> or <tabpanels>. 

Comment 14

[Nian Liu(n/a in a long time)]

> > for vbox/hbox, as i stated in comment 4, i plan to file another bug to remove
> > all the vbox/hbox in tabpanels after this bug is fixed.
> I'm still not clear on this, but we can work on that in the other bug. I think
> you mean there are some places where we use <vbox> or <hbox> but we should be
> using a more semantic element like <tabpanel> or <tabpanels>. 
> 
could be no more right

Comment 15

[Nian Liu(n/a in a long time)]

(In reply to comment #14)
> 
> > > for vbox/hbox, as i stated in comment 4, i plan to file another bug to remove
> > > all the vbox/hbox in tabpanels after this bug is fixed.
> > I'm still not clear on this, but we can work on that in the other bug. I think
> > you mean there are some places where we use <vbox> or <hbox> but we should be
> > using a more semantic element like <tabpanel> or <tabpanels>. 
> > 
> could be no more right
oops. should be "could not be no more right"
> 

Comment 16

[Nian Liu(n/a in a long time)]

Attachment: patch

1)patch also fixed bug 356415
2)we don't have support for aaa:labelledby in native code side, right?

Comment 17

[Ginn Chen]

*** Bug 356415 has been marked as a duplicate of this bug. ***

Comment 18

[Aaron Leventhal]

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

1. Please remove the |dump| statements in XBL
2. I suggest you put your labelledby support in a <constructor> section instead
3. I suggest not using single letter variable names like |i|. It's too hard to do find/replace on those, and it's not very descriptive. Perhaps something like panelIndex.
4. Use temporary variables if it will make the code look simpler. For example, this is not very readable |(this.parentNode.parentNode.childNodes[0].childNodes[index].id| and you repeat it twice. Some comments might help as well.


What do you mean by this:
> we don't have support for aaa:labelledby in native code side, right?
We support labelledby in nsAccessible::GetAccessibleRelated()

Comment 19

[Nian Liu(n/a in a long time)]

(In reply to comment #18)
> (From update of attachment 244785 [details] [diff] [review] [edit])
> 1. Please remove the |dump| statements in XBL
sorry, my fault
> 2. I suggest you put your labelledby support in a <constructor> section instead
i can't find a way to do an equal judgement in constructor since the object creation isn't done. also use length of tabpanels doesn't work since it's already set to the number of all panels. any suggestions?

> 3. I suggest not using single letter variable names like |i|. It's too hard to
> do find/replace on those, and it's not very descriptive. Perhaps something like
> panelIndex.

not sure what you mean. i just use i as a tmp loop variable. 
> 4. Use temporary variables if it will make the code look simpler. For example,
> this is not very readable
> |(this.parentNode.parentNode.childNodes[0].childNodes[index].id| and you repeat
> it twice. Some comments might help as well.
> 
ok.
> 
> What do you mean by this:
> > we don't have support for aaa:labelledby in native code side, right?
> We support labelledby in nsAccessible::GetAccessibleRelated()
> 

i don't think we support aaa:labelledby. we only support kNameSpaceID_None::labelledby. also not work for GetHTML/XULName since those use kNameSpaceID_WAIProperties::labelledby.

Comment 20

[Aaron Leventhal]

(In reply to comment #19)
> > 2. I suggest you put your labelledby support in a <constructor> section instead
> i can't find a way to do an equal judgement in constructor since the object
> creation isn't done. also use length of tabpanels doesn't work since it's
> already set to the number of all panels. any suggestions?
I'll leave that to Mano when he reviews.

> > 3. I suggest not using single letter variable names like |i|. It's too hard to
> > do find/replace on those, and it's not very descriptive. Perhaps something like
> > panelIndex.
> 
> not sure what you mean. i just use i as a tmp loop variable. 
Instead of |i| use |index| or |count|. It's easier to edit the code later, in my experience, when you search for the variable, etc.

> > 4. Use temporary variables if it will make the code look simpler. For example,
> > this is not very readable
> > |(this.parentNode.parentNode.childNodes[0].childNodes[index].id| and you repeat
May I suggest?
var tabBar = this.parentNode;
var numTabs = tabBar.childNodes.length;
for (var tabIndex = 0; tabIndex < numTabs tabIndex++) {
  if (this == tabBar.childNodes[tabIndex]) {
    var tabLabel = tabBar.parentNode.childNodes[0].childNodes[tabIndex];
    if (tabLabel.id) {
      this.setAttributeNS("http://www.w3.org/2005/07/aaa", "labelledby", tabLabel.id);
    }
    break;
  }
}

> i don't think we support aaa:labelledby. we only support
> kNameSpaceID_None::labelledby. also not work for GetHTML/XULName since those
> use kNameSpaceID_WAIProperties::labelledby.

Okay, here's how it works.
kNameSpaceID_WAIProperties is the constant used in C++ for nsIContent::HasAttr and nsIContent::GetAttr etc.
If you look at the namespace manager code, you can see that constant being associated with "http://www.w3.org/2005/07/aaa"
In XHTML or XUL, when you want to use that namespace, you must use an xmlns statement, and then you can say aaa:labelledby etc. See http://lxr.mozilla.org/seamonkey/source/browser/components/preferences/security.xul#88
Then XHTML or XUL can refer to it.
Or, if you want to use it via a DOM call you need setAttributeNS/hasAttributeNS/getAttributeNS and pass in "http://www.w3.org/2005/07/aaa"
But they all refer to the same namespace.

Comment 21

[Nian Liu(n/a in a long time)]

Attachment: patch

Comment 22

[Aaron Leventhal]

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

The a11y part passes my review, but I would like labelledby to be set before the accessibleType is asked for. After all, an extension like Fire Vox which doesn't use accessible objects (relies on the DOM) may want that attribute as well.

Passing to Mano for the final polish.

Comment 23

[Asaf Romano (gone)]

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

No, I don't think this is right. We should instead expose a property (I'm not sure via which interface) which would return the tab element (not its id). And then 1) We can keep labelledby as a way to override the accessible name 2) The DOM isn't modified.

Comment 24

[[e-mail]]

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

>+tabpanel {
>+  -moz-binding: url("chrome://global/content/bindings/tabbox.xml#tabpanel");
>+}
It might be worth a scan of the codebase to see whether we historically are more likely to have used <vbox> or <hbox> - I'm guessing <vbox> in which case -moz-box-orient: vertical; would be a useful default. Or would it be better to attach the binding to tabpanels > hbox and tabpanels > vbox instead or as well?

>+    <content>
>+      <children includes="tabpanel"/>
>+    </content>
The bug summary says "container element which doesn't have a[n] xbl def" - what about container elements that do have an xbl def? Surely they should still be allowed?

>-[scriptable, uuid(1a57d854-12df-4b7b-8552-a3cd1fa90618)]
>+[scriptable, uuid(bc203f7d-e6da-4f6e-98ac-69ae9442e790)]
Do you have to do this when adding constants?

Comment 25

[Nian Liu(n/a in a long time)]

(In reply to comment #24)
> (From update of attachment 247049 [details] [diff] [review] [edit])
> >+tabpanel {
> >+  -moz-binding: url("chrome://global/content/bindings/tabbox.xml#tabpanel");
> >+}
> It might be worth a scan of the codebase to see whether we historically are
> more likely to have used <vbox> or <hbox> - I'm guessing <vbox> in which case
> -moz-box-orient: vertical; would be a useful default. Or would it be better to
> attach the binding to tabpanels > hbox and tabpanels > vbox instead or as well?
vbox/hbox doesn't have a xbl def. so it's impossible for a11y to build the corresponding a11y structure for it.
> 
> >+    <content>
> >+      <children includes="tabpanel"/>
> >+    </content>
> The bug summary says "container element which doesn't have a[n] xbl def" - what
> about container elements that do have an xbl def? Surely they should still be
> allowed?
> 
just want to make only tabpanel valid under tabpanels, not vbox/hbox. hence, make it more accessible.
> >-[scriptable, uuid(1a57d854-12df-4b7b-8552-a3cd1fa90618)]
> >+[scriptable, uuid(bc203f7d-e6da-4f6e-98ac-69ae9442e790)]
> Do you have to do this when adding constants?
> 
what's the general rule? i'd thought uuid need to be changed once idl content changes.

Comment 26

[Aaron Leventhal]

CC'ing Neil -- notsure if he would otherwise see your last comment.

Comment 27

[[e-mail]]

(In reply to comment #25)
>vbox/hbox doesn't have a xbl def. so it's impossible for a11y to build the
>corresponding a11y structure for it.
The point is, we can give tabpanels > vbox an xbl definition if necessary.

>just want to make only tabpanel valid under tabpanels, not vbox/hbox. hence,
>make it more accessible.
In that case you'd better update all our existing tabpanels first! There are only about 28 of them that don't use tabpanel. And you didn't answer the -moz-box-orient question either.

>what's the general rule? i'd thought uuid need to be changed once idl content
>changes.
I had thought it depended on binary compatibility, but don't quote me on that.

Comment 28

[Nian Liu(n/a in a long time)]

(In reply to comment #27)
> (In reply to comment #25)
> >vbox/hbox doesn't have a xbl def. so it's impossible for a11y to build the
> >corresponding a11y structure for it.
> The point is, we can give tabpanels > vbox an xbl definition if necessary.
> 
an example would be better. how can i set a xbl for tabpanels>vbox and at the same time implement nsIAccessibleProvider interface.

> >just want to make only tabpanel valid under tabpanels, not vbox/hbox. hence,
> >make it more accessible.
> In that case you'd better update all our existing tabpanels first! There are
> only about 28 of them that don't use tabpanel. And you didn't answer the
> -moz-box-orient question either.
it's on the plan to remove all vbox/hbox using under tabpanel. i agree vbox can be a default
> 
> >what's the general rule? i'd thought uuid need to be changed once idl content
> >changes.
> I had thought it depended on binary compatibility, but don't quote me on that.
> 

Comment 29

[Nian Liu(n/a in a long time)]

Attachment: patch that in a11y scope

this patch doesn't change dom structure as Mano suggested.

the decision to either change DOM structure to help other a11y app as FireVox or keep DOM unchanged is really out of my scope.

Aaron, Mano and Neil, pls. comment.

Comment 30

[[e-mail]]

(In reply to comment #28)
>an example would be better. how can i set a xbl for tabpanels>vbox and at the
>same time implement nsIAccessibleProvider interface.
+tabpanel, tabpanels > vbox, tabpanels > hbox {
+  -moz-binding: url("chrome://global/content/bindings/tabbox.xml#tabpanel");
+}
But I'm still interested in whether it's valid to have other accessible providers as children of the tabpanels element.

>it's on the plan to remove all vbox/hbox using under tabpanel.
>i agree vbox can be a default
Yes, most of our tabboxes currently have vbox children, although I also noticed a <browser>, a <grid>, a <groupbox> and a couple of <domi-panel>s.

Venkman uses some sort of custom tabpanel children, which I wasn't able to decipher, but I don't know how accessible Venkman has been anyway.

Comment 31

[[e-mail]]

Comment on attachment 247655 [details] [diff] [review]
patch that in a11y scope

>+    <content>
>+      <children includes="tabpanel"/>
>+    </content>
This doesn't do anything useful... if there are only tabpanels then the default behaviour would have appended them anyway and if there are non-tabpanels then it falls back to the default behaviour anyway.

Comment 32

[[e-mail]]

Comment on attachment 247655 [details] [diff] [review]
patch that in a11y scope

>+static already_AddRefed<nsIAccessible> GetCorrespondingAccessible(nsIAccessible* aAcc,
>+                                                                     PRInt32 aIndex);
>+
>+//aIndex == 0, get tab from tabpanel
>+//aIndex == 1, get tabpanel from tab
>+static already_AddRefed<nsIAccessible> GetCorrespondingAccessible(nsIAccessible* aAcc,
>+                                                                     PRInt32 aIndex)
>+{
>+  PRInt32 index;
>+  aAcc->GetIndexInParent(&index);
>+  nsIAccessible* tmp;
>+  aAcc->GetParent(&tmp); //tabpanels
>+  tmp->GetParent(&tmp); //tabbox
>+  tmp->GetChildAt(aIndex, &tmp);  //tabs
>+  tmp->GetChildAt(index, &tmp); //tab
>+  return tmp;
>+}
I can see two problems here. The first is that <tab>s have a "linkedpanel" attribute so that the panel number doesn't have to match up to the tab number. The second is that the <tabbrowser> uses a "linkedBrowser" property to link the tabs to the browsers. The only way it can link browsers to tabs is to enumerate the tabs and find the one with the relevant linked browser.

Comment 33

[Aaron Leventhal]

Comment on attachment 247655 [details] [diff] [review]
patch that in a11y scope

I'll hold off until Neil's comments are dealt with.

Comment 34

[Nian Liu(n/a in a long time)]

(
> I can see two problems here. The first is that <tab>s have a "linkedpanel"
> attribute so that the panel number doesn't have to match up to the tab number.
so how can i get the 1-1 mapping between tab and tabpanel? since linkedpanel can be used or not.
> The second is that the <tabbrowser> uses a "linkedBrowser" property to link the
> tabs to the browsers. The only way it can link browsers to tabs is to enumerate
> the tabs and find the one with the relevant linked browser.
> 
this is in tabbox scope. however seems not work with tabbox in tabbrowser. thanks for pointing this.

Comment 35

[Nian Liu(n/a in a long time)]

i can't find a way to build tab/tabpanel relationship when linkedPanel is used. anyone has an idea?

Comment 36

[[e-mail]]

(In reply to comment #35)
>I can't find a way to build tab/tabpanel relationship when linkedPanel is used.
>Anyone has an idea?
Are there accessibility attributes that authors could use in conjunction with linkedPanel to build the relationship?

Comment 37

[Nian Liu(n/a in a long time)]

Attachment: patch

what this patch does:
1.make all box under a deck has a corresponding accesible object to make the accessible tree more organizable.(prefpane, tabpanles are fixed here)
2.remove useless code in nsXULTabbox

what this patch does not:
1.build 1-1 relationship with tab and tab panel. it will be traced in another bug
2.wizard deck works uncorrectlly. reason is under investigating.

Comment 38

[Nian Liu(n/a in a long time)]

1-1 relationship is traced with bug 366527

Comment 39

[[e-mail]]

(In reply to comment #37)
>1.make all box under a deck has a corresponding accesible object to make the
>accessible tree more organizable.(prefpane, tabpanles are fixed here)
What happens if the box already has an accessible object? As I mentioned earlier, some of our tabs use <browser> <grid> or <groupbox> (note that I don't know which of these are accessible).

Comment 40

[Nian Liu(n/a in a long time)]

( As I mentioned
> earlier, some of our tabs use <browser> <grid> or <groupbox> (note that I don't
> know which of these are accessible).
> 
thanks for pointing this. i'll modified the implementation to assign a type to the node only if the node belonging to a nsBoxFrame which is directlly under a nsDeckFrame doesn't implement nsIAccessibleProvider interface. I think that will solve that. what do you think?

also, for the wizard implementation, it seems really wired. 
<content>
      <xul:hbox class="wizard-header" chromedir="&locale.dir;" anonid="Header"/>

      <xul:deck class="wizard-page-box" flex="1" anonid="Deck">
        <children includes="wizardpage"/>
      </xul:deck>
      <children/>

      <xul:hbox class="wizard-buttons" anonid="Buttons" xbl:inherits="pagestep,firstpage,lastpage"/>
    </content>

where the wizardpage is put in. under xul:deck or directlly under content. can you explain it for me?

Comment 41

[[e-mail]]

(In reply to comment #40)
>thanks for pointing this. i'll modified the implementation to assign a type to
>the node only if the node belonging to a nsBoxFrame which is directlly under a
>nsDeckFrame doesn't implement nsIAccessibleProvider interface. I think that
>will solve that. what do you think?
Sounds good to me.

>also, for the wizard implementation, it seems really wired. 
>     <content>
>       <xul:hbox class="wizard-header" chromedir="&locale.dir;" anonid="Header"/>
>       <xul:deck class="wizard-page-box" flex="1" anonid="Deck">
>         <children includes="wizardpage"/>
>       </xul:deck>
>       <children/>
>where the wizardpage is put in. under xul:deck or directlly under content. can
>you explain it for me?

OK, so from the point of view of the DOM:
<wizardpage>.parentNode = <wizard>
<wizard>.childNodes = [<wizardPage>, ...]

While from the point of view of the XBL service and frame layout
<wizardpage>.parentFrame = <wizardpage>.insertionPoint = <xul:deck>

To find the insertion point use
aContent->GetOwnerDoc()->BindingManager()->GetInsertionParent(aContent,
    getter_AddRefs(insertionParent));

Comment 42

[Nian Liu(n/a in a long time)]

> While from the point of view of the XBL service and frame layout
> <wizardpage>.parentFrame = <wizardpage>.insertionPoint = <xul:deck>
thanks for the explanation.

it sounds same with prefpane and tabpanels. it's strange that the code doesn't work for wizard then.

anyway, i'll investigate it.

Comment 43

[Nian Liu(n/a in a long time)]

Neil,

I can't find a reason why the code not work for wizard. wizardpage frame's parent is not the xul:deck. any hint?

Comment 44

[[e-mail]]

So, <wizardpage> has CSS overflow: auto; which introduces a scroll frame;
would that make a difference?

Comment 45

[Aaron Leventhal]

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

I want to review when the wizardpage issue is either resolved or separated into a different patch/bug.

Comment 46

[Nian Liu(n/a in a long time)]

(In reply to comment #44)
> So, <wizardpage> has CSS overflow: auto; which introduces a scroll frame;
> would that make a difference?
> 

bingo! one shoot kill!

is there a good reason that we use "overflow: auto" for wizardpage? i checked cvs blame and didn't find related bug.

Comment 47

[[e-mail]]

(In reply to comment #46)
>is there a good reason that we use "overflow: auto" for wizardpage?
Not sure... maybe it's so we can specify a fixed size for wizards.

Comment 48

[Nian Liu(n/a in a long time)]

Attachment: patch v2

resolve wizardpage issue too

Comment 49

[Nian Liu(n/a in a long time)]

Comment on attachment 252020 [details] [diff] [review]
patch v2

Neil, is there any other container type used under a deckframe besides boxframe and scrollframe?

Comment 50

[[e-mail]]

Comment on attachment 252020 [details] [diff] [review]
patch v2

Not that I know of.

Comment 51

[Aaron Leventhal]

Interesting. For HTML we associate a type of accessible with a type of frame directly via nsIFrame::GetAccessible(), rather than switching based on the frame type. Would that work?

I'm not exactly sure what the new code does in GetAccessibleByType does when !node. Some comments would help.

I hadn't previously noticed that we used a variable called |node| for accessible provider when we have an nsIDOMNode *aNode there. It's confusing. Can you call it something like, |nsCOMPtr<nsIAccessibleProvider> accProv;| ? Usually a variable called |node| would be an nsIDOMNode, nsINode or something like that.

Comment 52

[Aaron Leventhal]

Comment on attachment 252020 [details] [diff] [review]
patch v2

Need answers to previous questions before proceeding with review.

Comment 53

[Nian Liu(n/a in a long time)]

(In reply to comment #51)
> Interesting. For HTML we associate a type of accessible with a type of frame
> directly via nsIFrame::GetAccessible(), rather than switching based on the
> frame type. Would that work?
> 
nsIFrame::GetAccessible() is for html content, this patch is for container frame directlly under a deck frame(xul content)
> I'm not exactly sure what the new code does in GetAccessibleByType does when
> !node. Some comments would help.
> 
i'll add it. basiclly what it does is that
1)find out what kind of frame the xul content in
2)if a container frame as box frame/scroll frame, find out what its parent frame is
3)if parent frame's a deck frame, create an accessible object.

this patch will make prefpane, tabpanel and wizard more orginazable in accessible tree.
> I hadn't previously noticed that we used a variable called |node| for
> accessible provider when we have an nsIDOMNode *aNode there. It's confusing.
> Can you call it something like, |nsCOMPtr<nsIAccessibleProvider> accProv;| ?
> Usually a variable called |node| would be an nsIDOMNode, nsINode or something
> like that.
> 
i'll fix it.

Comment 54

[Aaron Leventhal]

> nsIFrame::GetAccessible() is for html content, this patch is for container
> frame directlly under a deck frame(xul content)
Yes, it's been for HTML so far. Would it make sense to implement nsIFrame::GetAccessible for the container XUL frames directly under a deck frame? Just a question -- I don't know myself because I haven't looked closely. But it's a bit unfortunate to start adding this special case code to such a nice general method as GetAccessibleByType(). Start with a nice clean method and then start adding all these special cases and it will start to grow and be harder to understand.

I don't want to make you work on this code forever, but it seems a little hacky right now :/

Another possibility is to override CacheChildren() on the parent accessible that can have these children, and have it look for those box/scroll children as it creates accessibles.

Comment 55

[Nian Liu(n/a in a long time)]

Attachment: patch v3

compared with last patch, 
1)fix wrong variable name "node"
2)move hack code to a more reasonable place

Comment 56

[Nian Liu(n/a in a long time)]

(In reply to comment #54)
> > nsIFrame::GetAccessible() is for html content, this patch is for container
> > frame directlly under a deck frame(xul content)
> Yes, it's been for HTML so far. Would it make sense to implement
> nsIFrame::GetAccessible for the container XUL frames directly under a deck
> frame? Just a question -- I don't know myself because I haven't looked closely.
i'll keep it clean as html usage
> But it's a bit unfortunate to start adding this special case code to such a
> nice general method as GetAccessibleByType(). Start with a nice clean method
> and then start adding all these special cases and it will start to grow and be
> harder to understand.
> 
> I don't want to make you work on this code forever, but it seems a little hacky
> right now :/
> 
> Another possibility is to override CacheChildren() on the parent accessible
> that can have these children, and have it look for those box/scroll children as
> it creates accessibles.
> 
i don't use that since parent may not have accessible too.(deckframe here is the frame of the parent)

Comment 57

[Aaron Leventhal]

Sorry, I don't think that's a better place -- it's worse. Makes a long method even longer. I would have preferred to put it on nsIFrame::GetAccessible() for deckframe. Why not that?

Comment 58

[Nian Liu(n/a in a long time)]

(In reply to comment #57)
> Sorry, I don't think that's a better place -- it's worse. Makes a long method
> even longer. I would have preferred to put it on nsIFrame::GetAccessible() for
> deckframe. Why not that?
> 

i don't agree with you at this point.

1)the logic of GetAccessible is to create accessible in different scenario. so put there is not wrong.

2)there is not a elegant point to call nsIFrame::GetAccessible for boxFrame/scrollFrame

3)even we call nsIFrame::GetAccessible for boxFrame/scrolFrame, we still need lots of code in GetAccessible. what we can move out is just several lines.

Comment 59

[Aaron Leventhal]

1) It's a general method that shouldn't know about specific widgets unless we really have to
2) Do boxFrame/scrollFrame implement nsIFrame? If so, you can just implement GetAccessible on them
3) We may want to do this for another kind of XUL frame in the future.

If #2 is no, then let's create a GetAccessibleForXULFrame() method and call that.

Comment 60

[Nian Liu(n/a in a long time)]

(In reply to comment #59)
> 1) It's a general method that shouldn't know about specific widgets unless we
> really have to
> 2) Do boxFrame/scrollFrame implement nsIFrame? If so, you can just implement
> GetAccessible on them
what i mean is that there is not an elegant point to call nsIFrame::GetAccessible in GetAccessible for box/scroll frame.
> 3) We may want to do this for another kind of XUL frame in the future.
> 
> If #2 is no, then let's create a GetAccessibleForXULFrame() method and call
> that.
> 

Comment 61

[Nian Liu(n/a in a long time)]

Attachment: patchv4

based on previous, use nsIFrame::GetAccessible

Comment 62

[Nian Liu(n/a in a long time)]

aaron, any chanc to review this?

Comment 63

[Aaron Leventhal]

Comment on attachment 254246 [details] [diff] [review]
patchv4

Alexander, could you review this? Unintentionally I have been too busy, but I don't mean to make this patch sit a long time. If there's a better way to do this we should try to do that. Or we could approve this to unblock the Orca team and file follow-up patch to fix better later.

Comment 64

[alexander :surkov (:asurkov)]

Comment on attachment 254246 [details] [diff] [review]
patchv4

I talked with Nian Liu on irc, he will provide new patch. Clear requests for now.

Comment 65

[Nian Liu(n/a in a long time)]

Attachment: patchv4.1

addressing surkov's comment

Comment 66

[alexander :surkov (:asurkov)]

Comment on attachment 258388 [details] [diff] [review]
patchv4.1


>+    } else if (content->GetNameSpaceID() == kNameSpaceID_XUL) {

If you like to have this code working for any deck frame then it's wrong check because for example, xul:deck works fine with html content. Though I'm still not sure that we need accessible objects for content of xul:deck.

>+      nsIFrame* frame = nsnull;
>+      while (content) {
>+        frame = aPresShell->GetPrimaryFrameFor(content);
>+        if (frame) {
>+          break;
>+        }
>+        content = content->GetParent();
>+      }

I'm not layout guy but I think every XUL element has frame. Doesn't it? And if element that accessible object was requested for has not frame then we shouldn't create accessible object for its framed parent because we like to get accessible for the given element. Am I wrong?

Comment 67

[Nian Liu(n/a in a long time)]

(In reply to comment #66)
> (From update of attachment 258388 [details] [diff] [review])
> 
> >+    } else if (content->GetNameSpaceID() == kNameSpaceID_XUL) {
> 
> If you like to have this code working for any deck frame then it's wrong check
> because for example, xul:deck works fine with html content. Though I'm still
> not sure that we need accessible objects for content of xul:deck.
can you provide an example?
> 
> >+      nsIFrame* frame = nsnull;
> >+      while (content) {
> >+        frame = aPresShell->GetPrimaryFrameFor(content);
> >+        if (frame) {
> >+          break;
> >+        }
> >+        content = content->GetParent();
> >+      }
> 
> I'm not layout guy but I think every XUL element has frame. Doesn't it? And if
> element that accessible object was requested for has not frame then we
> shouldn't create accessible object for its framed parent because we like to get
> accessible for the given element. Am I wrong?
> 
nope, xul element may not have frame. and nope, parent frame accessible is created for structure issue.

Comment 68

[alexander :surkov (:asurkov)]

(In reply to comment #67)
> (In reply to comment #66)
> > (From update of attachment 258388 [details] [diff] [review] [details])
> > 
> > >+    } else if (content->GetNameSpaceID() == kNameSpaceID_XUL) {
> > 
> > If you like to have this code working for any deck frame then it's wrong check
> > because for example, xul:deck works fine with html content. Though I'm still
> > not sure that we need accessible objects for content of xul:deck.
> can you provide an example?

<xul:deck selectedIndex="1">
  <html:p>Hello1</html:p>
  <html:p>Hello2</html:p>
</xul:deck>

Do you like to see testcase?

> > >+      nsIFrame* frame = nsnull;
> > >+      while (content) {
> > >+        frame = aPresShell->GetPrimaryFrameFor(content);
> > >+        if (frame) {
> > >+          break;
> > >+        }
> > >+        content = content->GetParent();
> > >+      }
> > 
> > I'm not layout guy but I think every XUL element has frame. Doesn't it? And if
> > element that accessible object was requested for has not frame then we
> > shouldn't create accessible object for its framed parent because we like to get
> > accessible for the given element. Am I wrong?
> > 
> nope, xul element may not have frame. and nope, parent frame accessible is
> created for structure issue.
> 

Could you provide an example?

Comment 69

[Nian Liu(n/a in a long time)]

yes, pls. post it on. thanks a lot

Comment 70

[alexander :surkov (:asurkov)]

Attachment: deck with html

Comment 71

[Nian Liu(n/a in a long time)]

Attachment: patch

content can be anything now(xul, html, svg...)

Comment 72

[alexander :surkov (:asurkov)]

Nian, nsIAccessible doesn't have longer roles constants :)

I'm not sure about ROLE_PANE changing. Btw, please note if you change role for xul:tabpanel then you should change it for tabpanel ARIA role. I'll try to find out what role does MSSA wait from tabpanel.

Comment 73

[alexander :surkov (:asurkov)]

IIRC WC_TABCONTROL implements tabpanels as dialogs, MSAA dialog implements either ROLE_PROPERTYPAGE or ROLE_DIALOG (http://msdn.microsoft.com/library/default.asp?url=/library/en-us/msaa/msaapndx_6ypa.asp). I guess it's reasonable to keep ROLE_PROPERTYPAGE for xul:tabpanel. Though probably Aaron should answer this.

Comment 74

[alexander :surkov (:asurkov)]

(In reply to comment #13)
> > let me describe it in another way. nsXULTabPanelsAccessible::GetRole returns
> > ROLE_PROPERTYPAGE. i don't think that's correct. TabPanels should be just
> > Role_Pane and TabPanel should be ROLE_PROPERTYPAGE. I'll change it if you 
> > agree with me.
> Yes, agreed.

If I'm right then tabpanel has ROLE_PANE role with your patch. But now it haven't any role :)

Comment 75

[Nian Liu(n/a in a long time)]

Attachment: patch with modifying deck children role

Comment 76

[Aaron Leventhal]

ROLE_PROPERTYPAGE is good for tabpanel.

Comment 77

[alexander :surkov (:asurkov)]

Comment on attachment 259178 [details] [diff] [review]
patch with modifying deck children role


>-  /** The display area for a dialog or tabbrowser interface */
>-  const long XULTabPanels = 0x00001023;
>-
>   /** The collection of tab objects, useable in the TabBox and independant of
>    as well */
>   const long XULTabs = 0x00001024;

nit: If you would renumber other consts then it will be great.

>   /**
>    * Return accessible object for elements implementing nsIAccessibleProvider
>    * interface.
>    */
>   nsresult GetAccessibleByType(nsIDOMNode *aNode, nsIAccessible **aAccessible);
>+  
>+  // Return accessible object if parent is a deck frame
>+  nsresult GetAccessibleForDeckChildren(nsIDOMNode *aNode, nsIAccessible **aAccessible);

nit: please use JavaStyle comments like in method above.

>+  while (content) {
>+    frame = shell->GetPrimaryFrameFor(content);
>+    if (frame) {
>+      break;
>+    }
>+    content = content->GetParent();
>+  }

would it be better?

do {
  frame = shell->GetPrimaryFrameFor(content);
} while (!frame && (content = content->GetParent()));
    

>+  if (frame && (frame->GetType() == nsAccessibilityAtoms::boxFrame ||
>+                frame->GetType() == nsAccessibilityAtoms::scrollFrame)) { 

I thought deck can contain anything. Where does check boxFrame and scrollFrame go from?

Also, why don't you use nsGkAtoms, why do you need to reintroduce these atoms in ally. Who will catch if atoms will be eventually changed?

Comment 78

[Aaron Leventhal]

Apparently Mozilla sr='s don't like do {} while ( ) very much.
I guess while () { } is favored in all cases.

Comment 79

[alexander :surkov (:asurkov)]

(In reply to comment #78)
> Apparently Mozilla sr='s don't like do {} while ( ) very much.
> I guess while () { } is favored in all cases.
> 

Ok. Btw, do you know why they don't like? :) Is it code style issue?

Comment 80

[Aaron Leventhal]

Code style -- Scott Collins told me there's a general belief by top C++ easier that it's easier to read other people's code when you don't have do while usage.

Comment 81

[Nian Liu(n/a in a long time)]

(In reply to comment #77)
> (From update of attachment 259178 [details] [diff] [review])
> 
> >-  /** The display area for a dialog or tabbrowser interface */
> >-  const long XULTabPanels = 0x00001023;
> >-
> >   /** The collection of tab objects, useable in the TabBox and independant of
> >    as well */
> >   const long XULTabs = 0x00001024;
> 
> nit: If you would renumber other consts then it will be great.
> 
any strong reason for that? keep digit sequence?
> >   /**
> >    * Return accessible object for elements implementing nsIAccessibleProvider
> >    * interface.
> >    */
> >   nsresult GetAccessibleByType(nsIDOMNode *aNode, nsIAccessible **aAccessible);
> >+  
> >+  // Return accessible object if parent is a deck frame
> >+  nsresult GetAccessibleForDeckChildren(nsIDOMNode *aNode, nsIAccessible **aAccessible);
> 
> nit: please use JavaStyle comments like in method above.
> 
i see. thanks for pointing it.
> >+  while (content) {
> >+    frame = shell->GetPrimaryFrameFor(content);
> >+    if (frame) {
> >+      break;
> >+    }
> >+    content = content->GetParent();
> >+  }
> 
> would it be better?
> 
> do {
>   frame = shell->GetPrimaryFrameFor(content);
> } while (!frame && (content = content->GetParent()));
> 
> 
> >+  if (frame && (frame->GetType() == nsAccessibilityAtoms::boxFrame ||
> >+                frame->GetType() == nsAccessibilityAtoms::scrollFrame)) { 
> 
> I thought deck can contain anything. Where does check boxFrame and scrollFrame
> go from?
> 
> Also, why don't you use nsGkAtoms, why do you need to reintroduce these atoms
> in ally. Who will catch if atoms will be eventually changed?
> 
nsGkAtoms is name space limited to layout module. we must add item in nsAccessibilityAtomList if we want to use it.

Comment 82

[Nian Liu(n/a in a long time)]

not sure why that's happened after my add comment. reopen it

Comment 83

[alexander :surkov (:asurkov)]

(In reply to comment #81)
> (In reply to comment #77)
> > (From update of attachment 259178 [details] [diff] [review] [details])
> > 
> > >-  /** The display area for a dialog or tabbrowser interface */
> > >-  const long XULTabPanels = 0x00001023;
> > >-
> > >   /** The collection of tab objects, useable in the TabBox and independant of
> > >    as well */
> > >   const long XULTabs = 0x00001024;
> > 
> > nit: If you would renumber other consts then it will be great.
> > 
> any strong reason for that? keep digit sequence?

Yes. But it's a nit.

> > Also, why don't you use nsGkAtoms, why do you need to reintroduce these atoms
> > in ally. Who will catch if atoms will be eventually changed?
> > 
> nsGkAtoms is name space limited to layout module. we must add item in
> nsAccessibilityAtomList if we want to use it.
> 

Ah, I see. Then please add comments for new atoms that are come from nsGkAtoms.

Comment 84

[Nian Liu(n/a in a long time)]

Attachment: patch

addressing surkov's comment1&2

we usually just add peer in nsAccessibilityAtomList when we need it. no comment needed

Comment 85

[alexander :surkov (:asurkov)]

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

r=me with followin quesiton answred:

> >+  if (frame && (frame->GetType() == nsAccessibilityAtoms::boxFrame ||
> >+                frame->GetType() == nsAccessibilityAtoms::scrollFrame)) { 
> 
> I thought deck can contain anything. Where does check boxFrame and scrollFrame
> go from?

(In reply to comment #84)
> we usually just add peer in nsAccessibilityAtomList when we need it. no comment
> needed
> 

Some accessible atoms has comments where they are used (for example, // XForms, //XUL). I'd like to see comments too for new atoms.

Comment 86

[Nian Liu(n/a in a long time)]

(In reply to comment #85)
> (From update of attachment 259295 [details] [diff] [review])
> r=me with followin quesiton answred:
> 
> > >+  if (frame && (frame->GetType() == nsAccessibilityAtoms::boxFrame ||
> > >+                frame->GetType() == nsAccessibilityAtoms::scrollFrame)) { 
> > 
> > I thought deck can contain anything. Where does check boxFrame and scrollFrame
> > go from?
sorry, missed that. i confirmed with Neil that currently we only have two kinds of children for deck frame. we'll add more if we find something new
> 
> (In reply to comment #84)
> > we usually just add peer in nsAccessibilityAtomList when we need it. no comment
> > needed
> > 
> 
> Some accessible atoms has comments where they are used (for example, // XForms,
> //XUL). I'd like to see comments too for new atoms.
> 
well, new added is for all. what comments would you like?

Comment 87

[alexander :surkov (:asurkov)]

(In reply to comment #86)

> > Some accessible atoms has comments where they are used (for example, // XForms,
> > //XUL). I'd like to see comments too for new atoms.
> > 
> well, new added is for all. what comments would you like?
> 

Dunno. Probably "dupe of nsGkAtoms"?

Comment 88

[[e-mail]]

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

Sorry for not looking at this earlier; we've had a couple of long internet outages recently and our backup mailbox starts rejecting mail after a few hours.

>+  nsIFrame* frame = nsnull;
>+  nsIFrame* parentFrame = nsnull;
>+  nsCOMPtr<nsIContent> content(do_QueryInterface(aNode));
>+
>+  while (content) {
>+    frame = shell->GetPrimaryFrameFor(content);
>+    if (frame) {
>+      break;
>+    }
>+    content = content->GetParent();
>+  }
What's the point of this loop? Surely you're only interested in direct children.

Comment 89

[Nian Liu(n/a in a long time)]

(In reply to comment #88)
> What's the point of this loop? Surely you're only interested in direct
> children.
> 
in fact, what am i interested in is the direct frame holding the content.

Comment 90

[[e-mail]]

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

Then sr=me if you change that code to get the direct frame only and not loop.

Comment 91

[Nian Liu(n/a in a long time)]

Neil, sorry for misunderstanding.

I should say that what i'm interested in is the first frame in the parent chain from the content.

and without the loop, the patch doesn't work

Comment 92

[Aaron Leventhal]

Nian, I think Neil is asking for more clarification than that. Why is the loop necessary to get the frame you want?

Comment 93

[Nian Liu(n/a in a long time)]

the key point of this patch is the frame relationship between current frame and parent frame. while content may not have a frame, i need to get the frame in the parent chain.

Comment 94

[[e-mail]]

(In reply to comment #93)
>the key point of this patch is the frame relationship between current frame and
>parent frame. while content may not have a frame, i need to get the frame in
>the parent chain.
Here's the bit I understand: if some content doesn't already have an accessible, and it has a frame whose parent is a deck frame, then it's a propertypage.
Here's the bit I don't understand: why would some miscellaneous content without a frame ever have a propertypage accessible? Perhaps an example would help.

Comment 95

[Aaron Leventhal]

Ginn, Nian left for 6 months and this patch is almost complete. Can you help answer Neil's questions? It would be a shame to lose all the work.

Comment 96

[Ginn Chen]

Attachment: patch to checkin

I removed the loop since I didn't find any use case of it.