Closed Bug 1274381 Opened 3 years ago Closed 3 years ago

scope accessible elements search to inserted nodes

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: surkov, Assigned: surkov)

References

Details

Attachments

(1 file, 2 obsolete files)

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
Attached patch patchSplinter Review
Attachment #8754520 - Flags: review?(yzenevich)
(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?
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Flags: needinfo?(surkov.alexander)
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.
(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
Flags: needinfo?(surkov.alexander)
Hi Alex, should I wait with the review until some profiling is done?
Flags: needinfo?(surkov.alexander)
(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
Flags: needinfo?(surkov.alexander)
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?
Flags: needinfo?(surkov.alexander)
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?
Flags: needinfo?(surkov.alexander)
Attachment #8754520 - Flags: feedback?(mzehe)
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. :-)
Attachment #8754520 - Flags: feedback?(mzehe) → feedback+
(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 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
Attachment #8754520 - Flags: review?(yzenevich) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3acfc1fa613c6febd943a0875fcc22f406e4b88
Bug 1274381 - scope accessible elements search to inserted nodes, r=yzen, f=marcoz
https://hg.mozilla.org/mozilla-central/rev/b3acfc1fa613
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1281828
backed out for causing bug 1281828
Status: RESOLVED → REOPENED
Flags: needinfo?(surkov.alexander)
Resolution: FIXED → ---
Backout by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/efcb27bb03ac
Backed out changeset b3acfc1fa613 for causing regression (bug 1281828) and on request from marcoz
Please mark bug 1281828 status-firefox50:affected once this re-lands (assuming it'll be in 50, too). Thank you!
https://hg.mozilla.org/integration/mozilla-inbound/rev/43d72f3a7ae103cb195a6aff1e65defddf8209b9
Bug 1274381 - scope accessible elements search to inserted nodes, r=yzen, f=marcoz
relanding, as I cannot reproduce a problem from bug 1281828
Flags: needinfo?(surkov.alexander)
https://hg.mozilla.org/mozilla-central/rev/43d72f3a7ae1
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
This actually relanded on Mozilla 51 and was backed out of 50 earlier. Fixing flags.
Target Milestone: mozilla50 → mozilla51
Depends on: 1297131
Depends on: 1303728
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.
(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
Whiteboard: [checkin-needed-aurora]
Attached patch Patch for backout in 51 (obsolete) — Splinter Review
This backs the patch out of 51. Except for some fuzzy lines, the backout looked clean. Alex, can you check it over, please?
Attachment #8792894 - Flags: review?(surkov.alexander)
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.
Attachment #8792894 - Flags: review?(surkov.alexander) → review+
Attached patch Backout patch for Mozilla51 (obsolete) — Splinter Review
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.
Attachment #8792894 - Attachment is obsolete: true
Attachment #8792905 - Flags: review+
Attachment #8792905 - Flags: approval-mozilla-aurora?
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.
Attachment #8792905 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/6ad72777c10d

not sure where to set the firefox51 flag here
Whiteboard: [checkin-needed-aurora]
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.
Flags: needinfo?(surkov.alexander)
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.
Flags: needinfo?(surkov.alexander)
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.
Attachment #8792905 - Attachment is obsolete: true
Marking this as wontfix for 51 given comment 31. Feel free to switch it to 'fixed' if that sounds more accurate to you.
This is fixed by the landing of bug 1280551 on Aurora, see bug 1280551 comment #46. Changing to FIXED for 51.
You need to log in before you can comment on or make changes to this bug.