Try: Safari 13 fix round 2. #32767
Merged
Try: Safari 13 fix round 2. #32767
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jasmussen
added
the
Browser Issues
|
Size Change: +8 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
youknowriad
added
the
Backport to WP 6.7 Beta/RC
|
@WordPress/gutenberg-core Anyone still on Safari 13? Would be cool to get real testing (outside browserstack) |
youknowriad
added
the
[Type] Bug
This was referenced Jun 17, 2021
|
Okay I've got something. The minheight and height properties both fix different issues, but they can't work together, that's why one was removed in the previous fix. In one situation, lots of content works but very little does not, in the other configuration, little content works, but not the other way. TwentyNineteen put me on the right track. It loads an editor style, but does not provide a background color. Compare with TwentyTwenty:
Here's 19:
Adding a white background there fixes it:
But we don't want themes to need to do this. Presumably a ton of themes load editor styles, but forget to color the background since it's white anyway. So I'm going to tweak this PR and try and provide a very low specificity white background color by default, and verify it gets trivially overridden by themes that load their editor styles. |
|
Okay, we now provide a white baseline background color for the editing canvas, just like browsers do for empty HTML pages. I have verified that this color is easily overridden by themes that recolor the background, like TT1 or TT. This should fix the issue in TwentyNineteen. |
|
It's unfortunate that it's not easier to test old Safari versions. From what I can tell, chunks of it are built into the operating system, meaning you can only run Safari 13 on MacOS Catalina. I'll be testing this again on gutenberg.run in browserstack when I can. Through machinations I don't fully understand, the PR has to sit for a bit before gutenberg.run is able to run it, otherwise it treats it as "invalid pull request". Here's the URL for when it's ready: http://gutenberg.run/32767 |
jasmussen
force-pushed
the
try/safari-13-fix-round2
branch
from
4674078
to
5fcc58b
Compare
jasmussen
force-pushed
the
try/safari-13-fix-round2
branch
from
5fcc58b
to
77b26b5
Compare
|
Alright, as I tested the previous patch in Safari 13 in browserstack, everything looked good:
However in one of those sessions I discovered an extra scrollbar when viewing mobile or tablet content. Turns out the height rule we removed in #32581 was meant to solve that, and is incidentally also why the canvas container has inline styles. I've rejiggered the PR, a bit of a rollercoaster — sorry for all the pings, to rely on the new flex properties for height, and the explicit white background color that themes override. It now looks correctly to me:
I will test this on browserstack and verify it still fixes the Safari issues as intended. |
|
Okay, I think nth time is the charm. With both little content and a lot of content, in both themes with and without backgrounds, with the preview dialog not adding extra scroll, I think this one is good:
CC: @simison, you can test in http://gutenberg.run/32767 |
|
Thanks for working on it! Impressive sleuthing on difficult bug! I can't test today anymore but happy to help tomorrow. Hope someone from core team can also have a look. :-) |
|
Here's what I see when trying to create a new post on safari 14 |
|
Ack. For a variety of reasons I can't work more on this today. People are free to push to this if they can, otherwise I'll look at it tomorrow again. |
|
I had a chance to look at this after all. And I can't reproduce. This is what I see on that branch in Safari 14.1.1:
This is on TT1 blocks. I'll try to upgrade to latest nightly, but let me know if there were other steps. |
|
I can try to test again, maybe some cache thing? |
|
I can't reproduce anymore either 🤷 I think I was on a different commit from the branch, not the last one. |
youknowriad
approved these changes
Jun 17, 2021
|
@simison checks are green. Are you able to check a bit, perhaps safari 14.0 as well? |
|
While I'm feeling good about this one, I'd really appreciate just a quick test of a few themes using http://gutenberg.run/32767 for example. CC: @simison @fullofcaffeine. I will also need to be AFK a bit today, so in case those checks go well (as I expect them to), others should feel free to press the green button. |
simison
approved these changes
Jun 21, 2021
youknowriad
removed
the
Backport to WP 6.7 Beta/RC
youknowriad
pushed a commit
that referenced
this pull request
Jun 21, 2021
youknowriad
pushed a commit
that referenced
this pull request
Jun 21, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
In #32581 I pushed a fix targetting an older flexbox interpretation by Safari 13. In my testing, that worked. But a followup suggests it wasn't enough, or that a new issue emerged.
This PR fixes that. I tested using Browserstack and reproduced by adding more content than was visible in the viewport:
In this GIF I demonstrate what adding
height: 100%;back does to fix it. I'm not entirely sure why it worked before without the height, but it appears to be neded now.To test
Ideally test in browserstack, or a Safari 13 instance. Test an old theme, like TwentyNineteen. Test with no content on the page, test with a lot of content in the page, and verify you don't see any dark gray backgrounds.
Also test in modern browsers and verify no regressions there either.
Checklist:
*.native.jsfiles for terms that need renaming or removal).