Skip to content

bug: shadow dom form controls opt-in to modern syntax with aria-labelledby #26829

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

Closed
4 of 8 tasks
lincolnthree opened this issue Feb 20, 2023 · 34 comments
Closed
4 of 8 tasks
Labels
package: core @ionic/core package type: bug a confirmed bug report

Comments

@lincolnthree
Copy link

lincolnthree commented Feb 20, 2023

Prerequisites

Ionic Framework Version

  • v4.x
  • v5.x
  • v6.x
  • v7.x
  • Nightly

Current Behavior

ion-select appears to be logging warning messages about using ion-label even when there is no such ion-label element in the DOM. In fact, even a completely unconfigured ion-select component will cause the message to be printed:

image

Expected Behavior

Warning & deprecation messages should only be logged when criteria for those messages are met. (Unless you've decided you want to do this for some reason, in which case I defer to your judgement.)

Steps to Reproduce

<ion-select></ion-select>

Code Reproduction URL

No response

Ionic Info

$ ionic info

Ionic:

Ionic CLI : 6.20.3 (/Users/lincoln/.nvm/versions/node/v16.14.0/lib/node_modules/@ionic/cli)
Ionic Framework : @ionic/angular 7.0.0-dev.11676644846.156e8507
@angular-devkit/build-angular : 15.1.6
@angular-devkit/schematics : 15.1.6
@angular/cli : 15.1.6
@ionic/angular-toolkit : 8.0.0

Capacitor:

Capacitor CLI : 4.6.3
@capacitor/android : 4.6.3
@capacitor/core : 4.6.3
@capacitor/ios : 4.6.3

Utility:

cordova-res : not installed globally
native-run : 1.7.1

System:

NodeJS : v16.14.0 (/Users/lincoln/.nvm/versions/node/v16.14.0/bin/node)
npm : 8.3.1
OS : macOS Monterey

Additional Information

No response

@ionitron-bot ionitron-bot bot added the triage label Feb 20, 2023
@lincolnthree
Copy link
Author

I am also seeing this occur for ion-range and ion-toggle.

@liamdebeasi liamdebeasi self-assigned this Feb 20, 2023
@liamdebeasi
Copy link
Contributor

Thanks for the report. This is the intended behavior. Components such as ion-toggle or ion-select must have an associated label for accessibility reasons. For visible text, developers can use ion-label:

<ion-item>
  <ion-label>Toggle Label</ion-label>
  <ion-toggle></ion-toggle>
</ion-item>

The new way to do visible text in Ionic 7 is to pass the text into the component itself, removing the need for ion-label:

<ion-item>
  <ion-toggle>Toggle Label</ion-toggle>
</ion-item>

However, some components may not have visible text. In this case, developers need to use aria-label to provide context to screen readers:

<ion-item>
  <ion-toggle aria-label="Toggle Label without visible text"></ion-toggle>
</ion-item>

Your code sample uses neither visible text nor an aria label, so we cannot safely assume you are using the modern rendering syntax. Providing either visible text in the component or an aria-label should dismiss the warning. Developers who are opted-in to the modern syntax and wish to use the legacy syntax can set legacy="true" on the component.

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label Feb 20, 2023
@liamdebeasi liamdebeasi removed their assignment Feb 20, 2023
@ionitron-bot ionitron-bot bot removed the triage label Feb 20, 2023
@lincolnthree
Copy link
Author

lincolnthree commented Feb 21, 2023

Ah thank you. I misread the warning. I got confused by the part about ion-label and totally glossed over the case where label is required. Might I propose re-wording this a bit to prevent similar confusion in the future? Or making the warning more specific for the specific deprecation issue that's been encountered?

General rewording:

[Ionic Warning]: ion-select now requires either a set [label] attribute, or a set 'aria-label' attribute for compatibility with screen readers. Use of ion-label has been deprecated and will stop working in Ionic v8. To migrate, remove any attached ion-label component, and ensure the appropriate "label" or "aria-label" property is set on ion-select instead.

Example: <ion-select label="Favorite Color">...</ion-select>

For selects that do not have a visible label, developers should use "aria-label" so screen readers can announce the purpose of the select.

For selects that do not render the label immediately next to the select, developers may continue to use "ion-label" but must manually associate the label with the select by using "aria-labelledby". 

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Feb 21, 2023
@lincolnthree
Copy link
Author

Additionally, it appears that [interfaceOptions]={ header: 'My header' } does not satisfy this constraint. Should it?

@liamdebeasi
Copy link
Contributor

No, because header is passed to the overlay component (alert, popover) and not set on ion-select itself. I can update the deprecation message.

@lincolnthree
Copy link
Author

Understood. Thank you!

@liamdebeasi
Copy link
Contributor

liamdebeasi commented Feb 21, 2023

I added a PR in #26834 to clarify the deprecation messages.

@lincolnthree
Copy link
Author

lincolnthree commented Feb 21, 2023

Thanks, Liam. I've tried using a few of these options, but 'aria-labelledby' seems to result in a full-width control with no label. (The ion-toggle, ion-segment etc control takes up the full width of the container, and requires a width being set in order to leave space for the label to grow.)

Is this expected or am I just completely confused?

Screen Shot 2023-02-21 at 12 18 08 PM

  <ion-item>
    <ion-label id="foo">legacy</ion-label>
    <ion-select slot="end" placeholder="foo" [value]="'foo'">
      <ion-select-option value="asdf">asdf</ion-select-option>
    </ion-select>
  </ion-item>
  <ion-item>
    <ion-select placeholder="bar" [value]="'bar'" label="label attr">
      <ion-select-option value="bar">bar</ion-select-option>
    </ion-select>
  </ion-item>
  <ion-item>
    <ion-label id="baz">aria-labelledby attr</ion-label>
    <ion-select slot="end" placeholder="baz" [value]="'baz'" aria-labelledby="baz">
      <ion-select-option value="bar">baz</ion-select-option>
    </ion-select>
  </ion-item>
  <ion-item>
    <ion-select placeholder="cab" [value]="'cab'" aria-label="cab">
      <ion-select-option value="cab">cab</ion-select-option>
    </ion-select>
  </ion-item>

Repro:
https://github.com/lincolnthree/ionic-bug-ion-segment/tree/aria-labelledby

Essentially, unless I'm confused, the current state means that custom <ion-label> components cannot be used in conjunction with these new controls without adding a bunch of custom styling. I'm hoping this is a bug :)

