Closed Bug 1592518 Opened 5 years ago Closed 4 years ago

Missing events when some pages, like Mobile Twitter, initially load, causing missing content in NVDA's virtual buffer

Categories

(Core :: Disability Access APIs, defect, P1)

Product:
Core
Component:
Disability Access APIs
Platform:
x86_64
Windows 10
Type:
defect

Tracking

()

Status:
RESOLVED WONTFIX
Tracking Flags:
Tracking Status
firefox-esr68 --- unaffected
firefox70 --- wontfix
firefox71 --- wontfix
firefox72 --- wontfix
firefox73 --- wontfix

People

(Reporter: MarcoZ, Unassigned)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

Simplified test case
439 bytes, text/html
Details

Regression from work in bug 686400 and friends.

Steps to reproduce, with the NVDA screen reader:

  1. Log into https://twitter.com (which gives the new single-page application by default now).
  2. In a new tab, log into Gmail. Make sure it is in the foreground for the next steps.
  3. Make sure you have your tabs to restore on restart set.
  4. Cause a Nightly update. Or, cclose and restart Nightly. Happens in both scenarios, but might be a bit more likely after a Nightly update.
  5. After updating, the recent tabs will load. Gmail will be in the foreground again.
  6. Press CTRL+PageDown to switch to the Twitter tab.
    • Expected: The tab loads, and all content is accessible in the NVDA virtual buffer.
    • Actual: The tab loads, but the main navigation sidebar, where the Home, Notifications, Direct Messages, etc. links are, is not present.
  7. Press NVDA+F5 to refresh the buffer.
    • Result: Now, the side bar is there. This indicates that the tree is intact, but when the side bar gets inserted, we fail to fire an event to let NVDA know about thsi content update.

This reproduces almost 100% for me on two machines. If I am logged into multiple accounts via multi-account containers, I see this with every instance that is reloaded after a browser restart.

If we fix this, and the fix is verified and upliftable, please let me know if you think it should be considered for a dot release in 70. Otherwise, let's aim for a fix in 71.

status-firefox70: affected → fix-optional

Thanks, Liz, will do. However, the bug has become harder to reproduce since the fix for bug 1585851 landed. I still see it, but less frequently than before. While before,I saw it frequently several times per day, when restarting the browser or updating nightly, the frequency has dropped since that fix landed. However, I saw it just this morning, so it isn't completely fixed by that bug.

Marco, this bug is marked as P1 but is unassigned. Are there plans in your team to work on this bug for our releases in flight? Thanks

Flags: needinfo?(mzehe)

Yes, since this happens frequently enough. Investigation is ongoing into various related issues by Jamie and myself. Jamie, any news to share from your end?

Flags: needinfo?(mzehe) → needinfo?(jteh)

I've been able to determine two things so far:

  1. This doesn't seem to be related to AccessibleHandler. While it's much, much harder to reproduce with that enabled, it does still happen occasionally.
  2. When it occurs, the banner landmark is in the NVDA buffer, but anything inside it is missing. That suggests that NVDA isn't getting an event updating the banner landmark. I can understand why we might coalesce a reorder event up to an ancestor, but we should still fire a textInserted event on the banner landmark.
Flags: needinfo?(jteh)
Depends on: 1597916

We have shipped our last beta for 71, since this is a P1, I am keeping the status as fix-optional for 71 in case a safe uplift is possible in a future dot-release.

status-firefox70: fix-optional → wontfix
status-firefox71: affected → fix-optional

Unfortunately, I can still reproduce this on Twitter with a browser restart even after the fix for bug 1597916.

