Set explicit z-index on interface body to ensure it’s pinned under interface header #32732
Conversation
getdave
added
[Type] Bug
|
Size Change: +34 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
jasmussen
added
the
Browser Issues
|
Thanks for thinking about these issues. We've had a few Safari related issues lately, so I'd appreciate if @gwwar could also take this for a quick spin or just be aware of it. But from I can tell, this change is fine. It's a global change that affects all editors, so I focused mostly to see on whether there were any regressions in post or site editors, there don't appear to be. Here's Chrome and Safari:
In general I tested:
As noted, I don't see any issues with this, so I think we can 👍 👍 this one after Kerry looks. Thanks Dave! |
|
I'll try to 👀 today. If other folks would like to help I think we'll want to verify behavior on a native iOS device, a native android device and our normal browser desktop variants. For context, I noticed that there were a number of browser issues related to z-index and --webkit-overflow-scrolling, for example https://bugs.chromium.org/p/chromium/issues/detail?id=128325 or in webkit: https://bugs.webkit.org/buglist.cgi?quicksearch=touch%20z-index |
I wanted to suggest that if you are on a Mac you can install Xcode for free from the app store and use the built-in simulator, which should be as good as a physica device:
The benefit is that it's networked with your Mac so you can just type in
The downside is that Xcode takes half a day to download and install. But it's worth it. I assume you all know this already, but figured I'd document it for any others viewing the PR. |
|
Visiting http://gutenberg.run/32732 should do the trick too, if folks are using a physical phone. |
|
Just to say both videos in my PR desc are using the iOS simulator technique. |
|
On a physical pixel 4: This might be an unrelated bug, but I can easily get a paragraph stuck, where an enter keypress doesn't create a new paragraph. An easy way to do this is to select a paragraph after a scroll then try typing enter screen-20210616-133240.mp4 |
|
likely another unrelated one: On ios simulator, adding an image to the widgets screen makes the control width incorrect: widget-image.mp4 |
noisysocks
added
the
Backport to WP 6.7 Beta/RC
getdave
force-pushed
the
fix/layout-skeleton-background-bleed-ios-safari
branch
2 times, most recently
from
27c7e59 to
b9f83af
Compare
|
I have a real problem with the e2e tests here. Basically the e2e helper that clicks the publish button and waits for the snackbar to popup and say "Published" is failing. The reason for this (AFAIK) is that the "Publish" action simply isn't being triggered. I did think it was the snackbar not appearing due to the new If you try and mimic manually you will see that clicking I don't know what is going on here. |
|
Have you tried interactive tests? I can't recall the puppetteer attributes, but you can run them interactively so they show a small chromium window running each test. That might visualize the problem. |
|
Yup, 💯 for interactive tests as @jasmussen suggested. To run this locally, make sure wp-env is started (visit localhost:8889 to make sure things look okay and gutenberg is activated). Then type: If the suite is long it can help to add a temporary |
|
Thanks for the advice both. In fact that is exactly what I've been doing and indeed it was what first allowed me to narrow down the issue. I've now pushed up a "fix" which gets (or at least should get!) the tests to pass. The question is though why do we need this fix just because I added a Prior to the fix here was the flow for the publish button:
In this PR prior to my fix to
With my fix in place which has an additional check to see whether the panel is already open we give the logic a chance of allowing the button to be a true "Publish" button. What I don't understand is why it's necessary. I can't see this happen in |
|
My initial thought is that some event propagation has changed (for example a component that should be listening to the click doesn't get it). I'll try to help 🔍 later today |
|
I have a suspicion that we're expecting |
getdave
force-pushed
the
fix/layout-skeleton-background-bleed-ios-safari
branch
from
1d9e9db to
47315e4
Compare
|
We shouldn't need to add the second fix. My guess is similar to @gwwar 's that the |
|
Stepping through, the difference is that when the publish panel is open and we click the button again,
@getdave I'd recommend looking at the core/editor store dispatches next. Let me know if you're not able to track it down. For debugging, what works for me is observing the flow of state updates for the related reducer in trunk, and seeing what dispatched the event. After doing so I'd compare with the branch and see if any dispatches are missing or incorrect. |
If this is the case then it makes no sense. The I think perhaps we have a race condition. Diving deeper... |
…e interface header
getdave
force-pushed
the
fix/layout-skeleton-background-bleed-ios-safari
branch
from
47315e4 to
48fe0e6
Compare
|
I have the answer...incoming explanation 🥳 |
Direct ComparisonEssentially the issue is that we're not comparing apples with apples. Or to put it another way we aren't looking at the same Publish button when we're comparing What's happening is the Screen.Capture.on.2021-06-21.at.13-41-40.mp4With z-indexScreen.Capture.on.2021-06-21.at.13-41-04.mp4Without z-indexScreen.Capture.on.2021-06-21.at.13-40-08.mp4 |
Adding z-index to body causes Publish button in actions sidebar to appear underneath the layout header causing the “wrong” publish button to be shown. Applying z-index to content avoids this whilst achieving the same outcome in terms of avoiding the bleed of backgrounfd on mobile safari when scrolling the Widgets editor.
getdave
force-pushed
the
fix/layout-skeleton-background-bleed-ios-safari
branch
from
48fe0e6 to
c4908da
Compare
|
@getdave which fix are you referring to, did you push to the branch? |
| // On Safari iOS on smaller viewports lack of a z-index causes the background | ||
| // to "bleed" through the header. | ||
| // See https://github.com/WordPress/gutenberg/issues/32631 | ||
| z-index: z-index(".interface-interface-skeleton__body"); |
There was a problem hiding this comment.
Oh, gotcha the diff was hiding that this is being applied to .interface-interface-skeleton__content.
There was a problem hiding this comment.
Yeh I'm sorry. It was unclear because I forgot about the naming of this CSS var .interface-interface-skeleton__body. It should be .interface-interface-skeleton__content. Looks like @noisysocks sorted it after he merged early.
There was a problem hiding this comment.
I retested this in ios simulator, and verified that this fixes the background behavior. I also verified in Chrome/Safari that we can create a new post and publish.
Much appreciated if we can update the z-index name before 🚢 but no need for a re-review from me when that happens 👍
noisysocks
removed
the
Backport to WP 6.7 Beta/RC
…terface header (#32732) * Set explicit z-index on interface body to ensure it’s pinned under the interface header * Add z-index to skelton content to avoid publish button bug Adding z-index to body causes Publish button in actions sidebar to appear underneath the layout header causing the “wrong” publish button to be shown. Applying z-index to content avoids this whilst achieving the same outcome in terms of avoiding the bleed of backgrounfd on mobile safari when scrolling the Widgets editor.
…terface header (#32732) * Set explicit z-index on interface body to ensure it’s pinned under the interface header * Add z-index to skelton content to avoid publish button bug Adding z-index to body causes Publish button in actions sidebar to appear underneath the layout header causing the “wrong” publish button to be shown. Applying z-index to content avoids this whilst achieving the same outcome in terms of avoiding the bleed of backgrounfd on mobile safari when scrolling the Widgets editor.
Description
On smaller viewports on Safari iOS the interface header can (under limited circumstances) suffer a render glitch whereby the content below "bleeds" into it.
See screenshots for Before/After.
This PR fixes this by setting an explicit
z-indexfor the interface skeleton"body""content" and ensuring this is lower than that of the interface skeleton "header".Update: we had to revert applying to the "body" because that caused the "actions" panel to sit under the header when publishing a post. Applying to "content" achieves the same fix but without the nasty knock on effects.
Fixes #32631
How has this been tested?
Either on a real iOS device on the iOS simular spin up Safari and load WP with this branch built and enabled.
Before - verify the bug
After - verify the fix
Also verify this change does not break other screens such as Post Editor or Site Editor.
Screenshots
Before
Screen.Capture.on.2021-06-16.at.11-58-38.mp4
After
Screen.Capture.on.2021-06-16.at.12-20-45.mp4
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).