vbuf backend: If a node has been marked allowReuseInAncestorUpdate, ensure we propagate alwaysRerenderDescendants if appropriate before returning.

[jcsteh]

Link to issue number:

None.

Summary of the issue:

With the Firefox accessibility cache enabled, the "Show options" button above GitHub issue comments sometimes doesn't render correctly in browse mode. Pressing enter on it won't open the menu. Sometimes, completely irrelevant text (potentially even from other tabs) gets rendered instead of that button.

Description of user facing changes

Fixes the above issues.

Description of development approach

Previously, the vbuf backend check for allowReuseInAncestorUpdate was before alwaysRerenderDescendants. This meant that if the node had allowReuseInAncestorUpdate and its parent had alwaysRerenderDescendants, we wouldn't propagate alwaysRerenderDescendants down the tree. That meant that we might not pick up changes in a subtree which was moved at the same time as descendants were mutated. I fixed this by moving the check and propagation of alwaysRerenderDescendants before the allowReuseInAncestorUpdate check.

This still results in an early null return. It just ensures we propagate alwaysRerenderDescendants before we do.

Testing strategy:

1. Used Firefox Nightly with the accessibility cache enabled; instructions.
2. Opened Sometimes NVDA can't read aloud the track jcsteh/osara#801
3. Control+home, 3, up arrow to get to the Show options button for the first comment.
4. Pressed enter.
   • Before PR: Sometimes, the window title is announced and the options menu does not appear.
   • After PR: The menu always appears.
5. Even without the PR, this sometimes works. When it did work, I refreshed the page with control+r and repeated from step 2. This usually failed within 10 refreshes.

Notes:

• Although I could only reproduce this with the Firefox accessibility cache enabled, I don't believe this is a bug therein. We fire the same events regardless of the cache. I think the variable here is that the cache makes Firefox respond much faster, so the timing of events and buffer renders is very different.
• I tried really hard to come up with a distilled test case, but I failed. Creating the sequence of events needed to make this happen is pretty mind-boggling. I hope the logic makes sense, though: if we're not supposed to re-render descendants and we fail to propagate that to descendants, we're not going to honour that flag correctly.
• That unfortunately means I also wasn't able to come up with an automated test.

Known issues with pull request:

None known.

Change log entries:

Bug fixes
In Firefox, activating the Show options button on GitHub issue pages now works reliably.

Code Review Checklist:

• Pull Request description:
  • description is up to date
  • change log entries
• Testing:
  • Unit tests
  • System (end to end) tests
  • Manual testing
• API is compatible with existing add-ons.
• Documentation:
  • User Documentation
  • Developer / Technical Documentation
  • Context sensitive help for GUI changes
• UX of all users considered:
  • Speech
  • Braille
  • Low Vision
  • Different web browsers
  • Localization in other languages / culture than English
• Security precautions taken.

[jcsteh]

It's also worth noting that a stale NVDA vbuf tree can potentially lead to crashes or infinite loops, since accessible ids can eventually be reused and we might end up reusing a subtree which creates a loop in the tree. There are several bugs about crashes and freezes in NVDA vbuf code; https://bugzilla.mozilla.org/show_bug.cgi?id=1786676, https://bugzilla.mozilla.org/show_bug.cgi?id=1737688, https://bugzilla.mozilla.org/show_bug.cgi?id=1691928, #13540.

[AppVeyorBot]

• PASS: Translation comments check.
• PASS: Unit tests.
• PASS: Lint check.
• FAIL: System tests (tags: installer NVDA). See test results for more information.
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/07gewgid2e5ykbxi/artifacts/output/nvda_snapshot_pr14269-26848,94b71b27.exe
• CI timing (mins):
  INIT 0.0,
  INSTALL_START 0.7,
  INSTALL_END 0.8,
  BUILD_START 0.0,
  BUILD_END 22.0,
  TESTSETUP_START 0.0,
  TESTSETUP_END 0.2,
  TEST_START 0.0,
  TEST_END 13.9,
  FINISH_END 0.2

See test results for failed build of commit 94b71b27a4

[michaelDCurran]

This logic looks correct to me.
I'll still leave this to @seanbudd to approve and merge if he is happy.

[michaelDCurran]

For reference, some examples where alwaysRerenderDescendants is set to true are:

• Where the accessible name is used as content as the inner nodes did not produce meaningful content.
• Where a subtree in the DOM has moved from one place to another. In this case alwaysRerenderDescendants is necessary as other mutations may occur within the subtree at the same time, but browser event coalescence may hide the inner mutations from us.