1455511 - Sliders should always prune children (AKA can't switch to NVDA focus mode for sliders containing divs)

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[James Teh [:Jamie]]

STR (with NVDA):
1. Open this URL:
data:text/html,<div tabindex="0" role="slider" aria-valuenow="0"><div>text</div></div>
2. Focus the document and press control+home to move to read the first line.
Expected: NVDA should say "slider  0"
Actual: NVDA says "slider  text"
3. Press enter to switch to focus mode (as if to interact with the slider).
Expected: NVDA should switch to focus mode.
Actual: It doesn't.

This is because sliders expose children when there isn't just a single text leaf. So:
* NVDA descends to render the text inside the inner div (result from step 2 above).
* When NVDA checks if it needs to switch to focus mode, it queries the deepest node (excluding text leaves) at the cursor. In this case, it finds the inner div, so it doesn't switch to focus mode (result from step 3 above).

This is due to bug 865997. While I think that decision is appropriate in most cases, I don't think it is for sliders because of the above issue. I propose we prune for sliders but leave the rest alone. Note that Chrome does exactly this.

This currently causes problems for Video.js: https://videojs.com/

Related issues:
NVDA: https://github.com/nvaccess/nvda/issues/5837
Video.js: https://github.com/videojs/video.js/issues/3217

Comment 1

[James Teh [:Jamie]]

Attachment: Bug 1455511: Always prune the accessibility tree for sliders.

It doesn't make sense for a slider to have descendants and this confuses NVDA.

Review commit: https://reviewboard.mozilla.org/r/238852/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/238852/

Comment 2

[James Teh [:Jamie]]

Comment on attachment 8970082 [details]
Bug 1455511: Always prune the accessibility tree for sliders.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238852/diff/1-2/

Comment 3

[alexander :surkov (:asurkov)]

Comment on attachment 8970082 [details]
Bug 1455511: Always prune the accessibility tree for sliders.

https://reviewboard.mozilla.org/r/238852/#review244584

::: accessible/base/nsAccUtils.cpp:439
(Diff revision 2)
>  bool
>  nsAccUtils::MustPrune(Accessible* aAccessible)
>  {
>    roles::Role role = aAccessible->Role();
>  
> +  if (role == roles::SLIDER) {

I'm not sure if every optimizer can handle this, so I would keep that condition in return statement below.

You could put this comment right before the 'if', something like 'special cases section'.

Comment 4

[James Teh [:Jamie]]

Comment on attachment 8970082 [details]
Bug 1455511: Always prune the accessibility tree for sliders.

https://reviewboard.mozilla.org/r/238852/#review244584

> I'm not sure if every optimizer can handle this, so I would keep that condition in return statement below.
> 
> You could put this comment right before the 'if', something like 'special cases section'.

To clarify, are you saying you want me to remove the separate "if" condition and instead make it part of the "return" statement? The reason I'm confused is that you later mention putting a comment before the "if".

I'll make the change as you request, but my concern is that it makes the code far less readable for the sake of premature optimisation. :) It'd be great if you can take another look to make sure this is what you wanted.

Thanks!

Comment 5

[James Teh [:Jamie]]

Comment on attachment 8970082 [details]
Bug 1455511: Always prune the accessibility tree for sliders.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238852/diff/2-3/

Comment 6

[alexander :surkov (:asurkov)]

Sorry for such confusing pointers :) You are correct, that's what I suggested. It's not a bottleneck I believe, but it's a good practice to avoid branching. I like your right-in-return style comments. Perhaps you could use indentation for () blocks for better readability, for example

return
  role == SLIDER ||
  (
    (
     role == MENUITEM ||
     role == BUTTON
    ) &&
    aAccessible->ContentChildCount() == 1
  );

I'm not sure if that goes with moz code style though.

Comment 7

[James Teh [:Jamie]]

Comment on attachment 8970082 [details]
Bug 1455511: Always prune the accessibility tree for sliders.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238852/diff/3-4/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3f80d2aaef43
Always prune the accessibility tree for sliders. r=surkov

Comment 9

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/3f80d2aaef43