@liamdebeasi liamdebeasi changed the title bug: ion-select firing logging warning/deprecation message to console even when condition not met bug: shadow dom form controls opt-in to modern syntax with aria-labelledby Feb 21, 2023
@liamdebeasi
Copy link
Contributor

aria-labelledby should not be used with these components because they use the Shadow DOM. (aria-labelledby does not work across shadow roots). aria-labelledby can be used with ion-input and ion-textarea since they do not use the Shadow DOM.

I'll update our internal logic to not opt-in to the modern syntax in this scenario, but you'll still want to remove the aria-labelledby usage here. I also updated the deprecation warning in #26834. Looks like the aria-labelledby mention was a copy and paste mistake from ion-input and ion-textarea.

@liamdebeasi liamdebeasi added package: core @ionic/core package type: bug a confirmed bug report labels Feb 21, 2023
@lincolnthree
Copy link
Author

lincolnthree commented Feb 21, 2023

Hmm. Interesting. So what would the equivalent use-case be for <ion-select> components? When I add aria-label to them, they change their styling and break all sorts of things.

Correct layout - without aria-label (and emitting a warning to console):
image

Broken layout - with aria-label:
image

@liamdebeasi
Copy link
Contributor

Can you describe what is broken here?

@lincolnthree
Copy link
Author

lincolnthree commented Feb 21, 2023

Yeah, in the second 'broken' example, the ion-select has grown in height. The only difference was the addition of the aria-label attribute.

@liamdebeasi
Copy link
Contributor

Ah thanks. We updated the min-heights of ion-select in Ionic 7 because it did not match the iOS/MD specs:


You can account for this by modifying min-height.

@lincolnthree
Copy link
Author

lincolnthree commented Feb 21, 2023

Good to know, thanks. But why should applying the aria-label attribute change the height? Isn't that property supposed to be used when an actually visually displayed label does not exist? I'd expect this wouldn't alter the display styling. Am I missing something?

@liamdebeasi
Copy link
Contributor

Using aria-label opts you in to using the new form syntax. We use some heuristics to do this so developers don't need to manually opt-in in Ionic 7 and then remove that opt-in when Ionic 8 comes around.

So providing a label (either by visible text or an aria-label) will opt you into the new syntax. We provided an escape hatch using legacy="true" for when the heuristic gets it wrong.

https://ionicframework.com/docs/v7/api/input#migrating-from-legacy-input-syntax has more information on this.

@lincolnthree
Copy link
Author

lincolnthree commented Feb 21, 2023

Hmm.

I have to assume using legacy=true is not a great long-term solution, though, as I'd expect the legacy escape hatch to be removed in a future version, no?

Thanks for the info. I think I still consider it to be a bug or usability hazard at best -- styles changing when specifying an attribute that is supposed to only impact a11y functionality is set seems counter-intuitive. I understand your explanation of 'opting in to new display logic' but I'd expect that only to matter (only be opted in) if you are using a property that impacts the layout like 'label=foo'.

IMO. Overriding / extending the a11y properties and using them for layout decisions seems dangerous. It removes (or significantly complicates) developer control of how to design their app while still supporting a11y readers.

I'd expect using either of the aria- attributes would simply squelch the warning messages and meet the requirements for screen readers, nothing more. It sounds like you agree for aria-labelledby, and I'm wondering why not for aria-label.

I'll work on compensating for this in our CSS I guess :)

@lincolnthree
Copy link
Author

lincolnthree commented Feb 21, 2023

aria-labelledby should not be used with these components because they use the Shadow DOM. (aria-labelledby does not work across shadow roots).

Okay, so following that and we don't use aria-labelledby, but rather:

If the ion-select has no label, and has no aria- attributes, an aria-label will generated that contains the current value of the ion-select -- that's okay -- however, we still get a deprecation message that instructs us to use aria-label or label.

Now, this becomes problematic because once we do specify an aria-label in this scenario, Ionic does something that removes that aria-label from the dom entirely. We get no generated attribute and our attribute has disappeared.

@liamdebeasi
Copy link
Contributor

liamdebeasi commented Feb 21, 2023

I have to assume using legacy=true is not a great long-term solution, though, as I'd expect the legacy escape hatch to be removed in a future version, no?

That's correct. It is a temporary escape hatch for when a) you are not ready to migrate to the modern syntax and b) our heuristic incorrectly opts you in to the new syntax.

edit: We will also add a console warning to let developers know that legacy="true" is temporary.

Thanks for the info. I think I still consider it to be a bug or usability hazard at best -- styles changing when specifying an attribute that is supposed to only impact a11y functionality is set seems counter-intuitive. I understand your explanation of 'opting in to new display logic' but I'd expect that only to matter (only be opted in) if you are using a property that impacts the layout like 'label=foo'.

IMO. Overriding / extending the a11y properties and using them for layout decisions seems dangerous. It removes (or significantly complicates) developer control of how to design their app while still supporting a11y readers.

This is why the legacy escape hatch exists. If we only looked at the label prop, then developers who are only using aria-label would never be able to opt-in to the modern syntax until Ionic 8. We want to encourage developers to adopt this new syntax before the old syntax is removed. However, we recognize some developers will not be ready to migrate yet.

I'd expect using either of the aria- attributes would simply squelch the warning messages and meet the requirements for screen readers, nothing more. It sounds like you agree for aria-labelledby, and I'm wondering why not for aria-label.

We removed the aria-labelledby requirement because it does not work with Shadow DOM components. So it's not that we want to change the behavior, but it's more that using aria-labelledby didn't actually work, it just suppressed the warning message. In other words, the native form control was never associated with the element referenced by aria-labelledby.


We realize that the heuristic isn't going to get it right 100% of the time, but the alternative is to have devs manually opt in to the new syntax with modern="true" or something similar. We don't like this approach because it essentially penalizes early adopters by a) making them add modern="true" and then b) making them remove it in Ionic 8 when the modern syntax is the default.

@lincolnthree
Copy link
Author

This is why the legacy escape hatch exists.

I totally agree. modern=true is not the direction I'd take either. I'm essentially trying to figure out what the end state of this looks like once "migration is complete" and that escape hatch is removed. Trying to figure out it it breaks everything or can be compensated for with CSS.

I haven't found an incantation that works quite yet, but I think I'm getting closer. Thanks for listening, for your patience & time.

@liamdebeasi
Copy link
Contributor

Oh another thing to note is the labelPlacement and justify properties:

https://ionicframework.com/docs/v7/api/select#label-placement
https://ionicframework.com/docs/v7/api/select#justification

Justify replaces using an ion-label with a slot in ion-item. Label Placement lets you control which order the label/control show up in.

@lincolnthree
Copy link
Author

lincolnthree commented Feb 21, 2023

Out of curiosity, (and this might help provide some context) do you have a recommended pattern / example for using complex labels like this without using "Legacy" mode? Or is this something that's not really "intended"? Since aria-labelledby is not supported for some of these components, what is the alternative? aria-label and justification?

image

@liamdebeasi
Copy link
Contributor

Something like this should work:

<ion-item>
  <ion-toggle>
    <div>View mode</div>
    <ion-note>Choose how decks are presented visually when viewing or editing.</ion-note>
  </ion-toggle>
</ion-item>

Example: https://codepen.io/liamdebeasi/pen/KKxzdEQ

In this case both the "View mode" label header and the sub text ("Choose how decks...") are associated with the control and therefore announced by screen readers.

