WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#38476 closed defect (bug) (fixed)

Twenty Seventeen: Focused controls may be hidden by the top menu.

Reported by: afercia Owned by: davidakennedy
Milestone: 4.7 Priority: normal
Severity: normal Version:
Component: Bundled Theme Keywords: has-screenshots has-patch needs-testing dev-feedback
Focuses: ui, accessibility Cc:

Description

When a page is scrolled, the top nav menu become fixed. Depending on the page content, some focused links or buttons may be actually "hidden" behind the nav menu thus potentially creating confusion for keyboard users since the focus indication would actually "disappear" for a while.

See in the screenshot below, where I've made the nav menu semi-transparent just to make the issue clear:

https://cldup.com/bJBRj78gTT.png

Hat tip Sam Sidler at WCMIL Contributor's Day.

Attachments (9)

38476.diff (734 bytes) - added by Fencer04 4 years ago.
Added code to check if something with focus was in the region behind the fixed navigation.
38476.2.patch (1.7 KB) - added by davidakennedy 4 years ago.
38476.3.diff (1.5 KB) - added by Fencer04 4 years ago.
Update to bring the code back to the anonymous function and check for the fixed navigation class on each focus.
38476.4.patch (1.7 KB) - added by davidakennedy 4 years ago.
Account for Admin bar and footer links.
38476.5.patch (2.1 KB) - added by davidakennedy 4 years ago.
Use get_theme_file_uri() for enqueueing.
38476.6.patch (1.0 KB) - added by davidakennedy 4 years ago.
Expand possible controls that can receive focus and remove smooth scrolling.
38476.7.patch (1.2 KB) - added by davidakennedy 4 years ago.
Improve selector list for performance.
38476.8.patch (1.3 KB) - added by davidakennedy 4 years ago.
Added .filter to filter visible items.
38476.9.patch (1.1 KB) - added by davidakennedy 4 years ago.
Add context selector for better performance.

Download all attachments as: .zip

Change History (38)

#1 @afercia
4 years ago

  • Milestone changed from Awaiting Review to 4.7

#2 @Fencer04
4 years ago

I'd be wiling to work on this but what do we think an agreeable solution is?

#3 follow-up: @davidakennedy
4 years ago

Thanks @Fencer04! I'm open to ideas for this one as I haven't thought about it too deeply yet. Hopefully, it's something that doesn't require a ton of additional code.

Twenty Fourteen did a bit of this with its skip to content link. See: https://core.trac.wordpress.org/browser/trunk/src/wp-content/themes/twentyfourteen/js/functions.js#L63

#4 in reply to: ↑ 3 @Fencer04
4 years ago

@davidakennedy that makes sense, I'll review that type of solution and see what I can come up with.

@Fencer04
4 years ago

Added code to check if something with focus was in the region behind the fixed navigation.

#5 @Fencer04
4 years ago

  • Keywords has-patch needs-testing dev-feedback added

I posted a patch that I think is on the right track. I put it in global.js because I wasn't sure where else it should go. It uses the change focus event on all elements. I'm not sure if that is a bad thing or not. I'll need to update it with some comments but wanted to see if someone could review/test it to see if I'm on the right track.

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


4 years ago

#7 @afercia
4 years ago

  • Owner set to davidakennedy
  • Status changed from new to assigned

#8 @davidakennedy
4 years ago

