Closed Bug 739524 Opened 12 years ago Closed 12 years ago

replace TreeViewChanged DOM event on direct call from XUL tree

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: surkov, Assigned: capella)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 6 obsolete files)

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
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attached patch Patch (v1) - very rough (obsolete) — Splinter Review
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.
Attachment #610556 - Flags: feedback?(surkov.alexander)
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
Attachment #610556 - Flags: feedback?(surkov.alexander)
Attached patch Patch (v2) (obsolete) — Splinter Review
New version ... added back firing TreeViewChanged events, this allowed the mochitest tree tests to pass without modifications ...
Attachment #610556 - Attachment is obsolete: true
Attachment #610839 - Flags: feedback?(surkov.alexander)
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.
Attachment #610839 - Flags: feedback?(surkov.alexander) → feedback+
Attached patch Patch (v3) (obsolete) — Splinter Review
Attachment #610839 - Attachment is obsolete: true
Attachment #610851 - Flags: feedback?(surkov.alexander)
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
Attachment #610851 - Flags: review?(trev.saunders)
Attachment #610851 - Flags: review?(roc)
Attachment #610851 - Flags: feedback?(surkov.alexander)
Attachment #610851 - Flags: feedback+
Attached patch Patch (v4) (obsolete) — Splinter Review
Attachment #610851 - Attachment is obsolete: true
Attachment #610851 - Flags: review?(trev.saunders)
Attachment #610851 - Flags: review?(roc)
Attachment #610853 - Flags: feedback?(surkov.alexander)
Comment on attachment 610853 [details] [diff] [review]
Patch (v4)

Mark, you don't need to rerequest neither review nor feedback for trivial changes.
Attachment #610853 - Flags: review?(trev.saunders)
Attachment #610853 - Flags: review?(roc)
Attachment #610853 - Flags: feedback?(surkov.alexander)
Attachment #610853 - Flags: feedback+
Blocks: 740750
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 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
Attachment #610853 - Flags: review?(roc) → review+
Attached patch Patch (v5) (obsolete) — Splinter Review
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?
> 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.
Yes, I just meant you to swap the order of the "FireDOMEvent" and "TreeViewChanged" calls.
Attached patch patch( v6) (obsolete) — Splinter Review
Order swapped, and mView is passed all the way down to nsXULTreeAccessible.cpp to avoid the call to GetView().
Attachment #610853 - Attachment is obsolete: true
Attachment #611268 - Attachment is obsolete: true
Attachment #610853 - Flags: review?(trev.saunders)
Attachment #611350 - Flags: review?(trev.saunders)
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
Attachment #611350 - Flags: review?(trev.saunders) → review+
Attached patch Patch (v7)Splinter Review
Attachment #611350 - Attachment is obsolete: true
Blocks: 740725
https://hg.mozilla.org/mozilla-central/rev/16a7a0838a28
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: markcapella@twcny.rr.com
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: