update accessible tree when root element is changed

RESOLVED FIXED in mozilla2.0b8

Status

()

Core
Disability Access APIs
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks: 1 bug, {access})

unspecified
mozilla2.0b8
access
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

8 years ago
follow up from bug 570275.

When ContentRangeInserted/ContentRemoved are fired with null aContainer then we need to update accessible tree of the document, as well we need to update mContent of document accessible.
(Assignee)

Comment 1

8 years ago
bug 570275 comment #142:

> Currently we were under assumption root element or body isn't changed

Yeah, that's a bad assumption.  <body> will change on document.open, and
document.removeChild(document.documentElement) works just fine... ;)
(Assignee)

Updated

8 years ago
Blocks: 572951
No longer blocks: 540399
(Assignee)

Comment 2

8 years ago
request blocking, we should be protected from this, I don't recall if I've seen somebody does this in the web but that's quite possible.
blocking2.0: --- → ?
Yep. Blocking final.
blocking2.0: ? → final+
Assignee: nobody → surkov.alexander
(Assignee)

Comment 4

8 years ago
Created attachment 486003 [details] [diff] [review]
patch

1) Do not null root content when root content goes away because I think we rely it's not null when accessible is not defuct. Instead of this expose STATE_STALE on document when root content is out of date
2) Fix GetDocumentNode in the case when root content is out of date
3) Update root content when it's changed
4) Add notification to nsCSSFrameConstructor::ContentRangeRemoved when document element is inserted
Attachment #486003 - Flags: review?(marco.zehe)
Attachment #486003 - Flags: review?(bzbarsky)
Attachment #486003 - Flags: review?(bolterbugz)

Comment 5

8 years ago
Comment on attachment 486003 [details] [diff] [review]
patch

r=me for the tests. Marvelous! :)

One thing that struck me:
>-  nsIDocument* GetDocumentNode() const
>+  virtual nsIDocument* GetDocumentNode() const
>     { return mContent ? mContent->GetOwnerDoc() : nsnull; }

Wasn't there something about virtual methods no longer being inlineable?
Attachment #486003 - Flags: review?(marco.zehe) → review+
(Assignee)

Comment 6

8 years ago
(In reply to comment #5)
> Comment on attachment 486003 [details] [diff] [review]
> patch
> 
> r=me for the tests. Marvelous! :)

thank you!

> One thing that struck me:
> >-  nsIDocument* GetDocumentNode() const
> >+  virtual nsIDocument* GetDocumentNode() const
> >     { return mContent ? mContent->GetOwnerDoc() : nsnull; }
> 
> Wasn't there something about virtual methods no longer being inlineable?

yes, they aren't, perhaps I should move them into cpp, at least it's easier to find implementations by text search.
(Assignee)

Comment 7

