877124 - [AccessFu] Trust explicitly associated names for children of current pivot.

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Yura Zenevich [:yzen] (please use "needinfo")]

Currently the implementation only prunes the subtree of the pivot position, but it does not prune the subtrees of child items.

Comment by eeejay: Pruning the subtree of child object is less than trivial and we would need to change the way the PivotContext caches subtrees, since we just get a flat list with no knowledge of the hierarchy. I think it could be possible with an internal (to PivotContext) tree format, and generator functions that take arguments on whether to ignore the children of the current item.

Comment 1

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Tests for 877124.

Comment 2

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Patch for 877124 v1

Hi Eitan, this is an initial patch to get some feedback.

For this patch I did not cache a common subtree. I'm still a little uncertain whether it's necessary given that it would be only slightly different from the one that the accessible already contains (children). I also talked to Marco whether there are cases where the subtree does not need to be pruned in cases where we do now (and it does not seem like it): http://krijnhoetmer.nl/irc-logs/accessibility/20130603#l-148

Comment 3

[Eitan Isaacson [:eeejay]]

Comment on attachment 757484 [details] [diff] [review]
Patch for 877124 v1

Review of attachment 757484 [details] [diff] [review]:
-----------------------------------------------------------------

I don't like the logic leakage into Utils.

I think subtreePreorder/subtreePostorder should be generator functions[1]. They could either take a function as an argument that would return true or false depending whether to traverse into descendants. Or, you could use the special send() method to instruct the generator not to go in to descendants[2].


1. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Iterators_and_Generators
2. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Iterators_and_Generators#Advanced_generators

Comment 4

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Patch for 877124 v2

Comment 5

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Tests for 877124 v2.

Comment 6

[Eitan Isaacson [:eeejay]]

Comment on attachment 758395 [details] [diff] [review]
Tests for 877124 v2.

Review of attachment 758395 [details] [diff] [review]:
-----------------------------------------------------------------

This looks right.

Comment 7

[Eitan Isaacson [:eeejay]]

Comment on attachment 758394 [details] [diff] [review]
Patch for 877124 v2

Review of attachment 758394 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/jsat/Utils.jsm
@@ +383,5 @@
>    /*
>     * Traverse the accessible's subtree in pre or post order.
>     * It only includes the accessible's visible chidren.
> +   * Note: needSubtree is a function argument that can be used to determine
> +   * whether aAccessible's is required.

missing word there, "subtree"?

@@ +388,2 @@
>     */
> +  _traverse: function _traverse(aAccessible, preorder, needSubtree) {

Didn't catch this last time, but all function arguments are preceded by an 'a', so aNeedSubtree, and aPreorder

@@ +413,5 @@
>     * This is a flattened list of the accessible's subtree in preorder.
>     * It only includes the accessible's visible chidren.
>     */
> +  getSubtreePreorder: function getSubtreePreorder(needSubtree) {
> +    if (!this._subtreePreOrder) {

What if this function was called with some needSubtree, and then called again with another? Looks like we cache the first list and use it regardless. So the second time would return a cached list that could be wrong in light of the new argument.

What I would do:
1. Remove caching.
2. Change this function back to a getter attribute.
3. Make the getter return a generator object.
4. Communicate with the generator via send(), for example send True when we want to ignore the current node's children.

Extra bonus points:
1. Investigate the actual benifit of caching, and if there is one:
2. Reintroduce caching. The internal format would probably replicate the tree hierarchy, so that you can prune subtrees with the API suggested above.

Comment 8

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Eitan Isaacson [:eeejay] from comment #7)
> What if this function was called with some needSubtree, and then called
> again with another? Looks like we cache the first list and use it
> regardless. So the second time would return a cached list that could be
> wrong in light of the new argument.

Yep, I was worried that is probably an issue.

Comment 9

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Patch for 877124 v3

Comment 10

[Eitan Isaacson [:eeejay]]

Comment on attachment 760068 [details] [diff] [review]
Patch for 877124 v3

Review of attachment 760068 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good! One nit below.

::: accessible/src/jsat/Utils.jsm
@@ +388,4 @@
>     */
> +  _traverse: function _traverse(aAccessible, aPreorder, aStop) {
> +    if (aStop && aStop(aAccessible)) {
> +      return;

This return does not stop the entire generator?

@@ +396,5 @@
>        child.getState(state, {});
>        if (!(state.value & Ci.nsIAccessibleStates.STATE_INVISIBLE)) {
> +        if (aPreorder) {
> +          yield child;
> +          [yield node for (node of this._traverse(child, aPreorder, aStop))];

Wow. This is fancy, never saw that with yeild. I am assuming this works..

::: accessible/src/jsat/UtteranceGenerator.jsm
@@ +80,5 @@
> +      let nameRule = UtteranceGenerator.roleRuleMap[roleString] || 0;
> +      // Ignore subtree if the name is explicit and the role's name rule is the
> +      // NAME_FROM_SUBTREE_RULE.
> +      return (Utils.getAttributes(aAccessible)['explicit-name'] === 'true') &&
> +        (nameRule & NAME_FROM_SUBTREE_RULE);

I would swap these conditional parts. Checking a bitmask is lighter weight than getting attributes.

Comment 11

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Eitan Isaacson [:eeejay] from comment #10)
> Comment on attachment 760068 [details] [diff] [review]
> Patch for 877124 v3
> 
> Review of attachment 760068 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good! One nit below.
> 
> ::: accessible/src/jsat/Utils.jsm
> @@ +388,4 @@
> >     */
> > +  _traverse: function _traverse(aAccessible, aPreorder, aStop) {
> > +    if (aStop && aStop(aAccessible)) {
> > +      return;
> 
> This return does not stop the entire generator?

Yep, this only stops _traverse at a specific depth.

> 
> @@ +396,5 @@
> >        child.getState(state, {});
> >        if (!(state.value & Ci.nsIAccessibleStates.STATE_INVISIBLE)) {
> > +        if (aPreorder) {
> > +          yield child;
> > +          [yield node for (node of this._traverse(child, aPreorder, aStop))];
> 
> Wow. This is fancy, never saw that with yeild. I am assuming this works..

Should work yes, the test cases are passing, afaik.

> 
> ::: accessible/src/jsat/UtteranceGenerator.jsm
> @@ +80,5 @@
> > +      let nameRule = UtteranceGenerator.roleRuleMap[roleString] || 0;
> > +      // Ignore subtree if the name is explicit and the role's name rule is the
> > +      // NAME_FROM_SUBTREE_RULE.
> > +      return (Utils.getAttributes(aAccessible)['explicit-name'] === 'true') &&
> > +        (nameRule & NAME_FROM_SUBTREE_RULE);
> 
> I would swap these conditional parts. Checking a bitmask is lighter weight
> than getting attributes.

Sounds good.

Comment 12

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Patch for 877124 v4

Carrying over the r+.

Comment 13

[Eitan Isaacson [:eeejay]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/39664fe37dd3
https://hg.mozilla.org/integration/mozilla-inbound/rev/d79910d9e251

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/39664fe37dd3
https://hg.mozilla.org/mozilla-central/rev/d79910d9e251