From a28aa616f271feeb9b0fec385a0b52522d7d9361 Mon Sep 17 00:00:00 2001
From: Ioannis Papadopoulos <ioannis@harbrdata.com>
Date: Mon, 27 Jul 2020 11:43:40 +0100
Subject: [PATCH 1/2] fix(ByLabelText): improve error message when label is
 associated with non-labellable elements (#716)

---
 src/__tests__/element-queries.js | 108 +++++++++++++++++++++++++++++++
 src/queries/label-text.js        |  35 ++++++++--
 2 files changed, 139 insertions(+), 4 deletions(-)

diff --git a/src/__tests__/element-queries.js b/src/__tests__/element-queries.js
index 6a6df03b..7f8b5f53 100644
--- a/src/__tests__/element-queries.js
+++ b/src/__tests__/element-queries.js
@@ -407,6 +407,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()
diff --git a/src/queries/label-text.js b/src/queries/label-text.js
index 9f28f9dd..9f959832 100644
--- a/src/queries/label-text.js
+++ b/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}`

From bc54893f21e016eff402efb6fe4d142e490089a3 Mon Sep 17 00:00:00 2001
From: Ioannis Papadopoulos <ioannis@harbrdata.com>
Date: Wed, 29 Jul 2020 07:47:58 +0100
Subject: [PATCH 2/2] fix(ByLabelText) test coverage for #716

---
 src/__tests__/element-queries.js | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/__tests__/element-queries.js b/src/__tests__/element-queries.js
index 7f8b5f53..61a88acf 100644
--- a/src/__tests__/element-queries.js
+++ b/src/__tests__/element-queries.js
@@ -352,9 +352,9 @@ 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}))
@@ -362,7 +362,9 @@ test('label with no form control and fuzzy matcher', () => {
 "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>"