Skip to content

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

Merged
dancormier merged 6 commits intodevelopfrom
dcormier/input-focus-within
Feb 23, 2024
Merged

fix(form-elements): add focus-within focus styling#1646
dancormier merged 6 commits intodevelopfrom
dcormier/input-focus-within

Conversation

@dancormier
Copy link
Copy Markdown
Contributor

@dancormier dancormier commented Feb 12, 2024

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.

Related: #1629

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.

In the process of reviewing StackEng/StackOverflow#18700, we identified an issue with non-focusable elements styled to look like inputs with `.s-input` ([tag editor input for example](StackEng/StackOverflow#18700 (comment))). These elements will 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.
@netlify
Copy link
Copy Markdown

netlify Bot commented Feb 12, 2024

Deploy Preview for stacks ready!

Name Link
🔨 Latest commit d8f9b71
🔍 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...

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 dancormier requested a review from giamir February 12, 2024 16:59
Copy link
Copy Markdown
Contributor

@giamir giamir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on MDN definition the :focus-within CSS pseudo-class represents an element that is itself matched by the :focus pseudo-class or has a descendant that is matched by :focus. If I understand that correctly we can simply apply :focus-within and that would also take care of :focus.

Overall the changes are pretty simple and I am ok to merge them. I probably don't fully understand the creative ways we use those classes in Core though. See sidenote for more details. 🙂

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.

@dancormier
Copy link
Copy Markdown
Contributor Author

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).

image

<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
Copy link
Copy Markdown
Contributor

giamir commented Feb 16, 2024

@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
Copy link
Copy Markdown
Contributor Author

@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 87f10c4

@dancormier dancormier requested a review from giamir February 20, 2024 22:25
Copy link
Copy Markdown
Contributor

@giamir giamir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @dancormier for improving the docs with this use case. Apart from a small copy/paste error everything looks good to me.

I also noticed that the example doesn't look right in dark mode (see screenshot). I believe we could apply some atomic classes to make sure the page stays in harmony.

Screenshot 2024-02-21 at 08 59 48

Thank you. 🙂

Comment thread docs/product/components/inputs.html Outdated
@dancormier dancormier requested a review from giamir February 22, 2024 21:48
Copy link
Copy Markdown
Contributor

@giamir giamir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to go now. Thanks for addressing the comments. 🙂

@dancormier dancormier merged commit 655e3b2 into develop Feb 23, 2024
@dancormier dancormier deleted the dcormier/input-focus-within branch February 23, 2024 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants