emberjs / ember.js

Ember.js - A JavaScript framework for creating ambitious web applications
https://emberjs.com
MIT License
22.46k stars 4.21k forks source link

[Bug] Issues with Select rendering and JAWS #19115

Open pzuraq opened 4 years ago

pzuraq commented 4 years ago

🐞 Describe the Bug

We've gotten reports that the JAWS is unable to correctly read minimal select element created with Ember. Specifically, when an option is selected, the selected value is not read out by the screen reader. The workaround at the moment is to use aria-selected, but this seems problematic. Either JAWS has a bug, or Ember does, and we need to figure out which.

πŸ”¬ Minimal Reproduction

{{!-- This example works --}}
<select>
  <option>Alabama</option>
  <option selected>California</option>
  <option>Oregon</option>
</select>

{{!-- This example does not work --}}
<select>
  {{#each values as |value|}}
    <option selected={{eq value this.selectedValue}}></option>
  {{/each}}
</select>

Codepen with the first example: https://codepen.io/pzuraq/pen/oNxGvWz

πŸ˜• Actual Behavior

The screen reader does not read out the option label.

πŸ€” Expected Behavior

The screen reader should read out the option label.

🌍 Environment

βž• Additional Context

We've verified that the Ember version of the select is properly updating the properties that represent the selected state, and other A11y tools including the A11y tree in Chrome work properly here. The next step is to start reducing the differences between these two examples to figure out what exactly is causing the issue.

Noted differences:

  1. selected is set the first time as an attribute on the native example. It remains set as the select is updated. By contrast, the Ember example always sets the property, so no option will have the attribute set at any time. We should see if removing the attribute in the native example causes any difference in behavior.
  2. The Ember example will create its DOM dynamically, using document.createElement and manually appending it, whereas the native version does not. We should try to create a select using vanilla JS to see if that impacts behavior at all.
pzuraq commented 4 years ago

cc @syu15

rwjblue commented 4 years ago

I think next steps here would be to do manual DOM element building for this snippet:

<select>
  <option>Alabama</option>
  <option selected>California</option>
  <option>Oregon</option>
</select>

Something like:

const selectEl = document.createElement('select');

const option1 = document.createElement('option');
option1.appendChild(document.createTextNode('Alabama'));

const option2 = document.createElement('option');
option2.appendChild(document.createTextNode('California'));
option2.selected = true;

const option3 = document.createElement('option');
option3.appendChild(document.createTextNode('Oregon'));

selectEl.appendChild(option1);
selectEl.appendChild(option2);
selectEl.appendChild(option3);
document.body.appendChild(selectEl);

And then test JAWS/NVDA to see if they read the correct output. I've made https://codepen.io/rwjblue/pen/NWNaqWy for this snippet in case that makes it easier to test.

MelSumner commented 4 years ago

I was also able to reproduce the issue in Firefox and NVDA.

pzuraq commented 3 years ago

So I dug into this a bit more, and was unable to reproduce the issue in Chrome with NVDA, but was able to with Firefox. The issue appears to be coming from the fact that we append multiple text nodes to the <option> element. The screen reader only reads the first text node in these cases.

Plain JS reproduction: https://codepen.io/pzuraq/pen/PoNLmor

Ember-Twiddle reproduction: https://ember-twiddle.com/097b5e7a6d20c5dee88d65149f7b28ee

I opened up a bug report with Firefox because this seems to be an issue specific to that browser so far, but if anyone can reproduce with other screen readers and other browsers please add to this issue!

https://bugzilla.mozilla.org/show_bug.cgi?id=1667494

ijlee2 commented 2 years ago

Hi, I came across this bug today. I was caught by surprise, because template syntax (using {{eq}} helper) didn't just work. Could we push for a fix so that developers can write semantic HTML more easily?

In Ember 3.28.6 (tested on Chrome 98.0), the following template didn't set the selected attribute correctly:

{{!-- this.options is an array of strings, @value is string or undefined --}}

{{#each this.options as |opt|}}
  <option
    selected={{eq opt @value}}
    value={{opt}}
  >
    {{opt}}
  </option>
{{/each}}

I ended up using an if-else statement:

{{#each this.options as |opt|}}
  {{#if (eq opt @value)}}
    <option
      selected
      value={{opt}}
    >
      {{opt}}
    </option>

  {{else}}
    <option value={{opt}}>
      {{opt}}
    </option>

  {{/if}}
{{/each}}

As an aside, I didn't write the <option> tag as shown above, but used a yielded component:

{{!-- Some reusable option component --}}

<option
  disabled={{@isDisabled}}
  value={{@value}}
  ...attributes
>
  {{@label}}
</option>
ijlee2 commented 2 years ago

@locks on Discord

I can't reproduce with <select> and <option>, the problem might be in ...attributes instead

simonihmig commented 1 year ago

I just came across this, trying to set selected as an attribute using a simple template like <option selected={{this.isSelected}}>{{yield}}</option>.

I haven't tried what a screenreader does, but even when ignoring a11y for a second, then selected should nevertheless be set as an attribute and not a property I believe? Like in a SSR/FastBoot setup, you want a selected option to actually render as selected even before JS kicks in, right?

By contrast, the Ember example always sets the property, so no option will have the attribute set at any time.

I don't know what the heuristics is that Ember applies in determining the attribute vs. property question, does anyone here? At least it seems wrong here IMHO.

For now I ended up using the workaround that @ijlee2 suggested, but that's not really nice. Also now it does way more DOM manipulation when the selected option changes, as it destroys and creates new <option> elements for all the options, rather than doing a simple setAttribute('selected', ...). πŸ˜•