Yes, I unfortunately also still see this in the latest Nightly. :-(

Marco on slack also mentioned this being an issue on Matodon:

when initially loading timelines. The whole container that contains the timeline is not there unless I press NVDA+F5.

On Mastodon, you also need to ensure you have the advanced web interface enabled in Preferences -> Appearance, which is disabled by default.

In case it's helpful, in the Mastodon case, the 20 "articles" representing items in the timeline are all present in the buffer. However, the child "section" of each article is missing.

For Twitter, the banner landmark is present in the buffer, but everything inside it is missing.

I think I found the culprit non-events reliably in mastodon

I believe this is happening because the text inserted event is getting co-coalesced with the show event of the child during an accessible relocation in the hierarchy. I looked into preserving the text insertion events even when their related show events are coalesced, but i am not certain that won't spell trouble. Instead, I chose a simpler approach of not coalescing show events into an ancestor's relocation show event.

Marco, can you try this build and see if it helps? Thanks!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d42635728d194cf9491baec9efbf58aa0052b2d7

With this try build, things are a little better, but not entirely fixed.

  1. When I load the Mastodon instance, the stuff under the Home timeline now appears. This is not the case with a regular Nightly.
  2. However, the items under the Notifications heading still only appear after I refresh the NVDA buffer. So they are in the tree, but not yet picked up by NVDA automatically once they are shown.

After this, I also tested something I saw in Riot the other day: In the Electron app, each chat message gets a time stamp once I move into it with the virtual cursor. In Firefox, it does not. I also checked Chrome, and there, the time stamp also appears before the main text of the message. I then verified that the div with a link child that contains the time stamp is definitely in the tree once focus is within a particular message. But NVDA doesn't pick it up. Problem with this is: Once I refresh the buffer, NVDA's virtual cursor goes back to the top of the page, also taking away browser focus from the message. But the symptom feels very similar to the Mastodon case.

We're getting closer, but not the full way there yet. Something else still gets coalesced that should not.

Marco, regarding the Riot case, can you try turning off "Automatically set system focus to focusable elements" in NVDA Advanced Settings before hitting NVDA+f5? That should prevent the focus being moved. I want to be certain this is an event problem and not HyperText brokenness. (We did see HyperText brokenness for a while during the re-creation work.)

You're right, I can't even get the timestamp link to show up after refreshing the buffer with focus moving turned off. I verified this by using the hovering mouse to make stuff visible in the tree, then refreshing the buffer. The items were still visible for the same message, but the time stamp still didn't show up in the virtual buffer. So this is a different issue.

(In reply to Marco Zehe (:MarcoZ) from comment #15)

After this, I also tested something I saw in Riot the other day: In the Electron app, each chat message gets a time stamp once I move into it with the virtual cursor. In Firefox, it does not. I also checked Chrome, and there, the time stamp also appears before the main text of the message. I then verified that the div with a link child that contains the time stamp is definitely in the tree once focus is within a particular message. But NVDA doesn't pick it up.

The problem here is that the timestamp link has a screen width of 0, so NVDA ignores it. The width remains 0 even when the timestamp is showing. The link contains a span which is where the visual text comes from, but that span has aria-hidden. That span does have a non-0 width and height when the timestamp is showing. I guess the span must be floated outside the link or something. I'm not sure why chromium calculates its width differently to Firefox. Anyway, this is definitely a different issue, and I'd argue it's authoring error.

Report back on the mastodon case:

This happens when a subtree root recieves new children and simultaneously gets re-parented into a container accessible that was created because it was re-framed.

Since the subtree root is already known to NVDA it is not re-traversed when its new parent is inserted into the virtual buffer.

Attached file Simplified test case (obsolete) —
Attachment #9123700 - Attachment is obsolete: true

Interestingly, this test case causes re-parenting (but not re-creation) in Chrome, but doesn't seem to break NVDA:
data:text/html,<div class="outer"><div role="group" id="g"></div></div><script>setTimeout(function() { document.querySelector(".outer").setAttribute("role", "region"); g.innerHTML = '<article>a</article>'; }, 3000);</script>
I don't quite understand why. I'm guessing they must fire insertion events before the move, but I don't currently have a way of monitoring raw IA2 events. :(

Note that in Firefox, this causes re-creation (because that's what we do when you change the ARIA role), so this test case isn't useful in Firefox. It's just the only way I was able to cause re-parenting without re-creation in Chrome, since the test case in comment 21 doesn't create an intervening accessible in Chrome.

I spoke to Mick about this. We could probably fix this in NVDA by disallowing moves into a different parent. I'm not a huge fan of this because it can cause pointless subtree re-renders, but this is probably a pretty rare case. Code was actually added in NVDA PR #8930 to do this, but it was removed in PR #10188, as the underlying issue that required it got fixed.

We could fix this in Firefox by ensuring that show events for descendants in the re-parented node get fired before the hide/show on the re-parented node itself. That's going to be tricky, though.

Note that we can't fire show events for descendants of the re-parented node after the hide/show for the re-parented node because this breaks the "one show per subtree" requiement of e10s.

(In reply to James Teh [:Jamie] from comment #22)

Interestingly, this test case causes re-parenting (but not re-creation) in Chrome, but doesn't seem to break NVDA:
data:text/html,<div class="outer"><div role="group" id="g"></div></div><script>setTimeout(function() { document.querySelector(".outer").setAttribute("role", "region"); g.innerHTML = '<article>a</article>'; }, 3000);</script>

I was able to check the events here. As I suspected, Chrome fires a text inserted event on the grouping (for the inserted article) before it fires the text removed/inserted events on the document (for the removal of the grouping and the insertion of the outer region). This is ideally what Firefox would do.

(In reply to James Teh [:Jamie] from comment #23)

We could probably fix this in NVDA by disallowing moves into a different parent.

That fixes the attached test case, but if the insertions are deeper than children of the container being moved (e.g. grandchildren), it fails. The only workable fix in NVDA, then, is to nuke the subtree for hide events.

We could fix this in Firefox by ensuring that show events for descendants in the re-parented node get fired before the hide/show on the re-parented node itself.

I've discovered two problems with this:

  1. Aside from the difficulty of getting this right in CoalesceMutationEvents, we deliberately fire all shows after hides in ProcessMutationEvents, regardless of the order of the event queue.
  2. If we do this, we'll serialise the descendant subtrees twice for e10s, immediately throwing away the first serialisation when we hide the re-parented subtree. That seems egregiously wasteful.

I love how you are documenting your internal dialog.

I swapped out this bug. Mostly from the comment you made earlier that NVDA might disallow moves in the future, so there would be no point in messing with event order. Seems like that doesn't resolve the issue as you mention above.

I don't understand what NVDA "disallowing moves" is if not nuking the whole subtree.

(In reply to Eitan Isaacson [:eeejay] from comment #26)

I don't understand what NVDA "disallowing moves" is if not nuking the whole subtree.

When checking whether a node should be reused, it checks if the parents are different. If they are, it won't allow reuse. The problem is that while that causes the re-parented node to be re-rendered, it doesn't account for the re-parented node's children. They'll still get reused (because their parent is the same), so added grandchildren won't get picked up. I'm trying to figure out how to inherit this down the subtree without walking the entire subtree for hide events, which is something I'm very reluctant to do.

I submitted NVDA PR #10776 to deal with this. I'm still not thrilled about fixing this on the NVDA side - it means we always re-render for moves, whether or not there were insertions as well - but fixing this on the Gecko side seems super painful without a clear path forward.

See Also: → https://github.com/nvaccess/nvda/pull/10776

The NVDA PR was merged, which deals with the issues covered here. Closing as wontfix because we didn't change the events fired by Gecko.

Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → WONTFIX
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: