Skip to content

Core: Remove ancestor visibility requirement from :focusable selector #1583

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Core: Remove ancestor visibility requirement from :focusable selector #1583

wants to merge 1 commit into from

Conversation

westonruter
Copy link
Contributor

This is news to me, but it turns out that an element that has an ancestor with visibility:hidden can itself have visibility:visible and it will show that nested element even thought everything in the wrapped elements up to the ancestor will be hidden. Example: http://jsfiddle.net/westonruter/xuuzv5cu/

This being the case, the :focusable selector needs to remove the check for ancestors being visible.

This issue was discovered in a ticket on WordPress Core: https://core.trac.wordpress.org/ticket/33258#comment:10

@westonruter westonruter force-pushed the bugfix/focusable-visible branch from 956eaf7 to 3aff516 Compare August 5, 2015 00:10
aaronjorbin pushed a commit to aaronjorbin/develop.wordpress that referenced this pull request Aug 8, 2015
@ocean90
Customizer: Restore Shift + Clicking on widgets to open the widgets p…
2d6c10b 
…anel.

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

props westonruter.
fixes #33258.

git-svn-id: https://develop.svn.wordpress.org/trunk@33596 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Aug 8, 2015
@ocean90
Customizer: Restore Shift + Clicking on widgets to open the widgets p…
04c21a0 
…anel.

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

props westonruter.
fixes #33258.
Built from https://develop.svn.wordpress.org/trunk@33596


git-svn-id: http://core.svn.wordpress.org/trunk@33563 1a063a9b-81f0-0310-95a4-ce76da25c4cd
svn2github pushed a commit to svn2github/wp-svn-2-git that referenced this pull request Aug 8, 2015
Customizer: Restore Shift + Clicking on widgets to open the widgets p…
8bed548 
…anel.

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

props westonruter.
fixes #33258.
Built from https://develop.svn.wordpress.org/trunk@33596


git-svn-id: http://core.svn.wordpress.org/trunk@33563 1a063a9b-81f0-0310-95a4-ce76da25c4cd
@arschmitz
Copy link
Member

@westonruter we updated the core file last week ( there is not really a core file any more ) breaking it up into individual modules. Could you rebase / update this to account for that please?

@westonruter westonruter force-pushed the bugfix/focusable-visible branch from 3aff516 to 1f49e53 Compare August 12, 2015 00:56
@westonruter
Copy link
Contributor Author

@arschmitz done!

@jzaefferer
Copy link
Member

Diff looks good to me.

@arschmitz want to take a look as well? I guess we can take the PR description, put it in a Trac ticket a reference it when merging this PR.

@arschmitz
Copy link
Member

This looks good to me and will be faster which is good for an already slow selector

scottgonzalez
scottgonzalez reviewed Aug 31, 2015
View reviewed changes
tests/unit/core/selector.js
isNotFocusable( "#visibilityHiddenAncestor-input", "input, visibility: hidden parent" );
isNotFocusable( "#visibilityHiddenAncestor-span", "span with tabindex, visibility: hidden parent" );
isFocusable( "#visibilityHiddenAncestor-input", "input, visibility: hidden parent" );
isFocusable( "#visibilityHiddenAncestor-span", "span with tabindex, visibility: hidden parent" );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely not correct. You cannot focus something that's not visible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@scottgonzalez Something can be visible even if (unexpectedly) its ancestor is not visible. See http://jsfiddle.net/westonruter/xuuzv5cu/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that, but this is not what's happening here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@westonruter could you look into this again? Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I don't understand what is happening here then.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of these elements have visibility: visible on them, so they're all still hidden.

@scottgonzalez
Copy link
Member

Every assertion that was changed is incorrect. In addition, the scenario described above isn't actually tested.

@westonruter westonruter force-pushed the bugfix/focusable-visible branch 2 times, most recently from 24438a7 to 8bee28d Compare September 24, 2015 04:55
@westonruter
Core: Remove ancestor visibility requirement from :focusable selector
175e14f 
* Check computed visibility in addition to :visible
* Add tests for nested visibility override
@westonruter westonruter force-pushed the bugfix/focusable-visible branch from 8bee28d to 175e14f Compare September 24, 2015 05:00
@westonruter
Copy link
Contributor Author

Thanks all. I did a deeper dive into this and fixed the issues you identified. Part of my problem was I wasn't that :visible isn't a straight equivalent to checking if visibility is visible. I also added tests to demonstrate the nested visibility override behavior, and rebased against the latest from master.

@scottgonzalez
Copy link
Member

Thanks. I've filed a jQuery UI ticket for this and merged it.

@scottgonzalez
Copy link
Member

This change caused lots of failures in IE 8.

scottgonzalez added a commit to scottgonzalez/jquery-ui that referenced this pull request Sep 25, 2015
@scottgonzalez
Focusable: Fix handling of visibility: inherit
9b2353a 
Ref #14596
Ref jquerygh-1583
@scottgonzalez scottgonzalez mentioned this pull request Sep 25, 2015
Closed
@scottgonzalez
Copy link
Member

The IE 8 failure was caused by an incorrect default of visibility: inherit instead of visibility: visible. Honestly, I've always thought the spec was wrong and the IE 8 behavior was correct, but there's nothing we can do about that. This did reveal an actual bug in our code though, since we weren't handling the case of visibility: inherit. I've put together a patch in #1605.

scottgonzalez added a commit that referenced this pull request Sep 29, 2015
@scottgonzalez
Focusable: Fix handling of visibility: inherit
d302596 
Ref #14596
Ref gh-1583
Closes gh-1605
nylen pushed a commit to nylen/wordpress-develop-svn that referenced this pull request Feb 16, 2017
Customizer: Restore Shift + Clicking on widgets to open the widgets p…
b94ec1d 
…anel.

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

props westonruter.
fixes #33258.

git-svn-id: https://develop.svn.wordpress.org/trunk@33596 602fd350-edb4-49c9-b593-d223f7449a82
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@westonruter @arschmitz @jzaefferer @scottgonzalez @jquerybot