8 years ago
(In reply to comment #6)
> (In reply to comment #5)

> yes, they aren't, perhaps I should move them into cpp, at least it's easier to
> find implementations by text search.

ok, I fixed locally

Comment 9

8 years ago
I just tried this try-server build with the steps from bug 606207 Comment#1. The result is that the list now reappears in NVDA's virtual buffer, as it used to in Firefox 3.6.x. But after arrowing down into it, on the first or at latest, the second link, NVDA's virtual buffer disappears. Tabbing and shift tabbing still results in speech, so the "Mention", "follow" etc. links get spoken. But where the button should be, an unnamed "frame" is being announced. Shutting down and restarting NVDA does not bring back virtual buffer functionality. Object navigation (NVDA Key+NUMPAD 8, 2, 4 and 6) is completely broken. It appears as if there's no accessibles to work with underneath the main application frame any longer.

In essence, this appears similar to some of the things described in bug 599814, but much much worse in that the state is not recoverable without shutting down Firefox.
(Assignee)

Comment 10

8 years ago
Can't reproduce on my local build, though I have patch from bug 607882 applied.
Comment on attachment 486003 [details] [diff] [review]
patch

The mix of ToString + conversion and direct ToUTF8String in the debug code is weird, but ok.

The logic in nsDocAccessible::UpdateTree doesn't make much sense to me.  Won't it fail if a <body> is inserted into an existing <html> that had no <body>?

Where does the mContent member of nsDocAccessible actually get set, and to what, before this patch?
(Assignee)

Comment 12

8 years ago
(In reply to comment #11)
> Comment on attachment 486003 [details] [diff] [review]
> patch
> 
> The mix of ToString + conversion and direct ToUTF8String in the debug code is
> weird, but ok.

how can I improve this?

> The logic in nsDocAccessible::UpdateTree doesn't make much sense to me.

any details please?

>  Won't
> it fail if a <body> is inserted into an existing <html> that had no <body>?

it shouldn't. If we had a document without a body then we didn't created document accessible for it. But when body is inserted then we do this. If we had a document accessible (for example, body was removed after we created a document accessible). So, in either way we have document accessible that may have obsolete or correct mContent that refer to body.

Container accessible is document itself, we have a body (that's a nsCoreUtils::GetRoleContent()), we update mContent and then fall into usual processing, i.e. look into subtree, find accessible nodes, create accessibles, fire show events. 

> Where does the mContent member of nsDocAccessible actually get set, and to
> what, before this patch?

It's set when nsDocAccessible is created (http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsAccDocManager.cpp#459) or in UpdateTree() if body (or document element in the case of non HTML documents) is changed.
> how can I improve this?

Just use ToUTF8String if you want UTF8 strings?

> any details please?

Uh... the very next sentence had the details!

> If we had a document accessible (for example, body was removed after we
> created a document accessible).

Then what?

Simple testcase:  Take a document, remove the <body>, put a new <body> in.  What code will set mContent on the document accessible to the new <body>?
(Assignee)

Comment 14

8 years ago
(In reply to comment #13)
> > how can I improve this?
> 
> Just use ToUTF8String if you want UTF8 strings?
> 
> > any details please?
> 
> Uh... the very next sentence had the details!
> 
> > If we had a document accessible (for example, body was removed after we
> > created a document accessible).
> 
> Then what?

sorry, it should be "otherwise"

> Simple testcase:  Take a document, remove the <body>, put a new <body> in. 
> What code will set mContent on the document accessible to the new <body>?

1) we get notification that body was inserted
2) get container accessible ('container' variable), it's 'this' document accessible
3) get root content, it's new body
4) it's not null and different from mContent (mContent points to old body), update mContent

sounds correct?
> 2) get container accessible ('container' variable), it's 'this

Oh, so <html> gets no accessible?  Why not?
(Assignee)

Comment 16

8 years ago
(In reply to comment #15)
> > 2) get container accessible ('container' variable), it's 'this
> 
> Oh, so <html> gets no accessible?  Why not?

if you about technical reason, then I don't recall actually. But we never created an accessible for <HTML> element, it's not interesting for accessibility since it's not part of UI.
> it's not interesting for accessibility since it's not part of UI

I'm not sure what you mean.  It's possible to have tabbable content inside <html> and outside <body>, right?  I'd assume tabindex="1" on <html> would make <html> itself tabbable too, modulo bugs...
(Assignee)

Comment 18

