WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 5 years ago

#31203 closed defect (bug) (fixed)

Focus style for add-new-theme should match hover style

Reported by: celloexpressions Owned by: ocean90
Milestone: 4.2 Priority: normal
Severity: normal Version: 3.8
Component: Themes Keywords: has-patch dev-feedback
Focuses: accessibility Cc:

Description

This also fixes the issue for the Customizer Theme Switcher plugin. See https://make.wordpress.org/accessibility/2015/01/31/theme-switcher-accessibility-test-result/.

Attachments (3)

31203.diff (1.6 KB) - added by celloexpressions 6 years ago.
31203.2.diff (1.6 KB) - added by mrahmadawais 6 years ago.
Fallback Padding-bottom:10%; added since calc is only supported 82% and has these issues. Safari 7.0 and older and Chrome 26 and older don't support viewport units in calc() expressions (fixed since then). calc() doesn't work inside a transform in IE IE9 appears to ignore calc() expressions when display:table is used. Safari 6 has a bug where an element with a width defined using calc() has its width reset when changing the height using JS (fixed in Safari 7). IE10 crashes when a div with a property using calc() has a child with [same property with inherit](http://stackoverflow.com/questions/19423384/css-less-calc-method-is-crashing-my-ie10). IE 9 - 11 don't render box-shadow when calc() is used for any of the values. IE11 and other browsers to a lesser extent have trouble supporting calc() inside color or transform values.
31203.patch (1.4 KB) - added by ocean90 5 years ago.

Download all attachments as: .zip

Change History (12)

#1 @iseulde
6 years ago

  • Milestone 4.2 deleted
  • Resolution set to duplicate
  • Status changed from new to closed

See #29897, and there's a patch too which probably needs a refresh.

#2 @celloexpressions
6 years ago

  • Milestone set to 4.2
  • Resolution duplicate deleted
  • Status changed from closed to reopened

This has a patch that doesn't need a refresh and fixes one specific issue... so let's not hold up a fix here for a much broader ticket that will take much more time to polish and move forward. Especially since this also fixes the issue for the Customizer Theme Switcher feature-plugin that is being polished for merge proposal. This should be a pretty quick fix here, whereas we're likely looking at at least a few more weeks if this were to be fixed as part of a much broader ticket.

#3 follow-up: @iseulde
6 years ago

Do you really need the calc stuff there? I just remember making a patch for this, so might be worth looking at that other ticket. Not every single problem in that other ticket needs to be solved to commit a patch.

#4 in reply to: ↑ 3 @celloexpressions
6 years ago

Replying to iseulde:

Do you really need the calc stuff there? I just remember making a patch for this, so might be worth looking at that other ticket. Not every single problem in that other ticket needs to be solved to commit a patch.

I see nothing wrong with calc, and it will still be functional, just a bit shorter on IE8, but that's just there to preserve the spacing because all of the styling was based on the div having hover/focus instead of the a. The other option would be to restructure the html or re-do the CSS further, but I'm much in favor of a simpler fix.

Sidenote: I generally prefer more targeted tickets like this because that also avoids issues where the fact that similar fixes may exist as part of patches elsewhere isn't searchable or easy to remember seeing.

@mrahmadawais
6 years ago

Fallback Padding-bottom:10%; added since calc is only supported 82% and has these issues. Safari 7.0 and older and Chrome 26 and older don't support viewport units in calc() expressions (fixed since then). calc() doesn't work inside a transform in IE IE9 appears to ignore calc() expressions when display:table is used. Safari 6 has a bug where an element with a width defined using calc() has its width reset when changing the height using JS (fixed in Safari 7). IE10 crashes when a div with a property using calc() has a child with [same property with inherit](http://stackoverflow.com/questions/19423384/css-less-calc-method-is-crashing-my-ie10). IE 9 - 11 don't render box-shadow when calc() is used for any of the values. IE11 and other browsers to a lesser extent have trouble supporting calc() inside color or transform values.

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


6 years ago

#6 @DrewAPicture
6 years ago

  • Keywords dev-feedback added

Two things: I think some screenshots here would be helpful for illustrating the problem. A consult from @afercia would also be beneficial here.

#7 @afercia
6 years ago

Checked last patch, doesn't apply anymore and needs a refresh after the admin color changes in r31422.
The focus style on "Add New Theme" works nicely, see screenshot below, removing the box-shadow also make sense, nice touch :)
By the way, it's inside a <div> appended as last item in a <ul>. That's invalid code of course but I'm also concerned about semantics: entering the Theme customizer, screen reader users will hear, for example:

"Theme 16"

(that's the info in the heading). But then the following list has 17 items, screen readers will read out:

list with 17 items

maybe a bit confusing, also if the purpose of that list is to... list :) the themes, then should contain just themes. So the "Add New Theme" div should preferably be outside the list and at that point maybe can be just a link, not sure a <div> is really needed.

https://cldup.com/Z-XO3Lyc2B.png

@ocean90
5 years ago

#8 @ocean90
5 years ago

In 31950:

Customizer Theme Switcher: Fix invalid HTML markup when New Theme control is added.

see #31203.

#9 @ocean90
5 years ago

  • Owner set to ocean90
  • Resolution set to fixed
  • Status changed from reopened to closed

In 31952:

Themes: Improve focus styling of add-new-theme link.

props celloexpressions, mrahmadawais.
fixes #31203.

Note: See TracTickets for help on using tickets.