Opened 10 years ago
Closed 10 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)
Change History (12)
#1
@
10 years ago
- Milestone 4.2 deleted
- Resolution set to duplicate
- Status changed from new to closed
#2
@
10 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:
↓ 4
@
10 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
@
10 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.
@
10 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.
10 years ago
#6
@
10 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
@
10 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.
See #29897, and there's a patch too which probably needs a refresh.