NumberControl results in 2 calls to initializeAccessibleSlider

[mbarlow12]

From https://github.com/phetsims/coulombs-law/issues/100, we've noticed that NumberControls don't handle focus properly (this also happens in Rutherford Scattering). This problem may lie in the fact that NumberControl calls initializeAccessibleSlider in its top level but also has a constituent HSlider that makes the same call. What results is the following PDOM structure:

<input data-focusable="true" id="376-440-453-611-647-646" data-trail-id="376-440-453-611-647-646" type="range" aria-orientation="horizontal" step="1" min="1" max="1000" aria-valuetext="50 kilograms" style="width: 1px; height: 1px;">
    <button tabindex="-1" data-focusable="false" id="376-440-453-611-647-646-645-644-643-619" data-trail-id="376-440-453-611-647-646-645-644-643-619"></button>
    <button tabindex="-1" data-focusable="false" id="376-440-453-611-647-646-645-644-643-624" data-trail-id="376-440-453-611-647-646-645-644-643-624"></button>
    <input data-focusable="true" id="376-440-453-611-647-646-645-644-628" data-trail-id="376-440-453-611-647-646-645-644-628" type="range" aria-orientation="horizontal" step="1" min="1" max="1000" aria-valuetext="50 kilograms" style="width: 1px; height: 1px;">
</input>

I'm not sure if this is causing the odd focus issues, but we should likely investigate how we're constructing the PDOM in this scenario.

[zepumph]

One line of thinking for this issue is if we should be able to "turn off" the call to HSlider's accessibleSlider initialization. Perhaps with an option?

[jessegreenberg]

Agreed. @mbarlow12 mentioned over slack that another option could be to remove the initializeAccessibleSlider call from NumberControl. Using AccessibleSlider in NumberControl may not be necessary now that we have more control over a11y events in scenery.

[jessegreenberg]

I tried a quick and dirty removal of initializeAccessibleSlider, it seems to be working OK. I want to test a few things before moving forward with this.

But I also verified that removing the extra inputs fixed the original bug that was found in https://github.com/phetsims/coulombs-law/issues/100

[jessegreenberg]

Actually, I don't think this is working because of changes in scenery input, which is good because that will make this far easier to patch in the coulombs-law release branch.

[jessegreenberg]

I tested this in Chrome, FF, and Edge and the NumberControls are working in all three and I can no longer produce https://github.com/phetsims/coulombs-law/issues/100.

The arrow buttons and extra slider have been removed from the PDOM. Ill go ahead and commit.

[jessegreenberg]

This is done and was merged into the release branch of CL. Closing.

[jessegreenberg]

During 1/7/18 dev meeting @zepumph voiced concern that we removed AccessibleSlider from number control and kept all of the accessibility implementation in HSlider. Conceptually, the NumberControl behaves like a slider in the PDOM. But the entire a11y instrumentation comes from its child HSlider, so NumberControl's a11y code will be difficult to maintain.

[zepumph]

After further discussion, we think that https://github.com/phetsims/scenery/issues/920 will cover this case well enough. Though it is still up for discussion, the fix here would be to just use the focusin event, or whatever we decide to name the bubbling event for focus on the NumberControl. Closing again. Let me know if anyone disagrees.

[mbarlow12]

While the above fix handles the nested slider inputs, we're left with a PDOM representation that comes from a child node that's not public. In this case, if we need to update attributes or anything else about the PDOM from NumberControl or its parent(s) we're either force to expose the Node that handles the PDOM prepresentation (the slider in this case) or expose methods on NumberControl that manipulate the slider.

[mbarlow12]

One option proposed by @zepumph was to make the call to initializeAccessibleSlider optional for all Slider types. Parent Nodes could then call initializeAccessibleSlider and handle any necessary linking or implement another a11y implementation.

[pixelzoom]

@mbarlow12 you added this in https://github.com/phetsims/scenery-phet/commit/6014b5e3d7aa884e2f267ef7e00faab682f6ef3b:

    // @public - expose setter for aria-valuetext for the slider
    this.setAriaValueText = function( text ) {
      slider.ariaValueText = text;
    };

Should this be @public (read-only), or is the the client allowed to set setAriaValueText?

[pixelzoom]

... and should this we called setSliderAriaValueText, since it's setting "aria-valuetext for the slider"?

[jessegreenberg]

