Closed Bug 413778 Opened 17 years ago Closed 17 years ago

Extra nsDocAccessible quickly created and destroyed whenever new tab is opened

Categories

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

Product:
Core
Component:
Disability Access APIs
Platform:
All
Windows XP
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: aaronlev, Assigned: aaronlev)

References

Details

(Keywords: access)

Attachments

(1 file, 4 obsolete files)

Don't create doc accessibles for busy about:blank docs -- I recommend testing this together with the latest patch in bug 417249
2.58 KB, patch
ginnchen+exoracle
: review+
Details | Diff | Splinter Review 
This may be causing performance and crash problems.
The code in nsWindow.cpp is for fastback but apparently isn't necessary anymore. I tested and pages still load after fastback/forward with both JAWS and Window-Eyes. The original bug for that was bug 330624.
Attachment #298850 - Flags: superreview?(emaijala)
Attachment #298850 - Flags: review?(ginn.chen)
Attachment #298850 - Flags: superreview?(neil)
Attachment #298850 - Flags: superreview?(emaijala)
Attachment #298850 - Flags: review?(emaijala)
Aaron,

if you already return NS_OK here
+  if (eventType.EqualsLiteral("focus")) {
...
+    return NS_OK;
   }

How can it goes into
   if (eventType.EqualsLiteral("focus")) {
     // Keep a reference to the target node. We might want to change
     // it to the individual radio button or selected item, and send
     // the focus event to that.
     nsCOMPtr<nsIDOMNode> focusedItem(aTargetNode);

So the result is all focus events are gone.
Comment on attachment 298850 [details] [diff] [review]
Focus is too early to create nsDocAccessible because that doc may be an empty placeholder doc that's about to be destroyed and replaced with another

Note: this superreview does not make any claims for correctness of the fix.
Attachment #298850 - Flags: superreview?(neil) → superreview+
Ginn, right -- the return NS_OK was supposed to be at the end of the previous |if| block. I'll fix that.
Attached patch Address Ginn's comments (obsolete) — — Splinter Review 

        Attachment #298850 -
        Attachment is obsolete: true

        Attachment #298930 -
        Flags: review?(ginn.chen)

        Attachment #298930 -
        Flags: review?(emaijala)

        Attachment #298850 -
        Flags: review?(ginn.chen)

        Attachment #298850 -
        Flags: review?(emaijala)

    
    

    
  
With this version of patch, the problem is still there, at least on Unix.

Aaron, how did you verify the fix?

    
    

    
  
Ginn, I verified on Windows. Can you figure out what's going on with Linux for us? I think we should review and get this Windows fix checked in ASAP though because it could be causing some of the problems Marco is seeing. The Linux issue can be a follow-up bug.

    
    

    
  
Comment on attachment 298930 [details] [diff] [review]
Address Ginn's comments

Again, I realize this first fix only works for Windows, but I'd like to get it in ASAP for testing. We're trying to track down some very difficult bugs that this could affect (e.g. bug 405951).

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

    
    

    
  
Comment on attachment 298930 [details] [diff] [review]
Address Ginn's comments

I'm all for trying this.

        Attachment #298930 -
        Flags: review?(emaijala) → review+

    
  
Flags: blocking1.9?

    
    

    
  
Comment on attachment 298930 [details] [diff] [review]
Address Ginn's comments

><html><body><pre style="word-wrap: break-word; white-space: pre-wrap;">Index: widget/src/windows/nsWindow.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/widget/src/windows/nsWindow.cpp,v
>retrieving revision 3.722
>diff -p -u -5 -r3.722 nsWindow.cpp
>--- widget/src/windows/nsWindow.cpp	18 Jan 2008 18:39:52 -0000	3.722
>+++ widget/src/windows/nsWindow.cpp	24 Jan 2008 14:05:19 -0000
>@@ -4627,17 +4627,10 @@ PRBool nsWindow::ProcessMessage(UINT msg
>         gJustGotActivate = PR_FALSE;
>         gJustGotDeactivate = PR_FALSE;
>         result = DispatchFocus(NS_ACTIVATE, PR_TRUE);
>       }
> 
>-#ifdef ACCESSIBILITY
>-      if (nsWindow::gIsAccessibilityOn) {
>-        // Create it for the first time so that it can start firing events
>-        nsCOMPtr<nsIAccessible> rootAccessible = GetRootAccessible();
>-      }
>-#endif
>-

we do not need this because when accessibility is enabled then rool accessible is created automatically, right? Could you point me code where this happens?

    
    

    
  
It happens because we're listening for DOM focus events and the new doc gets focused.

    
    

    
  
(In reply to comment #11)
> It happens because we're listening for DOM focus events and the new doc gets
> focused.
> 

sorry?

    
    

    
  
> root accessible is created automatically, right? Could you point me code where this happens?

It's confusing to talk about this because the method names are confusing:
nsWindow::CreateRootAccessible should be called CreateDocOrRootAccessible
and the same thing for
nsAccssibilityService::CreateRootAccessible()
We should possible rename those, because if it's the top level window a root accessible is created, otherwise a doc accessible is created. The name made more sense when the a11y code was first being developed.

In any case, when you press Ctrl+N or something and a new window is created, here is where the root accessible is created. nsAccessibilityService::OnStateChange() is called when the chrome is finished loading. That then asks for an accessible for the DOM doc which will create a root or doc accessible depending on whether it's a top level xul doc or just a content doc.

If you just create a new content doc (e.g. Ctrl+T or typing in a URL), an accessible will be created in the handling of "focus" in nsRootAccessible::HandleEvent(). Or it will get created in the process of firing a doc load event.

    
    

    
  
Marco, can you test this patch? Here are try server builds for Firefox:
https://build.mozilla.org/tryserver-builds/2008-01-27_09:33-aaronleventhal@moonset.net-RedundantDocFocus413778/

We should test its effect on Tbird as well but you'll have to build that yourself.

    
  
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2

    
    

    
  
Comment on attachment 298930 [details] [diff] [review]
Address Ginn's comments

r=me

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

    
  

        Attachment #298930 -
        Flags: review?(ginn.chen)

    
  
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED

    
    

    
  
Aaron, the patch is good to checkin, but it didn't fix the problem on either Windows or Unix.

+  if (aTargetNode == mDOMNode && mDOMNode != gLastFocusedNode && eventType.EqualsLiteral("focus")) {
[snip]
+    return NS_OK;
   }
 
   nsCOMPtr<nsIAccessible> accessible;
   accService->GetAccessibleInShell(aTargetNode, eventShell,
                                    getter_AddRefs(accessible));

You're trying to stop the creation of nsDocAccessible, in this case, aTargetNode is nsHTMLDocument,
but mDOMNode of nsRootAccessible is nsXULDocument.
I don't think the patch would work.


Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
Hi Ginn. My testing showed that it works on Windows and requires that change to nsRootAccessible. Did you test on Windows and find something different?

Fine to keep it open for Linux. Will you help look for a solution there?

    
    

    
  
I tested it on Windows, and got the same result as on Linux.

You can just add printf in nsDocAccessible::nsDocAccessible, nsDocAccessible::~nsDocAccessible.
Make a11y on.
Press Ctrl+T will give 2 nsDocAccessible::nsDocAccessbile, and 1 nsDocAccessible::~nsDocAccessible between them.

    
    

    
  
Backed out this fix since it didn't work and caused bug 415919.

    
    

    
  


  

        Attachment #298930 -
        Attachment is obsolete: true

    
    

    
  
This is not fixed by the recent checkin to bug 417249 but I believe we're closer and it should be easier to deal with this bug.

    
    

    
  


  
The idea here is that about:blank isn't a document anyway. We don't need to fire doc load events for it.

However, when opening a new tab it doesn't fire the doc load start event. This doesn't appear to be a problem. And when you open a new tab with a blank page or load about:blank, you end up in the URL bar anyway. The doc accessible will be created when the about:blank doc gets focused.

We already check for about:blank in nsAccessibilityService::OnStateChange() however that check is only working for end-load events. For the start load we have to check the URL after the timeout.

        Attachment #305709 -
        Flags: review?(ginn.chen)

    
    

    
  
Comment on attachment 305709 [details] [diff] [review]
Needs testing for ill effects. Don't fire doc load events for about:blank

Not working.

nsDocAccessible is still created by "focus" event or EVENT_INTERNAL_LOAD.

        Attachment #305709 -
        Flags: review?(ginn.chen) → review-

    
    

    
  
Ginn, are you sure that's the extra doc? For me it only creates the real document from those.

    
    

    
  
Another question: are you loading the tab in the foreground or background?

    
    

    
  
(In reply to comment #24)
> Ginn, are you sure that's the extra doc? For me it only creates the real
> document from those.
> 

I think so, it get destroyed immediately.

> Another question: are you loading the tab in the foreground or background?

Foreground. Press Ctrl+T or click "open in a new tab" for bookmarks toolbar item.
The doc accessible is created for focus event.



    
    

    
  
Ginn your tabs must open in the foreground. Mine open in the background and don't get focus. I'll try with the foreground tab option.

    
    

    
  
Yes, the checking you add is inside EVENT_DOCUMENT_LOAD_START.
It doesn't work for "focus" event and INTERNAL_LOAD event.

The INTERNAL_LOAD event is harmless, the target must be a real document.

    
    

    
  
I have an idea -- patch coming up.

    
    

    
  
Comment on attachment 305745 [details] [diff] [review]
Don't create doc accessibles for busy about:blank docs -- I recommend testing this together with the latest patch in bug 417249

It works.

        Attachment #305745 -
        Flags: review?(ginn.chen) → review+

    
  
Status: REOPENED → RESOLVED
Closed: 17 years ago → 17 years ago
Resolution: --- → FIXED

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: