Customizer: Edit shortcuts buttons: clicking does not work in Firefox

[sirbrillig]

Steps to reproduce:

1. Open the customizer in Firefox previewing Twentyfifteen
2. Click anywhere to cause the edit shortcut icon buttons to appear
3. Click an edit shortcut icon

Expected: the sidebar would switch to the control for that element.
Actual: nothing happens.

The cause appears to be that button elements cannot have descendant click targets in Firefox.

[sirbrillig]

Ref: edit shortcuts were added in #27403

[helen]

Really should get this fixed before shipping beta 1, we can fix bugs in beta but I don't know about having this big of a known gap in testing coverage.

[sirbrillig]

Fix PR: ​https://github.com/xwp/wordpress-develop/pull/185

[westonruter]

Once we've confirmed this Firefox fix also works in Chrome, Safari, and IE9+ I'll commit.

[westonruter]

Styles need to be updated for edit shortcuts: ​https://wordpress.slack.com/archives/core-customize/p1477597142004102

Also, need double confirmation that fixes work in all core themes for Firefox, Chrome, Safari, and IE9+.

[celloexpressions]

Update: we're working in ​https://github.com/xwp/wordpress-develop/pull/187 to resolve this. As part of the processs, we're switching to containing the shortcuts in a <span>, using a <button> for the actual button, and switching to an SVG version of the icon.

We need to work through the styling changes, and most notably, ensuring that themes don't accidentally override styles here. Will likely to add a bunch of !importants.

[westonruter]

In 39018:

Customize: Prevent toggling edit shortcuts when doing shift-click or when clicking on a descendent of an interactive element.

See #38532, #27403.
Fixes #38554.

[Presskopp]

This is not working properly, I can't really explain in words, attached a video.

I'd also like to have the mouse pointer changed when pressing shift + clicking on 'edit', let's say to cursor: cell for example.

See also ​https://wordpress.slack.com/archives/core-themes/p1477968054004513 please.

[sirbrillig]

@Presskopp thank you for the report! I agree with Helen in the Slack conversation: I think the behavior looks correct except for the icon click area being very tiny in Firefox. If you (or anyone else ) would like to try out the patch, I'm about to attach the most recent one here.

[sirbrillig]

Most recent fix for Firefox

[celloexpressions]

If possible it would be good for themes to be able to use .customize-partial-edit-shortcut for their styling adjustments, as that's already a very long class name. We'll need to update Twenty Seventeen's usage of that in the patch here if we do change it. .customize-partial-edit-shortcut button feels okay and equivalent to .customize-partial-edit-shortcut:before, but from a themer's perspective needing to remember another very long class name for the inner button should be avoided.

Before commit, we also need to re-test with all eight bundled themes as well as a sampling of themes on .org to consider adding !importants to the core styles accordingly, based on the increased likelihood of inheriting theme styles on button. I'd like to see it work out of the box with a few changes as possible required by bundled themes, although the Twenty Seventeen ones will need to stay. Twenty Seventeen also has instances of this in inc/color-patterns.php that'll need updating, reference the initial commit on #27403.

[sirbrillig]

Thanks, @celloexpressions!

Updated the patch to switch to .customize-partial-edit-shortcut button.

Before commit, we also need to re-test with all eight bundled themes as well as a sampling of themes on .org to consider adding !importants to the core styles accordingly, based on the increased likelihood of inheriting theme styles on button. I'd like to see it work out of the box with a few changes as possible required by bundled themes, although the Twenty Seventeen ones will need to stay. Twenty Seventeen also has instances of this in inc/color-patterns.php that'll need updating, reference the initial commit on #27403.

This is a good point! For reference, here's the part in twenty fourteen that needs to be updated: https://core.trac.wordpress.org/changeset/38967/trunk/src/wp-content/themes/twentyfourteen/style.css

[sirbrillig]

Updated patch to shorten selectors

[delawski]

