ory / elements

Ory Elements is a component library that makes building login, registration and account pages for Ory a breeze. Check out the components library on Chromatic https://www.chromatic.com/library?appId=63b58e306cfd32348fa48d50
https://ory.sh
Apache License 2.0
85 stars 44 forks source link

feat: password with visibility toggle #119

Closed Benehiko closed 1 year ago

Benehiko commented 1 year ago

Related Issue or Design Document

Although this looks simple to achieve on the surface, this has many hidden problems when implemented with CSS only. The Account Experience only uses CSS with server-side rendering and has no support for JS on the client. This requires some more investigation into the viability of the feature with CSS-only and if it makes sense to persue this for the Account Experience at the current moment.

When using CSS only we need to set the input field to [type=text] and set the password styling using -webkit-security-text. https://caniuse.com/mdn-css_properties_-webkit-text-security This css property is not a standard and has only recently received support in firefox. There is a risk that users running on an obscure browser or using an old browser will always see their password with the toggle having no effect.

Some alternatives to consider:

  1. Add a flag that allows users consuming the library to set this (css only method)
  2. Add react support to swap out the input type using JS.

https://github.com/ory/elements/issues/116

image

Checklist

Further comments

LBBO commented 1 year ago

I really don't like having an insecure default for older browsers. How about adding two basically identical inputs, one with type="password" and one with type="text", have the text hidden by default and then use a @supports query to show the text input and hide the password input iff this feature is supported? ~I would assume that browsers that support @supports are a superset of those that support -webkit-security-text and~ (edit: turns out it's the other way around, but that doesn't change my opinion) this way, if a browser doesn't support either of the features we still have secure defaults and the user just misses out on the "view password" feature.

https://caniuse.com/?search=@supports https://developer.mozilla.org/en-US/docs/Web/CSS/@supports

LBBO commented 1 year ago

Hmm, while aparently most browsers added support for @supports way later than for webkit-text-security, it's the other way around for Firefox. And the percentages of supporting browsers are similar enough that I still think this would be a good idea.

Benehiko commented 1 year ago

Thank you for the input! I agree with this sentiment.

Benehiko commented 1 year ago

NextJS is failing to build due to the @supports(:has) css selector. The reason for this is NextJs bundles postcss and autoprefixer by default. There was a bug in autoprefixer which prevents it from understanding the @supports css selector with the pseudo parameter :has. See this issue https://github.com/postcss/autoprefixer/issues/1391

So according to the autoprefixer issue, this should be fixed by now in version 10.2.5, but on version 10.4.x this is still a problem.

I have added autoprefixer to vite so we can catch this on build time of our react components in the latest commit. I think we should also add a browser support list to the .browserlistrc file so that future css selectors added to components do not break the browsers we want to support.

 [vite:css] [postcss] Unknown node type in node creation
       file: /home/benehiko/Github/elements/src/theme/input-field.css.ts.vanilla.css:undefined:undefined
       error during build:
       Error: Unknown node type in node creation
           at /home/benehiko/Github/elements/src/theme/input-field.css.ts.vanilla.css:115:1
           at Rule.normalize (/home/benehiko/Github/elements/node_modules/postcss/lib/container.js:212:13)
           at Rule.append (/home/benehiko/Github/elements/node_modules/postcss/lib/container.js:30:24)
           at Supports.virtual (/home/benehiko/Github/elements/node_modules/autoprefixer/lib/supports.js:297:10)
           at Supports.prefixed (/home/benehiko/Github/elements/node_modules/autoprefixer/lib/supports.js:179:21)
           at /home/benehiko/Github/elements/node_modules/autoprefixer/lib/supports.js:35:29
           at Array.map (<anonymous>)
           at Supports.add (/home/benehiko/Github/elements/node_modules/autoprefixer/lib/supports.js:33:18)
           at Supports.process (/home/benehiko/Github/elements/node_modules/autoprefixer/lib/supports.js:231:16)
           at /home/benehiko/Github/elements/node_modules/autoprefixer/lib/processor.js:67:27
           at /home/benehiko/Github/elements/node_modules/postcss/lib/container.js:324:18
       npm ERR! Lifecycle script `build` failed with error:
       npm ERR! Error: command failed
       npm ERR!   in workspace: @ory/elements@0.0.0
       npm ERR!   at location: /home/benehiko/Github/elements/packages/react

@LBBO maybe you could take a look at this when you get a chance?

LBBO commented 1 year ago

I pushed some changes, let me know what you think!

LBBO commented 1 year ago

Tests are failing due to duplicate IDs with the two inputs now. Ideas:

a) only render the fallback and then switch using JS if the required features are supported b) turn the entire wrapper into a label so we don't need the htmlFor anymore. might cause SEO issues (and rightfully so) and isn't really any better than having the ID clash c) disambiguate the IDs using prefixes / postfixes & add another label that is shown / hidden with its input. would break any hard ID-references that current users have