Opened 8 years ago
Closed 8 years ago
#37980 closed defect (bug) (fixed)
Don't attach events to customizer section headings with cannot-expand class
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 4.7 | Priority: | normal |
Severity: | normal | Version: | 4.3 |
Component: | Customize | Keywords: | has-patch commit good-first-bug |
Focuses: | javascript | Cc: |
Description
Back when the customizer used accordion.js
, the .cannot-expand
class could be added to section headings that couldn't be expanded to prevent JS events and hover/focus styles from being applied. While the class continues to prevent styles from being applied (and is used in a couple places for core), JS events seem to be added now and those should be removed. See https://wordpress.slack.com/archives/themereview/p1473274466001022.
The fix should be reasonably straightforward - bailing early from attachEvents()
or onChangeExpanded()
in the section JS in customize-controls.js
if the event target has class cannot-expand - so I'm marking as a good first bug for now if anyone's interested in tackling it.
Attachments (4)
Change History (17)
This ticket was mentioned in Slack in #themereview by celloexpressions. View the logs.
8 years ago
#3
@
8 years ago
- Keywords has-patch commit added; needs-patch removed
Thanks for the patch @kkoppenhaver! This looks good. The only issue I saw is that there needs to be a space between the if
and the (
per the coding standards.
I noticed that the CSS side of cannot-expand
doesn't seem to work anymore, so I added that to the patch and it should be ready for commit now. Here's some code I tested with (in a customize register callback):
class Fifteen_Customize_Example_Section extends WP_Customize_Section { public $type = 'test'; public function render_template() { ?> <li id="accordion-section-{{ data.id }}" class="accordion-section control-section control-section-{{ data.type }} cannot-expand"> <h3 class="accordion-section-title"> {{ data.title }} </h3> <p>Some other text.</p> </li> <?php } } $wp_customize->add_section( new Fifteen_Customize_Example_Section( $wp_customize, 'test', array( 'title' => 'Test' ) ) ); $wp_customize->add_setting( 'test', array() ); $wp_customize->add_control( 'test', array( 'type' => 'text', 'section' => 'test' ) ); $wp_customize->register_section_type( 'Fifteen_Customize_Example_Section' );
#6
@
8 years ago
- Keywords needs-patch added; has-patch commit removed
- Owner changed from westonruter to celloexpressions
@celloexpressions There seems to be a problem in 37980.2.diff with regards to styling elements that are focused. See 37980.2-defect.mov. If the desire is to ensure there is a focus style for accessibility, even though it cannot be clicked, then I think the :hover
needs to be overridden by the :focus
.
#7
@
8 years ago
Cannot-expand headings should not be focusable; if there is a focusable element within them, it would be a distinct link or button element that's separate. Not applying the cannot-expand styling when the element is focusable is a good way to ensure that developers remember to remove the tabindex=0
from section markup that's copied from expandable core sections. On the other hand, it could look broken for users, but if we also unstyle it on focus, the accessible experience is even worse. Anyone else have thoughts here?
#8
@
8 years ago
Should this cover it? Basically, enforcing hover/focus on the various permutations of elements.
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
8 years ago
#10
@
8 years ago
I would actually still prefer not to prevent focus styling from applying, per 7 since this shouldn't be focusable if it cannot expand. This helps to expose the presence of tabindex
in a custom section here as a bug in the theme's implementation. Overriding the styles here on focus means that core doesn't object to themes leaving a tabindex
here, which is clearly wrong in terms of accessibility.
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
8 years ago
#12
@
8 years ago
- Keywords has-patch commit added; needs-patch removed
- Owner changed from celloexpressions to westonruter
@westonruter my preference would be to go with 37980.2.diff for the reasons outlined in 7 and 10, but 37980.3.diff should also work. You can commit whichever makes more sense to you.
First pass at this attached. This is my first ticket, so any feedback is more than welcome.