8 years ago
(In reply to comment #17)
> > it's not interesting for accessibility since it's not part of UI
> 
> I'm not sure what you mean.  It's possible to have tabbable content inside
> <html> and outside <body>, right?  I'd assume tabindex="1" on <html> would make
> <html> itself tabbable too, modulo bugs...

I'm not sure how we handle this if we do, perhaps you're right and we create an accessible in this case. I will check. A11y is not very well protected from crazy cases. Thank you for bringing attention to this.
(Assignee)

Comment 19

8 years ago
(In reply to comment #17)
> > it's not interesting for accessibility since it's not part of UI
> 
>  It's possible to have tabbable content inside
> <html> and outside <body>, right?

Could you give an example? I put HTML input outside of body and it was placed in body in DOM.

>  I'd assume tabindex="1" on <html> would make
> <html> itself tabbable too, modulo bugs...

Yes, it becomes tab navigable but accessible isn't created for it.

> > it's not interesting for accessibility since it's not part of UI
> I'm not sure what you mean.

I think controls are interesting for ATs like buttons, links, lists and etc, rich text is important. HTML document element is not like these. I don't see why AT would need it.
 
If HTML document element is focusable then a11y focus event is missed, so that tab makes nothing for AT user and that's a problem of course. Not huge since AT doesn't miss anything important on the page but might be annoying. Perhaps this can be considered as web page bug.

Jamie, what do you think?
Created attachment 487339 [details]
Testcase showing some assumptions are just wrong

> Could you give an example?

Here you go.  Note that we screw up tabbing to <link> too, but it's quite clickable.
And before you say this is contrived, I've debugged quite a number of webpages that stick stuff in using that documentElement.appendChild() pattern...
(Assignee)

Comment 22

8 years ago
(In reply to comment #20)
 
> > Could you give an example?
> 
> Here you go.  Note that we screw up tabbing to <link> too, but it's quite
> clickable.

all of these aren't accessible the meantime (thank you for testcase)

> And before you say this is contrived, I've debugged quite a number of webpages
> that stick stuff in using that documentElement.appendChild() pattern...

and therefore that makes me scary

> Created attachment 487339 [details]
> Testcase showing some assumptions are just wrong
(In reply to comment #21)

These accessibles should be placed under document accessible. What kind of assumptions in the patch are wrong?
I'm not saying anything about the patch; just that "all stuff that needs to be accessible is inside the <body>" is not true.

I think the code in UpdateTree could use a comment explaining the assumptions it's actually making (and if we don't have a test for those assumptions we should add one).
(Assignee)

Comment 24

8 years ago
Ok, that's wrong assumption that all accessible stuffs must be in body and therefore it's wrong assumption the document without body shouldn't be accessible. I think I should add XXX comment into UpdateTree method and file a bug for this.

That's not a bug for Firefox 3.6 and it's pretty serious problem we should address in Firefox 4 since wild web pages relies on this.

Do you need new patch with updated comment for review?
I think the comment would help me figure out whether the code is correct as-is, yes.
(Assignee)

Comment 26

8 years ago
Created attachment 487504 [details] [diff] [review]
patch2
Attachment #486003 - Attachment is obsolete: true
Attachment #487504 - Flags: review?(bzbarsky)
Attachment #487504 - Flags: review?(bolterbugz)
Attachment #486003 - Flags: review?(bzbarsky)
Attachment #486003 - Flags: review?(bolterbugz)
Comment on attachment 487504 [details] [diff] [review]
patch2

r=me thanks! Notes:

OK so we update the document's tree in ContentRangeInserted and ContentRemoved even when there is no container. This is the case of inserting or removing a document right? How about a little comment to say we don't check for a container because we would miss these cases? I realize you have tests, but comments are sometimes time savers.

>+#ifdef DEBUG_CONTENTMUTATION

BTW I much prefer these specific preprocessor blocks. Thanks. I don't really like us using the general #ifdef DEBUG actually.

Maybe you can ask Marco to scrutinize the tests.
Attachment #487504 - Flags: review?(bolterbugz) → review+
(Assignee)

Comment 28

8 years ago
(In reply to comment #27)
 
> OK so we update the document's tree in ContentRangeInserted and ContentRemoved
> even when there is no container. This is the case of inserting or removing a
> document right?

No, a document element.

> How about a little comment to say we don't check for a
> container because we would miss these cases?

sorry, I missed the suggestion, can you rephrase it?

> >+#ifdef DEBUG_CONTENTMUTATION
> 
> BTW I much prefer these specific preprocessor blocks. Thanks. I don't really
> like us using the general #ifdef DEBUG actually.

Yes, me too.

> Maybe you can ask Marco to scrutinize the tests.

I thought he did this, no?
(In reply to comment #28)
> (In reply to comment #27)
> > How about a little comment to say we don't check for a
> > container because we would miss these cases?
> 
> sorry, I missed the suggestion, can you rephrase it?

// We don't bail if the container is null since we need to handle the case where we [insert | remove] a document element.

> > Maybe you can ask Marco to scrutinize the tests.
> 
> I thought he did this, no?

You are correct! Great.
(Assignee)

Comment 30

8 years ago
(In reply to comment #29)

> // We don't bail if the container is null since we need to handle the case
> where we [insert | remove] a document element.

ok, thanks
(Assignee)

Comment 31

8 years ago
(In reply to comment #29)
> // We don't bail if the container is null since we need to handle the case
> where we [insert | remove] a document element.

the comment

// Update the whole tree of this document accessible when the container is
  // null (document element is inserted or removed).

after first existing comment of the method sounds good?
Comment on attachment 487504 [details] [diff] [review]
patch2

> +    // The document children were changed, the root content might be affected.

';', not ','.  r=me.
Attachment #487504 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 34

8 years ago
landed on 2.0 - http://hg.mozilla.org/mozilla-central/rev/b6bae363168f with Boris's nit fixed
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
(Assignee)

Updated

8 years ago
Blocks: 612830
You need to log in before you can comment on or make changes to this bug.