The default bug view has changed. See this FAQ.

replace TreeViewChanged DOM event on direct call from XUL tree

RESOLVED FIXED in mozilla14

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: surkov, Assigned: capella)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla14
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

5 years ago
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)

Updated

5 years ago
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
(Assignee)

Comment 1

5 years ago
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.
Attachment #610556 - Flags: feedback?(surkov.alexander)
(Reporter)

Comment 2

5 years ago
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)
(Assignee)

Comment 3

5 years ago
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 ...
Attachment #610556 - Attachment is obsolete: true
Attachment #610839 - Flags: feedback?(surkov.alexander)
(Reporter)

Comment 4

5 years ago
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+
(Assignee)

Comment 5

5 years ago
Created attachment 610851 [details] [diff] [review]
Patch (v3)
Attachment #610839 - Attachment is obsolete: true
Attachment #610851 - Flags: feedback?(surkov.alexander)
(Reporter)

Comment 6

5 years ago
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+
(Assignee)

Comment 7

5 years ago
Created attachment 610853 [details] [diff] [review]
Patch (v4)
Attachment #610851 - Attachment is obsolete: true
Attachment #610851 - Flags: review?(trev.saunders)
Attachment #610851 - Flags: review?(roc)
Attachment #610853 - Flags: feedback?(surkov.alexander)
(Reporter)

Comment 8

5 years ago
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+
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 11

5 years ago
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?
> 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.
(Assignee)

Comment 14

5 years ago
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().
Attachment #610853 - Attachment is obsolete: true
Attachment #611268 - Attachment is obsolete: true
Attachment #610853 - Flags: review?(trev.saunders)
(Reporter)

Updated

5 years ago
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+
(Assignee)

Comment 16

5 years ago
Created attachment 611390 [details] [diff] [review]
Patch (v7)
(Reporter)

Updated

5 years ago
Attachment #611350 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Blocks: 740725
(Reporter)

Comment 17

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/16a7a0838a28
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/16a7a0838a28
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Whiteboard: markcapella@twcny.rr.com
You need to log in before you can comment on or make changes to this bug.