Customizer: unnecessary events bound on some section titles

[afercia]

Noticed because screen readers announce them as "clickable" elements when arrowing around them. Looks like some section titles have unnecessary events attached, see screenshot:

the jQuery selector used here should be a bit more specific, and this can be done in several ways, I'd leave this decision to the Customizer team since they know the Customizer events intricacies far better than me. Maybe targeting just the ones that have a tabindex="0" attribute?

[afercia]

First pass for some events sanity. Discussed a bit on Slack, looks like the accordion-section-title is currently used (both in accordion.js and `customize-controls.js) to attach events that are unnecessary on some divs and break keyboard interaction when pressing Enter on buttons inside these divs.

• .accordion-section-title should be used just for accordions
• divs that aren't accordions should use a different class just for styling, here I'm using .was-accordion-section-title for clarity, of course the class name is temporary and should be changed in something more appropriate
• the .clear-results control is now a button and works with mouse, Enter and Spacebar as it's supposed to do natively
• same for the .customizer-screen-options-toggle
• some buttons missed the type="button" attribute, added

Would definitely need some help and review. Tested on latest Chrome, Firefox and IE 8.

[afercia]

JSHinted

[afercia]

Removed two unused event.

[helen]

Moving into 4.3 because it's broken behavior, and affects newly introduced features.

[celloexpressions]

We cannot remove/change any of the accordion terminology from the html classes in the Customizer, including .accordion-section-title. That is a potentially severe breaking change, so we've been living with the technical debt since 4.0 there (when we first decided to move away from accordions and panels were introduced). If we wanted to change it we should clean it all up very early in a release and just break everything at once, making extensive efforts to inform developers of the changes.

I don't have objections to the other changes here at a glance.

[obenland]

How can we fox the behavior without breaking backwards compatibility?

[afercia]

Not sure where it's actually breaking backwards compatibility, unless I'm missing something. @celloexpressions pointed out a "potentially severe breaking change" if changing the CSS classes which are responsible to attach events for the accordions. But that's not the case, the patch doesn't change the classes on accordions. It changes them on divs that are not accordions and currently have unnecessary events attached.

[obenland]

Replying to afercia:

the patch doesn't change the classes on accordions. It changes them on divs that are not accordions and currently have unnecessary events attached.

Changing classes can break visual backwards compatibility if plugins expect them to be there. If this is not something introduced in 4.3, we can't really remove them.

[afercia]

Hm... there are probably some workarounds, the first one that comes into my mind is adding a class .not-an-accordion and then filter out elements with that class in accordion.js. Not so clean I know, open to any suggestion to filter out elements that really don't need those events attached.

[afercia]

Thinking a bit about this, the Menu Customizer is new in 4.3 so we don't have to worry about visual backwards compatibility, correct? Please let me know if I'm missing something. If so, for the Menu Customizer we can change classes on divs that are not accordions, this would remove unnecessary events fixing a couple of issues like:

the clear search results button that doesn't work when pressing Enter, currently the unnecessary events here are attached by accordion.js:

the Menu options toggle that doesn't work when pressing Enter:

About the other cases, we can be a bit more specific when targeting divs that expand panels or sections and as far as I see they all have a tabindex="0" attribute so using .accordion-section-title[tabindex="0"] should work. This would avoid to attach unnecessary events on divs that don't need them and fix other things, for example the Widgets Help toggle that doesn't work when pressing Enter:

Thoughts?

[afercia]

Refreshed patch just to illustrate the last comment approach. Still uses a new class .was-accordion-section-title (temporary name) but just on the Menu Customizer. The other unnecessary events are removed using some more specific jQuery selectors in customize-controls.js

[celloexpressions]

I don't think we can assume the presence of tabindex="0", as plugins may be implementing their own accordions without that markup. We could make changes within the available menu items panel, but not anywhere else within menus, and that panel should use accordion.js. I'm still very against the current setup with the search, it really should be an accordion like the rest of the panel, and apparently it was not properly changed if there are issues there now with it conflicting with the accordion events.

The only viable solution that I can think of would be to use the cannot-expand class as needed to inform accordion.js to bail early in the appropriate places. However, accordion.js should not be acting at all on the main Customizer panel, if it is, can we require use of .accordion-container, or whatever is documented in the required markup at the beginning of that file? Or otherwise introduce an equivalent class to prevent that file from attaching events where we don't want them.

[afercia]

Alternate approach:

• the available menu items search shouldn't be inside .accordion-container in the first place, the available menu items template should be refactored a bit... I see also some unused stuff there, maybe it would need some cleaning... working on it
• about wp.customize.Section.attachEvents() and wp.customize.Panel.attachEvents() which are responsible to attach unnecessary events on the "titles", maybe we just need to filter out the titles with some more specific jQuery selectors

Thoughts?

[afercia]

Refreshed patch, no new CSS classes.

• refactored the available menu items template to move out the search from .accordion-container
• JS: filter out the "titles" just for Panels, as far as I see there's no need to do the same for Sections
• buttons should have a type="button" attribute so they don't need preventDefault()
• buttons need just a click event and we can remove keys filtering

[afercia]

Refreshed patch, as pointed out by @helen, to tweak itemSectionHeight(): should now exclude the search div.

[helen]

I think this is a smart way to handle this right now. Looks good to me.

[afercia]

The itemSectionHeight() calculation is wrong now, since the HTML/CSS has changed a bit. Though it's unrelated to the events stuff, would need some tweaking. Will leave the decision to committers of course :) Also, it would be better to get rid of the "magic numbers" there for at least a couple of reasons:

• any future CSS change might change the accordion sections height breaking the calculation based on magic numbers
• very long accordion section titles (custom post types, translations) might break in two lines and the accordion sections height wouldn't be "46" anymore

The refreshed patch refactors itemSectionHeight() and includes a fix for IE 8 that doesn't support window.innerHeight.

[afercia]

Fixes itemSectionHeight()

[ocean90]

The patch changes the width of #available-menu-items-search .accordion-section-content to 270px. The width should be 100% for screen sizes below 640px. Or switch to box-sizing: border-box and make it always 100%?

[afercia]

Hm the patch changes the selector but the 270px width was already there. Looks like the width it's not the only issue on small screens though, the position needs to be fixed too see below:

[afercia]

My bad, the previous patch removed the search customize-section-title which is really needed in the responsive view, see screenshot below. The refreshed patch restores it and also implements @ocean90 suggestion to use a 100% width and switch to box-sizing: border-box for the search results. Also fixes the absolute positioning (see previous comment).

This one was removed, now restored:

[afercia]

Also, on small screens itemSectionHeight() should take into account that some additional elements get visible. Maybe using the search container offset().top to be subtracted to the available vertical space.

[adamsilverstein]

I tested the latest patch with various mobile sizes on an emulator and this looks great!

Question: for long results lists, if you scroll down, the customize-section-title section scrolls off the top leaving only the search box pinned. How about instead this title remained pinned at the top with the search box? This is the route back to the previous screen and the most likely next action the user will take other than adding or refining the search.

Screencast showing the title scrolling off:

[afercia]

@adamsilverstein yes it would be great if it would remain pinned at the top :) Not 100% sure but I think this happens because on small screens .customize-section-title appears and it takes a 70px vertical space so while on large screens the #available-menu-items-search.open has a 100% height on small screens should be 100% - 70px instead. Alternate method (maybe): stretch its height using absolute positioning.
Quickly tried using calc height: calc( 100% - 70px ); would this work for you?

[obenland]

We will need to find a solution to this in the next 24 hours if it's meant to be shipped with 4.3.

[valendesigns]

I've made it so the .customizer-screen-options-toggle works with spacebar and enter in 33184.8.diff​. And as far as I can tell, all the other places of concern currently work with spacebar and enter. Both .customize-help-toggle & .clear-results are working for me in FF & Chrome. What is the issue beyond making spacebar and enter work? Do we really need to concern ourselves with mitigating all the unused events at this time if we can fix this a different way? I also fixed the search results for mobile.

IMHO we don't need to change all that CSS and JS this far into the release, it's just to late for that. What we can do is fix the issues with a different less invasive approach, but most of the issues are already in a working state from my perspective. Based on comment:3 & comment:12 I feel that 33184.8.diff​ is a good start or possibly end to solving what remains without changing the markup & CSS.

Please test and verify.

[adamsilverstein]

I tested keyboard actions, space and enter work equally well for adding menu items (see some inconsistencies in other places). Combined patches and added a few things:

33184.9.diff​

• Includes changes from .7 and .8 patches
• Improves scroll pinning for long search results, keeping title & navigation back pinned & visible
• Removes a height: auto style that was breaking previews ​on mobile (introduced in r31384)

Screencasts:

iPhone4

iPhone5

iPhone6

[obenland]

The idea of 33184.8.diff​ was to fix the most urgent issues for 4.3 without having to change all that CSS and JS this late in RC.

Does 33184.8.diff​ achieve that in your opinion?

[adamsilverstein]

Replying to obenland:

The idea of 33184.8.diff​ was to fix the most urgent issues for 4.3 without having to change all that CSS and JS this late in RC.

Does 33184.8.diff​ achieve that in your opinion?

Sorry, I will retest with that in mind. At a minimum we need to add the fix in my patch in src/wp-admin/css/customize-controls.css - without it mobile preview is *badly broken* in my testing in chrome debugger anyway, the height of the preview is truncated at less than 1/4 of my screen height.

[adamsilverstein]

With 33184.8.diff, I'm seeing an issue with the result area size on iphone5 size screen:

Unfortunately it looks like the culprit is the height calculations in itemSectionHeight. I tested without this max-height adjustment and didn't find much difference, whoever added it could speak to that.

I will do a bit more digging, patch incoming as well.

[adamsilverstein]

33184.10.diff​

• builds on 33184.8.diff​
• includes fix for preview height on mobile
• removes itemSectionHeight adjustment that was causing truncated search results on certain mobile screen sizes as the search was bound to the height of the available sections, not the search result. note: still investigating this one :)

[valendesigns]

We need to stop trying to fix things that are not part of this ticket. It is reasonable that you will have overlap in tickets but we are under a deadline and the issue is about events, not UI.

[valendesigns]

I just did some real iOS mobile testing with 33184.8.diff applied and I don't see truncated search results. What are you using to test mobile? Let's move forward with fixing the events and open new tickets for any other outstanding issues that we didn't get to in 4.3.

[adamsilverstein]

Um, oops! you are quite right! Ignore the above :) Apologies for cluttering the ticket.

Retesting with an emulator I don't see any of the issues mentioned in my comments above, so please disregard.

Replying to valendesigns:

I just did some real iOS mobile testing with 33184.8.diff applied and I don't see truncated search results. What are you using to test mobile? Let's move forward with fixing the events and open new tickets for any other outstanding issues that we didn't get to in 4.3.

[westonruter]

OK, 33184.8.diff​ works well, however it would still be good if @afercia could confirm his original issue is resolved. Please disregard patches 9 and 10.

Here is the proposed commit message describing the actual changes introduced:

Customize: Fix keyboard accessibility for toggling screen options and contextual help.

Also fix layout of search results in mobile.

Props valendesigns, afercia, adamsilverstein.
Fixes #33184.

[obenland]

'Replying to westonruter:

OK, 33184.8.diff​ works well, however it would still be good if @afercia could confirm his original issue is resolved.

That would be good but I understand he's on vacation. We shouldn't delay this ticket any further if possible.

[ocean90]

33184.8.diff​ fixes the mentioned issues in comment:44.

Do we already have a ticket for the truncated search results?

[obenland]

Replying to ocean90:

Do we already have a ticket for the truncated search results?

I don't think we do.

[westonruter]

Note that the truncated search results container issue seems to only appear when the dev tools are open. The height calculation isn't accounting for the height of the dev tools panel.

[helen]

Truncated search results happen for me all the time, patch is on #33375.

[westonruter]

In 33617:

Customize: Fix keyboard accessibility for toggling screen options and contextual help.

Also fix layout of search results in mobile.

Props valendesigns, afercia, adamsilverstein.
Fixes #33184.