Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#38651 closed defect (bug) (fixed)

Customizer edit icons may be partially off-screen in Device Preview mode

Reported by: afercia's profile afercia Owned by: westonruter's profile westonruter
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.7
Component: Customize Keywords: has-screenshots has-patch
Focuses: Cc:

Description

When activating the "Device Preview" mode, the new edit icons may be positioned partially off-screen. I guess this is also theme-dependent and there's no guarantee there will be enough space. Screenshot:

https://cldup.com/yI21Ae4T7l.png

Attachments (15)

38651.diff (838 bytes) - added by sstoqnov 7 years ago.
Fix Customizer edit icons on mobile/tablet "Device preview" mode
38651-tablet.png (2.0 MB) - added by sstoqnov 7 years ago.
38651-mobile.png (711.9 KB) - added by sstoqnov 7 years ago.
38651.desktop.png (27.6 KB) - added by ocean90 7 years ago.
38651.2.diff (1.1 KB) - added by sstoqnov 7 years ago.
tablet-2.png (679.1 KB) - added by sstoqnov 7 years ago.
mobile-2.png (174.9 KB) - added by sstoqnov 7 years ago.
38651-3.jpg (246.7 KB) - added by sstoqnov 7 years ago.
38651.3.diff (3.2 KB) - added by sirbrillig 7 years ago.
Remove left margin guard
38651.4.diff (3.9 KB) - added by MarcosAlexandre 7 years ago.
Add some more styles and other styles @media
38651.5.diff (2.6 KB) - added by sstoqnov 7 years ago.
38651.6.diff (4.9 KB) - added by sirbrillig 7 years ago.
Update patch to remove left guard and add tweaks for twentyfourteen and twentyseventeen
38651.7.diff (3.4 KB) - added by sirbrillig 7 years ago.
Updated patch to clean up left-over things
38651.8.diff (1.7 KB) - added by westonruter 7 years ago.
38651.9.diff (2.4 KB) - added by westonruter 7 years ago.

Change History (44)

@sstoqnov
7 years ago

Fix Customizer edit icons on mobile/tablet "Device preview" mode

@sstoqnov
7 years ago

@sstoqnov
7 years ago

#1 @sstoqnov
7 years ago

  • Keywords has-patch reporter-feedback added

@afercia It seems that the icons have been replaced by SVG icons, which look much better. I have made few small modifications, on mobile mode.

@ocean90
7 years ago

#2 @ocean90
7 years ago

The same can happen in a regular desktop view too, see 38651.desktop.png with the 2014 theme.

@sstoqnov
7 years ago

@sstoqnov
7 years ago

@sstoqnov
7 years ago

#3 @sstoqnov
7 years ago

  • Keywords 2nd-opinion added

@ocean90 I have tested all themes and it seems that only the Twenty Fourteen have different layout. All edit icons are now visible, but they are overlapping the text in Twenty Seventeen for example. At least the entire icons are visible, but I am not sure is this the best solution.

#4 @afercia
7 years ago

Hm, what about swapping the icons on the right under a certain viewport? Wouldn't entirely solve the problem but usually on the right there's more space, and less chances for the icons to hide other elements. On RTL should be the opposite... maybe a bit too complicated :)

@sstoqnov
7 years ago

#5 @sstoqnov
7 years ago

I don't think that this is a solution. Imagine if you have a very long title that takes the entire screen on mobile. The button will be invisible in the right side. Right now it's a little cut, but still visible.
I have tried to style the Twenty Fourteen theme buttons, but now the other looks little weird ( because of padding added to .site-title a, see attached screenshot).

I found that the Twenty Seventeen adds a class to the body twentyseventeen-customizer and I was wondering is there any chance that we can style the buttons by theme name. For example (twentysixteen-customizer, twentyfifhteen-customizer). The problem is that only Twenty Seventeen has such body class.

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


7 years ago

#7 @sirbrillig
7 years ago

I think that this issue could benefit from a more detailed description of what we want it to look like.

Mostly it seems that we want icons to "not be off the page and not overlapping things", but that's tricky because it begs the question: "where do the icons go, then"?

