1597916 - Removing a pseudo-element doesn't fire a textRemoved event

Status: RESOLVED FIXED

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

Description

[James Teh [:Jamie]]

STR (with the NVDA screen reader):

1. Open this test case:
   data:text/html,<style>.before::before { content: 'before' }</style><button class="before" onclick="this.className = ''; document.body.append(document.createElement('hr'))">go
2. Press the go button.
3. Press control+home.
   • Expected: NVDA should say "button go"
   • Actual: NVDA says "button beforego"
4. Press NVDA+f5 to refresh the buffer.
5. Press control+home.
   • Observe: NVDA says "button go" as it should have in step 3.

We correctly push hide and reorder events for the pseudo-element content. We don't fire a textRemoved event like we should. I don't know why. In many cases, the reorder event would be enough to update NVDA's buffer. Unfortunately, in this case (and this could - and almost certainly does - happen in the wild), the reorder event gets coalesced to the document, since we also add a node to the document in the same tick. So, NVDA never gets told to update the text of the button.

Marking as a regression from bug 686400 because before that, we would have re-created the accessible for the button. While not ideal for other reasons, that does effectively mean this problem didn't exist before. I'm also suspicious of bug 1572811. While that fix is necessary to fix a crash, it does mean we might bail on firing text events depending on where things happen.

I'm also wondering whether this could be the cause of bug 1592518. That almost certainly isn't related to pseudo-elements, but it's very likely due to missing text events.

Comment 1

[James Teh [:Jamie]]

Sometimes, I hate it when my hunches are correct. Commenting out the code from bug 1572811 "fixes" this. The question is: how to fix this without destabilising other things?

Comment 2

[James Teh [:Jamie]]

I also can't reproduce bug 1592518 with that code commented out.

Comment 3

[Marco Zehe (:MarcoZ)]

I still can partially reproduce it. The case that timeline contents doesn't appear on Mastodon for me until I do a buffer refresh is still reproducible with this try build.

Comment 4

[Eitan Isaacson [:eeejay]]

Attachment: Bug 1597916 - Allow frameless containers when dispatching text removed events. r?MarcoZ!

This reverts the change I made in bug 1572811, and instead fixes a much
more narrow case when the inner table frame is mid-construction.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/491c74e06395
Allow frameless containers when dispatching text removed events. r=Jamie

Comment 6

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/491c74e06395

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Is there a user impact here which justifies Beta uplift consideration for Fx73 or can this fix ride Fx74 to release?

Comment 8

[James Teh [:Jamie]]

Contrary to comment 2, I can still reproduce bug 1592518 on Twitter with a browser restart even with this patch.

That said, I think anything that causes updates not to be reflected in screen reader browse modes is potentially pretty serious. Eitan, are you comfortable with an uplift here?

Comment 9

[James Teh [:Jamie]]

This also fixes the crash in bug 1601998.

Comment 10

[Eitan Isaacson [:eeejay]]

Comment on attachment 9118432 [details]
Bug 1597916 - Allow frameless containers when dispatching text removed events. r?MarcoZ!

Beta/Release Uplift Approval Request

• User impact if declined: Screen reader users will have a stale buffer in certain situations
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):
• String changes made/needed:

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9118432 [details]
Bug 1597916 - Allow frameless containers when dispatching text removed events. r?MarcoZ!

Fixes a bug which can cause stale content and crashes in some circumstances for screen reader users. Approved for 73.0b5.

Comment 12

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e7bbba7189da