Bug 804461 - build the context dependent tree
 Summary: build the context dependent tree
 Status: RESOLVED FIXED Core Components Disability Access APIs (show other bugs) unspecified All All -- normal (vote) mozilla20 alexander :surkov 834120 treea11y Show dependency tree / graph

Reported: 2012-10-22 21:19 PDT by alexander :surkov
Modified: 2013-02-16 19:34 PST (History)
3 users (show)
ryanvm: in‑testsuite+
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Attachments
part1: preparations (7.37 KB, patch)
2012-11-14 23:54 PST, alexander :surkov
no flags Details | Diff | Review
part1: preparations (6.44 KB, patch)
2012-11-14 23:55 PST, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Review
part2: use context for tables (48.76 KB, patch)
2012-11-21 02:16 PST, alexander :surkov
no flags Details | Diff | Review
part3: use context for lists (16.44 KB, patch)
2012-11-21 20:25 PST, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Review
part2: use context for tables (v2) (48.25 KB, patch)
2012-11-27 06:35 PST, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Review
part2: use context for tables (v3) (48.75 KB, patch)
2012-11-28 22:35 PST, alexander :surkov
no flags Details | Diff | Review
part3: use context for lists (v2) (15.59 KB, patch)
2012-11-28 23:00 PST, alexander :surkov
no flags Details | Diff | Review

 alexander :surkov 2012-10-22 21:19:08 PDT Example: role="presentation" on HTML table gets rid all HTML table related descendants. We should do the same for role="presentation" on HTML list (like ul). On the another hand it doesn't make sense to expose context dependent accessible having no context like
if it's not contained by grid. Thoughts on implementation: every accessible class should have a context, a bit wise flags describing not allowed accessibles, for example, generic Accessible class doesn't allow table or list related descendants, HTMLTableAccessible and etc overrides it. Morph nsAccessibiltiyService::GetOrCreateAccessible to take a context. Trevor Saunders (:tbsaunde) 2012-10-23 08:23:30 PDT (In reply to alexander :surkov from comment #0) > Example: role="presentation" on HTML table gets rid all HTML table related > descendants. We should do the same for role="presentation" on HTML list > (like ul). On the another hand it doesn't make sense to expose context > dependent accessible having no context like
if it's not > contained by grid. > > Thoughts on implementation: > every accessible class should have a context, a bit wise flags describing > not allowed accessibles, for example, generic Accessible class doesn't allow > table or list related descendants, HTMLTableAccessible and etc overrides it. > Morph nsAccessibiltiyService::GetOrCreateAccessible to take a context. I'm not sure I see why it makes more sense to pass it some sort of context thing instead of just passing the parent directly. I think we can probably do most if not all of these checks just based on mFlags and or Role of the accessible. alexander :surkov 2012-10-23 08:35:52 PDT (In reply to Trevor Saunders (:tbsaunde) from comment #1) > I'm not sure I see why it makes more sense to pass it some sort of context > thing instead of just passing the parent directly. I think we can probably > do most if not all of these checks just based on mFlags and or Role of the > accessible. parent instead flags should be fine if we are ok to trust our nsAccessibilityService, just in case if it wants to call some "illegal" method. I wouldn't use Role() since it might require computations depending on the tree state. mFlags must be a perfect fit. So I would restrict it to passing mFlags as argument so nobody gets an idea to use Role() or anything else for context detection proposes. Sounds ok? Trevor Saunders (:tbsaunde) 2012-10-23 09:12:35 PDT (In reply to alexander :surkov from comment #2) > (In reply to Trevor Saunders (:tbsaunde) from comment #1) > > > I'm not sure I see why it makes more sense to pass it some sort of context > > thing instead of just passing the parent directly. I think we can probably > > do most if not all of these checks just based on mFlags and or Role of the > > accessible. > > parent instead flags should be fine if we are ok to trust our > nsAccessibilityService, just in case if it wants to call some "illegal" > method. I wouldn't use Role() since it might require computations depending well, trust ourselves not to allow people to add those things :) I think we should be able to fix Role() so its just a member lookup, which I've been meaning to do for a while. A least assuming we don't want to support accessibles changing there role. > on the tree state. mFlags must be a perfect fit. So I would restrict it to > passing mFlags as argument so nobody gets an idea to use Role() or anything > else for context detection proposes. Sounds ok? I'm fine with that if mFlags gives you everything you need, but we don't have that many free bits left in mFlags so we should be careful with adding more things just for this. David Bolter [:davidb] 2012-10-23 09:14:29 PDT Can you give me an example of what kind of flag we might be talking about? Something like eImposter? So for example an isolated div with role="cell" would have the imposter bit flipped so that we know we can't call table methods? Trevor Saunders (:tbsaunde) 2012-10-23 10:25:43 PDT (In reply to David Bolter [:davidb] from comment #4) > Can you give me an example of what kind of flag we might be talking about? > Something like eImposter? > > So for example an isolated div with role="cell" would have the imposter bit > flipped so that we know we can't call table methods? no, this is about accessible creation, so if eListAccessible & mFlags is not true then child accessible should not be a list item. David Bolter [:davidb] 2012-10-23 10:26:57 PDT OK thanks. alexander :surkov 2012-10-23 19:51:20 PDT (In reply to Trevor Saunders (:tbsaunde) from comment #3) > > parent instead flags should be fine if we are ok to trust our > > nsAccessibilityService, just in case if it wants to call some "illegal" > > method. I wouldn't use Role() since it might require computations depending > > well, trust ourselves not to allow people to add those things :) this is so true > I think we should be able to fix Role() so its just a member lookup, which > I've been meaning to do for a while. A least assuming we don't want to > support accessibles changing there role. it makes sense but not sure how. file a bug? > > on the tree state. mFlags must be a perfect fit. So I would restrict it to > > passing mFlags as argument so nobody gets an idea to use Role() or anything > > else for context detection proposes. Sounds ok? > > I'm fine with that if mFlags gives you everything you need, but we don't > have that many free bits left in mFlags so we should be careful with adding > more things just for this. we used 24 bits for now so we should be in a good shape still. If we run out of bits then we should start to be smarter (starting from idea that most of combinations described by mFlags aren't possible). alexander :surkov 2012-10-25 07:40:10 PDT Trev, would you be interested to take it? alexander :surkov 2012-11-14 23:54:14 PST Created attachment 681881 [details] [diff] [review] part1: preparations alexander :surkov 2012-11-14 23:55:21 PST Created attachment 681882 [details] [diff] [review] part1: preparations correct patch Trevor Saunders (:tbsaunde) 2012-11-15 10:56:22 PST Comment on attachment 681882 [details] [diff] [review] part1: preparations >+ nsAccTreeWalker(DocAccessible* aDoc, Accessible* aContext, >+ nsIContent* aContent, bool aWalkCache) : it'd be nice to get rid of the nsIContent* arg and just get it from the accessible maybe, but that seems for the xul text field case. >@@ -1516,8 +1516,7 @@ > { > // Search for accessible children starting from the document element since > // some web pages tend to insert elements under it rather than document body. >- nsAccTreeWalker walker(this, mDocument->GetRootElement(), hm, we usually use nsCoreUtils::RoleContent() to get the content node for the document accessible right? so it seems odd to just use mDocument->GetRootElement() here, but replacing nsCoreUtils::RoleContent() might be nice. >+bool >+XULColorPickerAccessible::CanHaveAnonChildren() why did you need to add this here? btw should this method be const? alexander :surkov 2012-11-15 22:01:44 PST (In reply to Trevor Saunders (:tbsaunde) from comment #11) > Comment on attachment 681882 [details] [diff] [review] > part1: preparations > > >+ nsAccTreeWalker(DocAccessible* aDoc, Accessible* aContext, > >+ nsIContent* aContent, bool aWalkCache) : > > it'd be nice to get rid of the nsIContent* arg and just get it from the > accessible maybe, but that seems for the xul text field case. For xul textbox and document, so we still need it. Anyway we need to rethink this code because bug 787858 doens't fit into existing approach. > >@@ -1516,8 +1516,7 @@ > > { > > // Search for accessible children starting from the document element since > > // some web pages tend to insert elements under it rather than document body. > >- nsAccTreeWalker walker(this, mDocument->GetRootElement(), > > hm, we usually use nsCoreUtils::RoleContent() to get the content node for > the document accessible right? so it seems odd to just use > mDocument->GetRootElement() here, but replacing nsCoreUtils::RoleContent() > might be nice. root element is not necessary role content element (for example html element and body element). Doing that we may loose accessible elements outside the body (that's valid case). > >+bool > >+XULColorPickerAccessible::CanHaveAnonChildren() > > why did you need to add this here? no need, I will remove them > btw should this method be const? It should be const method but I would rather try to replace it on flags approach, having it virtual doesn't make huge sense. But actually it may go away in nearest future at all (I keep in mind bug 787858). alexander :surkov 2012-11-15 22:29:19 PST You know we can remove DocAccessible from constructor (yep) and mDoc from members and replace it on mContext->Document() (maybe). Sounds ok? alexander :surkov 2012-11-16 22:13:04 PST (In reply to alexander :surkov from comment #13) > You know we can remove DocAccessible from constructor (yep) and mDoc from > members and replace it on mContext->Document() (maybe). Sounds ok? I'll land this one as is and put that as follow up to avoid merging in my queue Trevor Saunders (:tbsaunde) 2012-11-17 00:17:45 PST (In reply to alexander :surkov from comment #13) > You know we can remove DocAccessible from constructor (yep) and mDoc from > members and replace it on mContext->Document() (maybe). Sounds ok? that's probably fine, but it might be \epsilon slower if access to stuff near esp is faster than a random cached place in the heap. Maybe just get the document in the constructor? alexander :surkov 2012-11-17 04:19:03 PST ok alexander :surkov 2012-11-17 07:28:54 PST Comment on attachment 681882 [details] [diff] [review] part1: preparations http://hg.mozilla.org/integration/mozilla-inbound/rev/84c3118523eb Ryan VanderMeulen [:RyanVM] 2012-11-18 05:41:33 PST https://hg.mozilla.org/mozilla-central/rev/84c3118523eb alexander :surkov 2012-11-18 18:13:16 PST it's not finished yet alexander :surkov 2012-11-21 02:16:39 PST Created attachment 683939 [details] [diff] [review] part2: use context for tables it seems messy and it doesn't look less complicated to what we had, maybe we could reorg that later (like widget interface extension) but now I don't have good ideas David Bolter [:davidb] 2012-11-21 06:06:31 PST As long as it isn't messier I like the idea of passing a context. It might help in future cases and maybe we'll want more than an accessible, so we could create a AccessibleContext or something at that point. (Reminds me of the style context passed in css frame construction) alexander :surkov 2012-11-21 19:30:47 PST (In reply to David Bolter [:davidb] from comment #21) > As long as it isn't messier I like the idea of passing a context. at the quick glance it is a right and neat idea but it doesn't save us from tree traversal (or at least its proposed implementation), so if we need to traverse trees up in either case then the idea (or, granted, its implementation) is not brilliant. alexander :surkov 2012-11-21 20:25:27 PST Created attachment 684302 [details] [diff] [review] part3: use context for lists Trevor Saunders (:tbsaunde) 2012-11-26 16:26:31 PST Comment on attachment 683939 [details] [diff] [review] part2: use context for tables approach seems generally fine, though I feel like the accessible creation stuff could be reorganized to make a good bit more sense. >+ if (nsCoreUtils::IsHTMLTableHeader(aContent) && >+ aContext->Document()->HasAccessible(aContent->GetParentNode())) { same as below, why only check there is an accessible when checking its the context is stronger, and faster >- Accessible* accessible = new HTMLOutputAccessible(aContent, aDoc); >+ Accessible* accessible = >+ new HTMLOutputAccessible(aContent, aContext->Document()); use local var for doc to make things shorter? >+ if (aContext->IsOfType(Accessible::eTableAccessible) && why not use AsTable() ? >+ aContext->Document()->HasAccessible(aContent->GetParent())) { why not do aContext->GetContent() == content->GetParent() ? >+ // Accessible HTML table cell must be a child of accessible HTML table row. >+ if (aContext->IsOfType(Accessible::eHTMLTableRowAccessible)) { why is it important its not a row of a different type? I tend to think
foo
is fine all as a table. >+ if (parentFrame->GetType() == nsGkAtoms::tableOuterFrame && >+ aContext->Document()->HasAccessible(parentContent)) { why not do aContext->GetContent() == parentContent ? >+ newAcc = new HTMLTableRowAccessible(aContent, aContext->Document()); use a local var for the doc just to make things shorter? >+ eTableAccessible = 1 << 24, >+ eTableCellAccessible = 1 << 25, >+ eTableRowAccessible = 1 << 26, is there a good reason to use these instead of AsTable() / AsTableCell() / Role() or maybe NativeRole() ? >+ eTableRowGroupAccessible = 1 << 27, this doesn't seem to be used at all >+ eXULTreeAccessible = 1 << 30 hm, this gets us really close to out of bits >+class HTMLTableRowAccessible : public AccessibleWrap inherit from EnumAccessible maybe? >+ HTMLTableRowAccessible(nsIContent* aContent, DocAccessible* aDoc) : >+ AccessibleWrap(aContent, aDoc) >+ { mFlags |= eTableRowAccessible | eHTMLTableRowAccessible; } I've been sort of tempted to make Accessible's constructor take flags to add to mFlags before, I wonder if that would make more sense, or atleast doing that for EnumRoleAccessible then you wouldn't need this. >+ ~HTMLTableRowAccessible() { } virtual? >+ NS_DECL_ISUPPORTS_INHERITED why do you need to over ride this? >- testAccessibleTree("grid", accTree); >+ //testAccessibleTree("grid", accTree); why do you disable this? alexander :surkov 2012-11-27 05:49:58 PST (In reply to Trevor Saunders (:tbsaunde) from comment #24) > approach seems generally fine, though I feel like the accessible creation > stuff could be reorganized to make a good bit more sense. that'd be nice > >+ if (nsCoreUtils::IsHTMLTableHeader(aContent) && > >+ aContext->Document()->HasAccessible(aContent->GetParentNode())) { > > same as below, why only check there is an accessible when checking its the > context is stronger, and faster having a table context doesn't mean that this th belongs to that header, for example, it can belong to presentational table. > >- Accessible* accessible = new HTMLOutputAccessible(aContent, aDoc); > >+ Accessible* accessible = > >+ new HTMLOutputAccessible(aContent, aContext->Document()); > > use local var for doc to make things shorter? locals for inlines aren't preferable but I can do that I think. > > >+ if (aContext->IsOfType(Accessible::eTableAccessible) && > > why not use AsTable() ? it's alternative to not existing IsTable() method, usually we have IsSomething() and AsSomething(). Moreover AsTable is virtual > >+ aContext->Document()->HasAccessible(aContent->GetParent())) { > > why not do aContext->GetContent() == content->GetParent() ? that might do a trick > >+ // Accessible HTML table cell must be a child of accessible HTML table row. > >+ if (aContext->IsOfType(Accessible::eHTMLTableRowAccessible)) { > > why is it important its not a row of a different type? > I tend to think > >
>
foo
> > > is fine all as a table. they aren't really compatible if they are possible at all, sometimes people use crazy structures and we support some of them but I don't want to support any wild fantasy ;) at least unit it works and used in the wild > >+ eTableAccessible = 1 << 24, > >+ eTableCellAccessible = 1 << 25, > >+ eTableRowAccessible = 1 << 26, > > is there a good reason to use these instead of AsTable() / AsTableCell() / > Role() or maybe NativeRole() ? It seems we move away from Role() method usage and introduce AccessilbeType instead. AccessibleType is analogue of role. > >+ eTableRowGroupAccessible = 1 << 27, > > this doesn't seem to be used at all > > >+ eXULTreeAccessible = 1 << 30 > > hm, this gets us really close to out of bits bug 810572 will help > >+class HTMLTableRowAccessible : public AccessibleWrap > > inherit from EnumAccessible maybe? extra member to keep a role? virtual Role() method works fine. > >+ HTMLTableRowAccessible(nsIContent* aContent, DocAccessible* aDoc) : > >+ AccessibleWrap(aContent, aDoc) > >+ { mFlags |= eTableRowAccessible | eHTMLTableRowAccessible; } > > I've been sort of tempted to make Accessible's constructor take flags to add > to mFlags before, I wonder if that would make more sense, or atleast doing > that for EnumRoleAccessible then you wouldn't need this. I'm not sure, I thought about that, but ended up with that version > >+ NS_DECL_ISUPPORTS_INHERITED > > why do you need to over ride this? we do that if we inherit from wrap class to not conflict with MSAA QueryInterface alexander :surkov 2012-11-27 06:35:52 PST Created attachment 685611 [details] [diff] [review] part2: use context for tables (v2) Trevor Saunders (:tbsaunde) 2012-11-27 15:28:03 PST > > >+ if (nsCoreUtils::IsHTMLTableHeader(aContent) && > > >+ aContext->Document()->HasAccessible(aContent->GetParentNode())) { > > > > same as below, why only check there is an accessible when checking its the > > context is stronger, and faster > > having a table context doesn't mean that this th belongs to that header, for > example, it can belong to presentational table. example markup? or maybe I don't understand what your trying to do here. > > >- Accessible* accessible = new HTMLOutputAccessible(aContent, aDoc); > > >+ Accessible* accessible = > > >+ new HTMLOutputAccessible(aContent, aContext->Document()); > > > > use local var for doc to make things shorter? > > locals for inlines aren't preferable but I can do that I think. I tend to not really care one way or the other and go for what's shorter. > > > > >+ if (aContext->IsOfType(Accessible::eTableAccessible) && > > > > why not use AsTable() ? > > it's alternative to not existing IsTable() method, usually we have > IsSomething() and AsSomething(). Moreover AsTable is virtual iirc you suggested adding IsTable() that called AsTable() or something. > > I tend to think > > > >
> >
foo
> > > > > > is fine all as a table. > > they aren't really compatible if they are possible at all, sometimes people > use crazy structures and we support some of them but I don't want to support > any wild fantasy ;) at least unit it works and used in the wild true, I just wonder if its worth extra work to change what we do in crazy case. > > >+ eTableAccessible = 1 << 24, > > >+ eTableCellAccessible = 1 << 25, > > >+ eTableRowAccessible = 1 << 26, > > > > is there a good reason to use these instead of AsTable() / AsTableCell() / > > Role() or maybe NativeRole() ? > > It seems we move away from Role() method usage and introduce AccessilbeType > instead. AccessibleType is analogue of role. true, it would just be nice to have less dupplication. > > >+ eTableRowGroupAccessible = 1 << 27, > > > > this doesn't seem to be used at all > > > > >+ eXULTreeAccessible = 1 << 30 > > > > hm, this gets us really close to out of bits > > bug 810572 will help ok, are you planning to work on it? > > >+class HTMLTableRowAccessible : public AccessibleWrap > > > > inherit from EnumAccessible maybe? > > extra member to keep a role? virtual Role() method works fine. are you argueing to get rid EnumAccessible then? or maybe we should make it a class templated on the role to return? alexander :surkov 2012-11-27 18:07:32 PST (In reply to Trevor Saunders (:tbsaunde) from comment #27) > > > >+ if (nsCoreUtils::IsHTMLTableHeader(aContent) && > > > >+ aContext->Document()->HasAccessible(aContent->GetParentNode())) { > > > > > > same as below, why only check there is an accessible when checking its the > > > context is stronger, and faster > > > > having a table context doesn't mean that this th belongs to that header, for > > example, it can belong to presentational table. > > example markup? or maybe I don't understand what your trying to do here. this one is:
should have an accessible
shouldn't have an accessible
> > > use local var for doc to make things shorter? > > > > locals for inlines aren't preferable but I can do that I think. > > I tend to not really care one way or the other and go for what's shorter. I changed it already :) > > > > > > >+ if (aContext->IsOfType(Accessible::eTableAccessible) && > > > > > > why not use AsTable() ? > > > > it's alternative to not existing IsTable() method, usually we have > > IsSomething() and AsSomething(). Moreover AsTable is virtual > > iirc you suggested adding IsTable() that called AsTable() or something. well, we always move back and forward, mFlags approach should work too. Actually I feel a little mess about recently added things. I think we can sort out things when we go ahead. > > > >+ eTableRowGroupAccessible = 1 << 27, > > > > > > this doesn't seem to be used at all btw, I forgot to address it in latest patch > > > >+ eXULTreeAccessible = 1 << 30 > > > > > > hm, this gets us really close to out of bits > > > > bug 810572 will help > > ok, are you planning to work on it? I think I do > > > >+class HTMLTableRowAccessible : public AccessibleWrap > > > > > > inherit from EnumAccessible maybe? > > > > extra member to keep a role? virtual Role() method works fine. > > are you argueing to get rid EnumAccessible then? I'm not sure I ever liked it > or maybe we should make it > a class templated on the role to return? that seems reasonable, file a bug? alexander :surkov 2012-11-27 18:13:54 PST (In reply to alexander :surkov from comment #28) > > example markup? or maybe I don't understand what your trying to do here. > > this one is: it should be:
should have an accessible
shouldn't have an accessible
 Trevor Saunders (:tbsaunde) 2012-11-27 19:25:59 PST (In reply to alexander :surkov from comment #29) > (In reply to alexander :surkov from comment #28) in that case how is the context accessible for the inaccessible th a table row accessible? The one node I see there that should be a table row is the outer tr which has a accessible td child so we won't search the td's kids for accessible children of the tr. if the accessible we're looking for children of is below the accessible for the td then the context won't be a table row so of course we won't create a accessible for the th. The only case I can think of where were looking at the content parent might matter is something like
not table cell table cell
where the div isn't going to be accessible so we search its kids for accessible children. alexander :surkov 2012-11-27 19:32:11 PST (In reply to Trevor Saunders (:tbsaunde) from comment #30) > (In reply to alexander :surkov from comment #29) > > (In reply to alexander :surkov from comment #28) > > in that case how is the context accessible for the inaccessible th a table > row accessible? The one node I see there that should be a table row is the > outer tr which has a accessible td child so we won't search the td's kids > for accessible children of the tr. there are wild cases, they could do
 Trevor Saunders (:tbsaunde) 2012-11-27 19:47:29 PST (In reply to alexander :surkov from comment #31) > (In reply to Trevor Saunders (:tbsaunde) from comment #30) > > (In reply to alexander :surkov from comment #29) > > > (In reply to alexander :surkov from comment #28) > > > > in that case how is the context accessible for the inaccessible th a table > > row accessible? The one node I see there that should be a table row is the > > outer tr which has a accessible td child so we won't search the td's kids > > for accessible children of the tr. > > there are wild cases, they could do > >
> > > sure, but that doesn't seem to be a case where doc->HasAccessible(th->GetParent()) works, but aContext->GetContent() == th->GetParent() doesn't alexander :surkov 2012-11-27 21:00:39 PST it seems both of them will return false since th->GetParent() is tr which is not accessible. so they both seems working. Trevor Saunders (:tbsaunde) 2012-11-28 14:43:39 PST Comment on attachment 685611 [details] [diff] [review] part2: use context for tables (v2) >+ if (roleMapEntry) { >+ // Create pure ARIA grid/treegrid related accessibles if they weren't used >+ // on accessible HTML table elements. >+ if ((roleMapEntry->accTypes & Accessible::eTableCellAccessible)) { >+ if (aContext->IsOfType(Accessible::eTableRowAccessible) && >+ (frame->AccessibleType() != eHTMLTableCellAccessible || >+ !document->HasAccessible(content->GetParentNode()))) { why not use aContext->GetContent() != parentNode here? > if (tag == nsGkAtoms::abbr || > tag == nsGkAtoms::acronym || > tag == nsGkAtoms::blockquote || > tag == nsGkAtoms::dd || > tag == nsGkAtoms::form || > tag == nsGkAtoms::h1 || > tag == nsGkAtoms::h2 || > tag == nsGkAtoms::h3 || > tag == nsGkAtoms::h4 || > tag == nsGkAtoms::h5 || > tag == nsGkAtoms::h6 || > tag == nsGkAtoms::q) { nit, mind making this take fewer lines while your here? >+ if (aContext->IsOfType(Accessible::eTableAccessible) && >+ aContext->Document()->HasAccessible(aContent->GetParent())) { use aContext->GetContent() == aContent->GetParent() ? >+ // Accessible HTML table cell must be a child of accessible HTML table row. >+ if (aContext->IsOfType(Accessible::eHTMLTableRowAccessible)) >+ newAcc = new HTMLTableCellAccessibleWrap(aContent, document); > break; nit, blank line pls >+ eTableRowGroupAccessible = 1 << 27, remember to remove it :) don't you need to set eTableCellAccessible in HTMLTableCellAccessible XULTreeGridCellAccessible and set the appropriate flag in each of the classes for XULListBoxAccessible when its being a table? >+ accTree = { >+ role: ROLE_TABLE, >+ children: [ >+ { // div@role="row" >+ role: ROLE_ROW, >+ tagName: "DIV", >+ children: [ >+ { // caption text leaf >+ role: ROLE_TEXT_LEAF, >+ name: "caption", >+ children: [ ] >+ }, >+ { // th text leaf >+ role: ROLE_TEXT_LEAF, >+ name: "header1", >+ children: [ ] >+ }, >+ { // td@role="columnheader" >+ role: ROLE_COLUMNHEADER, >+ name: "header2", >+ children: [ { TEXT_LEAF: [ ] } ] >+ } >+ ] >+ }, >+ { // tr@role="row" >+ role: ROLE_ROW, >+ tagName: "TR", >+ children: [ >+ { // td text leaf >+ role: ROLE_TEXT_LEAF, >+ name: "cell1", >+ children: [ ] >+ }, >+ { // td@role="gridcell" >+ role: ROLE_GRID_CELL, >+ name: "cell2", >+ children: [ { TEXT_LEAF: [ ] } ] >+ } >+ ] >+ }, >+ { // div@role="row" >+ role: ROLE_ROW, >+ children: [ >+ { // div@role="gridcell" >+ role: ROLE_GRID_CELL, >+ children: [ >+ { // text leaf from presentational table >+ role: ROLE_TEXT_LEAF, >+ name: "cell3", >+ children: [ ] >+ }, >+ ] >+ } >+ ] >+ }, >+ { // div@role="row" >+ role: ROLE_ROW, >+ children: [ >+ { // div@role="gridcell" >+ role: ROLE_GRID_CELL, >+ children: [ >+ { // table >+ role: ROLE_TABLE, >+ children: [ >+ { // tr >+ role: ROLE_ROW, >+ children: [ >+ { // td >+ role: ROLE_CELL, >+ children: [ >+ { // caption text leaf of presentational table >+ role: ROLE_TEXT_LEAF, >+ name: "caption", >+ children: [ ] >+ }, >+ { // td text leaf of presentational table >+ role: ROLE_TEXT_LEAF, >+ name: "cell4", >+ children: [ ] >+ } >+ ] >+ } >+ ] >+ } >+ ] >+ } >+ ] >+ } >+ ] >+ } >+ ] >+ }; that's a *huge* test that I'm fairly sure really wants to be a couple seperate tree tests, mind too much making it so? I think it'll help debuging future test failures if the test cases that fail are smaller. >diff --git a/accessible/tests/mochitest/tree/test_brokencontext.html> var tree = > { PUSHBUTTON: [ // table >- { NOTHING: [ // tbody any idea how this used to pass? r=me but it might be nice to look at the changes you make Trevor Saunders (:tbsaunde) 2012-11-28 17:19:32 PST Comment on attachment 684302 [details] [diff] [review] part3: use context for lists >+ } else if (frame->AccessibleType() == eHTMLLiAccessible || >+ content->Tag() == nsGkAtoms::dt || >+ content->Tag() == nsGkAtoms::li || >+ content->Tag() == nsGkAtoms::dd) { why not switch the order of these checks? I'd really expect gettingthe tag is faster than the virtual call that may do some work... >+ if (aContext->IsOfType(Accessible::eListAccessible)) { >+ // If list item is a child of accessible list then create an accessible for >+ // it unconditionally by tag name. nsBlockFrame creates the list item >+ // accessible for other elements styled as list items. >+ if (aContext->Document()->HasAccessible(aContent->GetParent())) { I'm not completely sure something like
is bad, but if we're going to do this why not do aContext->GetContent() == content->GetParent() >+ if (aContext->IsOfType(Accessible::eListAccessible) && >+ aContext->Document()->HasAccessible(aContent->GetParent())) { same, use aContext->GetContent() here too. >+ if (aContent->Tag() == nsGkAtoms::dt || aContent->Tag() == nsGkAtoms::dd) >+ break; > newAcc = new HyperTextAccessibleWrap(aContent, aContext->Document()); > break; why not invert the test see we only have one break? nit, blank line after the if pls alexander :surkov 2012-11-28 22:17:13 PST (In reply to Trevor Saunders (:tbsaunde) from comment #34) > Comment on attachment 685611 [details] [diff] [review] > part2: use context for tables (v2) > >+ (frame->AccessibleType() != eHTMLTableCellAccessible || > >+ !document->HasAccessible(content->GetParentNode()))) { > > why not use aContext->GetContent() != parentNode here? ok > > tag == nsGkAtoms::h6 || > > tag == nsGkAtoms::q) { > > nit, mind making this take fewer lines while your here? I think I like the way they are (easy to skim through them), so I wouldn't change it. > >+ if (aContext->IsOfType(Accessible::eTableAccessible) && > >+ aContext->Document()->HasAccessible(aContent->GetParent())) { > > use aContext->GetContent() == aContent->GetParent() ? ok > nit, blank line pls I don't mind > >+ eTableRowGroupAccessible = 1 << 27, > > remember to remove it :) I did that already :) > don't you need to set eTableCellAccessible in HTMLTableCellAccessible > XULTreeGridCellAccessible and set the appropriate flag in each of the > classes for XULListBoxAccessible when its being a table? That'd be correct think but I would leave it until we have a case when we need it, especially, XULListboxAccessible that can be a table and can be a list. > that's a *huge* test that I'm fairly sure really wants to be a couple > seperate tree tests, mind too much making it so? I think it'll help > debuging future test failures if the test cases that fail are smaller. makes sense > >diff --git a/accessible/tests/mochitest/tree/test_brokencontext.html> var tree = > > { PUSHBUTTON: [ // table > >- { NOTHING: [ // tbody > > any idea how this used to pass? I think we ended up with gEmptyLandmark as roleMapEntry and created generic accessible because of it. Thank you for bringing this up. I hate to figure out motivation for not evident side changes after years. > r=me but it might be nice to look at the changes you make sure, I'll file a patch alexander :surkov 2012-11-28 22:35:58 PST Created attachment 686437 [details] [diff] [review] part2: use context for tables (v3) alexander :surkov 2012-11-28 23:00:53 PST Created attachment 686446 [details] [diff] [review] part3: use context for lists (v2) alexander :surkov 2012-11-30 03:40:31 PST (In reply to alexander :surkov from comment #37) > Created attachment 686437 [details] [diff] [review] > part2: use context for tables (v3) https://hg.mozilla.org/integration/mozilla-inbound/rev/1f2e0e82aa7d Ryan VanderMeulen [:RyanVM] 2012-12-01 07:47:26 PST https://hg.mozilla.org/mozilla-central/rev/1f2e0e82aa7d alexander :surkov 2012-12-02 03:38:23 PST (In reply to alexander :surkov from comment #38) > Created attachment 686446 [details] [diff] [review] > part3: use context for lists (v2) http://hg.mozilla.org/integration/mozilla-inbound/rev/791ce5d073bc Ed Morley [:emorley] 2012-12-03 11:11:50 PST https://hg.mozilla.org/mozilla-central/rev/791ce5d073bc