Skip to content
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.

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

Label tag should allow for htmlFor #1651

Closed
walidvb opened this issue Jun 26, 2020 · 6 comments
Closed

Label tag should allow for htmlFor #1651

walidvb opened this issue Jun 26, 2020 · 6 comments

Comments

@walidvb
Copy link

walidvb commented Jun 26, 2020

Is your feature request related to a problem? Please describe.
accessibilityRole allows to have better tags, such as <label>, along with aria labels. One thing that is missing is the for attribute, which allows for the user to click on the label to focus the field.

Describe a solution you'd like
Passing htmlFor to a <Text accessibilityRole="label" htmlFor="email">Email</Text> would yield the expected html

Additional context
While it doesn't concern UX directly, I use of capybara for my automated e2e testing, which heavily relies on this feature to interact with forms

I'm currently patching a follows:

diff --git a/node_modules/react-native-web/dist/exports/View/filterSupportedProps.js b/node_modules/react-native-web/dist/exports/View/filterSupportedProps.js
index a7a6b96..5a087a7 100644
--- a/node_modules/react-native-web/dist/exports/View/filterSupportedProps.js
+++ b/node_modules/react-native-web/dist/exports/View/filterSupportedProps.js
@@ -66,6 +66,7 @@ var supportedProps = {
   onMouseUp: true,
   // unstable escape-hatches for web
   href: true,
+  htmlFor: true,
   itemID: true,
   itemRef: true,
   itemProp: true,
diff --git a/node_modules/react-native-web/dist/modules/createDOMProps/index.js b/node_modules/react-native-web/dist/modules/createDOMProps/index.js
index 57d9054..5ce1440 100644
--- a/node_modules/react-native-web/dist/modules/createDOMProps/index.js
+++ b/node_modules/react-native-web/dist/modules/createDOMProps/index.js
@@ -72,6 +72,7 @@ var createDOMProps = function createDOMProps(component, props, styleResolver) {
       testID = _props.testID,
       accessible = _props.accessible,
       accessibilityRole = _props.accessibilityRole,
+      htmlFor = _props.htmlFor,
       domProps = _objectWithoutPropertiesLoose(_props, ["accessibilityLabel", "accessibilityLiveRegion", "accessibilityRelationship", "accessibilityState", "classList", "className", "disabled", "importantForAccessibility", "nativeID", "pointerEvents", "style", "testID", "accessible", "accessibilityRole"]);
 
   var disabled = accessibilityState != null && accessibilityState.disabled === true || providedDisabled;
@@ -142,7 +143,9 @@ var createDOMProps = function createDOMProps(component, props, styleResolver) {
     } else {
       domProps['data-focusable'] = true;
     }
-  } else if (AccessibilityUtil.buttonLikeRoles[role] || role === 'textbox') {
+  } else if (component === 'label'){
+    domProps.htmlFor = htmlFor;
+  }else if (AccessibilityUtil.buttonLikeRoles[role] || role === 'textbox') {
     if (accessible !== false && focusable) {
       domProps['data-focusable'] = true;
       domProps.tabIndex = '0';
diff --git a/node_modules/react-native-web/src/exports/View/filterSupportedProps.js b/node_modules/react-native-web/src/exports/View/filterSupportedProps.js
index 5c23416..1ec51c0 100644
--- a/node_modules/react-native-web/src/exports/View/filterSupportedProps.js
+++ b/node_modules/react-native-web/src/exports/View/filterSupportedProps.js
@@ -66,6 +66,7 @@ const supportedProps = {
   onMouseUp: true,
   // unstable escape-hatches for web
   href: true,
+  htmlFor: true,
   itemID: true,
   itemRef: true,
   itemProp: true,
diff --git a/node_modules/react-native-web/src/modules/createDOMProps/index.js b/node_modules/react-native-web/src/modules/createDOMProps/index.js
index 2e0980d..0eeabef 100644
--- a/node_modules/react-native-web/src/modules/createDOMProps/index.js
+++ b/node_modules/react-native-web/src/modules/createDOMProps/index.js
@@ -73,6 +73,7 @@ const createDOMProps = (component, props, styleResolver) => {
     pointerEvents,
     style: providedStyle,
     testID,
+    htmlFor,
     /* eslint-disable */
     accessible,
     accessibilityRole,
@@ -155,6 +156,8 @@ const createDOMProps = (component, props, styleResolver) => {
     } else {
       domProps['data-focusable'] = true;
     }
+  } else if(component === 'label'){
+    domProps.htmlFor = htmlFor;
   } else if (AccessibilityUtil.buttonLikeRoles[role] || role === 'textbox') {
     if (accessible !== false && focusable) {
       domProps['data-focusable'] = true;

I'll submit a PR ASAP for this, hopefully it makes sense to add it to the lib!

@necolas
Copy link
Owner

necolas commented Jun 26, 2020

Use of for isn't really necessary because you can do the same thing (and more) with aria-labelledby: https://www.w3.org/WAI/tutorials/forms/labels/. The htmlFor attribute is web-only and I think we can do better by avoiding adding more platform-specific APIs to React Native where general approach work just as well.

@walidvb
Copy link
Author

walidvb commented Jun 26, 2020

@necolas appreciate being platform agnostic, but aria-labelledby does not allow clicking on the label, example here.
Maybe there's a better API to use? name? or other? 🤔

@necolas
Copy link
Owner

necolas commented Jun 26, 2020

The web label behavior doesn't happen on native either

@walidvb
Copy link
Author

walidvb commented Jun 26, 2020

This asks a more general question then, should this library allow to do as much as the web does, or not? An alternative would then be to have a Label.web.js just for that one, which outputs html(kind of defeats the purpose, i think?)

My question is does this library plan on limiting behaviour that is cross-platform only? Meaning anything that is web based will be dismissed? My opinion is that it shouldn't, as the platforms are inherently different, and the code should be able(as much as possible) to provide functionality for both, rather than limiting to a subset.

This said, I really appreciate the work you and others did on this, and hoping it can fit my entire project!

@brucelawson
Copy link

I'd like to consider re-opening this. I'm doing accessibility testing of an R.N. project. The HTML output from react-native-web does a really good job of producing accessible code and mapping R.N. to wai-aria. However, the fact that the html 'labels' can't be clicked to put focus into the associated form field means that I can't use the generated web version, as it breaks user expectation and prohibits WCAG validation.

Given that React native web 'knows' which fields are associated with which 'labels' (as it takes the field's aria-label text from the contents of the labelling div) it would seem to be possible for it to (a) generate an id to uniquely identify the field, and use label for="generatedID" instead of div for the label (perhaps as a switch that can be turned on for those who need it, so it doesn't much up CSS or markup of existing projects?)

@necolas
Copy link
Owner

necolas commented Jun 7, 2021

Hi Bruce,

I'm doing accessibility testing of an R.N. project

Can you share the RN code snippet that's being run on web?

However, the fact that the html 'labels' can't be clicked to put focus into the associated form field means that I can't use the generated web version

This can be re-implemented in JavaScript, which will make the behavior cross-platform too.

Given that React native web 'knows' which fields are associated with which 'labels'...

React DOM might know that internally, but it doesn't expose any way for us to get that information.

...generate an id to uniquely identify the field...

This isn't straight-forward if there's server rendering and client hydration involved. React was meant to add a new hook to help with this, but it's caught in limbo and hasn't been shipped.

...use label for="generatedID" instead of div for the label

You can still output a <label> element by setting the accessibilityRole to "label". That could be used to wrap fields in a <label>. IIRC not every screen reader likes that pattern, which is why using accessibilityLabelledBy (web-only) along with JS to move the focus has benefits.


I like the idea in principle of inferring the for attribute from the accessibilityLabelledBy. That would reduce redundancy while retaining the historical behavior of clicking on labels. But AFAIK we can't do that. It would have to account for multiple labelling elements too. Creating a new prop just for use with label also seems quite heavy handed.

A different option is to drop down to the unstable_createElement API. You can do something like this:

// Label.js (for native)
import {Text} from 'react-native';
export function Label({ for, ...props }) {
  return <Text {...props} />
}

// Label.web.js (for web)
import {unstable_createElement} from 'react-native-web';
export function Label({ for, ...props }) {
  return unstable_createElement('label', { htmlFor: for, ...props });
}

This would allow you to use the same Label component in the cross-platform code, and use it like this <Label for={id} />. The id will do nothing on native but on web it will be added to the <label> element with the desired for attribute. This is my preferred option, as I think a framework should provide 90% of what you need but still give you APIs that allow full access to the host platform when needed. For example, RN has no way to markup tables in the public API either, but with this unstable_createElement API (and native equivalents) you could create your own React table components that render with the correct HTML tags.

Another option is to set the DOM attribute in JS using a ref callback, but this only works for client-side rendering:

const ref = React.useCallback((node) => {
  if (Platform.OS === 'web' && node != null) {
    node.setAttribute('for', id);
  }
}, []);

return (
  <>
    <Text ref={ref}>label</Text>
    <TextInput nativeID={id} />
  </>
)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants