WordPress.org

Make WordPress Core

Opened 3 weeks ago

Last modified 6 hours ago

#42052 reviewing defect (bug)

Customize Themes: Safari bugs in rendering themes browser header, theme details, and panel contents

Reported by: afercia Owned by:
Milestone: 4.9 Priority: high
Severity: critical Version: trunk
Component: Customize Keywords: has-screenshots needs-patch
Focuses: Cc:

Description

Screenshot:

https://cldup.com/NBZ1yuJ52w.jpg

the header with search (and filters) is not visible in Safari 11 for me.

In my case, the media query screen and (max-height:540px), screen and (max-width:1018px) kicks in when I open the Safari Web Inspector (keeping the inspector panel to a certain height) and switches the header to position relative so, a bit ironically, it gets visible again when trying to debug. But when the header has position fixed, it is not visible.

Suggestion: investigate overflow-y: scroll; set on ul.customize-themes-full-container

See #37661.

Attachments (5)

42052.diff (474 bytes) - added by benoitchantre 2 weeks ago.
42052.2.diff (966 bytes) - added by benoitchantre 2 weeks ago.
LTR + RTL
42052.3.diff (0 bytes) - added by benoitchantre 6 days ago.
42052.4diff (739 bytes) - added by benoitchantre 6 days ago.
42052.4.diff (493 bytes) - added by afercia 3 days ago.

Download all attachments as: .zip

Change History (38)

#1 @westonruter
2 weeks ago

  • Owner set to celloexpressions
  • Status changed from new to assigned

Note some changes to z-index are pending in https://github.com/xwp/wordpress-develop/pull/267

#2 @celloexpressions
2 weeks ago

  • Owner celloexpressions deleted

I don't have access to Safari, so I can't test fixes here and am probably not the best person to work on a fix. @afercia does bumping the z-index on .customize-preview-header.themes-filter-bar help?

#3 @afercia
2 weeks ago

@celloexpressions z-index is the first thing I've tried but that didn't help. Haven't fully figured out why this happens, not exluding a Safari bug.

#4 @afercia
2 weeks ago

One more issue in Safari 11: the theme details modal is stacked under the left sidebar:

https://cldup.com/A6-vFbRNh7.jpg

This ticket was mentioned in Slack in #core by afercia. View the logs.


2 weeks ago

@benoitchantre
2 weeks ago

@benoitchantre
2 weeks ago

LTR + RTL

#6 @benoitchantre
2 weeks ago

Here's the result of the patch:

https://cldup.com/dlqzzjzEZ9.png

The top margin has been set to 0 and the previous value has been added to the top padding.

#7 @benoitchantre
2 weeks ago

  • Keywords has-patch added; needs-patch removed

#8 @celloexpressions
2 weeks ago

42052.diff looks okay to me - I don't see any regressions in Chrome although I can't verify if it fixes the Safari issue. In addition to the basic behavior, be sure to test toggling the feature filters and at window/screen sizes where the filter bar is fixed and where it's not sticky - the margin and padding here are related to those distinct cases.

#9 follow-up: @celloexpressions
2 weeks ago

@benoitchantre note that it isn't necessary to patch the RTL files - those are done automatically as part of the build process. Preference is also to patch all the way back to src/wp-admin/css/customize-controls.css, working off of the develop repository. There's more information in the handbook, or you can ask in Slack for additional clarification on how that works.

#10 in reply to: ↑ 9 @benoitchantre
2 weeks ago

Replying to celloexpressions:

@benoitchantre note that it isn't necessary to patch the RTL files - those are done automatically as part of the build process. Preference is also to patch all the way back to src/wp-admin/css/customize-controls.css, working off of the develop repository. There's more information in the handbook, or you can ask in Slack for additional clarification on how that works.

Thanks for the info, I'm starting to contribute and doesn't know everything yet. I think I need to install Vagrant and everything, because at the moment I have no src directory.

Do I need to update the patch or is it ok?

This ticket was mentioned in Slack in #core by westonruter. View the logs.


2 weeks ago

#12 @westonruter
12 days ago

  • Owner set to afercia
  • Status changed from assigned to reviewing

@benoitchantre you don't need to supply a new patch to remove the RTL changes. Just be aware you can save yourself time and skip modifying the RTL.

You don't need the src directory as grunt:patch can apply it either way.

You can read more at https://make.wordpress.org/core/handbook/tutorials/working-with-patches/

@afercia would you review and commit if you confirm it fixes the issue?

This ticket was mentioned in Slack in #core by melchoyce. View the logs.


9 days ago

#14 follow-ups: @afercia
9 days ago

Tested 42052.diff and in the responsive view the "back" button is not visible in Safari 11

https://cldup.com/kZhakW8-Iw.jpg

As mentioned in the ticket description, removing the overflow-y: scroll; set on ul.customize-themes-full-container seems to fix it. However, I'm not sure if there are side-effects. I think this should be investigated in depth since we're patching it but we don't know exactly _what_ we are patching. In other words, the real reason of this bug hasn't been identified.

#15 @afercia
9 days ago

  • Owner afercia deleted

Sorry I don't have time to follow this ticket as owner. Best option would be a person whos; more familiar with the implementation and the CSS used here.

#16 in reply to: ↑ 14 @benoitchantre
8 days ago

Replying to afercia:

As mentioned in the ticket description, removing the overflow-y: scroll; set on ul.customize-themes-full-container seems to fix it. However, I'm not sure if there are side-effects. I think this should be investigated in depth since we're patching it but we don't know exactly _what_ we are patching. In other words, the real reason of this bug hasn't been identified.

When overflow-y: scroll; is removed, the scroll doesn't work anymore. I would like to investigate more, but the situation is a bit complicated now for me (I'm dad for the second time since Monday and have no time to contribute during office hours). I'll see what I can do in the next few days.

