Skip to content

get by label concat values #681

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Jul 20, 2020
Merged

get by label concat values #681

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4033c6e
add input node as label (#545)
Jun 30, 2020
b609b32
create an helper function to retrieve all the labels (#545)
Jun 30, 2020
732a8ab
get concat label through labels combination (#545)
Jul 3, 2020
eb8517d
remove useless comment and refactor function (#545)
Jul 3, 2020
f3a3c65
make queryAllByLabelText looks before for labelled elements and then …
Jul 3, 2020
df29ead
fix wrong splice usage (#545)
Jul 4, 2020
de06780
remove useless code (#545)
Jul 4, 2020
9d2be9f
improve test coverage (#545)
Jul 4, 2020
49c6148
retrieve elements with labels together the ones with aria-labelledby …
Jul 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/__tests__/element-queries.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,12 @@ test('can get form controls by label text', () => {
<label id="fifth-label-two">5th two</label>
<input aria-labelledby="fifth-label-one fifth-label-two" id="fifth-id" />
</div>
<div>
<input id="sixth-label-one" value="6th one"/>
<input id="sixth-label-two" value="6th two"/>
<label id="sixth-label-three">6th three</label>
<input aria-labelledby="sixth-label-one sixth-label-two sixth-label-three" id="sixth-id" />
</div>
</div>
`)
expect(getByLabelText('1st').id).toBe('first-id')
Expand All @@ -176,6 +182,9 @@ test('can get form controls by label text', () => {
expect(getByLabelText('4th').id).toBe('fourth.id')
expect(getByLabelText('5th one').id).toBe('fifth-id')
expect(getByLabelText('5th two').id).toBe('fifth-id')
expect(getByLabelText('6th one').id).toBe('sixth-id')
expect(getByLabelText('6th two').id).toBe('sixth-id')
expect(getByLabelText('6th one 6th two 6th three').id).toBe('sixth-id')
})

test('can get elements labelled with aria-labelledby attribute', () => {
Expand Down
72 changes: 58 additions & 14 deletions src/queries/label-text.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,30 +12,73 @@ import {
} from './all-utils'
import {queryAllByText} from './text'

function getCombinations(labels, matcher) {
const combs = [[]]
const matching = []
for (const label of labels) {
const copy = [...combs] // See note below.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what note this is referring to. Also, if I'm following this logic correctly, the for loop on the next line will only be run only once (for this iteration of the nested loop) and the value of the prefix will be [] every time. Any reason we need a loop here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I am totally getting your point, are you telling that this loop runs only once inside the outer loop?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. Because copy is [[]] and so it's an array with only one element in it. It'll never be....

Wait. I guess that's only the case on the first iteration of the outer loop. Future iterations could have more... Hmmm... I need to take more time to understand what's going on here...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think it's only the first iteration of the outer loop that makes the inner one run only once.

for (const prefix of copy) {
const combination = prefix.concat(label.textToMatch)
combs.push(combination)
if (matcher(combination.join(' '), label.node)) {
matching.push(label.node)
}
}
}
return matching
}
function queryAllLabels(container) {
return Array.from(container.querySelectorAll('label,input'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It occurs to me that an element can be labeled by anything, not just label and input. So while this may be solving for the input case, it doesn't solve for everything else 🤔

That said, I'm not sure how to handle "everything else." It seems to me that rather than searching for all possible elements that can label anything, we should be searching for everything that's labeled first, determine the contents of their label, and then return the elements whose label matches the query.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what it's worth, I think that searching for labelled items and then for their labels is more feasible than querying for all elements that could be labels. Like you said above!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you'd like to tack a whack at that, I think it would solve a lot of problems.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For sure I would like to do it!

.map(node => {
let textToMatch =
node.tagName.toLowerCase() === 'label'
? node.textContent
: node.value || null
// The children of a textarea are part of `textContent` as well. We
// need to remove them from the string so we can match it afterwards.
Array.from(node.querySelectorAll('textarea')).forEach(textarea => {
textToMatch = textToMatch.replace(textarea.value, '')
})

// The children of a select are also part of `textContent`, so we
// need also to remove their text.
Array.from(node.querySelectorAll('select')).forEach(select => {
textToMatch = textToMatch.replace(select.textContent, '')
})
return {node, textToMatch}
})
.filter(({textToMatch}) => textToMatch !== null)
}

function queryAllLabelsByText(
container,
text,
{exact = true, trim, collapseWhitespace, normalizer} = {},
) {
const matcher = exact ? matches : fuzzyMatches
const matchNormalizer = makeNormalizer({collapseWhitespace, trim, normalizer})
return Array.from(container.querySelectorAll('label')).filter(label => {
let textToMatch = label.textContent

// The children of a textarea are part of `textContent` as well. We
// need to remove them from the string so we can match it afterwards.
Array.from(label.querySelectorAll('textarea')).forEach(textarea => {
textToMatch = textToMatch.replace(textarea.value, '')
})
const textToMatchByLabels = queryAllLabels(container)

const nodesByLabelMatchingText = textToMatchByLabels
.filter(({node, textToMatch}) =>
matcher(textToMatch, node, text, matchNormalizer),
)
.map(({node}) => node)
const labelsNotMatchingTextAndNotEmpty = textToMatchByLabels.filter(
({node, textToMatch}) =>
Boolean(textToMatch) &&
nodesByLabelMatchingText.findIndex(nodeByLabelByText =>
nodeByLabelByText.isEqualNode(node),
) === -1,
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const labelsNotMatchingTextAndNotEmpty = textToMatchByLabels.filter(
({node, textToMatch}) =>
Boolean(textToMatch) &&
nodesByLabelMatchingText.findIndex(nodeByLabelByText =>
nodeByLabelByText.isEqualNode(node),
) === -1,
)
const labelsNotMatchingTextAndNotEmpty = textToMatchByLabels.filter(
({node, textToMatch}) =>
textToMatch && nodesByLabelMatchingText.some(n => n.isEqualNode(node)),
)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the right change should be:

const labelsNotMatchingTextAndNotEmpty = textToMatchByLabels.filter(
    ({node, textToMatch}) =>
      textToMatch && !nodesByLabelMatchingText.some(n => n.isEqualNode(node)),
  )

because I would like to retrieve all the labels in textToMatchByLabels but not empty and not included in nodesByLabelMatchingText


// The children of a select are also part of `textContent`, so we
// need also to remove their text.
Array.from(label.querySelectorAll('select')).forEach(select => {
textToMatch = textToMatch.replace(select.textContent, '')
})
const concatLabelsMatching = getCombinations(
labelsNotMatchingTextAndNotEmpty,
(textToMatch, node) => matcher(textToMatch, node, text, matchNormalizer),
)

return matcher(textToMatch, label, text, matchNormalizer)
})
return nodesByLabelMatchingText.concat(concatLabelsMatching)
}

function queryAllByLabelText(
Expand All @@ -50,6 +93,7 @@ function queryAllByLabelText(
exact,
normalizer: matchNormalizer,
})

const labelledElements = labels
.reduce((matchedElements, label) => {
const elementsForLabel = []
Expand Down