Closed
Bug 608887
Opened 15 years ago
Closed 15 years ago
elements appended outside the body aren't accessible
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
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)
|
13.03 KB,
patch
|
davidb
:
review+
MarcoZ
:
review+
|
Details | Diff | Splinter Review |
spun off the bug 606082
|
Assignee | |
Comment 1•15 years ago
|
||
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: --- → ?
|
|
Assignee | |
Comment 2•15 years ago
|
||
(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).
|
||
Comment 3•15 years ago
|
||
Do we have a minimal test case to explain this bug?
|
|
Assignee | |
Comment 4•15 years ago
|
||
see testcase from bug 606082
|
|
||
Updated•15 years ago
|
blocking2.0: ? → final+
|
|
||
Updated•15 years ago
|
Assignee: nobody → surkov.alexander
|
|
Assignee | |
Comment 5•15 years ago
|
||
Attachment #491122 -
Flags: review?(marco.zehe)
Attachment #491122 -
Flags: review?(bzbarsky)
Attachment #491122 -
Flags: review?(bolterbugz)
|
|
Assignee | |
Comment 6•15 years ago
|
||
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)
|
|
Assignee | |
Comment 7•15 years ago
|
||
Attachment #491134 -
Flags: review?(marco.zehe)
Attachment #491134 -
Flags: review?(bolterbugz)
|
|
Assignee | |
Updated•15 years ago
|
Status: NEW → ASSIGNED
|
||
Comment 8•15 years ago
|
||
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+
|
|
Assignee | |
Comment 9•15 years ago
|
||
(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 10•15 years ago
|
||
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.
|
|
Assignee | |
Comment 11•15 years ago
|
||
(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 12•15 years ago
|
||
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+
|
|
Assignee | |
Comment 13•15 years ago
|
||
(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.
|
|
Assignee | |
Comment 14•15 years ago
|
||
(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.
|
|
||
Comment 15•15 years ago
|
||
(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."
|
|
Assignee | |
Comment 16•15 years ago
|
||
(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.
|
|
||
Comment 17•15 years ago
|
||
(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?
|
|
Assignee | |
Comment 18•15 years ago
|
||
(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?
|
|
||
Comment 19•15 years ago
|
||
As per IRC, please make "while", "even while" :)
|
|
Assignee | |
Comment 20•15 years ago
|
||
landed on 2.0 - http://hg.mozilla.org/mozilla-central/rev/4fdda29db1f4
Status: ASSIGNED → RESOLVED
Closed: 15 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.
Description
•