-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Try: Safari 13 fix round 2. #32767
Try: Safari 13 fix round 2. #32767
Conversation
Size Change: +8 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
@WordPress/gutenberg-core Anyone still on Safari 13? Would be cool to get real testing (outside browserstack) |
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 |
4674078
to
5fcc58b
Compare
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. :-) |
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 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm approving cause it works for me but that's one of these things where it's very hard to test properly.
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests well on Safari 13, 14.1, and Chrome 91 with both TwentyTwentyOne (with background) and TwetyNineteen themes (without background). Tested both mobile and desktop widths, with short and long texts.
Good job!
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.js
files for terms that need renaming or removal).