Last Comment Bug 739524 - replace TreeViewChanged DOM event on direct call from XUL tree
: replace TreeViewChanged DOM event on direct call from XUL tree
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: Mark Capella [:capella]
:
Mentors:
Depends on:
Blocks: a11yperf treeupdatea11y 740725 740750
  Show dependency treegraph
 
Reported: 2012-03-26 23:01 PDT by alexander :surkov
Modified: 2012-04-19 18:40 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) - very rough (5.28 KB, patch)
2012-03-29 08:27 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v2) (5.71 KB, patch)
2012-03-30 02:23 PDT, Mark Capella [:capella]
surkov.alexander: feedback+
Details | Diff | Splinter Review
Patch (v3) (8.93 KB, patch)
2012-03-30 04:42 PDT, Mark Capella [:capella]
surkov.alexander: feedback+
Details | Diff | Splinter Review
Patch (v4) (8.85 KB, patch)
2012-03-30 04:48 PDT, Mark Capella [:capella]
roc: review+
surkov.alexander: feedback+
Details | Diff | Splinter Review
Patch (v5) (9.21 KB, patch)
2012-04-01 05:48 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
patch( v6) (11.21 KB, patch)
2012-04-01 22:22 PDT, Mark Capella [:capella]
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review
Patch (v7) (11.34 KB, patch)
2012-04-02 01:27 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review

Description alexander :surkov 2012-03-26 23:01:48 PDT
Currently we listen TreeViewChanged DOM event in nsRootAccessible and do nsXULTreeAccessible::TreeViewChanged(). We should replace DOM event on direct notification (http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp#522) for example add nsAccessibilityService::TreeViewChanged().

after that we can use raw pointer to keep nsITreeView instance (mTreeView member in all XUL tree related accessible classes).

benefits:
we get perf win because of integration since
1) no DOM events which is not most performant thing in the world
2) no cycle collection between tree implementation and accessible classes
Comment 1 Mark Capella [:capella] 2012-03-29 08:27:32 PDT
Created attachment 610556 [details] [diff] [review]
Patch (v1) - very rough

I think this attached is roughly the framework you're asking for, though I know it doesn't work (mochitest tree tests fail).

You want to have nsTreeBodyFrame call TreeViewChange() through a new nsAccessibility service, avoiding firing an event which is captured by nsRootAccessible and is then used to make the TreeViewChange() call.
Comment 2 alexander :surkov 2012-03-29 09:02:11 PDT
Comment on attachment 610556 [details] [diff] [review]
Patch (v1) - very rough

Review of attachment 610556 [details] [diff] [review]:
-----------------------------------------------------------------

ok, cool, thank you for doing this. you can do a second part (use nsITreeView* instead of nsRefPtr<nsITreeView>) as followup, up to you

