Closed Bug 606082 Opened 14 years ago Closed 14 years ago

update accessible tree when root element is changed

Categories

(Core :: Disability Access APIs, defect)

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

Tracking

()

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

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(2 files, 1 obsolete file)

Testcase showing some assumptions are just wrong
859 bytes, text/html
Details
patch2
22.15 KB, patch
davidb
: review+
bzbarsky
: review+
Details | Diff | Splinter Review 
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.
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... ;)
Blocks: treeupdatea11y
No longer blocks: treea11y
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
Attached patch patch (obsolete) — — Splinter Review 
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 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+
(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.
(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
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.
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?
(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>?
(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?
(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...
(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.
(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?
> 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...
(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).
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.
Attached patch patch2 — — Splinter Review 

        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+

    
    

    
  
(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.

    
    

    
  
(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

    
    

    
  
(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+

    
    

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

    
  
Blocks: 612830

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: