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

Fix : "Set featured image" button border flashes on focus #66092

Merged
merged 5 commits into from
Oct 17, 2024

Conversation

Vrishabhsk
Copy link
Contributor

@Vrishabhsk Vrishabhsk commented Oct 14, 2024

What?

  • Add box-shadow style for :focus:not(:disabled) state of "Set featured image" button.

Why?

There is a lag between the focus ring appearing and the normal border disappearing.

Fixes #65299

How?

  • The box-shadow inset property isn't affected by the transition delay
  • To prevent this, the style was implemented as per the steps mentioned here : #66007

Screenshots or screencast

Original

Screen.Recording.2024-10-14.at.4.14.16.PM.mov

Improved

Screen.Recording.2024-10-14.at.4.16.01.PM.mov

Copy link

github-actions bot commented Oct 14, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: Vrishabhsk <[email protected]>
Co-authored-by: t-hamano <[email protected]>
Co-authored-by: mirka <[email protected]>
Co-authored-by: tyxla <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link
Contributor

@t-hamano t-hamano left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I noticed that the moment the button was focused, the border changed to black.

See the slow motion video below:

8ad8e1d05bf8758e58904ad525842158.mp4

I looked for the cause and found that these three styles were affecting it. Since appropriate styles were already provided, it seemed safe to remove them.

Below is the slow motion video after removing these three styles:

f9c6dc6aef898cd64fc5ba858355caa0.mp4

@t-hamano t-hamano added [Type] Bug An existing feature does not function as intended [Package] Editor /packages/editor labels Oct 14, 2024
@mirka mirka linked an issue Oct 14, 2024 that may be closed by this pull request
Transition is removed to prevent a black border in between transitioning
@Vrishabhsk
Copy link
Contributor Author

Thanks @t-hamano for the catch.

  • Instead of removing all three styles, I instead removed only the transition line
  • This prevents the black border which appeared for a split-second before the transition was complete.
  • Also removing all three styles, does change the border color quite a bit from the original
  • Here is the preview of that, slowed down
Screen.Recording.2024-10-15.at.11.16.59.AM.mov

Let me know your thoughts on this. Thanks

@t-hamano
Copy link
Contributor

Thanks for the update!

Also removing all three styles, does change the border color quite a bit from the original

Does this have to do with this box-shadow? This style is always supposed to be overridden here, so I'm wondering why it's affecting the transition 🤔

@Vrishabhsk
Copy link
Contributor Author

Vrishabhsk commented Oct 15, 2024

Hi @t-hamano,

  • As per computed styles for the buttton, it has the following styles
    border-width: 2px;
    border-style: outset;
    border-color: buttonborder;
    border-image: initial;
  • This style is overridden by the .components-button selector, which sets border: 0
  • The transition used is applied on for all CSS properties.
  • Hence, it takes the border also into account, and it appears for a split second
  • This is also why removing that transition prevents the black border from appearing at all

@t-hamano t-hamano self-requested a review October 16, 2024 02:39
Copy link
Contributor

@t-hamano t-hamano left a comment

Choose a reason for hiding this comment

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

LGTM!

One last thing: what do you think about removing this mixin? Since this mixin exists in the Button component itself, it seems safe to remove it.

@Vrishabhsk
Copy link
Contributor Author

Hi @t-hamano 👋, I have removed the redundant mixin. Thanks for pointing that out.

@t-hamano t-hamano merged commit 6fc539d into WordPress:trunk Oct 17, 2024
61 checks passed
@github-actions github-actions bot added this to the Gutenberg 19.6 milestone Oct 17, 2024
karthick-murugan pushed a commit to karthick-murugan/gutenberg that referenced this pull request Nov 13, 2024
…66092)

* Enable smooth transition of box-shadow for feat. img.

* Remove fallback style and remove transition

Transition is removed to prevent a black border in between transitioning

* Update changelog

* Remove redundant style

Co-authored-by: Vrishabhsk <[email protected]>
Co-authored-by: t-hamano <[email protected]>
Co-authored-by: mirka <[email protected]>
Co-authored-by: tyxla <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Editor /packages/editor [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Set featured image" button border flashes on focus
3 participants