Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gecko vbuf backend: When an accessible is moved, do not reuse that node or its descendants #10776

Merged
merged 2 commits into from Feb 12, 2020

Conversation

jcsteh
Copy link
Contributor

@jcsteh jcsteh commented Feb 11, 2020

Link to issue number:

None.

Summary of the issue:

In Firefox, when loading Mastodon with the advanced web interface enabled (Preferences -> Appearance), The whole container that contains the timeline is not rendered in browse mode. When you press NVDA+f5, it appears.

This occurs because the accessible containing the timeline articles gets moved (re-parented) at the same time as the articles are inserted. See https://bugzilla.mozilla.org/show_bug.cgi?id=1592518 for further details.

Description of how this pull request fixes the issue:

  1. vbufBase: Allow alwaysRerenderDescendants to be set on a node after it is rendered to indicate that we must not reuse descendants in future.
    Previously, alwaysRerenderDescendants could be set on a node during rendering and that would prevent any reuse while rendering the subtree. However, if alwaysRerenderDescendants was set on a node after rendering, it had no effect on future reuse. Now, it does.

  2. gecko vbuf backend: When an accessible is moved, do not reuse that node or its descendants.
    When an accessible is moved, events are fired as if the accessible were removed and then inserted. The insertion events are fired as if it were a new subtree; i.e. only one insertion for the root of the subtree. This means that if new descendants are inserted at the same time as the root is moved, we don't get specific events for those insertions. Because of that, we mustn't reuse the subtree. Otherwise, we wouldn't walk inside it and thus wouldn't know about the new descendants.

    To achieve this, we set alwaysRerenderDescendants on any node which gets a hide event. If the node is being removed instead of moved (no subsequent insertion event), this is cheap and simply has no effect.

Testing performed:

  1. Tested with this simple test case. Verified that after 2 seconds, text appears in browse mode. (It did not before this PR.)
  2. Tested Mastodon as above and confirmed that the home timeline is rendered.

Known issues with pull request:

This means that moved subtrees are never reused. In practice, I don't think this occurs often (at least not with large subtrees) and thus shouldn't have any practical impact.

Alternatives

  1. Firefox could fire text inserted events for the inserted descendants. The problem is that this violates the "one insertion at the root of the subtree" rule, which causes problems elsewhere. It's also extremely difficult to implement and is likely to cause messy regressions.
  2. The Gecko vbuf backend could remove the entire subtree when a hide event is received. This was my initial thinking for an NVDA fix. However, I'm reluctant to do this kind of processing inside the event callback, especially since all other buffer rangling is done by VBufBackend_t::update.

Change log entry:

Bug fixes:

- In Firefox, when loading Mastodon with the advanced web interface enabled, all timelines now render correctly in browse mode.

jcsteh added 2 commits Feb 11, 2020
…t is rendered to indicate that we must not reuse descendants in future.

Previously, alwaysRerenderDescendants could be set on a node during rendering and that would prevent any reuse while rendering the subtree.
However, if alwaysRerenderDescendants was set on a node after rendering, it had no effect on future reuse.
Now, it does.
…de or its descendants.

When an accessible is moved, events are fired as if the accessible were removed and then inserted.
The insertion events are fired as if it were a new subtree; i.e. only one insertion for the root of the subtree.
This means that if new descendants are inserted at the same time as the root is moved, we don't get specific events for those insertions.
Because of that, we mustn't reuse the subtree.
Otherwise, we wouldn't walk inside it and thus wouldn't know about the new descendants.

To achieve this, we set alwaysRerenderDescendants on any node which gets a hide event.
If the node is being removed instead of moved (no subsequent insertion event), this is cheap and simply has no effect.
@jcsteh
Copy link
Contributor Author

jcsteh commented Feb 11, 2020

@MarcoZehe, could you please take this build for a spin and:

  1. Confirm the fix on Mastodon.
  2. Run with it for a while and see if you notice any perf regressions. I don't anticipate any, but stranger things have happened. :)

Thanks!

@MarcoZehe
Copy link
Contributor

MarcoZehe commented Feb 11, 2020

@jcsteh Both simple test case and Mastodon fixes confirmed with this try build. Now taking it for a longer test drive.

@MarcoZehe
Copy link
Contributor

MarcoZehe commented Feb 11, 2020

@jcsteh I have a feeling this build is actually a bit faster than normal NVDA versions, for example on Facebook with a lot of dynamic updates. As if some processing is more smooth now. As for correctness, have not observed any glitches yet. Will continue testing, but wanted to leave this observation here in the interim.

@MarcoZehe
Copy link
Contributor

MarcoZehe commented Feb 11, 2020

After running with it for a day, I think I can confidently give this a thumbs up. No perf regressions or missing/out of order content that I've noticed.

@jcsteh
Copy link
Contributor Author

jcsteh commented Feb 12, 2020

@jcsteh I have a feeling this build is actually a bit faster than normal NVDA versions, for example on Facebook with a lot of dynamic updates.

FWIW, I suspect that must have been circumstantial - I don't see any way it could be faster, since buffer tree traversal should always be faster than rendering - but regardless, I'm glad it hasn't regressed performance. :) Thanks for test driving.

@jcsteh jcsteh marked this pull request as ready for review Feb 12, 2020
@jcsteh jcsteh requested a review from michaelDCurran Feb 12, 2020
@michaelDCurran michaelDCurran merged commit bd3fea3 into nvaccess:master Feb 12, 2020
1 check passed
@nvaccessAuto nvaccessAuto added this to the 2020.1 milestone Feb 12, 2020
michaelDCurran added a commit that referenced this issue Feb 12, 2020
@jcsteh jcsteh deleted the mastodonMove branch May 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants