Skip to content

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

Merged
seanbudd merged 4 commits into
masterfrom
bufFixRerender
Oct 20, 2022
Merged

Conversation

@jcsteh

@jcsteh jcsteh commented Oct 18, 2022

Copy link
Copy Markdown
Contributor

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

jcsteh commented Oct 18, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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.

@jcsteh jcsteh marked this pull request as ready for review October 18, 2022 08:39
@jcsteh jcsteh requested a review from a team as a code owner October 18, 2022 08:39
@jcsteh jcsteh requested a review from seanbudd October 18, 2022 08:39
seanbudd
seanbudd reviewed Oct 18, 2022
View reviewed changes
Comment thread nvdaHelper/vbufBase/backend.cpp
@jcsteh
vbuf backend: If a node has been marked allowReuseInAncestorUpdate, e…
62f07ac 
…nsure we propagate alwaysRerenderDescendants if appropriate before returning.

Previously, the 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.
This caused intermittent broken controls and other weirdness on GitHub issue pages.
@AppVeyorBot

AppVeyorBot commented Oct 19, 2022

Copy link
Copy Markdown

See test results for failed build of commit 94b71b27a4

michaelDCurran
michaelDCurran approved these changes Oct 19, 2022
View reviewed changes

@michaelDCurran michaelDCurran left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@michaelDCurran

michaelDCurran commented Oct 19, 2022

Copy link
Copy Markdown
Member

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.

@seanbudd seanbudd merged commit 44a5290 into master Oct 20, 2022
@seanbudd seanbudd deleted the bufFixRerender branch October 20, 2022 01:28
@nvaccessAuto nvaccessAuto added this to the 2023.1 milestone Oct 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@seanbudd seanbudd seanbudd approved these changes

@michaelDCurran michaelDCurran michaelDCurran approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2023.1

Development

Successfully merging this pull request may close these issues.

5 participants

@jcsteh @AppVeyorBot @michaelDCurran @seanbudd @nvaccessAuto