WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 20 months ago

#20614 closed enhancement (fixed)

Making the menus work in Windows Phone 7, and work better in Android / iOS

Reported by: georgestephanis Owned by: azaozz
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.4
Component: Toolbar Keywords: has-patch mobile needs-testing
Focuses: Cc:

Description (last modified by helenyhou)

Currently, on Windows Phone 7/7.5, the Toolbar Dropdown menus are non-functional (as there is no ontouchstart functionality in the browser) and the admin flyout menus are also entirely non-functional. This translates roughly to ... there is no way to actually log out of WordPress! (as logout is in the user dropdown menu)

iOS has some built in :hover faking code, but we should also make the admin menu flyouts more accessible and usable on Android -- where you have to touch and hold to activate the flyout/dropdown, and then slide your finger off to not trigger the actual link.

Attachments (12)

20614.diff (3.4 KB) - added by georgestephanis 2 years ago.
20614.1.diff (3.3 KB) - added by georgestephanis 2 years ago.
Whoops! I forgot to use wp_is_mobile() at one place.
20614-3.patch (6.9 KB) - added by azaozz 21 months ago.
20614-4.patch (7.9 KB) - added by azaozz 21 months ago.
Touchstart works better than click on touch screen devices
20614-alternate-3.patch (985 bytes) - added by lessbloat 21 months ago.
20614-5.patch (8.0 KB) - added by azaozz 21 months ago.
Having stopPropagation() when "touching" menu parents works well but is not ideal in case another script uses touch events in the menu or toolbar
20614-6.patch (8.1 KB) - added by azaozz 21 months ago.
20614.7.diff (4.4 KB) - added by georgestephanis 20 months ago.
20614.7-tabs.diff (3.6 KB) - added by DrewAPicture 20 months ago.
spaces to tabs
20614.7-tabs.2.diff (4.0 KB) - added by SergeyBiryukov 20 months ago.
Additional tab fixes
20614-8.patch (7.1 KB) - added by azaozz 20 months ago.
20614-9.patch (8.9 KB) - added by azaozz 20 months ago.

Download all attachments as: .zip

Change History (57)

georgestephanis2 years ago

comment:1 georgestephanis2 years ago

  • Keywords tableteers mobile added

georgestephanis2 years ago

Whoops! I forgot to use wp_is_mobile() at one place.

comment:2 georgestephanis2 years ago

  • Keywords changed from has-patch tableteers, mobile to has-patch tableteers mobile

A little JS patch that will help the admin menu and the admin bar menus operate properly in mobile devices. This way, if it's hidden, the first click will show it, and prevent the link from going anywhere. Huzzah! 20614.1.diff

