Move auth components from shadow DOM to light DOM #18015

piitaya · 2023-09-25T13:41:12Z

Proposed change

Move the auth component from shadow DOM to light DOM to have better (full?) password manager support.
It's seems to work (at least with 1password). Tested in Chrome desktop, Safari desktop, Chrome Android and Safari iOS.

Some components are duplicated (ha-auth-form, ha-auth-textfield, ha-auth-form-string) so we don't need to touch original component to add light dom support.

All previous features works and the design is the same as before.

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New feature (thank you!)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Example configuration

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue or discussion:
Link to documentation pull request:

Checklist

The code change is tested and works locally.
There is no commented out code in this PR.
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

bramkragten · 2023-09-25T13:52:41Z

src/components/ha-form/ha-form.ts

+  protected fieldElementName(type: string) {
+    return `ha-form-${type}`;
+  }


This should be handled in core eventually, it can provide a different type for the auth text fields.

I thought that too at the beginning.
Don't you think we can keep this in front-end as it is a front-end issue. We can remove this code once shadow dom support is added to password manager (never? 😂)

bramkragten · 2023-09-25T13:52:56Z

src/auth/ha-auth-form.ts

+  protected fieldElementName(type: string): string {
+    if (type === "string") {
+      return "ha-auth-form-string";
+    }
+    return super.fieldElementName(type);
+  }


This should be handled in core eventually, it can provide a different type for the auth text fields.

Yes, so we don't have to duplicate ha-form but only add an option to render without shadow dom.

steverep · 2023-09-25T17:05:31Z

For now ha-auth-form-string is only a copy of ha-form-string but it should be styled with material guidelines and we need to add a11y support.

I can review for a11y if and when we might be ready to merge, so just let me know. Also, just throwing out an idea, maybe use Vaadin components here to gain most of the Material styling (e.g. their canned login form)? Advantage is that they already proxy everything to light DOM by design, and it might be easier to maintain/swap out when the bigger password managers actually decide to implement support.

bramkragten · 2023-09-25T19:39:52Z

If we use Vaadin we will stil need to place that inside the light DOM, and it gives us a lot less flexibility.

steverep · 2023-09-25T19:53:03Z

If we use Vaadin we will stil need to place that inside the light DOM, and it gives us a lot less flexibility.

AFAIK, they switched most if not all their components to use this "teleport" system to render in the light DOM. One of the impetuses was for better a11y because a lot of ARIA attributes take ID references, which quickly break when shadow trees get involved.

In other words, I think just using them inside the existing ha-auth-flow would allow deleting the polyfill.

bramkragten · 2023-09-25T20:00:23Z

Only if you use the modal version:

Login is incompatible with password managers if placed inside another component’s shadow root. [1] This isn’t an issue, though, when using Login’s modal overlay.

steverep · 2023-09-25T20:26:43Z

Hmm... RTFM Steve 😊

piitaya · 2023-09-25T21:12:34Z

I found a way to load ha-textfield styles. ~~The only thing missing is the required helper on the field but I think we can live without it 😅~~ Done !

@bramkragten I preferred to duplicate some component for auth instead of adding option to ha-form and ha-form-string and ha-textfield so it's easier to maintain and we only have to remove these file once the shadow dom is supported by password manager. WDYT?

piitaya · 2023-09-26T09:24:30Z

Ready to review !
@steverep, the html code isn't changed so accessibility support should be the same but I would prefer a review from you 🙂

bramkragten · 2023-09-26T14:46:24Z

src/auth/ha-auth-flow.ts

+      <style>
+        .action {
+          margin: 24px 0 8px;
+          text-align: center;
+        }
+        .store-token {
+          margin-top: 10px;
+          margin-left: -16px;
+        }
+      </style>


Let's keep this in static get styles, that should also work when using light dom right?

static styles doesn't work with light dom. That's the reason I use this hacky thing 😅

const style = document.createElement("style"); style.innerHTML = HaTextField.elementStyles as unknown as string; this.append(style);

I thought Lit would do that for use, but ok 🤷‍♂️

Because that's something that's done in the default createRenderRoot implementation.

steverep

Was in the middle of reviewing... 😞

Aside from my comments inline, there is a significant a11y issue now. The password field is labeled with the username text. This is because the input has aria-labelledby="label", so now it just finds the first element in the document with that ID, which is of course for the username. Note they don't have helpers so not strictly a problem, but aria-controls and aria-describedby would also be broken.

Either need to give everything distinct id attributes, or delete the aria-labelledby as it's redundant with the wrapping label element anyway.

steverep · 2023-09-26T17:55:39Z

src/auth/ha-auth-form.ts

+    // add parent style to light dom
+    const style = document.createElement("style");
+    style.innerHTML = HaForm.elementStyles as unknown as string;
+    this.append(style);


Most browsers don't care I think, but strictly speaking, it should be attached to document.head.

steverep · 2023-09-26T18:01:17Z

src/auth/ha-auth-textfield.ts

+    // add parent style to light dom
+    const style = document.createElement("style");
+    style.innerHTML = HaTextField.elementStyles as unknown as string;
+    this.append(style);


steverep · 2023-09-26T18:10:51Z

src/auth/ha-auth-form-string.ts