I've reviewed the PR and added a comment regarding min-width/min-height being used instead of width/height:

​https://github.com/xwp/wordpress-develop/pull/187/files/c43f1bb347a7b5fb74b2b0e4a28143a065d51fad#r86342988

Also, I've tested the patch with all 'Twenty' themes. I can confirm that the icons are clickable and are opening correct panel/section on Firefox now.

[celloexpressions]

I'd agree on using height/width, and we may even want some !importants happening. The questions for testing themes are whether the icons are positioned correctly and whether they inherit any styling from themes, including on hover/focus states. We need to check themes on small screens as well - icons should be at least partially visible on screens there, and every partial's icons should be visible. Twenty Sixteen still has them hidden on the site title, tagline, and custom logo I believe.

[delawski]

@celloexpressions - Edit icons doesn't show up in Twenty Sixteen (and other themes) for site title, tagline etc. because the self.Partial instance is not created for those elements inside wp.customize.selectiveRefresh singleton (and thus no span markup is created and appended to those elements). I haven't found out why is that happening so far, but I'll try to investigate it more. Anyway, CSS should not be the primary issue at this point.

[delawski]

Replying to delawski:

@celloexpressions - Edit icons doesn't show up in Twenty Sixteen (and other themes) for site title, tagline etc. because the self.Partial instance is not created for those elements inside wp.customize.selectiveRefresh singleton (and thus no span markup is created and appended to those elements). I haven't found out why is that happening so far, but I'll try to investigate it more. Anyway, CSS should not be the primary issue at this point.

Scratch that! I've been using old version of the theme. Now I can see the CSS issue and will have a look into it.

[sirbrillig]

IIRC the Twenty Sixteen issue is overflow: hidden on .site-branding, which was discussed... somewhere.

Update: here it is in the original PR: ​https://github.com/xwp/wordpress-develop/pull/179#issuecomment-256145685

[delawski]

@sirbrillig Have you tried using position: absolute instead of position: relative on the span element? I've noticed you tried position: fixed, but as you said, icons don't scroll then.

On my end position: absolute looks promising. Icons are shown even though the parent container has overflow: hidden applied (see screenshot in the previous message).

[sirbrillig]

@delawski wow, that seems to work really well. I think when trying to fix that previously the way the buttons was implemented prevented such a fix. The way this patch switches up the elements seems to have made it possible. Thanks! Pushed the change to the PR.

[delawski]

@sirbrillig That's great! Also, in regards to the width/height issue: it seems that the best solution is to use both width/height and min-width/min-height at the same time. This way icons are shown and are scaled uniformly.

[sirbrillig]

Updated patch to include better icon positioning and width/height

[sirbrillig]

Update patch to fix recent conflicts

[sirbrillig]

@delawski What do you think of the most recent patch?

[sirbrillig]

Updated patch to prevent svg size fluctuations in Firefox

[sirbrillig]

Updated patch to fix left-margin guard

[sirbrillig]

That last update should fix #38651 I think.

[afercia]

I'd consider to add a comment in the CSS to explain why there are all that declarations for the buttons and to clarify the need to avoid inheritance from themes.

Also noticed with @sirbrillig that some themes (e.g. Twenty Thirteen) use a max-width on the buttons so that property should be reset too. Actually, themes could use any property in their stylesheet so wondering if the edit buttons would need some sort of (reasonably) complete CSS reset.

[westonruter]

In 39136:

Customize: Use button with svg as click target instead of :before content, improving clickability of edit shortcuts in Firefox.

Props sirbrillig, celloexpressions, delawski.
See #38532.

[westonruter]

@sirbrillig I've committed your latest patch to be included in beta2 so you can follow up with refinements in beta3.

[afercia]

Testing in IE11, the svg positioning is a bit off. I'm not so sure the edit buttons should use flexbox at all, going to open a separate ticket. (red background in the screenshot is for testing purposes)

[helen]

Going to close this as fixed; new tickets for individual issues.