Don't use tabindex to remove elements from focus

[jessegreenberg]

From phetsims/resistance-in-a-wire#189.

tabindex="-1" is being used to remove all elements from the focus order (even those that are not focusable by default). This is necessary because IE11 by default gives ALL HTML elements a tabindex of "0", including list items, paragraphs, everything. See phetsims/john-travoltage#206.

We are counting on focusable elements having tabindex >= 0 in AccessibilityUtil.isElementFocusable, so as an easy workaround for IE11 we are setting tabindex of all elements in the PDOM to be -1 when not focusable.

But setting tabindex="-1" is now interfering with JAWS reading in Chrome. For some reason this causes lists (and probably other things) to be read twice. For instance, this list

<ul tabindex="-1">
  <li tabindex="-1">resistance, <b>R</b>, is 0.667 ohms</li>
  <li tabindex="-1">resistivity, <b>rho</b>, is 0.50 ohm centimeters</li>
  <li tabindex="-1">length, <b>L</b>, is 10.00 centimeters</li>
  <li tabindex="-1">area, <b>A</b>, is 7.50 centimeters squared</li>
</ul>

is read as
"resistance R is 0.667 ohms, resistivity rho is 0.50 ohm centimeters, length L is 10.00 centimeters, area A is 7.50 centimeters squared. List with four items. Bullet, resistance R is 0.667 ohms. Bullet, resistivity rho is 0.50 ohm centimeters. Bullet, length L is 10.00 cm, bullet area A is 7.50 centimeters squared."

As we are transitioning to support Chrome instead of FF, this needs to be fixed. We need a different way to flag elements as not focusable for IE11, while not interfering with how Chrome reads things with JAWS.

[jessegreenberg]

Also Edge behaves like IE11 for tabindex so the workaround is needed for both browsers.

[jessegreenberg]

I think the best way forward is to use the data attribute to flag that elements are not focusable. https://developer.mozilla.org/en-US/docs/Learn/HTML/Howto/Use_data_attributes

[jessegreenberg]

We need a way to set/get data attributes from AccessiblePeer. I think we can use setAttribute/getAttribute, but the spec defines a dataSet property on HTMLElements for this. I can't decide whether or not we should use this since we have AccessiblePeer.setAttributeToElement

EDIT: I verified that the data- prefix is sufficient for the browser to recognize that it is a data attribute, at least in Chrome.

[jessegreenberg]

This is a little tricky because now we can only set the tabindex when overriding the default browser behavior for any given element.

[jessegreenberg]

Above commit should handle this. tabindex is now only used when we are overriding default browser behavior. So input elements like buttons and inputs should only ever have tabindex="-1" if removed from navigation, and non-focusable elements like divs and ps should only ever have tabindex="0" if add to navigation order. @zepumph can you please review this? In particular do you have any concerns about this workaround so Chrome + JAWS sounds good while still working in IE11?

[zepumph]

@jessegreenberg and I talked, and he answered a bit of my confusion with this issue, and caught me up!
We discussed having a consistent way to setAttribute accross all of AccessiblePeer (see linked issue above). We also discussed how removing tabIndex when trying to clear custom behavior is better than (a) going back to the default (since in IE the default is zero, and setting something to zero will actually just always make it focusable) and (b) always setting it back to -1 (as seen by phetsims/resistance-in-a-wire#189).

Review:

 if ( defaultFocusable ? !focusable : focusable ) {

• This is a confusing line of code, can we refactor so it isn't a conditional inside a conditional?

• I would factor out the two places you have data-focusable to make it clear that those must be kept in sync.

• In the overrideFocusWithTabIndex test, I understand that createElement uses overrideFocusWithTabIndex, but in addition it would be nice to add a few tests that only call that method, and make sure that tabIndex is manipulated as expected.

[jessegreenberg]

All good ideas @zepumph, done in the above commits. Can you please check?

[jessegreenberg]

@mbarlow12 said that keyboard navigation is mostly broken in Safari, and there is also strange zooming in VO. This issue could be a cause since it changed how we notify the browser which elements we want to be focusable. We should make sure that we haven't broken things on all browsers with this change.

[jessegreenberg]

@mbarlow12 showed me what was happening on his machine. The keyboard navigation issue was in user settings because Mac disables keybaord nav by default for some reason. The zooming issue was still happening, but I doubt it is related to this issue. Ill make an issue to check on the zooming further.

[zepumph]

Review is great! I like the tests. Thanks. Ready to close?

[jessegreenberg]

I think so, thanks for reviewing!