WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#33258 closed defect (bug) (fixed)

Shift+Clicking on Widgets does not open the widgets panel in 4.3-RC

Reported by: obox Owned by: westonruter
Milestone: 4.3 Priority: normal
Severity: normal Version: 4.3
Component: Customize Keywords: has-patch commit
Focuses: ui, javascript Cc:

Description

This is a replication of an old bug #29529 which was found in 4.0, but seems to have made its way back.

Currently running 4.3-RC1-33563 Shift+Clicking on a widget has no affect on the top-level panels.

To replicate open up the customizer and Shift+Click on a widget as soon as you enter the page.

If you are not in the widgets panel, the widget will open but the panel will not, meaning that the operation appears broken.

If you click on "Widgets" and the corresponding sidebar, you'll see that the widget has in fact opened.

Attachments (3)

33258.patch (576 bytes) - added by ocean90 3 years ago.
33258.2.diff (857 bytes) - added by westonruter 3 years ago.
Remove offending visibility:hidden; restore (improved) section existence check
33258.3.diff (1.2 KB) - added by westonruter 3 years ago.
https://github.com/xwp/wordpress-develop/pull/112/files

Download all attachments as: .zip

Change History (28)

#1 @pavelevap
3 years ago

Yes, I noticed it yesterday, but I thought that I clicked wrong way because when I clicked once againg, it worked :-) Now I see, that I have to click twice to open corresponding widget and that is probably wrong...

#2 @afercia
3 years ago

Also, in 4.2 when the widget opens in the sidebar, focus is moved to the first focusable element. In trunk it isn't. Widgets are now initially hidden with display: none set on a couple of wrapper elements and jQuery UI :focusable doesn't get hidden elements.

#3 @afercia
3 years ago

  • Version set to trunk

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


3 years ago

#5 @obenland
3 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.3
  • Owner set to ocean90
  • Status changed from new to assigned

@ocean90
3 years ago

#6 @ocean90
3 years ago

  • Owner changed from ocean90 to westonruter
  • Status changed from assigned to reviewing

Looks like reverting the change to self.expandControlSection(); in https://core.trac.wordpress.org/changeset/32649#file4 fixes this issue.
api.section( self.section ) returns undefined for the first click.

#7 @afercia
3 years ago

About the secondary focus issue couldn't get :focusable return anything, maybe I'm missing something or maybe :focusable doesn't get elements that are initially hidden, really not sure. Instead, trying to emulate (more or less) what :focusable does, works:

focusContainer.find( ':input, a' ).filter( ':visible' ).eq( 0 ).focus();

#8 @westonruter
3 years ago

@afercia: I tracked down the issue with focusable. It turns out to have been introduced in [33069], specifically right here:

.wp-full-overlay.section-open #customize-controls .wp-full-overlay-sidebar-content {
        visibility: hidden; /* <== this causes the focusable failure */
        overflow-y: hidden;
}

The reason why this causes the :focusable selector to fail is that focusable function is defined to return false if any of the selected element's parents have visibility: hidden: https://github.com/jquery/jquery-ui/blob/cd6c751b88313ac1fa2e0fb46d7668ca6ce0f4b1/ui/core.js#L201-L209

If I comment-out the visibility: hidden; property, then the focus() call again works as expected.

@afercia: It seems very strange that a parent element #customize-controls for the customize controls should be getting visibility: hidden. I'm not even sure why anything in the Customizer pane is visible since it has an ancestor element that hidden.

@celloexpressions: Why was this needed for accessibility?

@westonruter
3 years ago

Remove offending visibility:hidden; restore (improved) section existence check

#9 @afercia
3 years ago

@westonruter yeah I figured out it was related to visibility. For accessibility we need to hide with display: none or visibility: hidden all elements that are out of view, otherwise if they're just off-screen (but "visible") you can still tab to the focusable element inside them and even activate controls (and screen readers will read out their content).
So using an "off-left" technique is not sufficient, we need to have them really hidden.

#10 follow-up: @westonruter
3 years ago

@afercia OK, but it seems we need a different approach to selecting which elements are hidden. Trying to hide an ancestor element of an element that is currently in view seems like a problem, and should be reverted. Instead of hiding an ancestor element, we should be hiding elements for sibling sections/panels. Or… maybe I (with jQuery UI) am just ignorant of nested a visibility feature of CSS, and there is a jQuery UI bug here? It seems that this behavior is normal for browsers: http://jsfiddle.net/westonruter/xuuzv5cu/

So if a nested visibility:visible override is standard and not a hack, then we should use an alternative to :focusable and jQuery UI should get patched.

#11 @celloexpressions
3 years ago

My understanding is that a primary purpose of the visibility property is to allow a parent element to be hidden with only certain child elements being visible, overriding that. So this would be a jQuery UI bug.

The current implementation in trunk relies heavily on this behavior for visibility, so trying to switch to an alternate technique has the potential to break more that it would fix at this point.

#12 in reply to: ↑ 10 @afercia
3 years ago

Replying to westonruter:

So if a nested visibility:visible override is standard and not a hack, then we should use an alternative to :focusable and jQuery UI should get patched.

Yup it's standard and that's one of the reason we're using it :)
http://www.w3.org/TR/CSS21/visufx.html#visibility

...
hidden
The generated box is invisible (fully transparent, nothing is drawn), but still affects layout. Furthermore, descendants of the element will be visible if they have 'visibility: visible'.

#13 @westonruter
3 years ago

I've opened a PR to fix the issue in jQuery UI Core: https://github.com/jquery/jquery-ui/pull/1583

#14 follow-up: @afercia
3 years ago

Probably not a jQuery UI bug either, maybe just a matter of timing. By the way if you look at what :focusable does internally, it does more or less something that we can replicate with a jQuery selector as in the example above. See
https://github.com/jquery/jquery-ui/blob/1.11.4/ui/core.js#L88
We're not interested in area or object elements, right? We just need to target visible :input and links.

Last edited 3 years ago by afercia (previous) (diff)

#15 in reply to: ↑ 14 @westonruter
3 years ago

Replying to afercia:

Probably not a jQuery UI bug either, maybe just a matter of timing. By the way if you look at what :focusable does internally, it does more or less something that we can replicate with a jQuery selector as in the example above. See
https://github.com/jquery/jquery-ui/blob/1.11.4/ui/core.js#L88
We're not interested in area or object elements, right? We just need to target visible :input and links.

It does look like a jQuery UI bug. I've amended my test case to demonstrate the issue, and how it is not a timing problem: http://jsfiddle.net/westonruter/xuuzv5cu/

#16 @westonruter
3 years ago

In my latest patch on #33220 I fix the :focusable issue via:

- focusContainer.find( ':focusable:first' ).focus(); 
- focusContainer[0].scrollIntoView( true );
+ // Note that we can't use :focusable due to a jQuery UI issue. See: https://github.com/jquery/jquery-ui/pull/1583 
+ focusContainer.find( 'input, select, textarea, button, object, a[href], [tabindex]' ).filter( ':visible' ).first().focus();

#17 @obenland
3 years ago

So is this ticket dependent on #33220 or should we get the focusable part fixed here and the patch updated?

#18 @westonruter
3 years ago

  • Keywords has-patch added; needs-patch removed

@obenland: In 33258.3.diff I've amended the patch with the fix from #33220.

#19 follow-up: @obenland
3 years ago

Tested the patch. Now works again on first click.

Pretty neat feature, I had no idea it existed. :)

#20 in reply to: ↑ 19 @afercia
3 years ago

Replying to obenland:

Pretty neat feature, I had no idea it existed. :)

You're not the only one :) Related: #33251.

#21 @ocean90
3 years ago

  • Focuses administration removed
  • Keywords commit added

33258.3.diff restores the same behavior as in 4.2 which includes some weirdness in IE. In IE 8 nothings get's focused, in IE 9 and 10 the toggle arrow (.widget-title-action a.widget-action:before) gets focussed. But that's stuff for another ticket.

Last edited 3 years ago by ocean90 (previous) (diff)

#22 @azaozz
3 years ago

33258.3.diff looks good, +1 for commit.

#23 @ocean90
3 years ago

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

In 33596:

Customizer: Restore Shift + Clicking on widgets to open the widgets panel.

Includes an alternative for jQuery UI's :focusable selector because it has an ancestor visibility requirement, see https://github.com/jquery/jquery-ui/pull/1583.

props westonruter.
fixes #33258.

#24 @scott.gonzalez
2 years ago

This is now fixed in jQuery UI, though you'll need to wait for the 1.12.0 release to pull in the fix.

#25 @westonruter
2 years ago

In 35584:

Customize: Fix focus and autofocus on nav_menu_item controls.

Embed the nav_menu_item control not only when the contained nav_menu section expands, but also if the control was autofocused (via the autofocus[control] query param). Also applies change from [33596] to work-around a broken :focusable selector in jQuery UI.

See #33258.
Fixes #34629.

Note: See TracTickets for help on using tickets.