search

WordPress / twentytwenty

Twenty Twenty is included in Core as of WordPress 5.3 🎉 To report any issues, please go here: https://core.trac.wordpress.org/newticket
Other
300 stars 138 forks source link

Fix ability to expand nav submenus on large-viewport touch interfaces #971

Closed westonruter closed 4 years ago

westonruter commented 4 years ago

In #949 changes were merged to fix #790, the inability to access a submenu item of the Desktop Horizontal Menu on tablet (>1000px). However, I found the fix is not working:

image

An alternative approach to adding JS to support touch interfaces is rather to use CSS, specifically media queries for interaction media features (caniuse, currently 92.57% of browsers globally).

An added benefit of using CSS here is that it will ensure that the nav menus work when JS is turned off in the browser (or it fails to load, or it is an AMP page). Nevertheless, since IE11 doesn't support this media query, perhaps some JS logic should be included to add a touch-enabled class name on the body and then add CSS rules for them as well (for IE11 users who have JS enabled: <7.43% of global users).

Props to @pierlon who found this solution when devising an AMP-compatible way to make the submenus accessible on large-viewport touch interfaces: https://github.com/ampproject/amp-wp/pull/3696.

pattonwebz commented 4 years ago

With this change here we rely on CSS and the interaction media queries feature but there is no support for that in IE 11 or before. About 1.85% global users are on IE11 according to the CanIUse stats.

I am interested to hear thoughs about how best to handle that. I don't think the figure is small enough to ignore it but at the same time I do not want to have to introduce any kind of a JS shim for IE because this PR actually removes some JS code making the menus a little bit easier to work with from a developers perspective.

westonruter commented 4 years ago

@pattonwebz I've added a polyfill for IE11 in 6f20c02.

pattonwebz commented 4 years ago

I see that, I am just trying to boot my laptop into windows so I can give this a live test on IE 11 but otherwise this seems good to me.

After some googling I was not able to find any solution other than a polyfill. It is a shame but that is just what we have to do to keep IE working :man_shrugging:

carolinan commented 4 years ago

I am not able to reproduce this JavaScript error.

In #949 changes were merged to fix #790, the inability to access a submenu item of the Desktop Horizontal Menu on tablet (>1000px). However, I found the fix is not working:

image

An alternative approach to adding JS to support touch interfaces is rather to use CSS, specifically media queries for interaction media features (caniuse, currently 92.57% of browsers globally).

An added benefit of using CSS here is that it will ensure that the nav menus work when JS is turned off in the browser (or it fails to load, or it is an AMP page). Nevertheless, since IE11 doesn't support this media query, perhaps some JS logic should be included to add a touch-enabled class name on the body and then add CSS rules for them as well (for IE11 users who have JS enabled: <7.43% of global users).

Props to @pierlon who found this solution when devising an AMP-compatible way to make the submenus accessible on large-viewport touch interfaces: ampproject/amp-wp#3696.