close
Skip to content

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

Merged
t-hamano merged 5 commits intoWordPress:trunkfrom
Vrishabhsk:fix/featured-img-transition
Oct 17, 2024
Merged

Fix : "Set featured image" button border flashes on focus#66092
t-hamano merged 5 commits intoWordPress:trunkfrom
Vrishabhsk:fix/featured-img-transition

Conversation

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

@github-actions
Copy link
Copy Markdown

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 <vrishabhsk@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@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 <vrishabhsk@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>
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