In Slack @helen made this proposed definition, which may be more workable:

Well, I think it’s confusing when it overlaps text that otherwise has enough or at least some amount of unused whitespace next to it that the shortcut could be in.

So maybe we could examine the icon position and the element position in the JavaScript and make some position decisions based on that information. I'll play around and see if anything works.

Please continue brainstorming and experimenting, though. This is a good conversation!

#8 @sirbrillig
7 years ago

Another thing I wanted to bring up: have we considered using smaller buttons for mobile? They wouldn't overlap things quite as dramatically.

#9 @westonruter
7 years ago

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

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


7 years ago

#11 @sirbrillig
7 years ago

I discovered something relevant here which is going into my upcoming patch: the left guard code (.customize-partial-edit-shortcut-left-margin) does not evaluate when the viewport is resized. So when you use the Customizer viewport controls to switch to mobile, the icon positioning remains as it was at desktop width.

@sirbrillig
7 years ago

Remove left margin guard

#12 @sirbrillig
7 years ago

I tried many different ways to reactivate the left margin guard and make it useful on all the Twenty* themes. The results were not great.

Some issues I ran into:

  1. The guard sensitivity is not the same for every theme because we don't really know quite how much space we have.
  2. The committed version of the guard uses left: 0 on the button, but that just causes the icon to overlap the element. Usually this was IMO a worse UX than having the icon partially off-screen (Twenty Fourteen is an exception and should have an override).
  3. It's possible to change the guard to use left: 37px on the span instead, which solves number 2, but can cause more problems because then its positioning depends on no ancestor having a position. Themes like Twenty Fifteen have trouble with this solution.
  4. The guard needs to be recalculated on every resize event (and possibly other DOM reflow events). This brings up the large complexity of global event handling again, which is tricky to manage inside the instance-based Partial architecture. We would need to track each event handler to prevent it from being added more than once and to remove it properly when the partial is removed.
  5. Even with the resize recalculation, some themes like Twenty Thirteen use a transition to reposition elements, so we need to also listen for that event, with all the requisite event handler management mentioned in number 4.

After some testing, it seems to me that the best solution at this time is to just remove the guard entirely since the results seem to be pretty decent in all the themes I tried. Certainly this doesn't quite solve the original issue as stated in this ticket (icons partially off-screen) but I think it's better than any alternative I can find. FWIW I'd love to be wrong about that.

The attached patch removes the guard and tweaks some things as suggested by the other patches on this ticket.

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


7 years ago

@MarcosAlexandre
7 years ago

Add some more styles and other styles @media

#14 @MarcosAlexandre
7 years ago

@sirbrillig I modified and included some styles and in the Twenty Seventeen theme, in my opinion, they were a little better than the 38651.2.diff (I used base). After I sent the patch I saw his (we were working quanse at the same time, rs). Can you test to see how it works for you?

Version 0, edited 7 years ago by MarcosAlexandre (next)

@sstoqnov
7 years ago

#15 @sstoqnov
7 years ago

Well guys, it seems that there is not a perfect solution.
I have reviewed the themes structure and in my opinion there are two ways to fix this issue:

  1. We can change the twentyfourteen theme and add a body class twentyfourteen, so we will have additional selector that we can use to fix issue. I don't like it, because we will add a theme fix in customizer styles. But it's good, because we will add only one row in twentyfourteen theme ($classes[] = 'twentyfourteen' )
  1. The other solution is to fix this at theme styles, which will require changing twentyfourteen and twentyseventeen styles. Attached a diff for this solution.

#16 @sirbrillig
7 years ago

@MarcosAlexandre Thank you for your efforts here! I think it does look better but I'm worried about adding too many element-specific rules in the core CSS (#site-navigation, .footer-widget-1, etc.). (Also I can't apply that patch to master directly so it's difficult to test.)

it seems that there is not a perfect solution.

@sstoqnov Agreed. I like your solution number 2. The patch works well in all the places I tested.

