Closed Bug 574312 Opened 9 years ago Closed 9 years ago

make accessible tree creation from parent to child always

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(2 files, 8 obsolete files)

Attached patch wip (obsolete) — Splinter Review
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
Attachment #453694 - Flags: feedback?(marco.zehe)
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.
(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.
Attached patch wip2 (obsolete) — Splinter Review
remove null parent restoration logic
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #453694 - Attachment is obsolete: true
Attachment #453694 - Flags: feedback?(marco.zehe)
Comment on attachment 453703 [details] [diff] [review]
wip2

mochitests failures, working on new patch
Attachment #453703 - Attachment is obsolete: true
Attached patch wip3 (obsolete) — Splinter Review
try server build is expected
(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/surkov.alexander@gmail.com-0c85981cd769/
Attached patch wip4 (obsolete) — Splinter Review
Attachment #453759 - Attachment is obsolete: true
(In reply to comment #9)
> Created an attachment (id=453812) [details]
> wip4

tr server build - http://build.mozilla.org/tryserver-builds/surkov.alexander@gmail.com-try-cebd142c4e68
Attached patch patch (obsolete) — Splinter Review
this one should be in good shape, try server build is expected
Attachment #453812 - Attachment is obsolete: true
Attachment #453979 - Flags: superreview?(neil)
Attachment #453979 - Flags: review?(marco.zehe)
Attachment #453979 - Flags: review?(bolterbugz)
Summary: make correct accessible tree → make accessible tree creation from parent to child always
(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/surkov.alexander@gmail.com-ebf1b7b9e7cf/
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.
Attachment #453979 - Flags: review?(marco.zehe) → review+
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.
(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.
Attached patch patch2 (obsolete) — Splinter Review
fixed few things I caught.
Attachment #453979 - Attachment is obsolete: true
Attachment #454048 - Flags: superreview?(neil)
Attachment #454048 - Flags: review?(bolterbugz)
Attachment #453979 - Flags: superreview?(neil)
Attachment #453979 - Flags: review?(bolterbugz)
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}
(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.
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 ;)
(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 ;)
Attachment #454048 - Flags: review?(bolterbugz) → review+
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.
ok, GetAccessibleOrContainer -> GetAccessibleOrContainerByRule
GetAccessibleFromAbove -> GetAccessibleOrContainer

sounds good?
Attached patch patch3 (obsolete) — Splinter Review
GetAccessibleOrContainer -> GetAccessibleByRule
GetAccessibleFromAbove -> GetAccessibleOrContainer
Attachment #454048 - Attachment is obsolete: true
Attachment #454325 - Flags: superreview?(neil)
Attachment #454048 - Flags: superreview?(neil)
(In reply to comment #22)
> ok, GetAccessibleOrContainer -> GetAccessibleOrContainerByRule
> GetAccessibleFromAbove -> GetAccessibleOrContainer
> 
> sounds good?

Yep, thanks.
Neil, ping on this.
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)
(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.
Attached patch patch4 (obsolete) — Splinter Review
updated to trunk and Neil's nit fixed
Attachment #454325 - Attachment is obsolete: true
Attachment #455084 - Flags: superreview?(neil)
Attachment #454325 - Flags: superreview?(neil)
Attachment #455084 - Attachment description: patch2 → patch4
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.
(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?
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 ;-)
Ok, thanks!
Attached patch patch5Splinter Review
nsDequeue is replaced on nsTArray
Attachment #455084 - Attachment is obsolete: true
Attachment #455117 - Flags: superreview?(neil)
Attachment #455084 - Flags: superreview?(neil)
Comment on attachment 455117 [details] [diff] [review]
patch5

>+    for (PRInt32 idx = nodes.Length() - 1; idx >=0; idx--) {
Nit: >= 0;
Attachment #455117 - Flags: superreview?(neil) → superreview+
thanks, landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/51b69f8d306f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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?)
It might be. I removed because it was looked irrelevant since GetRelevantContent and etc were gone.
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...
(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.?
(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)
(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? ;-)
Yes but I confess the asserts get lost in the noise sometimes :) We need to fix that noise.
Attached patch fix assertionSplinter Review
Attachment #467741 - Flags: review?(bolterbugz)
Attachment #467741 - Flags: approval2.0?
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.
Attachment #467741 - Flags: review?(bolterbugz)
Attachment #467741 - Flags: review+
Attachment #467741 - Flags: approval2.0?
Attachment #467741 - Flags: approval2.0+
You need to log in before you can comment on or make changes to this bug.