fix: use the first label for LabelText query suggestions

[marcosvega91]

What: Fix a little bug

Why: As reported here when getSuggestedQuery tried to get label text through aria-labelledby it threw an exception when the id had spaces or special chars

How: Surrounding the selector with " "

Checklist:

• Documentation added to the
  docs site
• Tests
• Typescript definitions updated
• Ready to be merged
  

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 70a3048:

Sandbox	Source
pedantic-shape-viihs	Configuration

[codecov]

Codecov Report

Merging #672 into master will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #672   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           24        24           
  Lines          595       596    +1     
  Branches       148       148           
=========================================
+ Hits           595       596    +1     

Impacted Files	Coverage Δ
src/suggestions.js	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6bc8e2e...70a3048. Read the comment docs.

[smeijer]

But the html spec doesn't accept spaces in id attributes. The compound labels, are meant to do something different.

That would mean that a fix to silence the error, should just only use the first id (which are separated by spaces).

const firstLabelId = ariaLabelledBy.split(' ')[0];
document.querySelector(`[id="${firstLabelId}"]`)

[marcosvega91]

yes about spec you're right.

I don't know which solution is better then the other, both solution are not correct because of #545

[smeijer]

True, both fixes are just hacks to stop the query from throwing errors, while making suggestions.

The difference is that by wrapping the query in quotes, the query won't find a matching element, and thereby won't make a suggestion. It feels wrong to me to build a selector that we know is incorrect and won't return any elements.

By taking only the first id, the query still finds a matching element and will make a valid suggestion. It won't give the best suggestion possible, but at least it will suggest something close that does return an element.

The best fix would be to contact the labels, but as I understand, that isn't currently supported?

If the partial match doesn't result in a valid query either (I haven't tested it), I propose to just return undefined instead. But I assume that the partial match works, as we're suggesting regexes.

const ariaLabelledBy = element.getAttribute('aria-labelledby')
    if (ariaLabelledBy) {
      // compound labels are currently unsupported, fall back to different query options
      label = ariaLabelledBy.includes(' ') ? undefined : document.querySelector(`[id="${ariaLabelledBy}"]`)
    }

concat labels

function getLabelTextFor(element) {
  let label =
    element.labels &&
    Array.from(element.labels).find(el => Boolean(normalize(el.textContent)))

  if (label) {
    return label;
  }

  // non form elements that are using aria-labelledby won't be included in `element.labels`
  const ariaLabelledBy = element.getAttribute('aria-labelledby');

  if (ariaLabelledBy) {
    const selector = ariaLabelledBy.split(' ').map(id => `[id="${id}"]`).join(', ');
    const labels = document.querySelectorAll(selector);
    return Array.from(labels).map(label => label.textContent).join(' ')
  }


  return undefined
}

[marcosvega91]

Yes the function should work even if the logic is more complex, look at this

The problem then is that the query will not work because screen. getByLabelText not support label concatenation.
@delca85 created a PR for the issue

[smeijer]

Yes, I see. So my proposal, to just only take the first label and use that instead. Sounds like a good fix to me.

Let's take the markup:

<div id="a">One</div>
<div id="b">Two</div>
<input
  type="text"
  aria-labelledby="a b"
/>

The full label is One Two.

I see 3 options:

1. Combine the labels to a single label, and use that. But this isn't supported (see Get element by label input value and by labels concat values #607)

2. Wrap the query in quotes, as this PR is currently doing. No element is matched ('id["a b"]' doesn't exist), so no suggestion is being made. Potentially resulting in suggesting a query that's a worse option (testids?), as a getByLabelText couldn't be created.

3. Only use the first label to generate the suggestion. This would result in a query suggestion like: screen.getByLabelText(/one/i). This is a valid query, that will result in returning the input.

As 1 isn't currently possible, that's not a real option. So only 2 and 3 are left.

[marcosvega91]

Yes I can implement the third option.

[kentcdodds]

🎉 This PR is included in version 7.18.1 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