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]
:
: alexander :surkov
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 User image 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 User image 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 User image 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 User image 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 User image 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 User image Mark Capella [:capella] 2012-03-30 04:42:35 PDT
Created attachment 610851 [details] [diff] [review]
Patch (v3)
Comment 6 User image 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 User image Mark Capella [:capella] 2012-03-30 04:48:42 PDT
Created attachment 610853 [details] [diff] [review]
Patch (v4)
Comment 8 User image 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 User image 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 User image Robert O'Callahan (:roc) (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 User image 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 User image 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 User image Robert O'Callahan (:roc) (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 User image 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 User image 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 User image Mark Capella [:capella] 2012-04-02 01:27:49 PDT
Created attachment 611390 [details] [diff] [review]
Patch (v7)
Comment 18 User image 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.