Make WordPress Core

Opened 7 years ago

Closed 7 years ago

#42052 closed defect (bug) (fixed)

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

Reported by: afercia's profile afercia Owned by: westonruter's profile westonruter
Milestone: 4.9 Priority: high
Severity: critical Version: 4.9
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 (7)

42052.diff (474 bytes) - added by benoitchantre 7 years ago.
42052.2.diff (966 bytes) - added by benoitchantre 7 years ago.
LTR + RTL
42052.3.diff (0 bytes) - added by benoitchantre 7 years ago.
42052.4diff (739 bytes) - added by benoitchantre 7 years ago.
42052.4.diff (493 bytes) - added by afercia 7 years ago.
42052.5diff (489 bytes) - added by sayedwp 7 years ago.
42052.6.diff (1.4 KB) - added by sayedwp 7 years ago.

Download all attachments as: .zip

Change History (47)

#1 @westonruter
7 years 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
7 years 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
7 years 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
7 years 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.


7 years ago

@benoitchantre
7 years ago

@benoitchantre
7 years ago

LTR + RTL

#6 @benoitchantre
7 years 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
7 years ago

  • Keywords has-patch added; needs-patch removed

#8 @celloexpressions
7 years 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
7 years 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
7 years 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.


7 years ago

#12 @westonruter
7 years 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.


7 years ago

#14 follow-ups: @afercia
7 years 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
7 years 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
7 years 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
7 years ago

  • Owner set to benoitchantre

Congrats! 🎉

@benoitchantre
7 years ago

#18 in reply to: ↑ 17 @benoitchantre
7 years ago

Replying to westonruter:

Congrats! 🎉

Thanks Weston!

#19 in reply to: ↑ 14 @benoitchantre
7 years 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
7 years ago

In 41863:

Customize: Fix visibility of theme browser header in Safari.

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

#21 follow-up: @westonruter
7 years 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
7 years 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
7 years ago

#23 @afercia
7 years 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
7 years 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
7 years ago

In 41871:

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

Props afercia.
See #42052, #37661.

#26 @westonruter
7 years 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
7 years 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
7 years ago

  • Owner benoitchantre deleted

#29 @Kopepasah
7 years 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
7 years 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 7 years ago by Kopepasah (previous) (diff)

#31 @melchoyce
7 years 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
7 years 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.


7 years ago

#34 @sayedwp
7 years ago

This seems to be a bug, but in general and from what I have learned from my research is that some browsers break things when we give position:fixed to an element and its parent has transform property set.
I didn't find any solution for it anywhere, but please see my patch to see what I have come up with.
This fixes the problem of section disappearing in safari. I have yet to work on the other issues @melchoyce mentioned.

@sayedwp
7 years ago

#35 @sayedwp
7 years ago

Also need to fix, section disappearing when typing in search input.

#36 @sayedwp
7 years ago

And this is the CSS way, which should fix the problem of section disappearing when searching themes or closing modal.

@sayedwp
7 years ago

This ticket was mentioned in Slack in #core-customize by sayedwp. View the logs.


7 years ago

#38 @melchoyce
7 years ago

  • Owner set to westonruter

Looks good to me, but could use a second review just in case 👍

This ticket was mentioned in Slack in #core-customize by sayedwp. View the logs.


7 years ago

#40 @westonruter
7 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 41948:

Customize: Fix rendering issues in theme browsing when opening theme details or performing search in Safari.

Props sayedwp.
See #37661.
Fixes #42052.

Note: See TracTickets for help on using tickets.