38476.2.patch builds on the work of @Fencer04. It:

  • Makes sure the scrollTo plugin loads on every page since it can be called at any time now.
  • Moves the work into the $( document ).ready( function() part of the file so we know jQuery is loaded, etc.
  • Targets links only in main body of page so selector is more performant.
  • Also, only calls function if the nav is sticky.

Nice way to kick things off @Fencer04! I still feel like this needs more work. It doesn't work on all cases for me. I'll try to make a screencast tomorrow. But encourage people to test this patch and iterate on it. @afercia Feel free to test too!

#9 @Fencer04
4 years ago

@davidakennedy this appears to be working for me, what circumstances caused issues on your end?

#10 @afercia
4 years ago

  • Keywords needs-refresh added

Tested a bit and the current patch works only if the navigation has the site-navigation-fixed CSS class on DOM ready, which is true only when the page is already scrolled and then refreshed.
The check for fixed nav should happen on every focus event so it should moved inside the anonymous function. Also, not sure there's the need for e.preventDefault();.

#11 follow-up: @Fencer04
4 years ago

@afercia That makes sense, I'll review the current patch and submit an update by Monday afternoon (EST).

#12 in reply to: ↑ 11 @Fencer04
4 years ago

@afercia I tested this in IE11 and the latest version of Chrome and don't seem to have the issue you described. After I visit a page and the fixed nav isn't in place if I scroll down and then tab to items that are hidden by the nav I am brought to them.

Can someone else test to make sure I'm not missing something?

#13 @afercia
4 years ago

@Fencer04 what I see in 38476.2.patch is:

// Fires on document ready
$( document ).ready( function() {
	...

	if ( $navigation.hasClass( 'site-navigation-fixed' ) ) {
		$( '#content a' ).focus( function( e ) {
			etc...

so on DOM ready, the top navigation doesn't have a CSS class site-navigation-fixed yet, thus the focus event doesn't get attached.

@Fencer04
4 years ago

Update to bring the code back to the anonymous function and check for the fixed navigation class on each focus.

#14 @Fencer04
4 years ago

  • Keywords needs-refresh removed

@afercia Sorry for the confusion. My old patch code was still in place while I was testing the new code. After correcting that I saw the issue you described. I uploaded a new patch that moves the code back to anonymous and checks for the fixed navigation class on each focus change.

One question: I included the changes to functions.php from patch number 2 in my patch. Is that the correct procedure or should you only include the changes made in this version?

Last edited 4 years ago by Fencer04 (previous) (diff)

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


4 years ago

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


4 years ago

@davidakennedy
4 years ago

Account for Admin bar and footer links.

@davidakennedy
4 years ago

Use get_theme_file_uri() for enqueueing.

#17 @davidakennedy
4 years ago

With 38476.5.patch, I've accounted for the admin bar and used get_theme_file_uri() just in case the script needs to be overridden.

#18 @karmatosed
4 years ago

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

In 39183:

Twenty Seventeen: Fixes focused controls hidden by top menu.

When a page is scrolled, the top nav menu became fixed. This resolves that.

Props afercia, Fencer04, davidakennedy
Fixes #38476

#19 follow-up: @afercia
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Sorry for reopening but I'd like to hear more feedback on this and have some more testing. I've tested ;) but more eyes would be appreciated. I'd recommend to test with Firefox (because Chrome tends to scroll to the middle of the page).

I think the 600ms interval should be way shorter, maybe 0. The thing is, when you keep the tab key pressed, and thus focus will move very quickly through controls, the interval is too long and will cause the page to "jump". That's because the browser has already moved focus on the following controls but the JS scrolling is still running.

The goal here shouldn't be to have a nice "smooth scrolling" effect, instead the fix should try to emulate as much as possible the native browsers behaviour.

Also, worth considering this works just for links and ignores form fields, buttons, or other focusable controls that may be hidden under the top nav. To test, just put the search widget in your sidebar. Wondering if the fix should work for any focusable control.

#20 in reply to: ↑ 19 @Fencer04
4 years ago

@afercia I had originally used * as the selector for the focus. Do you think changing it to "#content *, #colophon *" would do the trick? This was anything that can get focus would trigger the code?

#21 @afercia
4 years ago

@Fencer04 to be honest I'm more concerned about the 600 ms interval :) that should really be reduced to 0. About the elements to target, maybe I'd consider to add all form controls but I'd defer the decision to all the ones who know Twenty Seventeen better than me :)

@davidakennedy
4 years ago

Expand possible controls that can receive focus and remove smooth scrolling.

#22 @davidakennedy
4 years ago

@afercia Great points! I didn't think of those. :( But I'm totally for removing the smooth scrolling and expanding the selectors. Please see my latest patch and give it a test.

#23 @afercia
4 years ago

@davidakennedy I'd consider to:

  • target only what's inside the #content, the footer can probably be left out
  • filter the visible elements (e.g. to exclude hidden inputs)
  • the asterisk on tabindex and contenteditable seems redundant to me
  • performance: any suggestions to improve very welcome

Something like this maybe:

$( '#content' ).find( terrible pile of selectors here ).filter( ':visible' )

@davidakennedy
4 years ago

Improve selector list for performance.

#24 follow-up: @davidakennedy
4 years ago

Thanks @afercia for the feedback! In 38476.7.patch, I've made some changes to narrow the selectors. Do we need to filter for hidden elements? I thought they couldn't receive focus already? Also, .filter can be a performance hog, so I'm hesitant to use it.

this appears to be working for me, what circumstances caused issues on your end?

@Fencer04 I realized I didn't answer your question from above. It was not accounting for the admin bar height. So it worked, just not as well when logged in. Thanks for your continued work on this! It's been super helpful.

#25 in reply to: ↑ 24 @afercia
4 years ago

Replying to davidakennedy:

Do we need to filter for hidden elements? I thought they couldn't receive focus already?

They would have the focus event attached anyways, thinking it would be better to avoid that. And yes, they are out of the tab order, but they can receive focus programmatically.

Also, .filter can be a performance hog, so I'm hesitant to use it.

Yep I know 😔

@davidakennedy
4 years ago

Added .filter to filter visible items.

#26 @davidakennedy
4 years ago

@afercia In 38476.8.patch, I added :filter.

This ticket was mentioned in Slack in #accessibility by davidakennedy. View the logs.


4 years ago

@davidakennedy
4 years ago

Add context selector for better performance.

#28 @davidakennedy
4 years ago

In 38476.9.patch, I added context to the selector for better performance based on some feedback:

https://wordpress.slack.com/archives/accessibility/p1479133361000403

#29 @karmatosed
4 years ago

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

In 39225:

Twenty Seventeen: Resolves focused controls hidden by the top menu.

When a page is scrolled, the top nav menu become fixed. Depending on the page content this caused issue where focused links or buttons may be hidden behind the nav menu.

Props afercia, davidakennedy, Fencer04
Fixes #38476

Note: See TracTickets for help on using tickets.