WordPress.org

Make WordPress Core

Opened 6 weeks ago

Closed 4 weeks ago

#38532 closed defect (bug) (fixed)

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

Reported by: sirbrillig Owned by: sirbrillig
Milestone: 4.7 Priority: normal
Severity: normal Version: trunk
Component: Customize Keywords: needs-testing needs-patch
Focuses: Cc:

Description

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.

Attachments (8)

firefox-2017.mp4 (3.4 MB) - added by Presskopp 5 weeks ago.
38532.diff (4.5 KB) - added by sirbrillig 5 weeks ago.
Most recent fix for Firefox
38532.2.diff (4.7 KB) - added by sirbrillig 5 weeks ago.
Updated patch to shorten selectors
Screenshot 2016-11-03 21.31.47.png (210.7 KB) - added by delawski 5 weeks ago.
38532.3.diff (6.9 KB) - added by sirbrillig 5 weeks ago.
Updated patch to include better icon positioning and width/height
38532.4.diff (6.6 KB) - added by sirbrillig 5 weeks ago.
Update patch to fix recent conflicts
38532.5.diff (6.7 KB) - added by sirbrillig 5 weeks ago.
Updated patch to prevent svg size fluctuations in Firefox
38532.6.diff (7.5 KB) - added by sirbrillig 5 weeks ago.
Updated patch to fix left-margin guard

Change History (38)

#1 @sirbrillig
6 weeks ago

Ref: edit shortcuts were added in #27403

#2 @helen
6 weeks ago

  • Milestone changed from Awaiting Review to 4.7

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.

#4 @westonruter
6 weeks ago

  • Keywords needs-testing added
  • Owner set to westonruter
  • Status changed from new to accepted

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

#5 @westonruter
6 weeks ago

  • Keywords needs-patch added
  • Owner changed from westonruter to sirbrillig
  • Status changed from accepted to assigned

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+.

#6 @celloexpressions
5 weeks ago

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.

#7 @westonruter
5 weeks ago

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.

#8 @Presskopp
5 weeks ago

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.

Last edited 5 weeks ago by Presskopp (previous) (diff)

This ticket was mentioned in Slack in #core-themes by presskopp. View the logs.


5 weeks ago

#10 @sirbrillig
5 weeks ago

@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
5 weeks ago

Most recent fix for Firefox

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


5 weeks ago

#12 @celloexpressions
5 weeks ago

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.

#13 @sirbrillig
5 weeks ago

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
5 weeks ago

Updated patch to shorten selectors

#14 @delawski
5 weeks ago

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.

#15 @celloexpressions
5 weeks ago

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.

#16 follow-up: @delawski
5 weeks ago

@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.

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


5 weeks ago

#18 in reply to: ↑ 16 @delawski
5 weeks ago

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.

#19 @sirbrillig
5 weeks ago

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

Last edited 5 weeks ago by sirbrillig (previous) (diff)

#20 @delawski
5 weeks ago

@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).

#21 @sirbrillig
5 weeks ago

@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.

#22 @delawski
5 weeks ago

@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
5 weeks ago

Updated patch to include better icon positioning and width/height

@sirbrillig
5 weeks ago

Update patch to fix recent conflicts

#23 @sirbrillig
5 weeks ago

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

@sirbrillig
5 weeks ago

Updated patch to prevent svg size fluctuations in Firefox

@sirbrillig
5 weeks ago

Updated patch to fix left-margin guard

#24 @sirbrillig
5 weeks ago

That last update should fix #38651 I think.

#25 @afercia
5 weeks ago

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.

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


5 weeks ago

#27 @westonruter
5 weeks ago

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.

#28 @westonruter
5 weeks ago

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

#29 @afercia
4 weeks ago

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)

https://cldup.com/PDpHWb6fo5.jpg

#30 @helen
4 weeks ago

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

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

Note: See TracTickets for help on using tickets.