Skip to content
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

focusStrokeWidth not applied on text-field and number-field #5566

Conversation

fcollonval
Copy link
Contributor

Pull Request

📖 Description

Focus style on text and number fields does not use focusStrokeWidth (as e.g. do select).

Question: there is also a variation the text and number fields are using inset box-shadow when select is not. Does this need to be changed too? If so which one is the correct style to apply?

🎫 Issues

Fixes #5565

👩‍💻 Reviewer Notes

📑 Test Plan

✅ Checklist

General

  • I have included a change request file using $ yarn change
  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.

⏭ Next Steps

@ghost
Copy link

ghost commented Feb 7, 2022

CLA assistant check
All CLA requirements met.

@fcollonval fcollonval changed the title Fix/focusStrokeWidth-not-applied-on-text-field-and-number-field focusStrokeWidth not applied on text-field and number-field Feb 7, 2022
@chrisdholt chrisdholt merged commit ec5ceae into microsoft:master Feb 7, 2022
@chrisdholt
Copy link
Member

Thanks @fcollonval!

@bheston
Copy link
Collaborator

bheston commented Feb 8, 2022

Sorry I didn't get to this earlier today, but while the current code was not correct, the update isn't quite right either. Many of the controls that have an outline keep the outline in focus state and add an inner box-shadow to supplement the outline. So the current code assumes that the focus outline is always +1px from the outline stroke size. The correct way some of the components do this is to subtract the outline size from the focus size. This still assumes that the focus is larger than the outline, but that's probably pretty safe.

I have in mind a better way of doing this using outline instead, which we should consider as part of the visual refresh.

@fcollonval fcollonval deleted the fix/`focusStrokeWidth`-not-applied-on-text-field-and-number-field branch February 8, 2022 07:28
@fcollonval
Copy link
Contributor Author

@bheston if I get you correctly a better fix would be to set the shadow width to calc((${focusStrokeWidth} - ${strokeWidth}) * 1px)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

focusStrokeWidth not applied on text-field and number-field
4 participants