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.

Sign up for GitHub

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

Jump to bottom

fix: figcaption rendering bug in Safari #1382

Merged
merged 2 commits into from
Jul 5, 2021
Merged

fix: figcaption rendering bug in Safari #1382

merged 2 commits into from
Jul 5, 2021

Conversation

dkoo
Copy link
Contributor

@dkoo dkoo commented Jun 23, 2021 •
edited by laurelfulford
Loading

All Submissions:

Changes proposed in this Pull Request:

Fixes an obscure Safari rendering bug which causes captions inside image blocks with an alignment set to render in the wrong position.

Closes #1132

How to test the changes in this Pull Request:

  1. Add an image block to a post and choose an image that has a caption. Set an alignment on the block (left, center, or right).
  2. On master, view the post in Safari. Observe that the caption appears in the wrong place (if you don't see the bug, try hard-refreshing the post):

Screen Shot 2021-06-23 at 3 44 30 PM

  1. Check out this branch and hard-refresh the post. Confirm that the caption now renders as expected.
  2. Re-test with all alignment options and no alignment options, and in another browser to ensure that the fix doesn't change anything outside Safari.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@dkoo dkoo added [Type] Bug Incorrect behavior or functionality [Status] Needs Review The issue or pull request needs to be reviewed labels Jun 23, 2021
@dkoo dkoo requested a review from laurelfulford June 23, 2021 21:47
@dkoo dkoo self-assigned this Jun 23, 2021
laurelfulford
laurelfulford requested changes Jun 23, 2021
View reviewed changes
Copy link
Contributor

@laurelfulford laurelfulford left a comment

Choose a reason for hiding this comment

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

There's a version of this fix that's only applied to mobile in the _blocks.scss file though this location makes more sense! That one should probably be removed, too as part of this PR.

The theme did include this fix for screen sizes at one point, but it causes smaller floated images to have their captions run wider than the images themselves. There's some more information and screenshots for that here:

#1109

IIRC the too-small-images-and-wide-captions issue was only reported once while this fix was in the theme, so I think that might be an acceptable trade-off at this point, since the caption gap issue has come up pretty often. I've been manually adding CSS fixes to the sites that report it, but it's probably better to go the other way around and manually undo it on any sites that report the small-image-wide-caption issue instead (I remember the site that originally reported the issue, so I can take care of that ahead of this being merged).

Let me know what you think about the above! If it sounds okay, I think this will be good to go once the near-duplicate styles are removed from _blocks.scss.

@laurelfulford laurelfulford added [Status] Needs Changes or Feeback The issue or pull request needs action from the original creator and removed [Status] Needs Review The issue or pull request needs to be reviewed labels Jun 23, 2021
@dkoo
Copy link
Contributor Author

dkoo commented Jun 23, 2021 •
edited
Loading

Wow, thanks for the historical context around this bug! That's a pretty infuriating trade-off—either we fix the bug in Safari at the expense of introducing another bug with smaller images in all browsers, or we leave the bug in Safari...

As discussed off GitHub, maybe the best hybrid approach is to fix the bug only in Safari and leave core styles active for all other browsers. This will introduce the bug with small-images-with-both-alignment-and-long-captions on desktop viewports in Safari only, but that seems to be more of an edge case than the caption rendering bug for Safari users.

This StackOverflow thread discusses some CSS hacks that supposedly work to target CSS in specific versions of Safari. I tried the @supports ( -webkit-hyphens: none ) hack that targets Safari 9+ (introduced 2015) and it seems to do the trick. I also removed the duplicate styles with the media query targeting only mobile viewports in _blocks.scss as you requested. 64c9596

@dkoo dkoo requested a review from laurelfulford June 23, 2021 22:48
laurelfulford
laurelfulford approved these changes Jun 25, 2021
View reviewed changes
Copy link
Contributor

@laurelfulford laurelfulford left a comment

Choose a reason for hiding this comment

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

Thanks @dkoo!

I took this for a spin both locally and using Browserstack; I can confirm this fix works, and, by using a smaller image, confirm that it's only being applied to Safari 9+ (on Safari 8, and other browsers, the smaller image's caption matches its width).

Thanks for tackling this! I'm looking forward to never hearing about this one again, it's been an annoying one 😄

@github-actions github-actions bot added the [Status] Approved The pull request has been reviewed and is ready to merge label Jun 25, 2021
@laurelfulford laurelfulford removed the [Status] Needs Changes or Feeback The issue or pull request needs action from the original creator label Jun 25, 2021
@laurelfulford
Copy link
Contributor

@dkoo I'm on a bit of a merging spree so I'm going to go ahead and merge this one! :)

@laurelfulford laurelfulford merged commit e274499 into master Jul 5, 2021
@laurelfulford laurelfulford deleted the fix/safari-caption-bug branch July 5, 2021 16:16
@dkoo
Copy link
Contributor Author

dkoo commented Jul 5, 2021

Oh, thanks for merging!

matticbot pushed a commit that referenced this pull request Jul 6, 2021
@semantic-release-bot
chore(release): 1.39.3-alpha.1 [skip ci]
be3aec9 
## [1.39.3-alpha.1](v1.39.2...v1.39.3-alpha.1) (2021-07-06)

### Bug Fixes

* ads media queries ([239ff6e](239ff6e))
* correct custom fonts in block widget preview ([#1401](#1401)) ([878b6a1](878b6a1))
* figcaption rendering bug in Safari ([#1382](#1382)) ([e274499](e274499))
* improve donate block appearance for WP 5.8 ([#1398](#1398)) ([51fc4f7](51fc4f7))
* prevent white screen and errors on Widget Screen ([#1390](#1390)) ([5731581](5731581))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.39.3-alpha.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Jul 6, 2021
@semantic-release-bot
chore(release): 1.39.3 [skip ci]
b6f7a7a 
## [1.39.3](v1.39.2...v1.39.3) (2021-07-06)

### Bug Fixes

* ads media queries ([239ff6e](239ff6e))
* correct custom fonts in block widget preview ([#1401](#1401)) ([878b6a1](878b6a1))
* figcaption rendering bug in Safari ([#1382](#1382)) ([e274499](e274499))
* improve donate block appearance for WP 5.8 ([#1398](#1398)) ([51fc4f7](51fc4f7))
* prevent white screen and errors on Widget Screen ([#1390](#1390)) ([5731581](5731581))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.39.3 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@laurelfulford laurelfulford laurelfulford approved these changes

Assignees

@dkoo dkoo

Labels
released on @alpha released [Status] Approved The pull request has been reviewed and is ready to merge [Type] Bug Incorrect behavior or functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Image Block: Caption placement in Safari
3 participants
@dkoo @laurelfulford @matticbot