I've probably put some things in the wrong places, and I know nacin/koopersmith sounded like there was another optimization in the JS they'd like to see (koopersmith: we should bind two handlers when a menu is open. one to stop event propagation any time the #adminmenu is clicked, the other to close the menu any time the body is clicked), but I couldn't seem to wrap my head around it, so I'm tapping out to let someone else put the last bit of spit and polish on it.

Last edited 2 years ago by helenyhou (previous) (diff)

comment:3 helenyhou2 years ago

  • Component changed from UI to Toolbar
  • Description modified (diff)

comment:4 georgestephanis2 years ago

It's not just the toolbar, btw -- it's the admin menu as well.

comment:5 georgestephanis2 years ago

this is something of a continuance from #20013

comment:6 maxcutler2 years ago

I have tested this on WP7 again and confirmed that it works as advertised.

This issue has been around since at least 3.3 and prevents users from logging out since "Log out" is hidden in a flyout menu (when touching "Howdy, <x>", it executes a page navigation before the flyout can appear).

comment:7 maxcutler2 years ago

  • Cc maxcutler added

comment:8 azaozz2 years ago

Imho the best way to do this is to use "states" on the links when on a mobile device. The JS would be similar to the patch above and use an additional class on the link to keep its "state". The first click (or touch, or tap) would open the submenu if any and the second will follow the link:

( if is_mobile )

$('#wpcontent').bind('click.adminbar-mobile', function(e){
  var target = $( e.target );

  if ( target.parent().parent().hasClass('ab-top-menu') ) {
    // target is top level toolbar menu item
    if ( target.hasClass('touched') ) {
      return true; // follow the link
    } else {
      target.addClass('touched');
      target.parent().addClass('hover');
      e.preventDefault();
    }
  } else if ( target.hasClass('wp-menu-image') || target.hasClass('menu-top') ) {
    // target is top level admin menu item
    ...
  }
});

Alternatively we can disable the links for top level menu items completely and make them work only as toggles for the submenus. This may be preferable on mobile devices.

Will make a full patch probably tomorrow.

Last edited 2 years ago by azaozz (previous) (diff)

comment:9 georgestephanis2 years ago

Azaozz -- any word on this? Bit of a time crunch here, as I understand it, if it's to get in.

comment:10 nacin2 years ago

  • Keywords 3.5-early added
  • Milestone changed from Awaiting Review to Future Release
  • Type changed from defect (bug) to enhancement

Will make a full patch probably tomorrow.

comment:11 azaozz2 years ago

The problem here is that iOS and (afaik) the newest Android browsers work really well without any JS. In order for us to support older/different mobile browsers we have to disable the default behavior in the browsers that work which is probably not the best thing for them, and replace it with JS.

The alternative is to try to detect which mobile browsers work or don't work but that's quite hard as there isn't a direct test for this functionality.

comment:12 azaozz2 years ago

  • Milestone changed from Future Release to 3.5

comment:13 azaozz2 years ago

  • Keywords 3.5-early removed

comment:14 lessbloat23 months ago

Discussed in IRC today (https://irclogs.wordpress.org/chanlog.php?channel=wordpress-ui&day=2012-08-29&sort=asc#m54995). Azaozz said, "that needs refresh after the menu html changed".

Last edited 23 months ago by lessbloat (previous) (diff)

comment:15 nacin22 months ago

  • Keywords needs-refresh added; tableteers removed

comment:16 nacin22 months ago

  • Keywords punt added

This sounds like a punt at this point, with no activity for some time. But, if it is revived during the 3.5 beta period, it can be re-considered.

comment:17 azaozz21 months ago

Seems iOS6 changed something making this not-so-great there too. Should probably be considered for 3.5.

azaozz21 months ago

azaozz21 months ago

Touchstart works better than click on touch screen devices

comment:18 lessbloat21 months ago

Was working on this this afternoon, then went away for an hour before finishing, and when I returned, azaozz had beat me to it ;-). Figured I'd upload mine anyways (in 20614-alternate-3.patch), since I was almost done.

Disclaimers: All mine does is prevent the click on the main menu link, once the sub menu has expanded (and I only tested it on my iPad).

Last edited 21 months ago by lessbloat (previous) (diff)

comment:19 follow-up: georgestephanis21 months ago

Windows Phone doesn't have ontouchstart events. It needs click.

azaozz21 months ago

Having stopPropagation() when "touching" menu parents works well but is not ideal in case another script uses touch events in the menu or toolbar

comment:20 in reply to: ↑ 19 azaozz21 months ago

Replying to lessbloat:

Was working on this this afternoon, then went away for an hour before finishing, and when I returned, azaozz had beat me to it ;-).

Ugh, sry about that. Should have coordinated in #wordpress-dev.

Replying to georgestephanis:

Windows Phone doesn't have ontouchstart events...

Don't have one to test with. Click works so-so in iOS and Android but touchstart is a lot better. Perhaps we could detect Win phone and swap the events to "click"?

On the other hand, not sure how well we support Win phone at all. Can the admin be used from there?

comment:21 follow-up: georgestephanis21 months ago

It can mostly, but you can't log out, as the logout link is hidden in a dropdown menu.

I'd worked with Max Cutler on the initial version of this patch and he confirmed it worked perfectly for him under WP7.

comment:22 azaozz21 months ago

