nrkno / core-components

Accessible and lightweight Javascript components
https://static.nrk.no/core-components/latest/
MIT License
117 stars 10 forks source link

Timing issues with core-toggle and its aria-label #628

Closed wkillerud closed 3 years ago

wkillerud commented 3 years ago

I wanted to run an issue I'm seeing in @nrk/core-toggle by y'all. We're using the React wrapper in fremtind/jokul for our Select component. Because of a timing issue, aria-label is being set to include the value of the previously selected item instead of the currently selected.

The PR fremtind/jokul#2190 provides more context and a video from a debugging session.

What it boils down to is the source of the aria-label value and the timing at which aria-label is set. Our implementation ended up calling this setter before our React-component had a chance to render with an updated value for button.textContent.

Is the below patch of interest upstream? 😄 If so I can open a PR.

  set value (data = false) {
    if (!this.button || !this.popup.length) return
    const button = this.button
    const popup = (button.getAttribute('aria-label') || `,${this.popup}`).split(',')[1]
    const label = data.textContent || data || '' // data can be Element, Object or String

    if (popup === this.popup) {
      const target = button.querySelector('span') || button // Use span to preserve embedded HTML and SVG
      button.value = data.value || label
      target[data.innerHTML ? 'innerHTML' : 'textContent'] = data.innerHTML || label
-     button.setAttribute('aria-label', `${button.textContent},${this.popup}`)
+     button.setAttribute('aria-label', `${label},${this.popup}`)
    }
  }
skjalgepalg commented 3 years ago

Hey, I'll look into this and get back to you with regards to a PR. Thanks for the heads-up!

wkillerud commented 3 years ago

Nice, thank you! And good to see a familiar face 😄 Let me know if I can help.

skjalgepalg commented 3 years ago

@wkillerud reckon you could give it a try with @nrk/core-toggle@3.1.0?

Looks like the jsx -file published through npm for @nrk/core-toggle@3.0.8 sets aria-label prior to target being updated. This was fixed in 0a3473 but not released. The correct order has been released as part of core-toggle@3.1.0

Hopefully this should resolve your issue without the need to use label instead of textContent, but give me a shout if I've missed something

wkillerud commented 3 years ago

Can confirm 3.1.0 sets the correct aria-label on our end while still reading from textContent 🎉 @skjalgepalg