fix(ByLabelText): improve error message for invalid labels (#720)

* fix(ByLabelText): improve error message when label is associated with non-labellable elements (#716)

* fix(ByLabelText) test coverage for #716

Co-authored-by: Ioannis Papadopoulos <[email protected]>

Author: ipap360

src/__tests__/element-queries.js

@@ -352,17 +352,19 @@ test('label with no form control', () => {
 `)
 })
 
-test('label with no form control and fuzzy matcher', () => {
+test('label with "for" attribute but no form control and fuzzy matcher', () => {
   const {getByLabelText, queryByLabelText} = render(
-    `<label>All alone label</label>`,
+    `<label for="foo">All alone label</label>`,
   )
   expect(queryByLabelText('alone', {exact: false})).toBeNull()
   expect(() => getByLabelText('alone', {exact: false}))
     .toThrowErrorMatchingInlineSnapshot(`
 "Found a label with the text of: alone, however no form control was found associated to that label. Make sure you're using the "for" attribute or "aria-labelledby" attribute correctly.
 
 <div>
-  <label>
+  <label
+    for="foo"
+  >
     All alone label
   </label>
 </div>"
@@ -407,6 +409,114 @@ test('label with children with no form control', () => {
 `)
 })
 
+test('label with non-labellable element', () => {
+  const {getByLabelText, queryByLabelText} = render(`
+  <div>
+    <label for="div1">Label 1</label>
+    <div id="div1">
+      Hello
+    </div>
+  </div>
+  `)
+
+  expect(queryByLabelText(/Label/)).toBeNull()
+  expect(() => getByLabelText(/Label/)).toThrowErrorMatchingInlineSnapshot(`
+"Found a label with the text of: /Label/, however the element associated with this label (<div />) is non-labellable [https://html.spec.whatwg.org/multipage/forms.html#category-label]. If you really need to label a <div />, you can use aria-label or aria-labelledby instead.
+
+<div>
+  
+  
+  <div>
+    
+    
+    <label
+      for="div1"
+    >
+      Label 1
+    </label>
+    
+    
+    <div
+      id="div1"
+    >
+      
+      Hello
+    
+    </div>
+    
+  
+  </div>
+  
+  
+</div>"
+`)
+})
+
+test('multiple labels with non-labellable elements', () => {
+  const {getAllByLabelText, queryAllByLabelText} = render(`
+  <div>
+    <label for="span1">Label 1</label>
+    <span id="span1">
+      Hello
+    </span>
+    <label for="p1">Label 2</label>
+    <p id="p1">
+      World
+    </p>
+  </div>
+  `)
+
+  expect(queryAllByLabelText(/Label/)).toEqual([])
+  expect(() => getAllByLabelText(/Label/)).toThrowErrorMatchingInlineSnapshot(`
+"Found a label with the text of: /Label/, however the element associated with this label (<span />) is non-labellable [https://html.spec.whatwg.org/multipage/forms.html#category-label]. If you really need to label a <span />, you can use aria-label or aria-labelledby instead.
+
+Found a label with the text of: /Label/, however the element associated with this label (<p />) is non-labellable [https://html.spec.whatwg.org/multipage/forms.html#category-label]. If you really need to label a <p />, you can use aria-label or aria-labelledby instead.
+
+<div>
+  
+  
+  <div>
+    
+    
+    <label
+      for="span1"
+    >
+      Label 1
+    </label>
+    
+    
+    <span
+      id="span1"
+    >
+      
+      Hello
+    
+    </span>
+    
+    
+    <label
+      for="p1"
+    >
+      Label 2
+    </label>
+    
+    
+    <p
+      id="p1"
+    >
+      
+      World
+    
+    </p>
+    
+  
+  </div>
+  
+  
+</div>"
+`)
+})
+
 test('totally empty label', () => {
   const {getByLabelText, queryByLabelText} = render(`<label />`)
   expect(queryByLabelText('')).toBeNull()

src/queries/label-text.js

@@ -149,10 +149,27 @@ const getAllByLabelText = (container, text, ...rest) => {
   if (!els.length) {
     const labels = queryAllLabelsByText(container, text, ...rest)
     if (labels.length) {
-      throw getConfig().getElementError(
-        `Found a label with the text of: ${text}, however no form control was found associated to that label. Make sure you're using the "for" attribute or "aria-labelledby" attribute correctly.`,
-        container,
-      )
+      const tagNames = labels
+        .map(label =>
+          getTagNameOfElementAssociatedWithLabelViaFor(container, label),
+        )
+        .filter(tagName => !!tagName)
+      if (tagNames.length) {
+        throw getConfig().getElementError(
+          tagNames
+            .map(
+              tagName =>
+                `Found a label with the text of: ${text}, however the element associated with this label (<${tagName} />) is non-labellable [https://html.spec.whatwg.org/multipage/forms.html#category-label]. If you really need to label a <${tagName} />, you can use aria-label or aria-labelledby instead.`,
+            )
+            .join('\n\n'),
+          container,
+        )
+      } else {
+        throw getConfig().getElementError(
+          `Found a label with the text of: ${text}, however no form control was found associated to that label. Make sure you're using the "for" attribute or "aria-labelledby" attribute correctly.`,
+          container,
+        )
+      }
     } else {
       throw getConfig().getElementError(
         `Unable to find a label with the text of: ${text}`,
@@ -163,6 +180,16 @@ const getAllByLabelText = (container, text, ...rest) => {
   return els
 }
 
+function getTagNameOfElementAssociatedWithLabelViaFor(container, label) {
+  const htmlFor = label.getAttribute('for')
+  if (!htmlFor) {
+    return null
+  }
+
+  const element = container.querySelector(`[id="${htmlFor}"]`)
+  return element ? element.tagName.toLowerCase() : null
+}
+
 // the reason mentioned above is the same reason we're not using buildQueries
 const getMultipleError = (c, text) =>
   `Found multiple elements with the text of: ${text}`