1634619 - Slack fires spurious focusable state changes

Status: RESOLVED FIXED

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

Description

[James Teh [:Jamie]]

STR:

1. Focus the message entry text box in a Slack conversation.
2. Press up arrow to focus the message list.
   • Expected: No (or maybe one or two) state change events.
   • Actual: Quite a few focusable state change events for the list, groupings, etc.

This happened after the scrollable focusable state change patch from bug 1629162. If I revert that, these go away.

As far as I can tell, the list and the groupings are never focusable.
One possibility is that they have overflow: hidden. In that case, the element will never get scroll bars and thus won't ever be focusable, so we shouldn't fire in that case.
Another possibility is that Slack tweaks the overflow property several times for some reason. I'm not sure how we could deal with that.

One thing that puzzles me is that overflow: scroll is supposed to always yield scroll bars. However, this isn't focusable:
data:text/html,<div style="height: 100px; overflow: scroll;">x

The large number of events that Slack fires is a cause of user observable perf problems on Windows, so it'd be good to reduce this. Options:

1. Back this out for now. We do want this going forward, but we have no specific use case for it right now. We'd leave the tabindex patch, though.
2. Debug and try to fix in core.
3. Suppress focusable state changes in Windows only. I've no idea whether this impacts other platforms, but on Windows, extraneous property change events hurt us.

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Bug 1634619 - Don't fire focusable changes on overflow of non overflow:auto.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1629162

Comment 3

[Rachel Tublitz [:rachel]]

Jamie; do we have a reviewer/assignee here?

Comment 4

[James Teh [:Jamie]]

Slack have just gone and made perf a lot worse than it already was, so I don't think it's useful to try to evaluate Slack perf with respect to this. That said, given the perf issues associated with events, I'm reluctant to have known cases of unnecessary events. Since we don't "need" these events right now, Eitan and I agreed to back out that patch and potentially reintroduce it with fixes in future when needed. However, we'll keep the tabindex patch, since that helps Dev Tools and only fires events when necessary.

Regarding the patch Eitan posted in comment 1, he noted on a call that it causes test failures because if overflow: auto is cleared, an under/overflow DOM event gets fired, but since it's no longer overflow: auto, we don't fire a focusable state change even though focusability did actually change.

Comment 5

[Eitan Isaacson [:eeejay]]

Attachment: Bug 1634619 - Back out focusable state changes on overflow events. r?Jamie

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5b6c29e052f4
Back out focusable state changes on overflow events. r=Jamie

Comment 7

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/5b6c29e052f4

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:eeejay, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 9

[Eitan Isaacson [:eeejay]]

Comment on attachment 9146924 [details]
Bug 1634619 - Back out focusable state changes on overflow events. r?Jamie

Beta/Release Uplift Approval Request

• User impact if declined: Performance issues with screen readers in Slack
• 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): We are just cleanly reverting a change that went in late last cycle.
• String changes made/needed:

Comment 10

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9146924 [details]
Bug 1634619 - Back out focusable state changes on overflow events. r?Jamie

Looks safe, approved for 77 beta 7, thanks.

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/59a224f75189