Closed Bug 852150 Opened 11 years ago Closed 11 years ago

"ASSERTION: No accessible parent?!" with table

Categories

(Core :: Disability Access APIs, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla24

People

(Reporter: jruderman, Assigned: tbsaunde)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(3 files, 4 obsolete files)

testcase
(deleted), application/xhtml+xml
Details
stack (gdb)
(deleted), text/plain
Details
bug 852150 - handle removal of accessibles when reframe root doesn't have an accessible more correctly
(deleted), patch
surkov
: review+
Details | Diff | Splinter Review
Attached file testcase (deleted) — 
1. Enable accessibility.
2. Load the testcase.

###!!! ASSERTION: No accessible parent?!: 'parent', file accessible/src/generic/DocAccessible.cpp, line 1870
Attached file stack (gdb) (deleted) — 

    
    

    
  
So, it looks like the real problem here is that when the <table> gets appended to the div layout calls nsAccessibilityService::ContentRemoved() with the child being removed as the body element.  That means that we don't properly remove the accessible for the table from the tree and things go down hill from there.

Boris any idea why nsCSSFrameConstructor is passing the body element not the table, and how hard that would be to change?

    
    

    
  
Removing the table from the DOM triggers a reframe of the <body> because the following sibling of the <table> is an anonymous table wrapper around the <tr>.  See the 

  if (nextSibling && IsTablePseudo(nextSibling)) {

bit in nsCSSFrameConstructor::MaybeRecreateContainerForFrameRemoval.

We can actually stop doing that in this case, since we do in fact know what the prevsibling is in this situation, now that frames have cheap prev siblings...

In any case, the point is that we do a remove on the <body>'s frame followed by an insert on the <body>'s frame.  Which is what we notify to the accessibility code.

    
    

    
  
> We can actually stop doing that in this case, since we do in fact know what
> the prevsibling is in this situation, now that frames have cheap prev
> siblings...

I guess you can take care of fixing that if its something you want fixed, or would you like me to do something?

> In any case, the point is that we do a remove on the <body>'s frame followed
> by an insert on the <body>'s frame.  Which is what we notify to the
> accessibility code.

ok, makes sense, however I'm not actually sure if that's the behaviour we really want.

    
    

    
  
> I guess you can take care of fixing that if its something you want fixed

Yeah.

> however I'm not actually sure if that's the behaviour we really want.

I don't either.  I can't figure out what notifications a11y actually wants.  At one point it was basically "frame tree changes", but it sounds like this is no longer the case?

    
    

    
  
> > however I'm not actually sure if that's the behaviour we really want.
> 
> I don't either.  I can't figure out what notifications a11y actually wants. 
> At one point it was basically "frame tree changes", but it sounds like this
> is no longer the case?

well,  either that or the a11y logic to handle those notifications doesn't cover all cases of frame tree changes.

tbh I'm really not sure what the right answer is the ideal answer would be to get notified for the accessible at the root of the subtree that needs to go away, but that seems like  not very helpful answer.

I'd think removing content with nsIDocumentObserver would work fine for this case, but there's probably another one I'm missing where it won't.  (other than the xbl special case)

actually maybe this is a case get the world into a state in which flushing layout will change the bullet for a list item then change something in a seperate subtree to the list item but where the root of the frame tree that gets replaced contains the list item.  Then ideally a11y would get notified that the list item needs a new child accessible for the bullet, and the other thing needs updated, but afaik it is possible for layout to see that as one transaction, and you can't use nsIDocumentObserver for the list bullet thing because the content for the bullet is NAC.

Alex wanted to base a11y tree update on frame tree update more  at one point, but I'm not sure if that is still what he thinks or what his plan was exactly.

    
  
Blocks: 852138

    
    

    
  
(In reply to Trevor Saunders (:tbsaunde) from comment #6)

> Alex wanted to base a11y tree update on frame tree update more  at one
> point, but I'm not sure if that is still what he thinks or what his plan was
> exactly.

Frame tree is responsible for how elements appear on the screen and actually this kind of info should be exposed via accessible tree to screen readers. For example hidden DOM elements shouldn't be accessible or we should expose elements in conformity with their visual order (XUL tree columns). That's why I wanted to base accessible tree on frame tree. There's a downside of this approach like this bug or bug 686400. I think we will end up with something in the middle between DOM tree and frame tree.

For this bug I think we should fix our tree update logic (per comment #2) and file new bug blocking bug 686400 to not recreate all elements under body.

    
    

    
  
(In reply to alexander :surkov from comment #7)
> (In reply to Trevor Saunders (:tbsaunde) from comment #6)
> 
> > Alex wanted to base a11y tree update on frame tree update more  at one
> > point, but I'm not sure if that is still what he thinks or what his plan was
> > exactly.
> 
> Frame tree is responsible for how elements appear on the screen and actually
> this kind of info should be exposed via accessible tree to screen readers.
> For example hidden DOM elements shouldn't be accessible or we should expose
> elements in conformity with their visual order (XUL tree columns). That's
> why I wanted to base accessible tree on frame tree. There's a downside of
> this approach like this bug or bug 686400. I think we will end up with
> something in the middle between DOM tree and frame tree.

that seems reasonable.

> For this bug I think we should fix our tree update logic (per comment #2)

ok, I doubt it'll be hard to blow away everything under the accessible  of the nearest content ancestor with one, but that'll make slow stuff slower :(

> and file new bug blocking bug 686400 to not recreate all elements under body.

any idea how we'd accomplish that?  I don't think we need to come up with an answer today, but I think its something we should sit down and figure out  soon.

    
    

    
  
(In reply to Trevor Saunders (:tbsaunde) from comment #8)
> > For this bug I think we should fix our tree update logic (per comment #2)
> 
> ok, I doubt it'll be hard to blow away everything under the accessible  of
> the nearest content ancestor with one, but that'll make slow stuff slower :(

can you please describe in details what happens here?

> > and file new bug blocking bug 686400 to not recreate all elements under body.
> 
> any idea how we'd accomplish that?  I don't think we need to come up with an
> answer today, but I think its something we should sit down and figure out 
> soon.

I don't really have good ideas. Probably we should set next to bz and discuss it one day.

    
    

    
  
(In reply to alexander :surkov from comment #9)
> (In reply to Trevor Saunders (:tbsaunde) from comment #8)
> > > For this bug I think we should fix our tree update logic (per comment #2)
> > 
> > ok, I doubt it'll be hard to blow away everything under the accessible  of
> > the nearest content ancestor with one, but that'll make slow stuff slower :(
> 
> can you please describe in details what happens here?

I think my plan would be to change GetAccessible() at the top of DocAccessible::UpdateTree() to GetAccessibleOrContainer()

the general algorithm would be this
layout hands you a node that is the root of the tree that is getting reframed call this node curNode
while curNode doesn't have a accessible set curnode to curNode->GetParentNode()
given that the original cur node was visible I believe you must eventually get to the document, so in the worst case you'll get the DocAccessible for that document.
Now that you have an accessible call InvalidateChildren() on it. (yes that is horrible but what else to do?)

I suppose another option is to try and remove accessibles using the nsIDocumentObserver ContentRemoved() callback we already get if the stuff hasn't already been removed.

> > > and file new bug blocking bug 686400 to not recreate all elements under body.
> > 
> > any idea how we'd accomplish that?  I don't think we need to come up with an
> > answer today, but I think its something we should sit down and figure out 
> > soon.
> 
> I don't really have good ideas. Probably we should set next to bz and
> discuss it one day.

fine with if he's willing :)

    
    

    
  
I filed bug 852408 on not reframing here.

I'm happy to talk about this stuff, sure.

    
    

    
  
(In reply to Boris Zbarsky (:bz) from comment #11)
> I filed bug 852408 on not reframing here.

thank you for doing this. Btw, can you give an example explaining why parent reframing might be needed at all in this case? (I don't understand "this pseudo needs to get merged with the frame's prevSibling").

    
    

    
  
(In reply to Trevor Saunders (:tbsaunde) from comment #10)
> (In reply to alexander :surkov from comment #9)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #8)
> > > > For this bug I think we should fix our tree update logic (per comment #2)
> > > 
> > > ok, I doubt it'll be hard to blow away everything under the accessible  of
> > > the nearest content ancestor with one, but that'll make slow stuff slower :(
> > 
> > can you please describe in details what happens here?
> 
> I think my plan would be to change GetAccessible() at the top of
> DocAccessible::UpdateTree() to GetAccessibleOrContainer()

i.e. Accessible* child = GetAccessibleOrContainer(aChildNode)? it doesn't sound correct

> I suppose another option is to try and remove accessibles using the
> nsIDocumentObserver ContentRemoved() callback we already get if the stuff
> hasn't already been removed.

this sounds like a hack

actually I asked a different thing. I don't really understand what steps lead us to that assertion (your comment #2).

    
    

    
  
Sure.  Consider this markup:

  <body>
    <div style="display: table-cell">a</div>
    <div id="x">b</div>
    <div style="display: table-cell">c</div>
  </body>

Now say the <div id="x"> is removed. After the removal, the two table cells (which until that point are int two different tables) need to end up in a single row of a single table.

    
    

    
  
How is the frame tree look like? Is each table-cell div wrapped by row/table frames?

Wouldn't it make sense to invalidate table-cell and relative frames and do not touch unrelated things?

    
    

    
  
> Is each table-cell div wrapped by row/table frames?

Yes, until the div in between is removed.  Rowgroup too.

> Wouldn't it make sense to invalidate table-cell and relative frames and do not touch
> unrelated things?

There are many combinations like the above, each one requiring different fixup (e.g. each of the before/after things can be any of row, cell, row-group, column, column-group, caption).  In practice it's simpler to just rebuild the world to handle this edge case, as long as it's not being a serious performance problem on real-world content.

    
    

    
  
(In reply to alexander :surkov from comment #13)
> (In reply to Trevor Saunders (:tbsaunde) from comment #10)
> > (In reply to alexander :surkov from comment #9)
> > > (In reply to Trevor Saunders (:tbsaunde) from comment #8)
> > > > > For this bug I think we should fix our tree update logic (per comment #2)
> > > > 
> > > > ok, I doubt it'll be hard to blow away everything under the accessible  of
> > > > the nearest content ancestor with one, but that'll make slow stuff slower :(
> > > 
> > > can you please describe in details what happens here?
> > 
> > I think my plan would be to change GetAccessible() at the top of
> > DocAccessible::UpdateTree() to GetAccessibleOrContainer()
> 
> i.e. Accessible* child = GetAccessibleOrContainer(aChildNode)? it doesn't
> sound correct

hm, I'm not sure it might remove too much, but if it does we might find and recreate it all when new frames get added.

> > I suppose another option is to try and remove accessibles using the
> > nsIDocumentObserver ContentRemoved() callback we already get if the stuff
> > hasn't already been removed.
> 
> this sounds like a hack

it doesn't sound great to me either.

> actually I asked a different thing. I don't really understand what steps
> lead us to that assertion (your comment #2).

ok
1 DocAccessible::ContentRemoved(nsIContent*, nsIContent*) is called with <html> and <body> elements as args
2. in DocAccessible::UpdateTree() Accessible* child = NULL since <body> doesn't get an accessible.
3 we go through the else branch at DocAccessible.cpp:1770
4 TreeWalker doesn't find the <table> accessible because its content has already been removed.
6 we return to layout
7 InvalidateChildren() gets called on the doc accessible somehow (I haven't checked that but it seems really likely) so now accessible for table is detached from tree
8 CacheChildren() doesn't reattach accessible for table to doc accessible because its not a child anymore.
9 we get notified of another reframe of <body> this time because <tr> is being removed
10 TreeWalker finds <table> accessible because its a child of the display:none div (why it goes into the subtree I'm not sure but anyway)
11 UpdateChildren() is doing a remove so it tries and remove the accessible for <table> but it has no parent because of 7

    
    

    
  
It seems we should replace TreeWalker in case of removal (btw, TreeWalker doesn't seems right in case of insertion but it's different story) on accessible tree inspection. We need to run through children and remove those who has inaccessible child in between its container or doesn't have a DOM path to it container.

I think we can have mochitest by getting a table accessible after the table elm was moved under hidden element.

    
    

    
  
(In reply to alexander :surkov from comment #18)
> It seems we should replace TreeWalker in case of removal (btw, TreeWalker
> doesn't seems right in case of insertion but it's different story) on
> accessible tree inspection. We need to run through children and remove those
> who has inaccessible child in between its container or doesn't have a DOM
> path to it container.

that sounds slow / painful, but maybe paths will be short enough the slow isn't that bad hopefully...

> I think we can have mochitest by getting a table accessible after the table
> elm was moved under hidden element.

yeah, testing this should be possible.

    
    

    
  
(In reply to Trevor Saunders (:tbsaunde) from comment #19)
> (In reply to alexander :surkov from comment #18)
> > It seems we should replace TreeWalker in case of removal (btw, TreeWalker
> > doesn't seems right in case of insertion but it's different story) on
> > accessible tree inspection. We need to run through children and remove those
> > who has inaccessible child in between its container or doesn't have a DOM
> > path to it container.
> 
> that sounds slow / painful, but maybe paths will be short enough the slow
> isn't that bad hopefully...

rebuild all children isn't fast too (an alternative solution if I understand right).

but agree it's ugly a bit

    
    

    
  
and technically it should be faster since you need to do TreeWalker on container

    
    

    
  
(In reply to alexander :surkov from comment #20)
> (In reply to Trevor Saunders (:tbsaunde) from comment #19)
> > (In reply to alexander :surkov from comment #18)
> > > It seems we should replace TreeWalker in case of removal (btw, TreeWalker
> > > doesn't seems right in case of insertion but it's different story) on
> > > accessible tree inspection. We need to run through children and remove those
> > > who has inaccessible child in between its container or doesn't have a DOM
> > > path to it container.
> > 
> > that sounds slow / painful, but maybe paths will be short enough the slow
> > isn't that bad hopefully...
> 
> rebuild all children isn't fast too (an alternative solution if I understand
> right).
> 
> but agree it's ugly a bit

Well, so another trick is that "path needs to take into account accessible b is child of a, where b is for content in xbl binding and a is outside binding.

    
    

    
  
Does GetFlattenedTreeParent() help?

    
    

    
  
(In reply to alexander :surkov from comment #23)
> Does GetFlattenedTreeParent() help?

since it seems to return nsIContent* I assume it will never return the document so I think GetParentNode() is probably better?  I was really hoping for something in nsContentUtils to worry about this for me though :/

    
    

    
  
(In reply to Trevor Saunders (:tbsaunde) from comment #24)
> (In reply to alexander :surkov from comment #23)
> > Does GetFlattenedTreeParent() help?
> 
> since it seems to return nsIContent* I assume it will never return the
> document so I think GetParentNode() is probably better?  I was really hoping
> for something in nsContentUtils to worry about this for me though :/

oh, and if that wasn't fun enough the <table> element actually still thinks its parent is the <body> but the <body> doesn't think it has a kid that is a <table> presumalby that has to do with us being in the middle of removing the table as a child of the body.

Boris, does any other stuff deal with that somehow?

    
    

    
  
In what sense?

During ContentRemoved the kid has been removed from the parent but still has the parent as its parent, iirc because xbl or frame construction stuff depended on it.  The kid is unbound immediately after the ContentRemoved call.

Consumers of ContentRemoved shouldn't be examining the removed child's parent chain, in general.

    
    

    
  
(In reply to Boris Zbarsky (:bz) from comment #26)
> In what sense?
> 
> During ContentRemoved the kid has been removed from the parent but still has
> the parent as its parent, iirc because xbl or frame construction stuff
> depended on it.  The kid is unbound immediately after the ContentRemoved
> call.

ok, that lines up with what I see.

> Consumers of ContentRemoved shouldn't be examining the removed child's
> parent chain, in general.

that puts us  in sort of an unfortunate position, because the situation is this we're under ContentRemoved() and are being told that a parent of the removed content is what's being reframed, so we need to get rid of accessible objects that corespond to stuff in the sub tree that's being reframed however the frame at the root doesn't have an accessible object so we have to get the accessible object for a parent frame and see which kids are in the subtree.  However its kind of hard to tell that if you can't look at things parents.

    
    

    
  
You should be passed the object being removed and the thing it's being removed from, right?

I guess the issue is that you can't get to the <table> given that information?

    
    

    
  
(In reply to Boris Zbarsky (:bz) from comment #28)
> You should be passed the object being removed and the thing it's being
> removed from, right?

yes

> I guess the issue is that you can't get to the <table> given that
> information?

not exactly, the problem is more you have an object that points at the table and you need to know if the thing being removed is an ancestor of that, so that you can remove it from the tree if the removee is an ancestor and leave it if not.

    
    

    
  
Ah.  Well, that should work, since the parent chain is still correct, no?

    
    

    
  


  
this seems to pass tests and fix the assert but I want to clean it up some and look over it with fresh eyes before asking people to review it.  That said more eyes and opinions on the approach would be nice.

    
    

    
  
Trev, do you take this?
Severity: normal → major
OS: Mac OS X → All
Hardware: x86_64 → All

    
    

    
  
(In reply to alexander :surkov from comment #32)
> Trev, do you take this?

yeah, I've been working on it, if your ok with the idea of the wip I should have something reviewable in a day or so.

    
    

    
  
(In reply to Boris Zbarsky (:bz) from comment #30)
> Ah.  Well, that should work, since the parent chain is still correct, no?

yeah, I'd just written a buggy patch ;)

    
    

    
  
(In reply to Trevor Saunders (:tbsaunde) from comment #34)
> (In reply to Boris Zbarsky (:bz) from comment #30)
> > Ah.  Well, that should work, since the parent chain is still correct, no?
> 
> yeah, I'd just written a buggy patch ;)

that said it may be true that the approach of the wip doesn't work if there is an accessible for content that is getting reframed but is outside of the subtree being removed which seems like an odd case, but I suspect its possible to come up with one if you try hard enough.

    
    

    
  
Comment on attachment 729298 [details] [diff] [review]
wip bug 852150 - handle removal of accessibles when reframe root doesn't have an accessible more correctly

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

::: accessible/src/generic/DocAccessible.cpp
@@ +1791,5 @@
> +        return;
> +
> +      for (; firstChildIdx < lastChildIdx && lastChildIdx < childCount; lastChildIdx--) {
> +        nsINode* childContent = container->ContentChildAt(lastChildIdx)->GetContent();
> +        while ((childContent != aChildNode) && (childContent = childContent->GetParentNode()));

you don't need to get above container node
GetParentNode() works until it let you to cross xbl boundaries (that's why I told you about GetFlattenedParent)
Assignee: nobody → trev.saunders

    
    

    
  
(In reply to alexander :surkov from comment #36)
> Comment on attachment 729298 [details] [diff] [review]
> wip bug 852150 - handle removal of accessibles when reframe root doesn't
> have an accessible more correctly
> 
> Review of attachment 729298 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/generic/DocAccessible.cpp
> @@ +1791,5 @@
> > +        return;
> > +
> > +      for (; firstChildIdx < lastChildIdx && lastChildIdx < childCount; lastChildIdx--) {
> > +        nsINode* childContent = container->ContentChildAt(lastChildIdx)->GetContent();
> > +        while ((childContent != aChildNode) && (childContent = childContent->GetParentNode()));
> 
> you don't need to get above container node

seems right

> GetParentNode() works until it let you to cross xbl boundaries (that's why I
> told you about GetFlattenedParent)

yeah, GetFlattenedTreeParent() is probably better here but I wasn't worrying about xbl when I was trying to see if works

    
    

    
  
(In reply to Trevor Saunders (:tbsaunde) from comment #37)
> (In reply to alexander :surkov from comment #36)
> > Comment on attachment 729298 [details] [diff] [review]
> > wip bug 852150 - handle removal of accessibles when reframe root doesn't
> > have an accessible more correctly
> > 
> > Review of attachment 729298 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: accessible/src/generic/DocAccessible.cpp
> > @@ +1791,5 @@
> > > +        return;
> > > +
> > > +      for (; firstChildIdx < lastChildIdx && lastChildIdx < childCount; lastChildIdx--) {
> > > +        nsINode* childContent = container->ContentChildAt(lastChildIdx)->GetContent();
> > > +        while ((childContent != aChildNode) && (childContent = childContent->GetParentNode()));
> > 
> > you don't need to get above container node
> 
> seems right
> 
> > GetParentNode() works until it let you to cross xbl boundaries (that's why I
> > told you about GetFlattenedParent)
> 
> yeah, GetFlattenedTreeParent() is probably better here but I wasn't worrying
> about xbl when I was trying to see if works

actually I'm not sure we need to worry about xbl boundaries here.  The only way they could matter is when the root content being reframed is between an accessible containing content containing xbl and a child accessible in the xbl content.  I'm not sure but it seems fairly unlikely such intermediate content would exist, so maybe we can not worry about it for now since it should be faster to do so unless we have too.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  

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

    
  
Blocks: 855375

    
    

    
  
Looks like there is some 'Math.tan' cruft in the patch? :)

    
    

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

Boris, not going to make you look at this, but might be good to have a non-a11y person look over it :)

        Attachment #730234 -
        Flags: feedback?(bzbarsky)

    
    

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

>+      while (firstChildIdx <= lastChildIdx && lastChildIdx < childCount) {
>+        updateFlags |= UpdateTreeInternal(aContainer->ContentChildAt(firstChildIdx),
>+                                          aIsInsert, reorderEvent);
>+        lastChildIdx--;
>+      }

We're updating lastChildIdx, but passing firstChildIdx to ContentChildAt... why does this make sense?

    
    

    
  
(In reply to Boris Zbarsky (:bz) from comment #42)
> Comment on attachment 730234 [details] [diff] [review]
> patch
> 
> >+      while (firstChildIdx <= lastChildIdx && lastChildIdx < childCount) {
> >+        updateFlags |= UpdateTreeInternal(aContainer->ContentChildAt(firstChildIdx),
> >+                                          aIsInsert, reorderEvent);
> >+        lastChildIdx--;
> >+      }
> 
> We're updating lastChildIdx, but passing firstChildIdx to ContentChildAt...
> why does this make sense?

so, we need to remove children left to right (some text change computation depends on that, and lets ignore that that's broken for rtl languages)

so, for example you have children
a b c e
and want to remove middle 2
first remove b
now you have
a c e
then remove c to get
a e

while its sort of odd that loop actually models that.

you can't remove at lastChildIdx because that would be right to left and you can't increment firstChildIdx because you need to account for removals.

    
    

    
  
Ah.  That's worth at least some sort of code comment.  ;)

Also, why do we have to keep rechecking that lastChildIdx < childCount in that case?

    
    

    
  
(In reply to Boris Zbarsky (:bz) from comment #44)
> Ah.  That's worth at least some sort of code comment.  ;)

sounds reasonable

> Also, why do we have to keep rechecking that lastChildIdx < childCount in
> that case?

consider the case firstChildIdx = 0 then the first condition is trivially true.  maybe a better way to deal with both of these issues is to fiddle with the variables a little.

        Attachment #730234 -
        Flags: feedback?(bzbarsky) → feedback+

    
    

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

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

::: accessible/src/generic/DocAccessible.cpp
@@ +1720,5 @@
>      UpdateTree(aContainer, aInsertedContent->ElementAt(idx), true);
>    }
>  }
>  
> +#include "nsIContent.h"

are you sure you need this include? and if yes then put into beginning of the file

@@ +1775,3 @@
>  
> +      while ((child = walker.NextChild()))
> +        updateFlags |= UpdateTreeInternal(child, aIsInsert, reorderEvent);

nit: empty line please

@@ +1775,4 @@
>  
> +      while ((child = walker.NextChild()))
> +        updateFlags |= UpdateTreeInternal(child, aIsInsert, reorderEvent);
> +    } else {

i'd be good to have a comment describing what all this code is about

@@ +1780,5 @@
> +      uint32_t firstChildIdx = 0, childCount = aContainer->ContentChildCount();
> +      for (; firstChildIdx < childCount; firstChildIdx++) {
> +        nsINode* childNode = aContainer->ContentChildAt(firstChildIdx)->GetContent();
> +        while (childNode != aChildNode && childNode != containerNode &&
> +            (childNode = childNode->GetParentNode()));

nit: wrong offset

@@ +1782,5 @@
> +        nsINode* childNode = aContainer->ContentChildAt(firstChildIdx)->GetContent();
> +        while (childNode != aChildNode && childNode != containerNode &&
> +            (childNode = childNode->GetParentNode()));
> +
> +        if (childNode && childNode != containerNode)

containerNode shouldn't be null, thus first condition is not needed

@@ +1784,5 @@
> +            (childNode = childNode->GetParentNode()));
> +
> +        if (childNode && childNode != containerNode)
> +          break;
> +

nit: pls remove empty line

@@ +1791,5 @@
> +      if (firstChildIdx == childCount)
> +        return;
> +
> +      uint32_t lastChildIdx = childCount - 1;
> +      for (; firstChildIdx < lastChildIdx && lastChildIdx < childCount;

seems like second condition is excess

@@ +1792,5 @@
> +        return;
> +
> +      uint32_t lastChildIdx = childCount - 1;
> +      for (; firstChildIdx < lastChildIdx && lastChildIdx < childCount;
> +          lastChildIdx--) {

nit: wrong indent

@@ +1801,5 @@
> +        if (childNode && childNode != containerNode)
> +          break;
> +      }
> +
> +      while (firstChildIdx <= lastChildIdx && lastChildIdx < childCount) {

you do update everything in the middle, you know that sometimes the order in accessible tree doesn't correspond to the order in DOM for example we always keep HTML caption accessible at 0 index while HTML caption element can be at any index in DOM. I'd be safer to just check all of them.

::: accessible/tests/mochitest/events/test_focus_general.html
@@ +76,5 @@
>       * Do tests.
>       */
>  
>      //gA11yEventDumpID = "eventdump"; // debug stuff
> +    gA11yEventDumpToConsole = true;

don't forget to rollback

::: accessible/tests/mochitest/treeupdate/test_doc.html
@@ +203,5 @@
>  
>        this.invoke = function openIFrameDoc_invoke()
>        {
> +x = Math.tan(0.5);
> +dump("\n****\n x is " + x + "\n****\n");

here too and below

    
    

    
  
> ::: accessible/src/generic/DocAccessible.cpp
> @@ +1720,5 @@
> >      UpdateTree(aContainer, aInsertedContent->ElementAt(idx), true);
> >    }
> >  }
> >  
> > +#include "nsIContent.h"
> 
> are you sure you need this include? and if yes then put into beginning of
> the file

just cruft

> @@ +1782,5 @@
> > +        nsINode* childNode = aContainer->ContentChildAt(firstChildIdx)->GetContent();
> > +        while (childNode != aChildNode && childNode != containerNode &&
> > +            (childNode = childNode->GetParentNode()));
> > +
> > +        if (childNode && childNode != containerNode)
> 
> containerNode shouldn't be null, thus first condition is not needed

seems like that should probably be true.

> @@ +1791,5 @@
> > +      if (firstChildIdx == childCount)
> > +        return;
> > +
> > +      uint32_t lastChildIdx = childCount - 1;
> > +      for (; firstChildIdx < lastChildIdx && lastChildIdx < childCount;
> 
> seems like second condition is excess

consider what happens if firstChildIdx = 0

> @@ +1801,5 @@
> > +        if (childNode && childNode != containerNode)
> > +          break;
> > +      }
> > +
> > +      while (firstChildIdx <= lastChildIdx && lastChildIdx < childCount) {
> 
> you do update everything in the middle, you know that sometimes the order in
> accessible tree doesn't correspond to the order in DOM for example we always
> keep HTML caption accessible at 0 index while HTML caption element can be at
> any index in DOM. I'd be safer to just check all of them.

damn it forgot about that case, I wanted to avoid it to be faster, but I guess I can't :(

maybe a better approach would be virtual function on accessible to remove content child and have over load for classes that do weird things, but I'm not sure if its worth the effort or not.

    
    

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

waiting for another patch, right?

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

    
    

    
  
I was looking at transplanting the test case to a mochitest but using inspector I never see the <tr> in the DOM... ?

    
    

    
  
Using inspector where?  On the bugzilla attachment, or on your mochitest?

    
    

    
  
Oh just saw your comment bz. My mochitest.

It seems to be because I was using a regular html file... xhtml seems to keep the <tr> (thanks for suggesting that IRL Trev).

I didn't realize we (still) build the DOM differently. (?)

    
    

    
  
Parsing is totally different for HTML and XML, yes.  As a simple example, in XML you can do things like:

 <img>whatever</img>

which are obviously impossible in HTML.

    
    

    
  
Yeah. I guess I was thinking more about the overlapping syntax. But yeah makes obvious sense.

    
    

    
  
OK cool captured in a mochitest now. Handed patch back over to Trevor for final polish.

    
    

    
  
Comment on attachment 754623 [details] [diff] [review]
bug 852150 - handle removal of accessibles when reframe root doesn't have an accessible more correctly

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

::: accessible/src/generic/DocAccessible.cpp
@@ +1744,5 @@
>  
> +      while ((child = walker.NextChild()))
> +        updateFlags |= UpdateTreeInternal(child, aIsInsert, reorderEvent);
> +    } else {
> +      nsINode* containerNode = aContainer->GetNode();

no comment has been added, after few weeks it's hard to remember what we fix here, after half of year I guess nobody of us will have a clever idea what it is for.

@@ +1746,5 @@
> +        updateFlags |= UpdateTreeInternal(child, aIsInsert, reorderEvent);
> +    } else {
> +      nsINode* containerNode = aContainer->GetNode();
> +      for (uint32_t idx = 0; idx < aContainer->ContentChildCount();) {
> +        nsINode* childNode = aContainer->ContentChildAt(idx)->GetContent();

nit: seems it would be nicer to have a local for aContainer->ContentChildAt(idx) since it's used twice

@@ +1748,5 @@
> +      nsINode* containerNode = aContainer->GetNode();
> +      for (uint32_t idx = 0; idx < aContainer->ContentChildCount();) {
> +        nsINode* childNode = aContainer->ContentChildAt(idx)->GetContent();
> +        while (childNode != aChildNode && childNode != containerNode &&
> +            (childNode = childNode->GetParentNode()));

nit: strange indentation, it'd be good to align it to first condition

@@ +1750,5 @@
> +        nsINode* childNode = aContainer->ContentChildAt(idx)->GetContent();
> +        while (childNode != aChildNode && childNode != containerNode &&
> +            (childNode = childNode->GetParentNode()));
> +
> +        if (childNode && childNode != containerNode) {

not addressed from previous round?

::: accessible/tests/mochitest/treeupdate/test_bug852150.xhtml
@@ +18,5 @@
> +    function doTest()
> +    {
> +      var the_displayNone = document.getElementById("the_displaynone");
> +      var the_table = document.getElementById("the_table");
> +      var the_row = document.getElementById("the_row");

I think I'd prefer getNode()

@@ +25,5 @@
> +      ok(!isAccessible(the_table), "table in display none tree shouldn't be accessible");
> +
> +      setTimeout(function() {
> +        document.body.removeChild(the_row);
> +        // hopefully no more asserts!

it'd be good to have more descriptive comment

@@ +41,5 @@
> +</head>
> +<body>
> +
> +  <a target="_blank"
> +     title="Test parent edge case"

maybe it could be a better title

    
    

    
  


  

        Attachment #729298 -
        Attachment is obsolete: true

        Attachment #730234 -
        Attachment is obsolete: true

        Attachment #754623 -
        Attachment is obsolete: true

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

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

    
    

    
  
Comment on attachment 756676 [details] [diff] [review]
bug 852150 - handle removal of accessibles when reframe root doesn't have an accessible more correctly

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

::: accessible/src/generic/DocAccessible.cpp
@@ +1744,5 @@
>  
> +      while ((child = walker.NextChild()))
> +        updateFlags |= UpdateTreeInternal(child, aIsInsert, reorderEvent);
> +    } else {
> +      // AchildNode may not coorespond to a particular accessible, to handle

aChlidNode -> aChildNode

@@ +1746,5 @@
> +        updateFlags |= UpdateTreeInternal(child, aIsInsert, reorderEvent);
> +    } else {
> +      // AchildNode may not coorespond to a particular accessible, to handle
> +      // this we go through all the children of aContainer.  Then if a child
> +      // has aChildNode as an ancestor remove that child of aContainer.

actually this doesn't answer why we can't simply go through children of aChildNode

@@ +1757,5 @@
> +
> +        if (childNode != containerNode) {
> +          updateFlags |= UpdateTreeInternal(child, false, reorderEvent);
> +        } else {
> +          idx++;

maybe we could go from last child to first one, it allowed us to have simpler incrementation logic

    
    

    
  
> @@ +1746,5 @@
> > +        updateFlags |= UpdateTreeInternal(child, aIsInsert, reorderEvent);
> > +    } else {
> > +      // AchildNode may not coorespond to a particular accessible, to handle
> > +      // this we go through all the children of aContainer.  Then if a child
> > +      // has aChildNode as an ancestor remove that child of aContainer.
> 
> actually this doesn't answer why we can't simply go through children of
> aChildNode

What would cause you to believe that the direct children necessarily all have accessibles?

> @@ +1757,5 @@
> > +
> > +        if (childNode != containerNode) {
> > +          updateFlags |= UpdateTreeInternal(child, false, reorderEvent);
> > +        } else {
> > +          idx++;
> 
> maybe we could go from last child to first one, it allowed us to have
> simpler incrementation logic

that breaks some text change coalescence logic  Which I guess is broken for rtl languages today :/

    
    

    
  
(In reply to Trevor Saunders (:tbsaunde) from comment #59)
> > @@ +1746,5 @@
> > > +        updateFlags |= UpdateTreeInternal(child, aIsInsert, reorderEvent);
> > > +    } else {
> > > +      // AchildNode may not coorespond to a particular accessible, to handle
> > > +      // this we go through all the children of aContainer.  Then if a child
> > > +      // has aChildNode as an ancestor remove that child of aContainer.
> > 
> > actually this doesn't answer why we can't simply go through children of
> > aChildNode
> 
> What would cause you to believe that the direct children necessarily all
> have accessibles?

we used to have accTreeWalker that operated recursively iirc.

> > @@ +1757,5 @@
> > > +
> > > +        if (childNode != containerNode) {
> > > +          updateFlags |= UpdateTreeInternal(child, false, reorderEvent);
> > > +        } else {
> > > +          idx++;
> > 
> > maybe we could go from last child to first one, it allowed us to have
> > simpler incrementation logic
> 
> that breaks some text change coalescence logic  

really? I thought we coalesce them ok if they are removed in any direction

> Which I guess is broken for
> rtl languages today :/

maybe, but at least nobody complained :)

    
    

    
  
(In reply to alexander :surkov from comment #60)
> (In reply to Trevor Saunders (:tbsaunde) from comment #59)
> > > @@ +1746,5 @@
> > > > +        updateFlags |= UpdateTreeInternal(child, aIsInsert, reorderEvent);
> > > > +    } else {
> > > > +      // AchildNode may not coorespond to a particular accessible, to handle
> > > > +      // this we go through all the children of aContainer.  Then if a child
> > > > +      // has aChildNode as an ancestor remove that child of aContainer.
> > > 
> > > actually this doesn't answer why we can't simply go through children of
> > > aChildNode
> > 
> > What would cause you to believe that the direct children necessarily all
> > have accessibles?
> 
> we used to have accTreeWalker that operated recursively iirc.

yes, but if you know the parent accessible why bother with tree walker when you can just iterate over the kids.

> > > @@ +1757,5 @@
> > > > +
> > > > +        if (childNode != containerNode) {
> > > > +          updateFlags |= UpdateTreeInternal(child, false, reorderEvent);
> > > > +        } else {
> > > > +          idx++;
> > > 
> > > maybe we could go from last child to first one, it allowed us to have
> > > simpler incrementation logic
> > 
> > that breaks some text change coalescence logic  
> 
> really? I thought we coalesce them ok if they are removed in any direction

Its been a while but I certainly tried going back to front and had to debug test failures because of it.

> > Which I guess is broken for
> > rtl languages today :/
> 
> maybe, but at least nobody complained :)

true

    
    

    
  
(In reply to Trevor Saunders (:tbsaunde) from comment #61)
> (In reply to alexander :surkov from comment #60)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #59)
> > > > @@ +1746,5 @@
> > > > > +        updateFlags |= UpdateTreeInternal(child, aIsInsert, reorderEvent);
> > > > > +    } else {
> > > > > +      // AchildNode may not coorespond to a particular accessible, to handle
> > > > > +      // this we go through all the children of aContainer.  Then if a child
> > > > > +      // has aChildNode as an ancestor remove that child of aContainer.
> > > > 
> > > > actually this doesn't answer why we can't simply go through children of
> > > > aChildNode
> > > 
> > > What would cause you to believe that the direct children necessarily all
> > > have accessibles?
> > 
> > we used to have accTreeWalker that operated recursively iirc.
> 
> yes, but if you know the parent accessible why bother with tree walker when
> you can just iterate over the kids.

it sounds like new approach is equivalent to the old one while it's not. I'd be good to comment reasons why we have to traverse a parent chain instead

> > > that breaks some text change coalescence logic  
> > 
> > really? I thought we coalesce them ok if they are removed in any direction
> 
> Its been a while but I certainly tried going back to front and had to debug
> test failures because of it.

ok, I don't mind

    
    

    
  
(In reply to alexander :surkov from comment #62)
> (In reply to Trevor Saunders (:tbsaunde) from comment #61)
> > (In reply to alexander :surkov from comment #60)
> > > (In reply to Trevor Saunders (:tbsaunde) from comment #59)
> > > > > @@ +1746,5 @@
> > > > > > +        updateFlags |= UpdateTreeInternal(child, aIsInsert, reorderEvent);
> > > > > > +    } else {
> > > > > > +      // AchildNode may not coorespond to a particular accessible, to handle
> > > > > > +      // this we go through all the children of aContainer.  Then if a child
> > > > > > +      // has aChildNode as an ancestor remove that child of aContainer.
> > > > > 
> > > > > actually this doesn't answer why we can't simply go through children of
> > > > > aChildNode
> > > > 
> > > > What would cause you to believe that the direct children necessarily all
> > > > have accessibles?
> > > 
> > > we used to have accTreeWalker that operated recursively iirc.
> > 
> > yes, but if you know the parent accessible why bother with tree walker when
> > you can just iterate over the kids.
> 
> it sounds like new approach is equivalent to the old one while it's not. I'd
> be good to comment reasons why we have to traverse a parent chain instead

Well, the answer according to this bug is that the content has been removed from the DOM when this code runs.  However approach of looking at ancestors should be faster than scanning the whole tree so I'm not sure the change wouldn't be a good idea without assert.  I guess I can add comment that you shouldn't look at parents of aChildNode or expect its in the DOM.

    
    

    
  
(In reply to Trevor Saunders (:tbsaunde) from comment #63)

> Well, the answer according to this bug is that the content has been removed
> from the DOM when this code runs.  However approach of looking at ancestors
> should be faster than scanning the whole tree so I'm not sure the change
> wouldn't be a good idea without assert.  

while parent traversal should be faster than children traversal in general but it's not necessary in this case.

> I guess I can add comment that you
> shouldn't look at parents of aChildNode or expect its in the DOM.

something more detailed would be great

    
    

    
  


  

        Attachment #756676 -
        Attachment is obsolete: true

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

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

    
    

    
  
Comment on attachment 758722 [details] [diff] [review]
bug 852150 - handle removal of accessibles when reframe root doesn't have an accessible more correctly

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

::: accessible/src/generic/DocAccessible.cpp
@@ +1749,5 @@
> +      // this we go through all the children of aContainer.  Then if a child
> +      // has aChildNode as an ancestor remove that child of aContainer.
> +      // Note that when we are called aChildNode may already have been removed
> +      // from the DOM so we can't expect it to have a parent or what was it's
> +      // parent to have it as a child.

maybe something like:

aChildNode may not correspond to a particular accessible but may contain accessible children. The same time when we are called aChildNode may already have been removed from the DOM so we can't rely on DOM parent-child relations. Thus we shutdown all accessibles that are contained by aChildNode or don't have a connection with containerNode.

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

    
    

    
  
(In reply to alexander :surkov from comment #66)
> Comment on attachment 758722 [details] [diff] [review]
> bug 852150 - handle removal of accessibles when reframe root doesn't have an
> accessible more correctly
> 
> Review of attachment 758722 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/generic/DocAccessible.cpp
> @@ +1749,5 @@
> > +      // this we go through all the children of aContainer.  Then if a child
> > +      // has aChildNode as an ancestor remove that child of aContainer.
> > +      // Note that when we are called aChildNode may already have been removed
> > +      // from the DOM so we can't expect it to have a parent or what was it's
> > +      // parent to have it as a child.
> 
> maybe something like:
> 
> aChildNode may not correspond to a particular accessible but may contain
> accessible children. The same time when we are called aChildNode may already
> have been removed from the DOM so we can't rely on DOM parent-child
> relations. Thus we shutdown all accessibles that are contained by aChildNode
> or don't have a connection with containerNode.

other than covering case of node that isn't connected to either being removed I'm not sure I prefer that phrasing.

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/e56502eddeb8
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24

    
  
Depends on: 881636

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: