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.

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

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

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@westonruter
Contributor

westonruter commented Aug 5, 2015

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

@jquerybot jquerybot added CLA: Error and removed CLA: Error labels Aug 5, 2015

aaronjorbin pushed a commit to aaronjorbin/develop.wordpress that referenced this pull request Aug 8, 2015

Customizer: Restore Shift + Clicking on widgets to open the widgets p…
…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

Customizer: Restore Shift + Clicking on widgets to open the widgets p…
…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

ocean90
Customizer: Restore Shift + Clicking on widgets to open the widgets p…
…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

chutzimir pushed a commit to chutzimir/wordpress that referenced this pull request Aug 8, 2015

ocean90
Customizer: Restore Shift + Clicking on widgets to open the widgets p…
…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

This comment has been minimized.

Member

arschmitz commented Aug 11, 2015

@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

This comment has been minimized.

Contributor

westonruter commented Aug 12, 2015

@arschmitz done!

@jzaefferer

This comment has been minimized.

Member

jzaefferer commented Aug 14, 2015

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

This comment has been minimized.

Member

arschmitz commented Aug 14, 2015

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

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" );

This comment has been minimized.

@scottgonzalez

scottgonzalez Aug 31, 2015

Member

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

This comment has been minimized.

@westonruter

westonruter Aug 31, 2015

Contributor

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

This comment has been minimized.

@scottgonzalez

scottgonzalez Aug 31, 2015

Member

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

This comment has been minimized.

@jzaefferer

jzaefferer Sep 21, 2015

Member

@westonruter could you look into this again? Thanks.

This comment has been minimized.

@westonruter

westonruter Sep 23, 2015

Contributor

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

This comment has been minimized.

@scottgonzalez

scottgonzalez Sep 23, 2015

Member

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

@scottgonzalez

This comment has been minimized.

Member

scottgonzalez commented Aug 31, 2015

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

Core: Remove ancestor visibility requirement from :focusable selector
* Check computed visibility in addition to :visible
* Add tests for nested visibility override
@westonruter

This comment has been minimized.

Contributor

westonruter commented Sep 24, 2015

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

This comment has been minimized.

Member

scottgonzalez commented Sep 24, 2015

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

@scottgonzalez

This comment has been minimized.

Member

scottgonzalez commented Sep 25, 2015

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

This comment has been minimized.

Member

scottgonzalez commented Sep 25, 2015

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

nylen pushed a commit to nylen/wordpress-develop-svn that referenced this pull request Feb 16, 2017

ocean90
Customizer: Restore Shift + Clicking on widgets to open the widgets p…
…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