fix(form-elements): add focus-within focus styling

dancormier commented 5 months ago

In the process of reviewing https://github.com/StackEng/StackOverflow/pull/18700, we identified an issue with non-focusable elements styled to look like inputs with .s-input (tag editor input for example). These elements contain invisible or restyled input elements. We want to ensure that the elements with .s-input (or similar) receive focus styling when child elements are focused.

Note In this PR, I added :focus-within styling to all Stacks component form elements except s-uploader. That element has a unique structure that doesn't necessitate handling :focus-within.

netlify[bot] commented 5 months ago

Deploy Preview for stacks ready!

Name	Link
Latest commit	d8f9b71a1654b302b3469a63a640d375e84536e6
Latest deploy log	https://app.netlify.com/sites/stacks/deploys/65d7c0feafbeaa00088835df
Deploy Preview	https://deploy-preview-1646--stacks.netlify.app
Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

dancormier commented 5 months ago

Sidenote Do we incentivize the use of s-input, s-checkbox, etc... classes to elements that are not in fact input, input type="checkbox", etc...? If we do so we should have at least a use case example in the docs in my opinion documenting when that makes sense.

I am asking because an input type="checkbox" under regular circumstances would not have any focusable descendants and therefore there would not be a need to use :focus-within - In fact it would likely confuse the average reader unless there is at least a comment explaining why we use that over :focus.

This is more of a desire path situation. I wouldn't say that we explicitly incentivize the pattern of wrapping inputs in parents that include the input component class, but we have a few instances of it in Core (an example could be found on the question ask page).

<div class="js-tag-editor tag-editor multi-line s-input">
    <span>
        <span class="s-tag rendered-element">java<button class="s-tag--dismiss baw0 js-delete-tag" type="button" title="Remove tag"><svg …></svg></button></span>
    </span>
    <input type="text" id="tageditor-replacing-tagnames--input" class="s-input js-tageditor-replacing" …>
</div>

We could consider making this more specific and only applying the focus styles to :focus-within for s-input (and maybe s-textarea) instead of all the inputs. We could also consider adding an example tag input in the docs to show the recommended structure. @giamir What do you think? I'm happy to modify this PR however you recommend.

giamir commented 5 months ago

@giamir What do you think? I'm happy to modify this PR however you recommend.

I would say to keep the :focus-witihin only for the elements we know that there is a use case in Core and for those ones add a small blurp in the docs (or at a minimum a comment in the less code). Thank you 😊

dancormier commented 4 months ago

@giamir I've removed the :focus-within styling from all but the s-input and s-textarea components. I also added some user-facing documentation and a comment in the less code in https://github.com/StackExchange/Stacks/pull/1646/commits/87f10c41e4e139c17a65ff5bf83b58b78cbfa780

StackExchange / Stacks