In 20614-5.patch:

  • Disable hoverIntent on touch screen devices (it doesn't work well there).
  • Handle opening of submenus on the first touch and following the link on the second touch.
  • Close any open submenus on touch anywhere else on the screen.

This is implemented separately for the admin menu and the toolbar. There is quite a bit of code duplication as we need the toolbar functionality on the front-end too. A compromise would be to handle both the admin menu and the toolbar from admin-bar.js. Could possibly separate this in another JS file, would be a bit unclear as we need to disable hoverIntent in the "standard" JS.

Tested on iPad/iOS6 and Kindle Fire. Need testing on more mobile devices incl. newer Android.

comment:23 in reply to: ↑ 21 azaozz21 months ago

Replying to georgestephanis:

Ok, will try to redo it with "click" to accommodate Win phone or detect it and swap the events for it.

azaozz21 months ago

comment:24 follow-up: azaozz21 months ago

In 20614-6.patch:

  • Changed the JS events to 'click'. Some (newer) versions of Android work only with click too.
  • Added simple detection for Win phone 7-7.5 (IEMobile 7, 8, 9). Win phone 8/IEMobile10 that comes out in a few days supports the touch API.

comment:25 azaozz21 months ago

In [22262]:

Make the admin menu and toolbar work well on mobile devices, props georgestephanis, see #20614

comment:26 in reply to: ↑ 24 azaozz21 months ago

Replying to azaozz:

Chatting with @georgestephanis in #wordpress-dev, IEMobile 10 has a touch API but it's different, so extended the simple detection to catch it too.

comment:27 DrewAPicture21 months ago

  • Cc xoodrew@… added
  • Keywords needs-refresh punt removed

Confirmed working on Android (phone + touchpad), iPad 2 (IOS6). Closed #22065 as fixed.

comment:28 DrewAPicture21 months ago

Related-ish: #22064 - Unable to Read Help in Editor on iPad

Version 0, edited 21 months ago by DrewAPicture (next)

comment:29 nacin20 months ago

  • Keywords close added

Anything else left here?

comment:30 nacin20 months ago

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

Good to go. azaozz may open a new ticket for lingering iOS 6 issues.

comment:31 Otto4220 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

[22262] breaks the menu flyouts on my laptop.

This is technically a touchscreen laptop, though I leave the touchscreen off most of the time.

Well, in Chrome, it's apparently smart enough to detect this, because
alert('ontouchstart' in window);
returns true.

Thus, line 198 in common.js is turning on the touch screen support incorrectly in my case.

Only happens in Chrome. Firefox isn't that bright.

comment:32 georgestephanis20 months ago

Ideal resolution, imho, would be for it to not swap out this fix instead of the hover support -- but do an enhancement in addition to it.

comment:33 Otto4220 months ago

For reference, this is what the modernizr tells me with regard to touch support:

https://dl.dropbox.com/u/140824/touchsupport.png

http://modernizr.github.com/Modernizr/touch.html

comment:34 Otto4220 months ago

Argh. For reference, chrome is returning true for me for both of these as well:

alert(window.matchMedia("(pointer:coarse)").matches );
alert(window.matchMedia("(hover:0)").matches );

Dangit, chrome.

comment:35 nacin20 months ago

  • Keywords close removed

This resulted in #22382 as well. Seems to me like we are approaching revert status on [22262].

comment:36 georgestephanis20 months ago

Give me a chance to go all 'Fix-It Felix' this evening. If not, you can 'Wreck-It Ralph' tomorrow morning.

georgestephanis20 months ago

comment:37 georgestephanis20 months ago

  • Keywords needs-testing added

Patch attached. Instead of the initial "This Or That" statement, the normal HoverIntent is always run, and it's just a conditional for the touch enhancements.

comment:38 follow-up: georgestephanis20 months ago

Gah! Curse this new text editor I've been testing out. It swapped all the tabs for spaces in the patch!

/me commits ritual seppuku.

DrewAPicture20 months ago

spaces to tabs

comment:39 in reply to: ↑ 38 DrewAPicture20 months ago

Replying to georgestephanis:

Gah! Curse this new text editor I've been testing out. It swapped all the tabs for spaces in the patch!

Spaces to Tabs bundle in TextMate is a beautiful thing.

20614.7-tabs.diff converts your patch to tabs.

SergeyBiryukov20 months ago

Additional tab fixes

comment:40 nacin20 months ago

  • Owner set to azaozz
  • Status changed from reopened to assigned

azaozz20 months ago

comment:41 azaozz20 months ago

20614-8.patch attaches temporarily the 'click' event on touch devices then the toolbar is "touched" and detaches it after the "click" disabling hoverIntent for this click. That way if the device has mouse and the click was with the mouse, that won't be used and instead hoverIntent will handle it as usual.

comment:42 follow-up: Otto4220 months ago

Patch 8 fixes the left hand side menu hovering, but the expanded menus don't work properly.

When hovering over an item in the expanded menu (for example, when on the wp-admin/users.php page, the All Users/Add New/Your Profile links below the Users menu item), a single click does not work to take you to Your Profile, for example.

A second click works, but not if you unhover the link and then come back to it. So if you click, move the mouse, move the mouse back, and click again, then nothing happens. To access any menu item in that expanded menu, you have to click twice without leaving the hovered state on the item.

comment:43 in reply to: ↑ 42 azaozz20 months ago

Replying to Otto42:

Should be fixed in 20614-9.patch, could you re-test it. Also fixed some inconsistencies when the admin menu is folded.

azaozz20 months ago

comment:44 Otto4220 months ago

Ok, that one seems to work. Still need to test admin bar for multisite, but everything else looks okay.

comment:45 azaozz20 months ago

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

In 22636:

Make the admin menu and toolbar work well on mobile devices (take 2), props georgestephanis, fixes #20614, fixes #22382

Note: See TracTickets for help on using tickets.