We went over this during 1/14/19 a11y dev meeting:

We proceeded with @zepumph recommendation in https://github.com/phetsims/scenery-phet/issues/452#issuecomment-452540981, accessibility is optionally initialized in Slider (default to true). For NumberControl, this allows us to use all of the methods of AccessibleSlider.js directly on NumberControl, without having to redefine all the methods we need as in https://github.com/phetsims/scenery-phet/commit/6014b5e3d7aa884e2f267ef7e00faab682f6ef3b. We removed setAriaValueText from NumberControl.

By using isAccessible: false in numberControl, we don't have a duplicate representation in the PDOM.

[jessegreenberg]

Closing this issue.

[mbarlow12]

@jessegreenberg this means that clients can set the .ariaValueText property or callsetAriaValueText() directly, correct?

[zepumph]

That's right. NumberControl now has AccessibleSlider mixed into it.

[pixelzoom]

Why does NumberControl have AccessibleSlider mixed into? It's not a slider. Shouldn't that be handled by NumberControl's HSlider subcomponent?

[zepumph]

It's not a slider.

It is indeed! In the PDOM the number control itself is represented as a slider. In the beginning of this issue we thought it may make more sense for NumberControl to try to use HSlider's PDOM representation as its own, but it didn't work for multiple reasons, some of which were illuminated by you in https://github.com/phetsims/scenery-phet/issues/452#issuecomment-453215053.

[zepumph]

I'll also add that the design team decided many months ago (perhaps years?) that the best way to have a number control in the PDOM was with a slider.

[pixelzoom]

So NumberControl is a slider that contains a slider?....

[zepumph]

A NumberControl Node is a UI component that contains Arrow Buttons and a Slider, as well as a readout.

In the PDOM it looks like a label and an input. There are also added listeners to the slider that visually "press" the arrow buttons when interacting with the NumberControl slider with the shift key pressed. Like HSlider, NumberControl uses the mixin/trait AccessibleSlider and as a result has almost no need base a11y related code in the type. The HSlider in a NumberControl has no a11y instrumentation, and doesn't show up in the PDOM at all.

@pixelzoom can you outlines any qualms with the current approach, and list ways in which you would like to see it improved?

[pixelzoom]

NumberControl is a convenience class whose responsibilities are primarily layout of 4 UI components (HSlider, 2 ArrowButtons, NumberDisplay) and wiring those UI components to a NumberProperty. I'm having a difficult time understanding why it requires any additional instrumentation, except possibly for description of that collection of UI components. And I would think that the choice of layout function should determine the order of tab traversal.

[pixelzoom]

Imo, a11y instrumentation of NumberControl is unnecessary. NumberControl is an implementation detail that determines layout, it's not a UI component. The only a11y code that should live in NumberControl is determining the order of tab traversal for it's 4 UI components based on layout.

[pixelzoom]

To clarify...

If I have this in my UI:

... it could have been implemented as individual UI components that I put together, as a NumberControl, or using some other layout manager. But the user doesn't care how it was implemented, because it looks like the same 4 UI components, regardless of how it was assembled. And traversal should behave the same way regardless of how those 4 UI components were assembled; that is, it moves between the 4 UI components.

The one a11y responsibility that NumberControl should have is in determining the order of traversal of its 4 UI components. E.g., if I used this layout for NumberControl (see below), the traversal order should be different than the above layout.

[jessegreenberg]

Having both the tweaker buttons and the slider in the navigation order is redundant because you can increment/decrement the Property by the tweaker button deltas by holding the shift modifier key on the keyboard. So it was decided that the tweaker buttons be removed from keyboard navigation and the entire NumberControl behave like a slider.

When focus is on the NumberControl it looks like this

[pixelzoom]

Why does that require the NumberControl behaving like a slider? Why not have NumberControl remove the ArrowButtons from the navigation order, and then have the slider behave like a slider?

And why is it necessary for the entire NumberControl to have focus, vs just the slider? As explained in https://github.com/phetsims/scenery-phet/issues/452#issuecomment-454259871, that will result in similar collections of controls not having this type of focus. NumberControl is an implementation detail for handling layout, it's not a UI control.

[pixelzoom]

@zepumph said:

I'll also add that the design team decided many months ago (perhaps years?) that the best way to have a number control in the PDOM was with a slider.

