Closed Bug 608887 Opened 9 years ago Closed 9 years ago

elements appended outside the body aren't accessible

Categories

(Core :: Disability Access APIs, defect, major)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla2.0b8
Tracking Status
blocking2.0 --- final+

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

spun off the bug 606082
requesting blocking since real web pages do this and this is regression from Firefox 3.6.

there are two problems
1) elements inserted outside the document body aren't accessible
2) we don't create document accessible without body what's wrong because it can be removed and elements can be appended under the document accessible
Severity: normal → major
blocking2.0: --- → ?
(In reply to comment #1)
> 2) we don't create document accessible without body what's wrong because it can
> be removed and elements can be appended under the document accessible

though perhaps this is ok to not create document accessible body but we need to make sure it works without body (in case if it was removed).
Do we have a minimal test case to explain this bug?
see testcase from bug 606082
Assignee: nobody → surkov.alexander
Attached patch patch (obsolete) — Splinter Review
Attachment #491122 - Flags: review?(marco.zehe)
Attachment #491122 - Flags: review?(bzbarsky)
Attachment #491122 - Flags: review?(bolterbugz)
Comment on attachment 491122 [details] [diff] [review]
patch

missed tests, I'll file new one
Attachment #491122 - Attachment is obsolete: true
Attachment #491122 - Flags: review?(marco.zehe)
Attachment #491122 - Flags: review?(bzbarsky)
Attachment #491122 - Flags: review?(bolterbugz)
Attached patch patch2Splinter Review
Attachment #491134 - Flags: review?(marco.zehe)
Attachment #491134 - Flags: review?(bolterbugz)
Status: NEW → ASSIGNED
Comment on attachment 491134 [details] [diff] [review]
patch2

>+  <body>
>+    Hey, I'm a <body> with three links that are not inside me and an input
>+    that's not inside me.
>+  </body>
>+</body>