If you have visible text most of the migration should be moving it from ion-label to being passed into the component.

@liamdebeasi
Copy link
Contributor

Though for ion-select it looks like we only support the label property (so plain text only, no HTML). If you created a feature request for passing a label to ion-select via slotted content like you can with ion-toggle I think it's likely that we would add it.

@lincolnthree
Copy link
Author

Very interesting. Thank you. This really is a big change. I presume the same would work for ion-checkbox?

It looks like ion-range already uses the slot syntax. Though... question... would it make sense to support the slot=label syntax on ALL of these components, regardless of whether or not they use the shadow dom? That would make things more consistent, even if it's not technically "required". Thoughts?

I'll create a feature request for ion-select and link it here.

@liamdebeasi
Copy link
Contributor

Yes, the same works for checkbox. Checkbox, Radio, and Select have a default slot to pass in the label. Range only has a named slot (the 'label' slot) because it has other slots already ('start' and 'end'). Unfortunately, input and textarea do not use the Shadow DOM, so they can't take advantage of slots.

@lincolnthree
Copy link
Author

lincolnthree commented Feb 21, 2023

Why not send the default slot to label for all of the input components (for consistency)?

Also, I didn't think Shadow DOM was required for slotting? I have plenty of components that don't use Shadow DOM that supports this via <ng-content select="[slot=end]">. Though this is Angular, not necessarily one of the other frameworks so I don't know if it would work there.

@liamdebeasi
Copy link
Contributor

liamdebeasi commented Feb 21, 2023

We could, but it increase the boilerplate for certain components. So instead of <ion-toggle>My Label</ion-toggle> you'd have to do <ion-toggle><div slot="label">My Label</div></ion-toggle>.

Though we have discussed allowing for the label property for simple text. If we made all the default slots named slots we could add the label prop for simpler use cases to avoid the additional boiler plate.


Web Component slots is a Shadow DOM-only feature. Stencil lets you use slots in scoped components, but it's a bit buggy.

@lincolnthree
Copy link
Author

lincolnthree commented Feb 21, 2023

RE: Web component slots/shadow dom. Gotcha.

For the rest, I may have been vague. Sorry. I was trying to ask why not send all nested content to the label slot by default, since that seems to be what happens for components that don't use the Shadow DOM?:

<ion-toggle>My Label</ion-toggle>
<ion-select>My Label</ion-select>
<ion-select>My Label<div>Also part of the label</div></ion-select>

Where all of that gets put into the label slot by default if the component is slotted/Shadow DOM. For non Shadow DOM components like ion-toggle this already works so nothing to change there.

E.g. Why isn't this how <ion-range> works for nested content, why is slot=label required there. Yes it has start and end, but unspecified/default content could be sent to label by default, no?

@lincolnthree
Copy link
Author

lincolnthree commented Feb 21, 2023

Oh, I guess with ion-select you already have <ion-select-option> children that need to be put in the right place. So you'd need the slot=label designation. Is it possible pull specific nodes into slots, by type rather than by slot attribute? (Again, doable in angular, but...)

So I guess this could work for ion-range, but at that point, you've already got two syntaxes so it doesn't really matter which you use here. I could see ion-select and ion-range being the 'outliers' (or any other input components that use the Shadow DOM, to put an explicit rule in place.)

@lincolnthree
Copy link
Author

Opened an issue to support slot=label on ion-select instances: #26838

@liamdebeasi
Copy link
Contributor

Yeah it's a bit tricky with the slot naming. I think we'd do ion-select as a named slot so we could add adornments in the future via slots (i.e. #26297).

Thanks for filing an issue! I'll take a look at that.

@lincolnthree
Copy link
Author

lincolnthree commented Feb 21, 2023

I think we'd do ion-select as a named slot so we could add adornments in the future via slots (i.e. #26297).

Yeah, this is where I ended up as well. I also think all ion-* components should just use slot=label even if it doesn't strictly require it, technically. More boilerplate? Yes, but also more consistent developer experience and more future-proof. If people don't want boilerplate, they can still use label='my label text' -- bring on the shadow dom

You're welcome! Let me know if I can do anything else to assist. Excited for V7. It solves a lot of problems we've been having with (ionChange) events and such, and is overall very nice! Most things work after upgrading without any effort at all.

@liamdebeasi
Copy link
Contributor

Thanks for the issue. This has been resolved via #26836, and a fix will be available in an upcoming release of the Ionic v7 beta. Updates on the select label feature will be posted in #26838.

@ionitron-bot
Copy link

ionitron-bot bot commented Mar 24, 2023

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Mar 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
package: core @ionic/core package type: bug a confirmed bug report
Projects
None yet
Development

No branches or pull requests

2 participants