+  protected createRenderRoot() {
+    // add parent style to light dom
+    const style = document.createElement("style");
+    style.innerHTML = HaFormString.elementStyles as unknown as string;


Should use textContent instead for security and performance

piitaya · 2023-09-26T19:43:16Z

Was in the middle of reviewing... 😞

Aside from my comments inline, there is a significant a11y issue now. The password field is labeled with the username text. This is because the input has aria-labelledby="label", so now it just finds the first element in the document with that ID, which is of course for the username. Note they don't have helpers so not strictly a problem, but aria-controls and aria-describedby would also be broken.

Either need to give everything distinct id attributes, or delete the aria-labelledby as it's redundant with the wrapping label element anyway.

@steverep I pushed a fix for this issue : #18032. Good catch!
I also improved the style definition because there style are global and not scoped.

KTibow · 2023-10-01T14:51:54Z

This is causing some jankiness, at least in Firefox. See the attached screen recording. This was recorded using a development build on the latest version of dev.
Screencast from 2023-10-01 07-48-13.webm

frenck · 2023-10-01T16:36:49Z

@KTibow Let's keep track of such things in issues, instead of handled/closed PRs.

Thanks 👍

../Frenck

home-assistant bot added the cla-signed label Sep 25, 2023

bramkragten reviewed Sep 25, 2023

View reviewed changes

bramkragten mentioned this pull request Sep 25, 2023

Improve login page #18005

Closed

8 tasks

piitaya force-pushed the improve_password_manager_support branch from 8a17af6 to b3bb005 Compare September 25, 2023 21:10

piitaya marked this pull request as ready for review September 26, 2023 09:21

piitaya added 8 commits September 26, 2023 15:45

Move auth components from shadow DOM to light DOM

8f0c458

Move style inside component

e8bd549

Add textfield style

344c2f6

Clean authorize template

ef4c5b3

Clean light dom ref

9f0db2a

Fix build

8449e2c

Restore validation message

792de52

Fix ha form

4f9df62

bramkragten reviewed Sep 26, 2023

View reviewed changes

piitaya force-pushed the improve_password_manager_support branch from 667a84a to 4f9df62 Compare September 26, 2023 15:32

bramkragten approved these changes Sep 26, 2023

View reviewed changes

piitaya merged commit d8c98d8 into dev Sep 26, 2023
8 checks passed

piitaya deleted the improve_password_manager_support branch September 26, 2023 18:17

steverep reviewed Sep 26, 2023

View reviewed changes

piitaya mentioned this pull request Sep 27, 2023

[Password Managers] Some password manager icons do not show up on input fields #10274

Closed

3 tasks

KTibow mentioned this pull request Oct 1, 2023

Authorize page inputs are buggy with dev build #18084

Closed

3 tasks

DavidMStraub mentioned this pull request Feb 14, 2024

Error 422 when attempting to log in with password manager gramps-project/gramps-web#310

Open

github-actions bot locked and limited conversation to collaborators Sep 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move auth components from shadow DOM to light DOM #18015

Move auth components from shadow DOM to light DOM #18015

piitaya commented Sep 25, 2023 •

edited

Loading

bramkragten Sep 25, 2023

piitaya Sep 26, 2023

bramkragten Sep 25, 2023

piitaya Sep 25, 2023

steverep commented Sep 25, 2023

bramkragten commented Sep 25, 2023

steverep commented Sep 25, 2023 •

edited

Loading

bramkragten commented Sep 25, 2023 •

edited

Loading

steverep commented Sep 25, 2023

piitaya commented Sep 25, 2023 •

edited

Loading

piitaya commented Sep 26, 2023

bramkragten Sep 26, 2023 •

edited

Loading

piitaya Sep 26, 2023 •

edited

Loading

bramkragten Sep 26, 2023

KTibow Sep 27, 2023

steverep left a comment

steverep Sep 26, 2023

steverep Sep 26, 2023

steverep Sep 26, 2023

piitaya commented Sep 26, 2023 •

edited

Loading

KTibow commented Oct 1, 2023

frenck commented Oct 1, 2023

Move auth components from shadow DOM to light DOM #18015

Move auth components from shadow DOM to light DOM #18015

Conversation

piitaya commented Sep 25, 2023 • edited Loading

Proposed change

Type of change

Example configuration

Additional information

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steverep commented Sep 25, 2023

bramkragten commented Sep 25, 2023

steverep commented Sep 25, 2023 • edited Loading

bramkragten commented Sep 25, 2023 • edited Loading

steverep commented Sep 25, 2023

piitaya commented Sep 25, 2023 • edited Loading

piitaya commented Sep 26, 2023

bramkragten Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

piitaya Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steverep left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piitaya commented Sep 26, 2023 • edited Loading

KTibow commented Oct 1, 2023

frenck commented Oct 1, 2023

piitaya commented Sep 25, 2023 •

edited

Loading

steverep commented Sep 25, 2023 •

edited

Loading

bramkragten commented Sep 25, 2023 •

edited

Loading

piitaya commented Sep 25, 2023 •

edited

Loading

bramkragten Sep 26, 2023 •

edited

Loading

piitaya Sep 26, 2023 •

edited

Loading

piitaya commented Sep 26, 2023 •

edited

Loading