NumberControl results in 2 calls to initializeAccessibleSlider

[mbarlow12]

From phetsims/coulombs-law#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 phetsims/coulombs-law#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 phetsims/coulombs-law#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 phetsims/scenery#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 6014b5e:

    // @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]

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 #452 (comment), 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 6014b5e 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 #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 #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.