574312 - make accessible tree creation from parent to child always

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: wip

We create accessible based on node/frame information and always it's not enough to know if the node should or shouldn't be accessible, we have hacks like GetAttachedAccessible and etc to workaround this for specific cases. However it's not enough. Parent should be responsible for child creation.

wip shows an idea, the rest code isn't suited for the changed part yet. try-server build will be a bit later

Comment 1

[Marco Zehe (:MarcoZ)]

Comment on attachment 453694 [details] [diff] [review]
wip

Interesting concept, looking forward to trying it out! Wonder how this will impact performance, or *when* this case where we have to push and pop the nsINodes will really occur.

Comment 2

[alexander :surkov (:asurkov)]

try server build - http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-48eec8cf3d23/

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #1)
> (From update of attachment 453694 [details] [diff] [review])
> Interesting concept, looking forward to trying it out! Wonder how this will
> impact performance, or *when* this case where we have to push and pop the
> nsINodes will really occur.

It shouldn't affect on performance much. 

1) it doesn't affect on accessible tree creation by accwalker
2) internally we're trying to get an accessible for accessible node
3) it might affect on DOM mutation processing but there we get container accessible so we have tree traversal any way

I need to look at 3d more closely. But in general it's just interesting how it affects on browsing - that's a reason I put this wip for your preview.

Comment 4

[alexander :surkov (:asurkov)]

Attachment: wip2

remove null parent restoration logic

Comment 5

[alexander :surkov (:asurkov)]

try sever build for wip2 - http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-926cf26d06a4/

Comment 6

[alexander :surkov (:asurkov)]

Comment on attachment 453703 [details] [diff] [review]
wip2

mochitests failures, working on new patch

Comment 7

[alexander :surkov (:asurkov)]

Attachment: wip3

try server build is expected

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> Created an attachment (id=453759) [details]
> wip3
> 
> try server build is expected

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-0c85981cd769/

Comment 9

[alexander :surkov (:asurkov)]

Attachment: wip4

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #9)
> Created an attachment (id=453812) [details]
> wip4

tr server build - http://build.mozilla.org/tryserver-builds/[e-mail]-try-cebd142c4e68

Comment 11

[alexander :surkov (:asurkov)]

Attachment: patch

this one should be in good shape, try server build is expected

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #11)
> Created an attachment (id=453979) [details]
> patch
> 
> this one should be in good shape, try server build is expected

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-ebf1b7b9e7cf/

Comment 13

[Marco Zehe (:MarcoZ)]

Comment on attachment 453979 [details] [diff] [review]
patch

r=me for the test part and the functionality. iframes, heavily loaded pages like blindcooltech.com etc., all still load completely. BZ's table test case works much faster with the combination of bug 573706 and this one.

Comment 14

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 453979 [details] [diff] [review]
patch

I think "GetAccessibleFromAbove" should be "GetAccessibleFromNodeOrFirstAccessibleParent". Then you can get rid of more comments.

You could use a boolean instead of EWhatAccToGet but it is ok.

"while accessible in not a tree" should be "while accessible not in tree".

I'll need another pass or 2.

Comment 15

[alexander :surkov (:asurkov)]

(In reply to comment #14)
> (From update of attachment 453979 [details] [diff] [review])
> I think "GetAccessibleFromAbove" should be
> "GetAccessibleFromNodeOrFirstAccessibleParent". Then you can get rid of more
> comments.

Oh, I really want short and catchy name, probably with more comments before the method. I'm not sure FromAbove is really good english phrase, but it makes me think it sounds like slang what shouldn't be bad actually.

> You could use a boolean instead of EWhatAccToGet but it is ok.

I couldn't ;) but if I could I prefer enum since code is more readable.

Comment 16

[alexander :surkov (:asurkov)]

Attachment: patch2

fixed few things I caught.

Comment 17

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 454048 [details] [diff] [review]
patch2

>+   * Return an accessible for the given DOM node or container accessible if
>+   * the node is not accessible.
>+   */
>+  inline nsAccessible* GetAccessibleFromAbove(nsINode* 

While sexy, this name makes me think the accessible is for something above the given node.

How about changing GetAccessibleOrContainer to GetAccessibleByRule, and then you can have:

// pseudocode
GetAccessibleOrContainer { GetAccessibleByRule eGetAccForNodeOrContainer}

GetAccessibleContainer { GetAccessibleByRule eGetAccForContainer}

Comment 18

[alexander :surkov (:asurkov)]

(In reply to comment #17)
> (From update of attachment 454048 [details] [diff] [review])
> >+   * Return an accessible for the given DOM node or container accessible if
> >+   * the node is not accessible.
> >+   */
> >+  inline nsAccessible* GetAccessibleFromAbove(nsINode* 
> 
> While sexy, this name makes me think the accessible is for something above the
> given node.

Yes, that's a minus. But can't find good name to point the accessible is for node and if there's no accessible then first accessible in parent chain.

> How about changing GetAccessibleOrContainer to GetAccessibleByRule, and then
> you can have:

I like GetAccessibleOrContainer because it's more well defined. When I'm looking at it I know what accessible may be returned, if we choose a rule in the name then you should always look at arguments to know what else it can return.

Comment 19

[David Bolter [:davidb] (NeedInfo me for attention)]

Note I was suggesting (implicitly) that you would still have GetAccessibleOrContainer but it would be called instead of GetAccessibleFromAbove. Note I was using {}'s to show impl, not ()'s to show args ;)

Comment 20

[alexander :surkov (:asurkov)]

(In reply to comment #19)
> Note I was suggesting (implicitly) that you would still have
> GetAccessibleOrContainer but it would be called instead of
> GetAccessibleFromAbove.

I think I find this ok.

> Note I was using {}'s to show impl, not ()'s to show
> args ;)

Though it's worth to find something better than GetAccessibleByRule ;)

Comment 21

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 454048 [details] [diff] [review]
patch2

The <area> element work is interesting - I assume you tested it?

>+    // XXX: while nsAccessible::GetParent() tries to repair broken tree and
>+    // may not return cached parent then we use GetAccessibleOrContainer().

Nit: I wonder if 'disconnected' is closer to what we mean than 'broken'.

'GetAccessibleByRule' would be a private method... not sure what else to call it :)

I hope you make the suggested naming change but I can r+ either way. Overall I like this patch so r=me if image map tests ok. I'd like to a ping for the final patch if Neil has issues but I'm just not currently seeing any blocking problems.

Comment 22

[alexander :surkov (:asurkov)]

ok, GetAccessibleOrContainer -> GetAccessibleOrContainerByRule
GetAccessibleFromAbove -> GetAccessibleOrContainer

sounds good?

Comment 23

[alexander :surkov (:asurkov)]

Attachment: patch3

GetAccessibleOrContainer -> GetAccessibleByRule
GetAccessibleFromAbove -> GetAccessibleOrContainer

Comment 24

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #22)
> ok, GetAccessibleOrContainer -> GetAccessibleOrContainerByRule
> GetAccessibleFromAbove -> GetAccessibleOrContainer
> 
> sounds good?

Yep, thanks.

Comment 25

[alexander :surkov (:asurkov)]

Neil, ping on this.

Comment 26

[[e-mail]]

Comment on attachment 454325 [details] [diff] [review]
patch3

Not sure why you requested sr on this as all the changes looked pretty boring.

>+nsAccessibilityService::GetOrCreateAccessible(nsINode* aNode,
>+                                              nsIPresShell* aPresShell,
>+                                              nsIWeakReference* aWeakShell,
>+                                              PRBool* aIsHidden)
> {
>   if (!aPresShell || !aWeakShell || !aNode || gIsShutdown)
>     return nsnull;
I happened to notice that all the callers have to use do_QueryReferent on the weak shell so that they can pass in both the strong and the weak reference. Why not remove the aPresShell parameter and get the strong ref here i.e.
nsCOMPtr<nsIPresShell> presShell = do_QueryReferent(aWeakShell);
if (!presShell || !aNode || gIsShutdown)
  return nsnull;

>+  enum EWhatAccToGet {
>+    eGetAccForNode = 0x1,
>+    eGetAccForContainer = 0x2,
>+    eGetAccForNodeOrContainer = 0x3
>+  };
I assume this depends on 0x3 == 0x2 | 0x1. Might be worth documenting that eGetAccForNodeOrContainer == eGetAccForNode Or eGetAccForContainer. Or maybe just rewrite the declaration to make it a little more obvious:
enum EWhatAccToGet {
  eGetAccForNode = 1 << 0,
  eGetAccForContainer = 1 << 1,
  eGetAccForNodeOrContainer = eGetAccForNode | eGetAccForContainer
};
(assuming this compiles)

Comment 27

[alexander :surkov (:asurkov)]

(In reply to comment #26)
> (From update of attachment 454325 [details] [diff] [review])
> Not sure why you requested sr on this as all the changes looked pretty boring.

Primary it was to check GetAccessilbeByRule logic, nsDeque usage is defensible there, and I need your sr for layout/inspector changes.

> >+nsAccessibilityService::GetOrCreateAccessible(nsINode* aNode,
> >+                                              nsIPresShell* aPresShell,
> >+                                              nsIWeakReference* aWeakShell,
> >+                                              PRBool* aIsHidden)
> > {
> I happened to notice that all the callers have to use do_QueryReferent on the
> weak shell so that they can pass in both the strong and the weak reference. Why
> not remove the aPresShell parameter and get the strong ref here i.e.
> nsCOMPtr<nsIPresShell> presShell = do_QueryReferent(aWeakShell);
> if (!presShell || !aNode || gIsShutdown)
>   return nsnull;

This method is also called from nsAccTreeWalker that gets presshell one time for all GetOrCreateAccessible calls.

>   eGetAccForNodeOrContainer = eGetAccForNode | eGetAccForContainer
> };

sure, thank you.

Comment 28

[alexander :surkov (:asurkov)]

Attachment: patch4

updated to trunk and Neil's nit fixed

Comment 29

[[e-mail]]

Comment on attachment 455084 [details] [diff] [review]
patch4

Thanks for drawing my attention to this.

>+  // Go up looking for the nearest accessible container stored in cache.
>+  nsDeque nodes;
>+  nsINode* node = aNode;
>+  while ((node = node->GetNodeParent()) &&
>+         !(cachedAcc = GetCachedAccessible(node, aWeakShell)))
>+    nodes.Push(node);
All you're doing here is collecting ancestor nodes, so that you can loop through them later. This barely calls for a stack, let alone a deque; you only need an nsTArray<nsINode*>. Append nodes to the end, then...

>+    while ((node = static_cast<nsINode*>(nodes.Pop()))) {
>+      cachedAcc = GetCachedAccessible(node, aWeakShell);
>+      if (cachedAcc) {
>+        cachedAcc->EnsureChildren();
>+        containerAcc = cachedAcc;
>       }
...just loop (backwards) through the array.

Comment 30

[alexander :surkov (:asurkov)]

(In reply to comment #29)

> All you're doing here is collecting ancestor nodes, so that you can loop
> through them later. This barely calls for a stack, let alone a deque; you only
> need an nsTArray<nsINode*>. Append nodes to the end, then...

nsTArray should be more efficient here? When is nsDeque preferable over nsTArray?

Comment 31

[[e-mail]]

Prefer nsTArray when
* You are only adding to one end of the queue (AppendElement) AND
 * You are only removing from the same end of the queue ([Remove]ElementAt) OR
 * You are going to remove all of the elements at once (for loop and Clear)
Prefer nsDeque when
* You are adding and removing single elements from opposite ends of the queue
Although there will always be exceptions to the rule ;-)

Comment 32

[alexander :surkov (:asurkov)]

Ok, thanks!

Comment 33

[alexander :surkov (:asurkov)]

Attachment: patch5

nsDequeue is replaced on nsTArray

Comment 34

[[e-mail]]

Comment on attachment 455117 [details] [diff] [review]
patch5

>+    for (PRInt32 idx = nodes.Length() - 1; idx >=0; idx--) {
Nit: >= 0;

Comment 35

[alexander :surkov (:asurkov)]

thanks, landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/51b69f8d306f

Comment 36

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 455117 [details] [diff] [review]
patch5


> void
> nsRootAccessible::FireCurrentFocusEvent()
> {
>   if (IsDefunct())
>     return;
> 
>+  // Simulate a focus event so that we can reuse code that fires focus for
>+  // container children like treeitems.
>   nsCOMPtr<nsINode> focusedNode = GetCurrentFocus();
>   if (!focusedNode) {
>     return; // No current focus
>   }
> 
>-  // Simulate a focus event so that we can reuse code that fires focus for container children like treeitems
>   nsCOMPtr<nsIDOMDocumentEvent> docEvent = do_QueryInterface(mDocument);
>   if (docEvent) {
>     nsCOMPtr<nsIDOMEvent> event;
>     if (NS_SUCCEEDED(docEvent->CreateEvent(NS_LITERAL_STRING("Events"),
>                                            getter_AddRefs(event))) &&
>         NS_SUCCEEDED(event->InitEvent(NS_LITERAL_STRING("focus"), PR_TRUE, PR_TRUE))) {
>-      // Get the target node we really want for the event.
> 
>-      nsINode *targetNode =
>-        GetAccService()->GetRelevantContentNodeFor(focusedNode);
>-      if (targetNode) {
>-        // If the focused element is document element or HTML body element
>-        // then simulate the focus event for the document.
>-        nsINode *document = targetNode->GetOwnerDoc();
>-        if (targetNode == nsCoreUtils::GetRoleContent(document)) {
>-          HandleEventWithTarget(event, document);
>-          return;
>-        }
>-
>-        // Otherwise simulate the focus event for currently focused node.
>-        nsCOMPtr<nsIPrivateDOMEvent> privateEvent(do_QueryInterface(event));
>-        nsCOMPtr<nsIDOMEventTarget> target(do_QueryInterface(focusedNode));
>-        privateEvent->SetTarget(target);
>-        HandleEventWithTarget(event, targetNode);
>-      }
>+      nsCOMPtr<nsIPrivateDOMEvent> privateEvent(do_QueryInterface(event));
>+      nsCOMPtr<nsIDOMEventTarget> target(do_QueryInterface(focusedNode));
>+      privateEvent->SetTarget(target);
>+      HandleEvent(event);
>     }
>   }
> }

Alexander do you recall why you needed this change? (Is this causing the fall out bugs?)

Comment 37

[alexander :surkov (:asurkov)]

It might be. I removed because it was looked irrelevant since GetRelevantContent and etc were gone.

Comment 38

[[e-mail]]

Comment on attachment 455117 [details] [diff] [review]
patch5

>       nsCOMPtr<nsINode> realFocusedNode = GetCurrentFocus();
>-      nsCOMPtr<nsIContent> realFocusedContent = do_QueryInterface(realFocusedNode);
>-      nsCOMPtr<nsIContent> targetContent = do_QueryInterface(aTargetNode);
>-      nsIContent *containerContent = targetContent;
>+      nsIContent* realFocusedContent = realFocusedNode->AsElement();
>+      nsIContent* containerContent = targetContent;
Sadly it turns out that GetCurrentFocus() can return a document, so you can't blindly cast it to an element...

Comment 39

[[e-mail]]

(In reply to comment #38)
> (From update of attachment 455117 [details] [diff] [review])
> >       nsCOMPtr<nsINode> realFocusedNode = GetCurrentFocus();
> >-      nsCOMPtr<nsIContent> realFocusedContent = do_QueryInterface(realFocusedNode);
> >-      nsCOMPtr<nsIContent> targetContent = do_QueryInterface(aTargetNode);
> >-      nsIContent *containerContent = targetContent;
> >+      nsIContent* realFocusedContent = realFocusedNode->AsElement();
> >+      nsIContent* containerContent = targetContent;
> Sadly it turns out that GetCurrentFocus() can return a document, so you can't
> blindly cast it to an element...
Could you just use nsIContent* containerNode = realFocusedNode; etc.?

Comment 40

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #39)
> (In reply to comment #38)
> > (From update of attachment 455117 [details] [diff] [review])
> > >       nsCOMPtr<nsINode> realFocusedNode = GetCurrentFocus();
> > >-      nsCOMPtr<nsIContent> realFocusedContent = do_QueryInterface(realFocusedNode);
> > >-      nsCOMPtr<nsIContent> targetContent = do_QueryInterface(aTargetNode);
> > >-      nsIContent *containerContent = targetContent;
> > >+      nsIContent* realFocusedContent = realFocusedNode->AsElement();
> > >+      nsIContent* containerContent = targetContent;
> > Sadly it turns out that GetCurrentFocus() can return a document, so you can't
> > blindly cast it to an element...
> Could you just use nsIContent* containerNode = realFocusedNode; etc.?

You are right about GetCurrentFocus and we should tweak this code, but I'm curious what issue you are currently seeing? (Or did you notice this simply by inspection)

Comment 41

[[e-mail]]

(In reply to comment #40)
> You are right about GetCurrentFocus and we should tweak this code, but I'm
> curious what issue you are currently seeing? (Or did you notice this simply by
> inspection)
AsElement() asserts if you call it on a document. You build debug, right? ;-)

Comment 42

[David Bolter [:davidb] (NeedInfo me for attention)]

Yes but I confess the asserts get lost in the noise sometimes :) We need to fix that noise.

Comment 43

[alexander :surkov (:asurkov)]

Attachment: fix assertion

Comment 44

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 467741 [details] [diff] [review]
fix assertion

r=me. Looks fine -- I haven't yet confirmed this fixes the assertion but I have faith.

Comment 45

[alexander :surkov (:asurkov)]

fix assertion is landed on 2.0 - http://hg.mozilla.org/mozilla-central/rev/9c5898c4570d