Make WordPress Core

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: celloexpressions's profile celloexpressions Owned by: westonruter's profile westonruter
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)

cannot-expand-no-events.37980.diff (592 bytes) - added by kkoppenhaver 8 years ago.
37980.2.diff (1.4 KB) - added by celloexpressions 8 years ago.
Restore cannot-expand CSS.
37980.2-defect.mov (622.7 KB) - added by westonruter 8 years ago.
37980.3.diff (1.6 KB) - added by kkoppenhaver 8 years ago.

Download all attachments as: .zip

Change History (17)

This ticket was mentioned in Slack in #themereview by celloexpressions. View the logs.


8 years ago

#2 @kkoppenhaver
8 years ago

First pass at this attached. This is my first ticket, so any feedback is more than welcome.

@celloexpressions
8 years ago

Restore cannot-expand CSS.

#3 @celloexpressions
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' );

#4 @kkoppenhaver
8 years ago

Sounds good, thanks for the feedback!

#5 @celloexpressions
8 years ago

  • Owner set to westonruter
  • Status changed from new to reviewing

#6 @westonruter
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 @celloexpressions
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 @kkoppenhaver
8 years ago

Should this cover it? Basically, enforcing hover/focus on the various permutations of elements.

@kkoppenhaver
8 years ago

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


8 years ago

#10 @celloexpressions
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 @celloexpressions
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.

#13 @westonruter
8 years ago

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

In 38900:

Customize: Skip attaching expansion event handlers to section containers with the cannot-expand class.

Props kkoppenhaver, celloexpressions.
Fixes #37980.

Note: See TracTickets for help on using tickets.