1274381 - scope accessible elements search to inserted nodes

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

it should be beneficial to scope the search within an inserted node vs using a global walker with the stopper node mechanism, because
* global Prev() and then global Next() is less effective as longer tree is traversed
* I've run into an example, where global Prev() + Next() combo skipped a stopper node, and we end up traversing a whole children tree
* It seems the logic of scoped search is more straightforward than using a stopper node

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[Marco Zehe (:MarcoZ)]

(In reply to alexander :surkov from comment #0)
> * I've run into an example, where global Prev() + Next() combo skipped a
> stopper node, and we end up traversing a whole children tree
Can this be turned into a Mochitest?

Comment 3

[Marco Zehe (:MarcoZ)]

I just ran with this patch in a local build, and it increases the time the testcase for bug 522847 needs to run from an average of 1930 MS to 5100 MS. That's more than 250% slower. I also have a feeling this testcase and other stuff loads slower with that patch.

However, when I use Mozilla's IRCCloud, and switch to the #developers channel, over-all responsiveness there is much better with this patch. No exact measures, but waling through the chat history with NVDA's virtual buffer is more responsive with this patch applied.

So the patch improves some things, but makes others slower.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to Marco Zehe (:MarcoZ) from comment #2)
> (In reply to alexander :surkov from comment #0)
> > * I've run into an example, where global Prev() + Next() combo skipped a
> > stopper node, and we end up traversing a whole children tree
> Can this be turned into a Mochitest?

I cannot think of, it's same tree, same events in the end

(In reply to Marco Zehe (:MarcoZ) from comment #3)
> I just ran with this patch in a local build, and it increases the time the
> testcase for bug 522847 needs to run from an average of 1930 MS to 5100 MS.
> That's more than 250% slower. I also have a feeling this testcase and other
> stuff loads slower with that patch.

this is weird, no ideas how it could make things worse, I'll profile it. Thank you for catching it.

> However, when I use Mozilla's IRCCloud, and switch to the #developers
> channel, over-all responsiveness there is much better with this patch. No
> exact measures, but waling through the chat history with NVDA's virtual
> buffer is more responsive with this patch applied.
> 
> So the patch improves some things, but makes others slower.

it's good to know

Comment 5

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

Hi Alex, should I wait with the review until some profiling is done?

Comment 6

[alexander :surkov (:asurkov)]

(In reply to Yura Zenevich [:yzen] from comment #5)
> Hi Alex, should I wait with the review until some profiling is done?

that might be useful, you could find a problem, but it's up to you of course

Comment 7

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

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

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

have a couple of qustions:

::: accessible/base/TreeWalker.cpp
@@ +57,5 @@
>    MOZ_COUNT_DTOR(TreeWalker);
>  }
>  
> +Accessible*
> +TreeWalker::Scope(nsIContent* aChildNode)

header has aAnchorNode

@@ +64,5 @@
> +
> +  mAnchorNode = aChildNode;
> +
> +  bool skipSubtree = false;
> +  Accessible* a11ement = AccessibleFor(aChildNode, 0, &skipSubtree);

nit name :) can't we just do acc or better anchor?

@@ +70,5 @@
> +    mPhase = eAtEnd;
> +    return a11ement;
> +  }
> +
> +  return skipSubtree ? nullptr : Next();

Does it mean we can acutally end up in the tree different from aChildNode subtree? if !a11ement and !skipSubtree?

Comment 8

[alexander :surkov (:asurkov)]

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

I see about same numbers (about 4s) running the test on both before and after builds on my machine. Sometimes I experience the web page slowness (eventually layout bombs us with text change notifications which takes significant time to process), but I do experience this slowness on both builds.

Marco, can you give the patch another try please?

Comment 9

[Marco Zehe (:MarcoZ)]

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

Yes it seems to be OK, don't know if that original finding was a fluke. Yzen, I think it's OK to seriously review that patch now. :-)

Comment 10

[alexander :surkov (:asurkov)]

(In reply to Yura Zenevich [:yzen] from comment #7)

> > +  return skipSubtree ? nullptr : Next();
> 
> Does it mean we can acutally end up in the tree different from aChildNode
> subtree? if !a11ement and !skipSubtree?

Next() should put a children iterator for anchorNode on top of the stack, so we shouldn't get outside anchorNode, correct?

Comment 11

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

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

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

Looks good, I think, just the other nits need to be addressed. Thanks

Comment 12

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b3acfc1fa613c6febd943a0875fcc22f406e4b88
Bug 1274381 - scope accessible elements search to inserted nodes, r=yzen, f=marcoz

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b3acfc1fa613

Comment 14

[Carsten Book [:Tomcat]]

backed out for causing bug 1281828

Comment 15

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/efcb27bb03ac
Backed out changeset b3acfc1fa613 for causing regression (bug 1281828) and on request from marcoz

Comment 16

[Andrew Overholt [:overholt]]

Please mark bug 1281828 status-firefox50:affected once this re-lands (assuming it'll be in 50, too). Thank you!

Comment 17

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7cdd47b386e

Comment 18

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/43d72f3a7ae103cb195a6aff1e65defddf8209b9
Bug 1274381 - scope accessible elements search to inserted nodes, r=yzen, f=marcoz

Comment 19

[alexander :surkov (:asurkov)]

relanding, as I cannot reproduce a problem from bug 1281828

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/43d72f3a7ae1

Comment 21

[Marco Zehe (:MarcoZ)]

This actually relanded on Mozilla 51 and was backed out of 50 earlier. Fixing flags.

Comment 22

[Marco Zehe (:MarcoZ)]

Given that this is responsible for multiple yet unfixed regressions, I pledge for it to be backed out of 51 once that moves to Aurora. It is better for it to wait another round and us to fix the regressions. The gains don't justify the hassle of uplifting multiple patches afterwards.

Comment 23

[Carsten Book [:Tomcat]]

(In reply to Marco Zehe (:MarcoZ) from comment #22)
> Given that this is responsible for multiple yet unfixed regressions, I
> pledge for it to be backed out of 51 once that moves to Aurora. It is better
> for it to wait another round and us to fix the regressions. The gains don't
> justify the hassle of uplifting multiple patches afterwards.

setting the flag so that we can backout when the tree open again

Comment 24

[Marco Zehe (:MarcoZ)]

Attachment: Patch for backout in 51

This backs the patch out of 51. Except for some fuzzy lines, the backout looked clean. Alex, can you check it over, please?

Comment 25

[alexander :surkov (:asurkov)]

Comment on attachment 8792894 [details] [diff] [review]
Patch for backout in 51

looks correct, the funny thing is I feel more confident with the patch, rather than with the old code. having said that, I agree that the patch, blamed of unfixed regressions twice, should be backed out on non-nightly channels.

Comment 26

[Marco Zehe (:MarcoZ)]

Attachment: Backout patch for Mozilla51

Approval Request Comment
[Feature/regressing bug #]: This bug is causing regressions like bug 1281828 and others (see dependencies) and needs to be backed out and stopped from riding the trains in 51.
[User impact if declined]: Inconsistent behavior after any future update, possible crashes, inconsistent content rendering to screen readers.
[Describe test coverage new/current, TreeHerder]: On Nightly since early 51 cycle, see dependency tree.
[Risks and why]: Low risk, stabilizes a situation introduced by original patch for this bug.
[String/UUID change made/needed]: None.

Comment 27

[Gerry Chang [:gchang]]

Comment on attachment 8792905 [details] [diff] [review]
Backout patch for Mozilla51

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

This patch backout the codes causing regression. Take it in 51 aurora.

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6ad72777c10d

not sure where to set the firefox51 flag here

Comment 29

[Carsten Book [:Tomcat]]

seems something is wrong with this patch, had to back this out since got massive failures like https://treeherder.mozilla.org/logviewer.html#?job_id=3611598&repo=mozilla-aurora after this backout landed.

Comment 30

[Marco Zehe (:MarcoZ)]

Hold off on trying to land this for now, I have a feeling bug 1280551 might fix the regressions. Pending further testing, we may want to backport that to 51, and then void the backout request.

Comment 31

[Marco Zehe (:MarcoZ)]

Comment on attachment 8792905 [details] [diff] [review]
Backout patch for Mozilla51

Revoking request to land this on Aurora and backout the original patch. It looks like bug 1280551, request for uplifts pending, will fix the regression introduced by this patch and documented in bug 1281828, too. Backing out this patch from Aurora will no longer be necessary.

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Marking this as wontfix for 51 given comment 31. Feel free to switch it to 'fixed' if that sounds more accurate to you.

Comment 33

[Marco Zehe (:MarcoZ)]

This is fixed by the landing of bug 1280551 on Aurora, see bug 1280551 comment #46. Changing to FIXED for 51.