This last closing tag should be </html>, correct? Or is this part of the testcase? r=me with that fixed/answered.
Attachment #491134 - Flags: review?(marco.zehe) → review+
(In reply to comment #8)
> Comment on attachment 491134 [details] [diff] [review]
> patch2
> 
> >+  <body>
> >+    Hey, I'm a <body> with three links that are not inside me and an input
> >+    that's not inside me.
> >+  </body>
> >+</body>
> 
> This last closing tag should be </html>, correct? 

right.

> Or is this part of the
> testcase? r=me with that fixed/answered.

not a part of the test, thanks for the catch.
Comment on attachment 491134 [details] [diff] [review]
patch2

>diff --git a/accessible/src/base/nsDocAccessible.cpp b/accessible/src/base/nsDocAccessible.cpp
>--- a/accessible/src/base/nsDocAccessible.cpp
>+++ b/accessible/src/base/nsDocAccessible.cpp
>@@ -1447,26 +1447,24 @@ nsDocAccessible::UpdateTree(nsIContent* 
>   nsAccessible* container = nsnull;
>   if (aIsInsert) {
>     container = aContainerNode ?
>       GetAccService()->GetAccessibleOrContainer(aContainerNode, mWeakShell) :
>       this;
> 
>     // The document children were changed; the root content might be affected.
>     if (container == this) {
>+      // If new root content has been inserted then update it.
>       nsIContent* rootContent = nsCoreUtils::GetRoleContent(mDocument);
>+      if (rootContent && rootContent != mContent)
>+        mContent = rootContent;

What if we just set mContent to rootContent (i.e. let it be null sometimes)?

>+      // Update the tree even we don't have root content because elements
>+      // may be inserted under document element while there's no body element
>+      // in HTML case.

Maybe: "Continue to update the tree even if we don't have root content. Elements may be inserted under the document element while there is no body element."

What about the case of an element inserted under the document element while there is a body element? Are they always inserted under the body?

>+void
>+nsDocAccessible::CacheChildren()
>+{
>+  // 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(mWeakShell, mDocument->GetRootElement(),
>+                         GetAllowsAnonChildAccessibles());
>+
>+  nsRefPtr<nsAccessible> child;
>+  while ((child = walker.GetNextChild()) && AppendChild(child));
>+}

Good.
(In reply to comment #10)

> >       nsIContent* rootContent = nsCoreUtils::GetRoleContent(mDocument);
> >+      if (rootContent && rootContent != mContent)
> >+        mContent = rootContent;
> 
> What if we just set mContent to rootContent (i.e. let it be null sometimes)?

if we don't get a crash sometimes then document methods will return e_fail as the accessible is defunct. I'm not sure what's preferable stale state + wrong result or failure.

> What about the case of an element inserted under the document element while
> there is a body element? Are they always inserted under the body?

No, they are inserted under the document element.
Comment on attachment 491134 [details] [diff] [review]
patch2

r=me with one comment change (please)

(In reply to comment #11)
> (In reply to comment #10)
> 
> > >       nsIContent* rootContent = nsCoreUtils::GetRoleContent(mDocument);
> > >+      if (rootContent && rootContent != mContent)
> > >+        mContent = rootContent;
> > 
> > What if we just set mContent to rootContent (i.e. let it be null sometimes)?
> 
> if we don't get a crash sometimes then document methods will return e_fail as
> the accessible is defunct. I'm not sure what's preferable stale state + wrong
> result or failure.

Yes I'm not sure either. I think I would lead to failing, but your way we also avoid a potential (not sure how likely) crash. I think you can leave it as is.

> 
> > What about the case of an element inserted under the document element while
> > there is a body element? Are they always inserted under the body?
> 
> No, they are inserted under the document element.

OK please change:
>+      // Update the tree even we don't have root content because elements
>+      // may be inserted under document element while there's no body element
>+      // in HTML case.

To:
// Update the tree event when we don't root (body) content, because elements may still be inserted under the document element.
Attachment #491134 - Flags: review?(bolterbugz) → review+
(In reply to comment #12)

> OK please change:
> >+      // Update the tree even we don't have root content because elements
> >+      // may be inserted under document element while there's no body element
> >+      // in HTML case.
> 
> To:
> // Update the tree event when we don't root (body) content, because elements
> may still be inserted under the document element.

I don't get why it's better. It doesn't say about non HTML documents and I'm not sure I get what root means here.
(In reply to comment #12)
 
>I think I would lead to failing, 

I don't think fail is expected here. No body means the document might not have ARIA stuffs, nothing else.
(In reply to comment #14)
> (In reply to comment #12)
> 
> >I think I would lead to failing, 
> 
> I don't think fail is expected here. No body means the document might not have
> ARIA stuffs, nothing else.

Oh, then I misunderstood your point. I think I'm mixing up root, document, body concepts in this discussion.

(In reply to comment #13)
> (In reply to comment #12)
> 
> > OK please change:
> > >+      // Update the tree even we don't have root content because elements
> > >+      // may be inserted under document element while there's no body element
> > >+      // in HTML case.

I don't clearly understand this comment as it is, probably because it combines a number of points in a way I'm not used to. Is the following correct?

"We don't bail when we don't have root content because elements may still be inserted under the document element. There might not even be a body element."
(In reply to comment #15)

> I don't clearly understand this comment as it is, probably because it combines
> a number of points in a way I'm not used to. Is the following correct?
> 
> "We don't bail when we don't have root content because elements may still be
> inserted under the document element. There might not even be a body element."

Correct, though it's about HTML only and context dependent. If add "for example" then does it make my comment clearer:

Update the tree even we don't have root content because for example elements may be inserted under document element while there's no body element in HTML case.
(In reply to comment #16)
> (In reply to comment #15)
> 
> > I don't clearly understand this comment as it is, probably because it combines
> > a number of points in a way I'm not used to. Is the following correct?
> > 
> > "We don't bail when we don't have root content because elements may still be
> > inserted under the document element. There might not even be a body element."
> 
> Correct, though it's about HTML only and context dependent. If add "for
> example" then does it make my comment clearer:
> 
> Update the tree even we don't have root content because for example elements
> may be inserted under document element while there's no body element in HTML
> case.

The way I read this, it implies that the elements can not be inserted under the document element when there is a body element. Is that intended?
(In reply to comment #17)

> > Update the tree even we don't have root content because for example elements
> > may be inserted under document element while there's no body element in HTML
> > case.
> 
> The way I read this, it implies that the elements can not be inserted under the
> document element when there is a body element. Is that intended?

No, just reread it and I miss what may make you think this way. What's wrong in wording?
As per IRC, please make "while", "even while" :)
landed on 2.0 - http://hg.mozilla.org/mozilla-central/rev/4fdda29db1f4
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
You need to log in before you can comment on or make changes to this bug.