WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 2 years 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 3 years ago.
20614.1.diff (3.3 KB) - added by georgestephanis 3 years ago.
Whoops! I forgot to use wp_is_mobile() at one place.
20614-3.patch (6.9 KB) - added by azaozz 2 years ago.
20614-4.patch (7.9 KB) - added by azaozz 2 years ago.
Touchstart works better than click on touch screen devices
20614-alternate-3.patch (985 bytes) - added by lessbloat 2 years ago.
20614-5.patch (8.0 KB) - added by azaozz 2 years 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 2 years ago.
20614.7.diff (4.4 KB) - added by georgestephanis 2 years ago.
20614.7-tabs.diff (3.6 KB) - added by DrewAPicture 2 years ago.
spaces to tabs
20614.7-tabs.2.diff (4.0 KB) - added by SergeyBiryukov 2 years ago.
Additional tab fixes
20614-8.patch (7.1 KB) - added by azaozz 2 years ago.
20614-9.patch (8.9 KB) - added by azaozz 2 years ago.

Download all attachments as: .zip

Change History (57)

@georgestephanis3 years ago

comment:1 @georgestephanis3 years ago

  • Keywords tableteers mobile added

@georgestephanis3 years ago

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

comment:2 @georgestephanis3 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 3 years ago by helenyhou (previous) (diff)

comment:3 @helenyhou3 years ago

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

comment:4 @georgestephanis3 years ago

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

comment:5 @georgestephanis3 years ago

this is something of a continuance from #20013

comment:6 @maxcutler3 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 @maxcutler3 years ago

  • Cc maxcutler added

comment:8 @azaozz3 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 3 years ago by azaozz (previous) (diff)

comment:9 @georgestephanis3 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 @nacin3 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 @azaozz3 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 @azaozz3 years ago

  • Milestone changed from Future Release to 3.5

comment:13 @azaozz3 years ago

  • Keywords 3.5-early removed

comment:14 @lessbloat3 years 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 3 years ago by lessbloat (previous) (diff)

comment:15 @nacin2 years ago

  • Keywords needs-refresh added; tableteers removed

comment:16 @nacin2 years 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 @azaozz2 years ago

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

@azaozz2 years ago

@azaozz2 years ago

Touchstart works better than click on touch screen devices

comment:18 @lessbloat2 years 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 2 years ago by lessbloat (previous) (diff)

comment:19 follow-up: @georgestephanis2 years ago

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

@azaozz2 years 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 @azaozz2 years 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: @georgestephanis2 years 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 @azaozz2 years 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 @azaozz2 years 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.

@azaozz2 years ago

comment:24 follow-up: @azaozz2 years 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 @azaozz2 years ago

In [22262]:

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

comment:26 in reply to: ↑ 24 @azaozz2 years 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 @DrewAPicture2 years ago

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

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

comment:28 @DrewAPicture2 years ago

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

Version 0, edited 2 years ago by DrewAPicture (next)

comment:29 @nacin2 years ago

  • Keywords close added

Anything else left here?

comment:30 @nacin2 years 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 @Otto422 years 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 @georgestephanis2 years 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 @Otto422 years 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 @Otto422 years 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 @nacin2 years ago

  • Keywords close removed

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

comment:36 @georgestephanis2 years ago

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

@georgestephanis2 years ago

comment:37 @georgestephanis2 years 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: @georgestephanis2 years 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.

@DrewAPicture2 years ago

spaces to tabs

comment:39 in reply to: ↑ 38 @DrewAPicture2 years 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.

@SergeyBiryukov2 years ago

Additional tab fixes

comment:40 @nacin2 years ago

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

@azaozz2 years ago

comment:41 @azaozz2 years 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: @Otto422 years 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 @azaozz2 years 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.

@azaozz2 years ago

comment:44 @Otto422 years ago

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

comment:45 @azaozz2 years 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.