Closed Bug 357969 Opened 13 years ago Closed 13 years ago

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

Categories

(Firefox :: Disability Access, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: nian.liu, Assigned: ginnchen+exoracle)

References

(Blocks 2 open bugs)

Details

(Keywords: access)

Attachments

(3 files, 11 obsolete files)

334 bytes, application/vnd.mozilla.xul+xml
Details
18.20 KB, patch
surkov
: review+
Details | Diff | Splinter Review
17.04 KB, patch
Details | Diff | Splinter Review
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
Summary: container xul element which doesn't have a xbl def has no accessible object → container xul element which doesn't have a xbl def under a deck frame has no accessible object
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.
Keywords: access
(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.
> 

Attached patch patch (obsolete) — Splinter Review
solution 1
Attachment #243597 - Flags: review?(mano)
seems children includes="" doesn't work. if necessary, i'll file a bug to clear tabpanels with hbox/vbox issue as in page info.
(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 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...
Attachment #243597 - Flags: review?(mano) → review-
Nian, why the label property?
> 
> 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^_^
> 
> 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
> 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.
> 

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

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


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

Attached patch patch (obsolete) — Splinter Review
1)patch also fixed bug 356415
2)we don't have support for aaa:labelledby in native code side, right?
Attachment #243597 - Attachment is obsolete: true
Attachment #244785 - Flags: review?(aaronleventhal)
Attachment #244785 - Flags: review?(aaronleventhal) → review?(ginn.chen)
*** Bug 356415 has been marked as a duplicate of this bug. ***
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()
Attachment #244785 - Flags: review?(ginn.chen) → review-
(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.
(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.

Attached patch patch (obsolete) — Splinter Review
Attachment #244785 - Attachment is obsolete: true
Attachment #247049 - Flags: review?(aaronleventhal)
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.
Attachment #247049 - Flags: review?(mano)
Attachment #247049 - Flags: review?(aaronleventhal)
Attachment #247049 - Flags: review+
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.
Attachment #247049 - Flags: review?(mano) → review-
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?
(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.
CC'ing Neil -- notsure if he would otherwise see your last comment.
(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.
(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.
> 

Attached patch patch that in a11y scope (obsolete) — Splinter Review
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.
Attachment #247655 - Flags: review?(aaronleventhal)
(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 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 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 on attachment 247655 [details] [diff] [review]
patch that in a11y scope

I'll hold off until Neil's comments are dealt with.
Attachment #247655 - Flags: review?(aaronleventhal)
(
> 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.
i can't find a way to build tab/tabpanel relationship when linkedPanel is used. anyone has an idea?
(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?
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #247049 - Attachment is obsolete: true
Attachment #247655 - Attachment is obsolete: true
Attachment #251037 - Flags: review?(aaronleventhal)
1-1 relationship is traced with bug 366527
(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).
( 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?
(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));
> 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.
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?
So, <wizardpage> has CSS overflow: auto; which introduces a scroll frame;
would that make a difference?
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.
Attachment #251037 - Flags: review?(aaronleventhal)
(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.
(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.
Attached patch patch v2 (obsolete) — Splinter Review
resolve wizardpage issue too
Attachment #251037 - Attachment is obsolete: true
Attachment #252020 - Flags: review?(aaronleventhal)
Comment on attachment 252020 [details] [diff] [review]
patch v2

Neil, is there any other container type used under a deckframe besides boxframe and scrollframe?
Attachment #252020 - Flags: review?(neil)
Comment on attachment 252020 [details] [diff] [review]
patch v2

Not that I know of.
Attachment #252020 - Flags: review?(neil) → review+
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 on attachment 252020 [details] [diff] [review]
patch v2

Need answers to previous questions before proceeding with review.
Attachment #252020 - Flags: review?(aaronleventhal)
(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.
> 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.
Blocks: xula11y
No longer blocks: keya11y
Attached patch patch v3 (obsolete) — Splinter Review
compared with last patch, 
1)fix wrong variable name "node"
2)move hack code to a more reasonable place
Attachment #253586 - Flags: review?(aaronleventhal)
(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)

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?
Attachment #253586 - Flags: review?(aaronleventhal) → review-
(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.
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.
(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.
> 

Attached patch patchv4 (obsolete) — Splinter Review
based on previous, use nsIFrame::GetAccessible
Attachment #254246 - Flags: review?(aaronleventhal)
aaron, any chanc to review this?
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.
Attachment #254246 - Flags: review?(surkov.alexander)
Comment on attachment 254246 [details] [diff] [review]
patchv4

I talked with Nian Liu on irc, he will provide new patch. Clear requests for now.
Attachment #254246 - Flags: review?(surkov.alexander)
Attachment #254246 - Flags: review?(aaronleventhal)
Attached patch patchv4.1 (obsolete) — Splinter Review
addressing surkov's comment
Attachment #252020 - Attachment is obsolete: true
Attachment #253586 - Attachment is obsolete: true
Attachment #258388 - Flags: review?(surkov.alexander)
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?
(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.

(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?
yes, pls. post it on. thanks a lot
Blocks: orca
Attached patch patch (obsolete) — Splinter Review
content can be anything now(xul, html, svg...)
Attachment #254246 - Attachment is obsolete: true
Attachment #258388 - Attachment is obsolete: true
Attachment #259065 - Flags: review?(surkov.alexander)
Attachment #258388 - Flags: review?(surkov.alexander)
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.
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.
(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 :)
Attachment #259178 - Flags: review?(surkov.alexander)
ROLE_PROPERTYPAGE is good for tabpanel.
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?
Attachment #259065 - Attachment is obsolete: true
Attachment #259065 - Flags: review?(surkov.alexander)
Apparently Mozilla sr='s don't like do {} while ( ) very much.
I guess while () { } is favored in all cases.
(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?
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.
(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.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
not sure why that's happened after my add comment. reopen it
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
Attached patch patchSplinter Review
addressing surkov's comment1&2

we usually just add peer in nsAccessibilityAtomList when we need it. no comment needed
Attachment #259295 - Flags: review?(surkov.alexander)
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.
Attachment #259295 - Flags: review?(surkov.alexander) → review+
Attachment #259178 - Attachment is obsolete: true
Attachment #259178 - Flags: review?(surkov.alexander)
(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?
(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"?
Attachment #259295 - Flags: superreview?(neil)
Whiteboard: sr= request from Neil is from Feb
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.
(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 on attachment 259295 [details] [diff] [review]
patch

Then sr=me if you change that code to get the direct frame only and not loop.
Attachment #259295 - Flags: superreview?(neil) → superreview+
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
Nian, I think Neil is asking for more clarification than that. Why is the loop necessary to get the frame you want?
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.
(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.
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.
Assignee: nian.liu → ginn.chen
Status: REOPENED → NEW
Blocks: 356617
Attached patch patch to checkinSplinter Review
I removed the loop since I didn't find any use case of it.
Status: NEW → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Whiteboard: sr= request from Neil is from Feb
You need to log in before you can comment on or make changes to this bug.