phetsims / scenery

Scenery is an HTML5 scene graph.
MIT License
52 stars 12 forks source link

How to handle Space vs Enter for buttons #939

Closed zepumph closed 3 years ago

zepumph commented 5 years ago

From https://github.com/phetsims/scenery/issues/934, in the browser they are different, @pixelzoom and @zepumph discussed at one point whether or not that adds value to phetsims, it would be good to discuss that further, and then implement from there. This also may relate to how we are handling sun buttons, and that should be looked at in this issue to.

Related potentially to https://github.com/phetsims/sun/issues/424, there is likely little use for the default behavior of Enter to be fire multiple events.

Even though it is browser default, we know we want to support users at PhET that are younger, and may not have as much keyboard experience, it could easily be overwhelming to have a button fire many times without a good, pedagogical reason to. Expanding on that, then why would we make that the default behavior for Enter?

I think from discussing with @pixelzoom that we are mostly in agreement (correct me if I'm wrong) that the default behavior for both should (a) be the same, and (b) be a single click for buttons. See https://github.com/phetsims/scenery/issues/931 for adding support for options that would deal with "fire on hold" features.

emily-phet commented 5 years ago

I believe @zepumph will bring this up in today's meeting. Some things that seem relevant to go over in that meeting include:

jessegreenberg commented 5 years ago

Our buttons respond to the click event which is triggered at various times depending on whether or not an assistive device is in use. When an assistive device is enabled the click event is usually not accompanied by keydown or keyup events. For example, the events shown in this test will be different depending on whether NVDA is enabled: https://jsfiddle.net/gtjnfw8d/4/

Since we can't always listen to keyup and keydown, I don't see a way we can detect key releases to make enter and spacebar behave the same way or only fire buttons once per key press.

Having a good way to delay firing listeners on the click event in https://github.com/phetsims/scenery/issues/931 seems like our best option.

pixelzoom commented 5 years ago

@emily-phet said:

.... Maybe having a pretty long delay for enter before repeated fires on hold - and maybe a shorter delay for 'fire on hold' buttons.

This would match how "fire on hold" typically works. Both the delay and interval should be configurable. Some PhET UI components have this feature, some do not. Sun push buttons do support this feature, see PushButtonModel:

      // fire-on-hold feature
      fireOnHold: false,
      fireOnHoldDelay: 400, // start to fire continuously after pressing for this long (milliseconds)
      fireOnHoldInterval: 100, // fire continuously at this interval (milliseconds)
pixelzoom commented 5 years ago

@jessegreenberg said:

... Since we can't always listen to keyup and keydown, ...

Why can't we? And why are we explicitly writing handlers that listener for click? Should we be using a scenery listener that abstracts out those specifics?

jessegreenberg commented 5 years ago

When an assistive device is enabled the click event is usually not accompanied by keydown or keyup events.

Assistive devices can change which DOM events are sent, even for identical user interactions. For instance, press "enter" on a button without NVDA and keydown, click, keyup events will be sent in that order. But turn on NVDA and ONLY the click DOM event will be sent. There are no keydown or keyup events to listen for. That is why we are writing handlers for click.

pixelzoom commented 5 years ago

Hmm... Then ComboBox will need to change - it's currently listening for keyup (it was previously listening for keydown.) And why are things like PhetMenu looking for keydown? Do those need to change?

Are keydown and keyup only absent with NVDA for Space and Enter? I'm guessing that must be the case, otherwise we couldn't get other key presses (arrows, etc.)

jessegreenberg commented 5 years ago

The DOM events are determined by the role of the element that has focus. In ComboBox, focus is on an element with the ARIA role option, so keyup will be received. For elements with the button role, keydown and keyup are never sent for any key press. This is so the user can continue to read with the virtual cursor if focus is on a button. For example, pressing "b" while focus is on a button with screen reader enabled will not send a keydown event, but move focus to the next button in the page.

zepumph commented 5 years ago

Here is a stack overflow I found that helps describe why the default browser behavior is the way it is

https://stackoverflow.com/questions/16090578/why-do-enter-and-space-keys-behave-differently-for-buttons

pixelzoom commented 5 years ago

Re the stackoverflow article... I see conjecture, but no authoritative reference. If we accept this hypothesis, which is in the response with the largest number of votes:

Enter is to accept, Esc is to reject, Space bar is to press the focused button. So if the default button is the OK, when you press Enter you actually accept the changes and not press the button.

If the semantics of Enter are "accept", then why does it fire repeatedly when the button is held down?

pixelzoom commented 5 years ago

@jessegreenberg Thanks for the clarification. I didn't realize that you were referring exclusively to elements with role button.

pixelzoom commented 5 years ago

Here's something else that makes no sense to me, observed in the current implementation of ComboBox. If the combo box button has focus, pressing Enter pops up the listbox, and releasing Enter immediately pops down the listbox.

Demonstrated in Molarity:

  1. Tab to the combo box, so that its button has focus.
  2. Press and hold the Enter key. The button's click listener fires. The listbox pops up and the item that is currently selected is given focus.
  3. Release the Enter key. The selected item's keyup listener fires, and the listbox pops down.

So by clicking Enter (which supposedly means "accept") the related events get sent to whatever UI component happens to have focus at the time. That Enter was meant for the button, and only the button. The listbox and its items shouldn't be receiving anything related to that user action.

jessegreenberg commented 5 years ago

That is another problem, and also happens with click event, not just keyup https://jsfiddle.net/3bgkcdes/ We should have a general solution for this if possible, though the challenges will come from the same lack of events we face in this issue.

I think we should create another issue to investigate this (https://github.com/phetsims/scenery/issues/942). Working on the timing for firing buttons with the keyboard (probably in PressListener) will be continued in https://github.com/phetsims/scenery/issues/931. Does everyone agree that this can be closed?

terracoda commented 5 years ago

@zepumph, @pixelzoom, @jessegreenberg, currently from phettest and using Safari on Mac OS (with or without a screen reader on), the button that pops up the the list of options is not operable with the Enter key, only the Space key.

It's a button, so it needs to be operable via the Enter key.

Perhaps this article by Paul J. Adam from Deque on building accessible buttons will help us gain some understanding. It is important to note, however, that our implementation is a pop-up button. I don't think we are using aria-expanded, but rather, aria-haspopup="listbox".

Note, we are using a real button, more specifically, a real pop-up button so our job should be a lot easier. From article:

When a JavaScript onclick event is attached to a native

Further down in the article, for native button's:

To make a button keyboard operable all you need is the onclick event and it works with Enter and Spacebar keys!

The pattern that we adapted for the PhET Combobox if I am remembering correctly from the example of a Collapsible Drop Down List. Of course, we pop up rather than down.

The short of it is that we need the Enter key to work on the button that pops up the list.

Enter key should pop up the list and the list should stay visible with focus shown on the selected solute until Space, Enter or Esc are pressed. Arrow keys are used to move focus within the list of options.

terracoda commented 5 years ago

I think we can close this issue if we agree that we should be supporting both Space and Enter keys on buttons.

terracoda commented 5 years ago

Please re-assign to me as needed.

terracoda commented 5 years ago

Adding links to previous research in other issues https://github.com/phetsims/a11y-research/issues/64 and https://github.com/phetsims/sun/issues/314

I appreciate all the effort going into the PhET Combobox!

pixelzoom commented 5 years ago

@terracoda reported:

... currently from phettest and using Safari on Mac OS (with or without a screen reader on), the button that pops up the the list of options is not operable with the Enter key, only the Space key.

Thanks @terracoda. This specific issue with ComboBox is being tracked in https://github.com/phetsims/sun/issues/447. A general solution is preferrable, and workaround in ComboBox is a last resort, and we've been deferring that issue until this more general issue is addressed.

pixelzoom commented 5 years ago

@terracoda said:

I think we can close this issue if we agree that we should be supporting both Space and Enter keys on buttons.

I DO think we're all in agreement that Enter and Space both need to be tracked. But this issue deals with how to support them both. Specifically, we need to address differences when Enter and Space fire, how and when events are delivered to the sim, click vs keyup vs keydown approaches, what should happen when either button is held down, what options should be available to the programmer (e.g. fireOnHold and associated delays), what should be handled by Scenery event listener, etc. So I think we have some work to do before this issue is ready to close.

pixelzoom commented 5 years ago

Over in https://github.com/phetsims/sun/issues/447#issuecomment-460010180 (the problem specific to ComboBox), I said:

... I added logging of a11y keyboard events for ComboBox and its components. Run Molarity with ?a11y&log and you'll see these events logged to the browser console. This demonstrates the problem that is specific to ComboBox, as well as the more general issues with Enter and Space keys.

It would be great if everyone played around with this before our Wednesday 2/6 conference call, so that we're all up to speed on what the current behavior is.

terracoda commented 5 years ago

@pixelzoom thanks for the clarification in https://github.com/phetsims/scenery/issues/939#issuecomment-459781374. I was pretty sure we were all on the same page, and just wanted to be sure.

zepumph commented 5 years ago

Notes from discussion today:


CM: PhET, at the beginning of HTML5 work, decided not to mimic HTML buttons in phet sims.
TS: if PDOM buttons act differently then HTML buttons, then it is potentially important to convey that to 
the user (with custom interaction instead of using native HTML buttons in the PDOM)
CM: what are people "relying on" in terms of difference between ui of enter and space? Because it will be
 easier to inline them in the long run
    TS and JG: don't think that many basic users (not super users) that understand the nuance of 
difference between the two.

What do we want:

We want space and enter to both be a way of firing a button.
Currently space fires on up, enter fires on down, and will continue to fire.

one thought: 
have enter and space bar both run on keyup.

behavior for fire on hold:

for buttons, here are the way that events work 

no screen reader
    space
    keydown 
    keydown 
    keyup
    click

    enter
    keydown
    click
    keydown
    click
    keyup

yes screen reader:
    space
    click

    enter
    click
    click

TS, perhaps using enter is "built in fire on hold"
CM: that doesn't give us a way to specify parameters, like we do in PushButtonModel

Seems like a priority to investigate turning off fireOnHold for enter key

CM: maybe we should make our own thing instead of trying to work with HTML buttons in the PDOM.

JG: does changing to "<div role=button>"  solve all our problems. Perhaps screen readers
TS: as a reference: https://www.deque.com/blog/accessible-aria-buttons/
MK: but does it change use for screen readers. 
    For example can you use 'b' to navigate through div role buttons?
        TS: yes hopefully the role will handle that for you.
JG: we tried div with button role before but decided, after testing, that things weren't great (past issue reference to come) 
TS and many: We really hope that a div with button role gives us all events, but we may find that screen 
readers suppress keydown/up.
CM JG: hopefully we don't need click 

Ideal behavior for buttons:
(perhaps we can do this with div role button)

In a mixin like `AccessibleButton.js`
space and enter: act the same (ideally) as PushButtonModel

// default to fire on keyup
fireOnDown: false, // true: fire on pointer down; false: fire on pointer up if pointer is over button
fireOnHold: false, // is the fire-on-hold feature enabled?
fireOnHoldDelay: 400, // start to fire continuously after pressing for this long (milliseconds)
fireOnHoldInterval: 100,
a11yFireOnHold: `default to fireOnHold but overridable just for a11y` same with other values.
emily-phet commented 5 years ago

Reflection from today's meeting:

Thoughts?

pixelzoom commented 5 years ago

@emily-phet asked:

... Thoughts?

@jbphet was the primary architect of sun buttons, so it might be good to get him involved.

@jonathanolson is the primary architect of scenery, so he's in a good position to help with how input is handled by scenery.

terracoda commented 5 years ago

I agree with @emily-phet that it would be great to have some collaborative time

to assess the current range of available buttons and to organize, design, and create the central button behavior and necessary parameters to meet the current and foreseeable button needs.

Currently, the names for buttons describe a mixture of different button attributes some related to the how the button functions and some related to how the button looks.

We have already had many long discussions about whether some buttons should be represented in the PDOM as an HTMLcheckbox.

To complicate things further, some PhET buttons look like buttons but in terms of HTML they would not have a button role. For example, the PhET radio buttons would likely be represented as actual HTML radiobuttons in the PDOM, even though they can visually look like a group of labelled square buttons in a simulation.

Many of the visual qualities of PhET buttons are designed to scaffold students in a particular way. I'd like a collaborative re-categorization to focus around scaffolding across multiple modalities for diverse users.

jessegreenberg commented 5 years ago

Also, before any changes are made we will want to carefully test that <div role="button"> works well on the AT we want to support.

This could also have implications for supporting switch like devices in the future, since those have no keyboard. Depending on how they interact with div role="button" buttons we may still need to observe click.

terracoda commented 5 years ago

@jessegreenberg, I said this in our meeting, but I will repeat it here as we experiment with <div role="button">. Aria roles give us only "semantics". The Accessibility Tree will tell the AT that this thing is a button. We will have to provide all the functionality to make it accessible.

Since the Accessibility Tree will say it is a button, that should give us some hope that the AT will recognize it as a button and do its proper thing from the the AT's perspective - i.e. work with screen reader commands.

jessegreenberg commented 5 years ago

I totally agree @terracoda, that is true. I made this JSFiddle to test: https://jsfiddle.net/cepk8m69/1/

This fiddles tests a native button a div role="button and a div aria-roledescription="button". Ill give it a try with NVDA.

Log from testing without NVDA:


    Native Button: keyup event --> tab to native button
    Native Button: keydown event --> enter down on native button
    Native Button: click event
    Native Button: keyup event --> enter up on native button
    Native Button: keydown event --> tab to button role
    Button Role: keyup event
    Button Role: keydown event --> enter down on button role
    Button Role: keyup event
    Button Role: keydown event --> tab to div with aria-roldescription
    Button RoleDesc: keyup event
    Button RoleDesc: keydown event --> enter down on button with aria-roldescription
    Button RoleDesc: keyup event

Log from testing with NVDA:

    Native Button: click event --> enter down on native button
    Native Button: keydown event --> tab to button role
    Button Role: keyup event
    Button Role: click event --> enter down on button role
    Button Role: keydown event --> tab to button with aria-roledescription
    Button RoleDesc: keyup event
    Button RoleDesc: click event --> enter down on div with aria-roledescription
jessegreenberg commented 5 years ago

So after testing with NVDA, I am seeing that none of the elements in https://jsfiddle.net/cepk8m69/1/ receive keyup and keydown events, they only receive click events with enter and spacebar. Furthermore, without NVDA, neither of the non-native buttons receive click events! This surprises me. The only way for us to receive keyup and keydown events is if the screen reader is in "forms mode", which it only is for certain ARIA roles.

So even if div role="button worked well everywhere, it would not solve this issue.

pixelzoom commented 5 years ago

@jessegreenberg thanks for investigating div role="button".

So even if div role="button worked well everywhere, it would not solve this issue.

Bummer :(

jessegreenberg commented 5 years ago

With this "button", we get keyup and keydown events as we would hope. But it is a monster, and I have only tested on NVDA.

<div id="application-button" role="application" aria-roledescription="button" tabindex="0" aria-label="Button Application">Button Application</div>

role="application" to receive keyup and keydown events. aria-roledescription so the AT describes the role of the elements as "button" instead of "application". aria-label so that the accessible name is read read (from https://github.com/phetsims/a11y-research/issues/116).

But it can't be found by normal screen reader strategies. And it forces "forms" mode so the user can't continue reading with virtual cursor. I don't think we should do this for all buttons yet.

pixelzoom commented 5 years ago

I'm going to unassign myself from this issue, but will remain subscribed so that I can follow progress. Mention me if you need any input.

terracoda commented 5 years ago

@jessegreenberg, thanks for making a test so quickly. I'm not sure the test code has everything that is needed.

I am reading through Paul J. Adam's article on accessible buttons. Could you verify that all the pieces mentioned in the article are included in your test code. The author notes that you need the following to make a <div role="button"> accessible:

Code pieces:

tabindex=0, role=button, onkeydown, .keyCode == 13, .keyCode == 32, event.preventDefault()

**Why pieces are needed***

  1. tabindex="0" to make the div focusable which you already have,
  2. onkeydown event as tabindex="0" only makes the div focusable, not operable
  3. .keyCode == 13, .keyCode == 32 to check if the Enter or Space keys are pressed
  4. .preventDefault() to prevent Space key's default behaviour of scrolling

Then in addition, we have to manage focus once the button is activated, but I don't think that is needed for your example.

@jessegreenberg, apologies if the mentioned pieces are all in place. I have trouble fully understanding the javascript in the js fiddle example.

As an aside, unrelated to this issue I just want to note that aria-roledescription is no use to us in this case; it is not an ARIA role. It is just a way to rename an element, so a screen reader hears the custom name rather than the HTML name for the element. The element in question would still need a valid role. In my opinion, the aria-roledescription attribute could be very useful to us to provide specific names for sim objects, but at the moment it is not well supported (only supported in VoiceOver). Thus far, we have only tried used aria-roledescription for custom interactions with role="application", like the Yellow balloon in BASE and the Chemistry book in Friction. We haven't explored the benefit of using aria-roledescription on an native HTML elements, or custom elements with native roles.

jessegreenberg commented 5 years ago

@terracoda and I met to discuss the examples. We tried something more like the example from https://www.deque.com/blog/accessible-aria-buttons/, where the listeners were added as inline attributes. Here is the link to the JSFiddle: https://jsfiddle.net/07gs8cfe/1/

The HTML that we tried:

<div role="button" tabindex="0" onclick="alert('custom ARIA button activated')" onkeydown="if(event.keyCode==13){alert('custom ARIA button activated with Enter key')}; if(event.keyCode==32){event.preventDefault();alert('custom ARIA button activated with spacebar key')}">Inline Button</div>

We tested with NVDA/Firefox and results matched the behavior in https://github.com/phetsims/scenery/issues/939#issuecomment-461524582. When NVDA is off we only get keydown/keyup events. When NVDA is on, we only get click events.

In the example from deque, keyup and keydown are added for non-AT use and click is added just for screen readers. For sims to behave the same with and without screen readers, we would need to support only receiving click events anyway. So I don't see benefit in PhET doing this since it creates two separate implementations for buttons.

terracoda commented 5 years ago

@jessegreenberg, I tried the js fiddle example with VoiceOver. For the inline div role button, the dialogs popped up, and no events were logged. For the others, the events were logged and there were no click events. Events were even logged even when I tabbed to the buttons. Not sure if that was supposed to happen.

terracoda commented 5 years ago

@jessegreenberg regarding https://github.com/phetsims/scenery/issues/939#issuecomment-461824374

But it can't be found by normal screen reader strategies. And it forces "forms" mode so the user can't continue reading with virtual cursor. I don't think we should do this for all buttons yet.

I agree, your "monster" is a bit wild to consider.

In interviews, thus far, screen reader users (adult screen reader users) have always been a bit confused about the behaviour of objects with role application.

@jessegreenberg, question: is aria-roledescription now recognized by NVDA?

jessegreenberg commented 5 years ago

OK, thanks for testing @terracoda, that is good to know. And yes aria-roledescription is now working with NVDA! 🎉

@zepumph and I also reviewed the list of roles in https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques, but didn't see any others that would be appropriate to use or fix this problem.

So based on the above comments, we are back to the fact that elements with "role=button" will not receive keydown and keyup events with screen readers. Our two options are 1) Continue with click events for most PhET buttons. For things like RoundMomentaryButton where we must have keyup/keydown to get the button to work correctly, we use something different so we get these events, possibly https://github.com/phetsims/scenery/issues/939#issuecomment-461634445 (with role="application" aria-roledescription="button"). 2) Use https://github.com/phetsims/scenery/issues/939#issuecomment-461634445 (with role="application aria-roledescription=button) everywhere so all PhET buttons are the same, and we get the ability to use keydown and keyup on all buttons.

If we do option 2 for buttons, would we want to do this for checkboxes, links, and every other kind of native input element too? If we go down that road, all of our input elements become non-standard for a11y, and likely confusing for a screen reader user. We lose two of the greatest benefits of the PDOM, native interaction with assistive devices and ability to find UI elements with common screen reader navigation strategies.

So I think we should go with option 1. But there are drawbacks to each, so it would be good to discuss with the team again.

jessegreenberg commented 5 years ago

@terracoda @emily-phet @zepumph @pixelzoom can you please review https://github.com/phetsims/scenery/issues/939#issuecomment-462521440? If everyone is on board with option 1, I think we can close this issue and continue with the specific issues that have come out of this one. If not, I think we should have another brief discussion.

pixelzoom commented 5 years ago

... If everyone is on board with option 1, ...

Does option 1 support our requirements? I was under the impression that relying solely on click events made it difficult (impossible?) to implement the button options that we agreed were essential in https://github.com/phetsims/scenery/issues/939#issuecomment-461103903, that is:

fireOnDown: false, // true: fire on pointer down; false: fire on pointer up if pointer is over button
fireOnHold: false, // is the fire-on-hold feature enabled?
fireOnHoldDelay: 400, // start to fire continuously after pressing for this long (milliseconds)
fireOnHoldInterval: 100,
a11yFireOnHold: ... // default to fireOnHold but overridable just for a11y` same with other values.
jessegreenberg commented 5 years ago

Option 1 will support those options but only where we need them. The HTML in those cases will not use role="button" so that we get the keydown and keyup events we need. But the rest of PhET buttons will continue to use role="button" and listen for click events so they support all of the standard accessibility features.

The down side is that not all buttons will listen to the same DOM events, but it supports all of our requirements.

pixelzoom commented 5 years ago

Option 1 will support those options but only where we need them. ...

All PhET buttons currently have the options that we identified. So my feeling is that all PhET buttons need those options. So what buttons are you planning to use role="button" for? For example, would you use it for ResetAllButton, which is currently (and undesirably) firing repeatedly with the Enter key?

jessegreenberg commented 5 years ago

I would use role="buton" for ResetAllButton. We can address that issue generally by limiting the frequency of calling listeners in response to click events. Maybe that will use options a11yFireInterval or just fireOnHoldInterval. But I don't believe buttons like ResetAllButton need any of the other options for keyboard/alternative input.

pixelzoom commented 5 years ago

@jessegreenberg said:

.... But I don't believe buttons like ResetAllButton need any of the other options for keyboard/alternative input.

Then you'll need to prevent those options from being changed and passed to the superclass. Because RoundPushButton (and the other sun button base classes) are general and do need to handle those options.

terracoda commented 5 years ago

Re https://github.com/phetsims/scenery/issues/939#issuecomment-462521440 I do not believe option 2 is at all advisable. The use of role="application has not yet been understandable by users in interviews (i.e. BASE, Faraday's Law, Friction), mind you these were all drag and drop type interactions.

The buttons that we have implemented thus far have been intuitively accessible to participants in interviews. These buttons include:

We have not yet worked on a sim with a "fire-on-hold" button. We do not know what keyboard users or screen reader users will do when they come across a fire-on-hold button like the eye dropper button. They might know exactly what the differences are between Enter and Space key or they might try both and realize that one adds drops while the other keeps the eye dropper squeezed and adds a stream.

@jessegreenberg, I am really uncomfortable with the idea of re-coding the Reset All button. It works well for users as it is. I don't think users will hold down the button to hear multiple alerts. Are the multiple firings a performance issue?

pixelzoom commented 5 years ago

@jessegreenberg, I am really uncomfortable with the idea of re-coding the Reset All button. It works well for users as it is. I don't think users will hold down the button to hear multiple alerts. Are the multiple firings a performance issue?

ResetAllButton is a RoundPushButton with a specific look. There will certainly be a11y specific features required for buttons like Reset All button. But those features should be related to its specialization as a Reset All button, not things that are general to RoundPushButton . As much of the a11y functionality as possible should be in base classes (like RoundPushButton), not in subclasses (like ResetAllButton). And RestAllButton should most definitely not be deciding whether it fires multiple times, that should be handled consistently for all buttons in a base class. This is a problem that needs to be solve generally. And once it's solved generally, ResetAllButton will inherit that solution.

Re multiple firings... They are in general undesirable. They can result in performance issues, and they will result in multiple messages in the PhET-iO data stream. And for a button like ResetAllButton that is used in all sims, we cannot predict it will cause a performance issue. I can tell you that in Equality Explorer, there were performance issues with a single reset call.

terracoda commented 5 years ago

@pixelzoom, I really like the way you are thinking of a holistic general solution. I agree that all buttons need to inherit their accessibility from a base class. That would be ideal.

I think we need to talk about buttons based on their function and not how they look. Whether a button is round or not has no relevance to accessibility. I realize that these names have evolved from a visual design language are quite useful for the visual design of PhET sims. Unfortunately the interaction design patterns associated with these visual design patterns have excluded the keyboard. This has lead to a plethora of buttons with very nuanced meaning and very nuanced inheritance patterns based on visuals and based on mouse/touch interactions.

I am wondering if we have far fewer functional buttons than we think?

For example, am I understanding correctly that "RoundPushButton" and "ResetAllButton" are PhET button classes?

"ResetAllButton" based on your comment inherits from "RoundPushButton", but is the Reset All button really a "push" button in the PhET sense, aside from the fact that it is round? What interactions do PhET software engineers use "push" buttons for? It is the "push" part of the class that is important for accessibility.

To me, the Reset All button is a simple button. Upon activation:

From an accessibility perspective we want buttons:

In addition, focus has to be managed carefully when a button is activated clearly communicating whether or not there is a change in context (i.e. focus is sent to another element). There is indeed a change in context in the case of the Solute button that pops up the list of solutes in the Molarity sim's visual ComboBox. Focus has to move from the Solute button to the selected Solute in the popped-up list.

Apologies if I missed something. I am really trying to follow along. Perhaps if we identified the base types of buttons we need based on function alone, we could narrow our discussion.

Do these types cover it?

pixelzoom commented 5 years ago

@terracoda said:

.... This has lead to a plethora of buttons with very nuanced meaning and very nuanced inheritance patterns based on visuals and based on mouse/touch interactions. ...

It is not the case that we have a plethora of buttons. PhET's buttons are modeled on a standard button taxonomy, and most of the functionality is in a few base classes. You said that things like whether a button is round or rectangular have no relevance to a11y, and that is indeed the case regardless of a11y. In the implementation of PhET buttons there is a clear separation between how a button looks and how it processes raw events (whether it fires on up or down, whether it fires on hold, etc.). How a button processes events is handled by a base class (e.g. PushButtonModel, ButtonModel). Details about how a button looks are added by subclasses (e.g. RoundPushButton, RectangularPushButton). What a button specifically does when it has been told to "fire" is handled in more still more specialized classes (e.g. ResetAllButton). How a button decides when to fire is independent of how a button looks (e.g. round or rectangular, icon, etc.), and independent of what the button does when it fires (e.g. "reset the sim"). This type of separation and subclassing is typical of most UI toolkits -- Java Swing, X/Motif, Qt, ...

So when you said in https://github.com/phetsims/scenery/issues/939#issuecomment-464512679 that you are "uncomfortable with the idea of re-coding the Reset All button"... Processing of the raw Enter and Space events is not (and should not) be coded in ResetAllButton, it is the responsibility of a base class. ResetAllButton is told by the base class to "fire", and then does what is specific to "reset all". So resolving the issue of how Enter/Space events are converted to "fire" is not a matter of changing the implementation of the Reset All button.

To me, the Reset All button is a simple button. Upon activation:

  • it fires once to reset the sim
  • it triggers a single aria-live alert that the sim has been reset
  • there is no label change that often indicates a toggled state
  • there is no meaningful continued pressed state, requiring multiple firings
  • there is no change in context, so focus remains on the button

This is a good summary. Everything here is specific to the "reset all" function of the button. And how it "fires once" is the responsibility of the more general base class. So I think we're on the same page.

From an accessibility perspective we want buttons:

  • to have a bright bold visible focus style that respects the bounds of the button's visual presentation (round,square, or whatever)
  • to have very visible styles to communicate critical state information (e.g. pressed, grabbed, etc)
  • to be operable via the keyboard using Space key and Enter key

In addition, focus has to be managed carefully when a button is activated clearly communicating whether or not there is a change in context (i.e. focus is sent to another element). ...

The first 2 bullets are related to how the button is presented, that seems to be working well, and it is being handled in the base classes. Focus also seems to be working well. The 3rd bullet (handling of raw events) is where we're having problems. We don't have a solution that works in the base classes, and we cannot afford to have specific buttons (or worse, client code that uses those button), work around things like multiple firings, etc.

Do these types cover it? ...

Sort of, but this doesn't quite match the PhET button taxonomy. PhET buttons live in https://github.com/phetsims/sun/tree/master/js/buttons, and I would encourage anyone who is interested to poke around there. It's a combination of base classes and some very specific subclasses (ArrowButton, CarouselButton).

terracoda commented 5 years ago

@pixelzoom thank you for the detailed explanations. How do I identify the base button classes? There are 36 buttons in https://github.com/phetsims/sun/tree/master/js/buttons including radiobuttons that actually do not have the button role when coded in HTML.

jessegreenberg commented 5 years ago

Thanks for the careful review and comments everyone. I thought of a different solution to this that we should explore that may allow us to keep using standard HTML/roles for a11y while also supporting all the options in https://github.com/phetsims/scenery/issues/939#issuecomment-463718627. I started https://github.com/phetsims/scenery/issues/945 to play with the idea. Please see that issue for more details if interested, and will report back here when we know more.

pixelzoom commented 5 years ago

Back to this question that @terracoda asked in https://github.com/phetsims/scenery/issues/939#issuecomment-464512679:

@jessegreenberg, I am really uncomfortable with the idea of re-coding the Reset All button. It works well for users as it is. I don't think users will hold down the button to hear multiple alerts. Are the multiple firings a performance issue?

The reason that ResetAllButton works well for users is due to this workaround:

    // a11y - when reset all button is fired, disable alerts so that there isn't an excessive stream of alerts
    // while many Properties are reset. When callbacks are ended for reset all, enable alerts again and announce an
    // alert that everything was reset.
    this.isFiringProperty.lazyLink( function( isFiring ) {
      utteranceQueue.enabled = !isFiring;

      if ( isFiring ) {
        utteranceQueue.clear();
      }
      else {
        utteranceQueue.addToBack( new Utterance( {
          alert: resetAllAlertString,

          // only one "Reset" alert should be in the queue at a time so the user doesn't get spammed with alerts
          // relating to Reset if the button is pressed rapidly
          uniqueGroupId: 'resetAllButtonAlert',

          // Wait this long in ms before announcing the alert so that old utterances with the same uniqueGroupId
          // are removed before they can be anounced. This way only a single utterance for Reset is
          // announced even if "enter" is held down to click the button every few milliseconds.
          // TODO: This is a general problem that will come up for many buttons, see
          // https://github.com/phetsims/sun/issues/424 - when that issue is resolved this line can potentially
          // be removed
          delayTime: 500
        } ) );
      }
    } );

This is a workaround that prevents multiple announcements that could occur with the Enter key. This workaround is almost 25% of the code in ResetAllButton, and it's complicated. The same workaround will be needed by other buttons - the expand/collapse button on AccordionBox comes to mind. We can't be duplicating something like this.

terracoda commented 5 years ago

Maybe @jessegreenberg's idea will produce a quiet button.