::: accessible/src/base/nsAccessibilityService.cpp
@@ +598,5 @@
>  
>  void
> +nsAccessibilityService::TreeViewChanged(nsIContent* aContent)
> +{
> +   nsRefPtr<nsXULTreeAccessible> treeAcc;

I guess wrong indentation? should be two spaces

@@ +599,5 @@
>  void
> +nsAccessibilityService::TreeViewChanged(nsIContent* aContent)
> +{
> +   nsRefPtr<nsXULTreeAccessible> treeAcc;
> +   treeAcc = do_QueryObject(aContent);

nsIContent doesn't implement nsXULTreeAccessible interface :)
you need to get a document accessible and then get an accessible for the given content and then do do_QueryObject on that accessible.

and please file a bug to add downcasting to nsXULTreeAccessible (see bug 636945 as example)

::: accessible/src/base/nsAccessibilityService.h
@@ +151,5 @@
>                                nsIContent* aChild);
>  
>    virtual void UpdateText(nsIPresShell* aPresShell, nsIContent* aContent);
>  
> +  virtual void TreeViewChanged(nsIContent* aContent);

no virtual is needed
and please add a comment

::: layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp
@@ +101,5 @@
>  #include "nsDisplayList.h"
>  #include "nsTreeBoxObject.h"
>  #include "nsRenderingContext.h"
>  #include "nsIScriptableRegion.h"
> +#include "nsAccessibilityService.h"

#ifdef ACCESSIBILITY

@@ +519,5 @@
>    Invalidate();
>   
>    nsIContent *treeContent = GetBaseElement();
>    if (treeContent) {
> +    nsAccessibilityService* accService = nsIPresShell::AccService();

#ifdef ACCESSIBILITY too

@@ -518,5 @@
>    Invalidate();
>   
>    nsIContent *treeContent = GetBaseElement();
>    if (treeContent) {
> -    FireDOMEvent(NS_LITERAL_STRING("TreeViewChanged"), treeContent);

I wouldn't remove it. Even if it was introduced for accessibility needs then it's hard to say who else depends on it now

but yes, you can remove TreeViewChanged events testing from a11y mochitests

@@ +521,5 @@
>    nsIContent *treeContent = GetBaseElement();
>    if (treeContent) {
> +    nsAccessibilityService* accService = nsIPresShell::AccService();
> +    if (accService)
> +        accService->TreeViewChanged(treeContent);

wrong indentation I guess
Comment 3 Mark Capella [:capella] 2012-03-30 02:23:39 PDT
Created attachment 610839 [details] [diff] [review]
Patch (v2)

New version ... added back firing TreeViewChanged events, this allowed the mochitest tree tests to pass without modifications ...
Comment 4 alexander :surkov 2012-03-30 02:45:45 PDT
Comment on attachment 610839 [details] [diff] [review]
Patch (v2)

Review of attachment 610839 [details] [diff] [review]:
-----------------------------------------------------------------

looks ok, you still need to remove TreeViewChanged part from a11y mochitest (http://mxr.mozilla.org/mozilla-central/search?string=TreeViewChanged&find=accessible)

::: accessible/src/base/nsAccessibilityService.cpp
@@ +611,5 @@
> +      nsRefPtr<nsXULTreeAccessible> treeAcc = do_QueryObject(accessible);
> +      if (treeAcc) 
> +        treeAcc->TreeViewChanged();
> +    }
> +  }

nit: I'd say you don't need all these comments since the code is clear enough

::: accessible/src/base/nsAccessibilityService.h
@@ +152,5 @@
>  
>    virtual void UpdateText(nsIPresShell* aPresShell, nsIContent* aContent);
>  
>    /**
> +   * Update the TreeView.

perhaps: Update XUL:tree accessible tree when treeview is changed.
Comment 5 Mark Capella [:capella] 2012-03-30 04:42:35 PDT
Created attachment 610851 [details] [diff] [review]
Patch (v3)
Comment 6 alexander :surkov 2012-03-30 04:44:16 PDT
Comment on attachment 610851 [details] [diff] [review]
Patch (v3)

Review of attachment 610851 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/tests/mochitest/events/test_tree.xul
@@ +10,1 @@
>  -->

please remove this section at all since it's covered by bug numbers below
Comment 7 Mark Capella [:capella] 2012-03-30 04:48:42 PDT
Created attachment 610853 [details] [diff] [review]
Patch (v4)
Comment 8 alexander :surkov 2012-03-30 05:00:19 PDT
Comment on attachment 610853 [details] [diff] [review]
Patch (v4)

Mark, you don't need to rerequest neither review nor feedback for trivial changes.
Comment 9 Trevor Saunders (:tbsaunde) 2012-03-30 11:45:18 PDT
Comment on attachment 610853 [details] [diff] [review]
Patch (v4)

this looks fine

>+    nsAccessibilityService* accService = nsIPresShell::AccService();
>+    if (accService)
>+      accService->TreeViewChanged(PresContext()->GetPresShell(), treeContent);

so, we know  what the tree view is here, and UpdateTree() will want to  want to find that out, it seems like it would be better to just pass it here than to call GetTreeView()

I suspect we may already have marked the accessibles as defunct by that point, but the tree view obviously changes / the ref gets dropped when the  tree body frame is destroyed.
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-30 13:53:14 PDT
Comment on attachment 610853 [details] [diff] [review]
Patch (v4)

Review of attachment 610853 [details] [diff] [review]:
-----------------------------------------------------------------

It doesn't make a difference to runtime behavior, but it's a little more clear if you run the accessibility code before FireDOMEvent, since it will happen before that event runs (and needs to).

With that, r+ on the layout part
Comment 11 Mark Capella [:capella] 2012-04-01 05:48:11 PDT
Created attachment 611268 [details] [diff] [review]
Patch (v5)

Posting for status check ... 

1) So we now fire the event and call the accessibility service to do the TreeUpdate before we invalidate the tree ...

2) I've currently got the accessibility service coded as Alex suggested in comment#2 

>>you need to get a document accessible and then get an accessible for 
>>the given content and then do do_QueryObject on that accessible.

Trev, you'd rather I pass the nsITreeView* mTree instead of the parms I'm currently passing?
Comment 12 Trevor Saunders (:tbsaunde) 2012-04-01 11:45:00 PDT
> 1) So we now fire the event and call the accessibility service to do the
> TreeUpdate before we invalidate the tree ...

na, I think roc meant you should only change when a11y is notified not when the DOM event is fired.

> >>you need to get a document accessible and then get an accessible for 
> >>the given content and then do do_QueryObject on that accessible.
> 
> Trev, you'd rather I pass the nsITreeView* mTree instead of the parms I'm
> currently passing?

na, just add a third argument that is the treeview.
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-04-01 20:47:39 PDT
Yes, I just meant you to swap the order of the "FireDOMEvent" and "TreeViewChanged" calls.
Comment 14 Mark Capella [:capella] 2012-04-01 22:22:42 PDT
Created attachment 611350 [details] [diff] [review]
patch( v6)

Order swapped, and mView is passed all the way down to nsXULTreeAccessible.cpp to avoid the call to GetView().
Comment 15 Trevor Saunders (:tbsaunde) 2012-04-01 23:46:46 PDT
Comment on attachment 611350 [details] [diff] [review]
patch( v6)

>+nsAccessibilityService::TreeViewChanged(nsIPresShell* aPresShell, nsIContent* aContent, nsITreeView* aView)

over 80 chars.

> #include "mozilla/a11y/FocusManager.h"
> 
>+#include "nsITreeView.h"

forward declaration isn't enough?

>   /**
>+   * Update XUL:tree accessible tree when treeview is changed.
>+   */
>+  void TreeViewChanged(nsIPresShell* aPresShell, nsIContent* aContent, nsITreeView* aView);

over 80 chars.

r=me with those
Comment 16 Mark Capella [:capella] 2012-04-02 01:27:49 PDT
Created attachment 611390 [details] [diff] [review]
Patch (v7)
Comment 18 Matt Brubeck (:mbrubeck) 2012-04-02 11:10:16 PDT
https://hg.mozilla.org/mozilla-central/rev/16a7a0838a28

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