#17 follow-up: @westonruter
8 days ago

  • Owner set to benoitchantre

Congrats! 🎉

@benoitchantre
6 days ago

@benoitchantre
6 days ago

#18 in reply to: ↑ 17 @benoitchantre
5 days ago

Replying to westonruter:

Congrats! 🎉

Thanks Weston!

#19 in reply to: ↑ 14 @benoitchantre
5 days ago

Replying to afercia:

Tested 42052.diff and in the responsive view the "back" button is not visible in Safari 11

This is now fixed in 42052.4diff (one dot is missing in the filename).

I'll try to understand the root cause.

#20 @westonruter
4 days ago

In 41863:

Customize: Fix visibility of theme browser header in Safari.

Props benoitchantre, afercia.
See #42052, #37661.

#21 follow-up: @westonruter
4 days ago

@benoitchantre I committed your fix in [41863] while you're looking into the root cause, but in my testing I found another big problem with Safari and that is when you start typing into the search field, the sections on the left slide out into oblivion: https://youtu.be/6s8qCJy90AY

Would you be able to dig into that as well?

#22 @afercia
4 days ago

when you start typing into the search field, the sections on the left slide out into oblivion: https://youtu.be/6s8qCJy90AY

This happens also in two more scenarios for me:

  • when tabbing from the sidebar to the search field: the sidebar content disappears
  • when opening a theme details modal and then closing it: the sidebar content disappears

https://cldup.com/huYU0wvMF3.jpg

Seems to me .wp-full-overlay-sidebar-content just scrolls horizontally, notice some of the following panels there do take space even if they use visibility: hidden; height: 0; they actually have some padding padding: 12px;. Strangely enough, removing this padding fixes the scrolling but still doesn't identify the root cause.

https://cldup.com/jGZC7cFTUy.jpg

The scrolling seems to happen just in Safari though, I guess because Safari has a different (buggy? stricter?) overflow implementation. Just googling a bit, there are several references to not expected overflow behaviors with Safari. This doesn't explain why the scrolling happens when typing in the search field though. I guess there's something going on with the translateX for panels that are translated to the right together with overflow weirdness in Safari but couldn't identify what triggers the scrolling.

@afercia
3 days ago

#23 @afercia
3 days ago

About the details modal being clipped in Safari:
The only reference I was able to find that could partially explain the Safari behavior is in the W3C CSS basic box model working draft (outdated but still referenced by the CSS Overflow Module Level 3)

https://www.w3.org/TR/css3-box/#overflow

The computed values of ‘overflow-x’ and ‘overflow-y’ are the same as their specified values, except that some combinations with ‘visible’ are not possible: if one is specified as ‘visible’ and the other is ‘scroll’ or ‘auto’, then ‘visible’ is set to ‘auto’.

This probably opened the doors to different implementations across browsers.

Long story short, 42052.4.diff sets back the themes container overflow-y to visible when the details modal is open, making the modal fully visible in Safari:

https://cldup.com/jBAycrYUav.jpg

The only drawback I've noticed, is that the themes list behind the modal will scroll to the top when the modal opens, and scroll back down when the modal closes. Worth noting this already happens in Chrome and Firefox without the patch applied.

#24 @westonruter
3 days ago

  • Keywords needs-patch added; has-patch removed

@afercia I'm still not seeing 42052.4.diff fix the issue. Here's what I see with the patch applied: https://youtu.be/kTn5j_H1Uss

#25 @westonruter
3 days ago

In 41871:

Customize: Prevent theme details modal from being clipped in Safari.

Props afercia.
See #42052, #37661.

#26 @westonruter
3 days ago

  • Priority changed from normal to high
  • Summary changed from Customize Themes: themes browser header not visible in Safari 11 to Customize Themes: Safari bugs in rendering themes browser header, theme details, and panel contents

@afercia Sorry, I see you were specifically fixing the one clipping issue.

when opening a theme details modal and then closing it: the sidebar content disappears

Here is a video depicting this issue: https://youtu.be/ehp9ViukViE

We need to figure out why panel's contents are disappearing in Safari.

#27 in reply to: ↑ 21 @benoitchantre
3 days ago

Replying to westonruter:

@benoitchantre I committed your fix in [41863] while you're looking into the root cause, but in my testing I found another big problem with Safari and that is when you start typing into the search field, the sections on the left slide out into oblivion: https://youtu.be/6s8qCJy90AY

Would you be able to dig into that as well?

My free time is now near to 0. I have looked at that a bit yesterday, but not enough to find a solution. Can someone take the relay?

#28 @melchoyce
2 days ago

  • Owner benoitchantre deleted

#29 @Kopepasah
44 hours ago

After playing around with this for a bit, I believe that the disappearing issue Weston mentioned is a bug in Safari. You can get the element to reappear in the correct location simply by unchecking and rechecking the positioning on the .wp-full-overlay-sidebar-content element.

#30 @Kopepasah
42 hours ago

Tried to debug and see if it is related to the translate property on the parent and child panes, but there was also no change to the disappearing act.

Last edited 42 hours ago by Kopepasah (previous) (diff)

#31 @melchoyce
11 hours ago

  • Severity changed from normal to critical

Yeah, this is still looking really, really broken in Safari: https://cloudup.com/cy6VMq981Jd

Increasing severity.

#32 @westonruter
8 hours ago

In 41919:

Customize: Fix visibility of overlay notifications in Safari.

Props sayedwp, westonruter.
Amends [41390].
See #35210, #42024, #42052.

This ticket was mentioned in Slack in #core by westonruter. View the logs.


6 hours ago

Note: See TracTickets for help on using tickets.