I think we should probably remove the left-margin guard anyway, due to the issues described in my previous comment, so perhaps we can combine both approaches? I will attached one now.

@sirbrillig
7 years ago

Update patch to remove left guard and add tweaks for twentyfourteen and twentyseventeen

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


7 years ago

#18 @sirbrillig
7 years ago

@karmatosed and/or @davidakennedy could you check the changes to twentyfourteen and twentyseventeen in the last patch to see if they are cool with you?

@sirbrillig
7 years ago

Updated patch to clean up left-over things

#19 @sirbrillig
7 years ago

Uploaded a new version of the patch without the theme-specific changes. Here's a very rough draft of a possible commit message (never written one for core before, so please take with a grain of salt):

Customizer: Remove left-margin guard from edit shortcuts and adjust for small widths

Removes the .customize-partial-edit-shortcut-left-margin class which was not effective on some themes, created a worse experience for some themes, and which did not recalculate when the preview was reflowed or resized. Instead some small width media queries were added to handle common cases while more dramatic issues can be handled by the theme.

Also renames Partial.positionEditShortcut()to Partial.addEditShortcutToPlacement() which is a more accurate description of its function.

Props @sirbrillig, @sstoqnov

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


7 years ago

#21 @westonruter
7 years ago

In 39202:

Customize: Remove left-margin guard from edit shortcuts and adjust for small screen sizes.

Removes the .customize-partial-edit-shortcut-left-margin class which was not effective on some themes, created a worse experience for other themes, and which did not recalculate when the preview was reflowed or resized. Now some small-width media queries are added to handle common cases while more dramatic issues can be handled by the theme. Also renames Partial.positionEditShortcut()to Partial.addEditShortcutToPlacement() which is a more accurate description of its function.

Props sirbrillig, sstoqnov.
See #38651, #27403.

#22 @westonruter
7 years ago

  • Keywords reporter-feedback 2nd-opinion removed
  • Owner changed from sirbrillig to karmatosed
  • Status changed from assigned to reviewing

@karmatosed & @davidakennedy, please give indication of whether the bundled theme changes in 38651.6.diff should be committed.

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


7 years ago

#24 @westonruter
7 years ago

  • Owner changed from karmatosed to davidakennedy

#25 @davidakennedy
7 years ago

@westonruter These changes look fine to me. The only small bit of feedback I'd add is to maybe add a CSS comment above the styles just to make it clear where these changes take effect and why the theme is styling something from core like this. It may not be completely clear just by the class names, and its rare themes have to do something like this, so let's provide an explanation.

@westonruter
7 years ago

@westonruter
7 years ago

#26 @westonruter
7 years ago

  • Owner changed from davidakennedy to westonruter
  • Status changed from reviewing to accepted

In 38651.9.diff I realized that the underlying customize-preview.css was adding style properties to the site-title unnecessarily. Namely, the styles should only be added if the edit shortcuts are shown. And we can know this by adding body.customize-partial-edit-shortcuts-shown to the selectors. This should ensure that when the customizer controls are collapsed, the page layout should appear as it will on the frontend, and the adjustments to the layout will only be done when there needs to be room made for the edit shortcuts.

#27 @westonruter
7 years ago

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

In 39233:

Customize: Adjust layout for edit shortcuts only when shown.

Add theme-specific positioning of edit shortcuts in Twenty Fourteen and Twenty Seventeen.

Props sirbrillig, sstoqnov, westonruter.
Fixes #38651.

#28 @westonruter
7 years ago

In 39685:

Customize: Remove extra left padding in core for site title and widgets in preview.

These styles should only be applied by theme stylesheets as needed.

Props sirbrillig.
Reverts parts of [39202] and [39233].
See #38651.
Fixes #39349.

#29 @dd32
7 years ago

In 39695:

Customize: Remove extra left padding in core for site title and widgets in preview.

These styles should only be applied by theme stylesheets as needed.

Props sirbrillig.
Reverts parts of [39202] and [39233].
See #38651.
Merges [39685] to the 4.7 branch.
Fixes #39349.

Note: See TracTickets for help on using tickets.