Good to know. Also surprising that the design team doesn't seem to be interested in including the original designer/implementer (that would be me) in the loop. This is the first I've heard about it. That's an approach that you might want to reconsider.

[jessegreenberg]

Why not have NumberControl remove the ArrowButtons from the navigation order, and then have the slider behave like a slider?

Right, that is what we initially did for this issue. But then we found that we will want the functions in AccessibleSlider to be available with each NumberControl. For example, on a NumberControl you can call setKeyboardStep or setAriaValueText directly. Alternatives involved making the NumberControl slider public or adding several functions like https://github.com/phetsims/scenery-phet/commit/6014b5e3d7aa884e2f267ef7e00faab682f6ef3b to NumberControl.

Also surprising that the design team doesn't seem to be interested in including the original designer/implementer (that would be me) in the loop. This is the first I've heard about it. That's an approach that you might want to reconsider.

Yes I agree, that sounds great!

[zepumph]

Is this issue ready for close? I think everything has been implemented. Anyone feel free to reopen if there is more to discuss.

[zepumph]

@jessegreenberg and I are rethinking our previous decision on this issue, especially in light of https://github.com/phetsims/scenery-phet/issues/519. Reopening.

[zepumph]

Our proposal:

• NumberControl's HSlider is the accessible slider
• NumberControl doesn't mixin accessible slider
• NumberControl's arrow buttons are tweaked to work with the nested a11y slider implementation
• NumberControl keeps its same focus highlight.
• All accessible options/content will be passed via options.sliderOptions
• NumberControl will expose a this.slider field to keep available its public a11y api (things like slider.setAccessibleName() etc).

We think this addresses our concern for good api from https://github.com/phetsims/scenery-phet/issues/519, and also takes into account @pixelzoom's considerations about design patterns for composite components.

@jessegreenberg will take on this change. Thanks!

[jessegreenberg]

I think this also means we can remove the isAccessible option from Slider.js. Is that worth keeping around?

[zepumph]

I don't think so. Let's get rid of it.

[jessegreenberg]

This is done in the above commits. I reviewed sims in perennial/accessibility and couldn't find any sims using AccessibleValueHandler API through NumberControl. The original discussion around this was because we needed to set ariaValueText outside of NumberControl. But that function has since been removed as well. I left this.slider public because I still think the AccessibleValueHandler API should be available. But it meant no sim changes were required from this.

aqua tests are passing and I tested NumberControls with a keyboard to make sure key strokes still work. @zepumph could you please review?

[zepumph]

Review:

I reviewed sims in perennial/accessibility and couldn't find any sims using AccessibleValueHandler API through NumberControl.

• This doesn't seem quite complete to me. We would also need to see where any Accessibility.js options were being passed to NumberControl instead of to the slider via sliderOptions. I looked through all usages of NumberControl.call and the callee constructions. I also looked at all usages of new NumberControl and usages of createNumberControl. I found that Rutherford Scattering and the scenery-phet demo were passing a11y options directly to the NumberControl. I updated and committed those fixes, @jessegreenberg please review.

• I set all the prototype methods of Accessibility to Accessibility.object, and then added this to the bottom of NumberControl, which throws an error if you called an a11y method not in the white list on a NumberControl instance:

  
   Object.keys( Accessibility.object ).forEach( key => {
     const methodWhiteList = [ 'initializeAccessibility', 'hasAccessibleContent','hasAccessibleOrder',
       'setGroupFocusHighlight', 'getAccessibleVisible', 'onAccessibleAddChild', 'getAccessibleOrder',
       'accessibleAudit', 'getEffectiveChildren', 'isFocusable', 
        'disposeAccessibility', 'setAccessibleOrder' ];
     if ( methodWhiteList.indexOf( key ) === -1 &&
          !Object.getOwnPropertyDescriptor( Accessibility.object, key ).hasOwnProperty( 'get' ) ) {
       NumberControl.prototype[ key ] = () => { throw new Error( `nono, ${key}` )}
     }
   } );

  I found that there were no NumberControls calling a11y methods in the project.

• Added doc about the appropriate way to provide accessible content to NumberControl.

• searched for isAccessible and only found usages for the member of Sim.

@jessegreenberg please review

[jessegreenberg]

We would also need to see where any Accessibility.js options were being passed to NumberControl instead of to the slider via sliderOptions

Thank you, good catch. And thanks for checking the other things. With that, I think this can be closed, please reopen if you disagree.