Unable to scroll toolbar menu items or show contextual links on mobile browsers

I confirm that this issue is still open. It is also a problem on Android 5.1.1 (Nexus 4).

Working on this issue now (in Barcelona), first trying to reproduce.

I have to finish here, this is what I found so far:
- Could replicate using Google Chrome's developer tool (using device mode).
- The problem is probably around /core/modules/toolbar/js/views/ToolbarVisualView.js line 21-22

        event.preventDefault();
        event.target.click();

The preventDefault() does not have an effect because this event gets fired when the user starts to touch-scroll, and preventDefault() can't be used until the event (scrolling) finishes.
Javascript console displays this warning:
"Ignored attempt to cancel a touchend event with cancelable=false, for example because scrolling is in progress and cannot be interrupted."

Hope this helps to further investigate the issue.

Looking into this now at dcon. Problem is that touchend always triggers click, but we should probably check if touchstart and touchend are on the same element and only then trigger click.

Further looking at the issue and ToolbarVisualView.js, the problem is caused by the fix introduced in https://www.drupal.org/node/2175637#comment-8495035

      return {
        'click .toolbar-bar .toolbar-tab': 'onTabClick',
        'click .toolbar-toggle-orientation button': 'onOrientationToggleClick',
        'touchend .toolbar-bar .toolbar-tab': touchEndToClick,
        'touchend .toolbar-toggle-orientation button': touchEndToClick
      };

The event mapping sets onTabClick and touchEndToClick to trigger on every child element of .toolbar-tab. For example these events are triggered when you click on an empty area of the toolbar. Thus touchEndToClick is triggered when you start a touch event over a menu item link, calling event.preventDefault(); and event.target.click();

My proposed solution is to extend the element selector with an additional .trigger to limit triggering these event only to the tab buttons:

      return {
        'click .toolbar-bar .toolbar-tab .trigger': 'onTabClick',
        'click .toolbar-toggle-orientation button': 'onOrientationToggleClick',
        'touchend .toolbar-bar .toolbar-tab .trigger': touchEndToClick,
        'touchend .toolbar-toggle-orientation button': touchEndToClick
      };
    },

I have tested my patch on iPad and desktop Chrome, looks OK to me.

Read the other issue, don't get why we bind touchstart/end. May have made sense before but right now not binding them works fine. Also the touchstart/end events binding are the reason why contextual links don't work on mobile.

That patch fixes the issue, that's what we get for trying to be smarter than browsers.

( edit ) oh, it's an istuff issue. Anyone with a recent apple phone can test this patch?

Sorry the patch #11 is not working on iOS.
Tested on iPad (2nd generation, iOS 8): sometimes - not always - I have to tap on a menu item twice to get it activated.

Re-adding patch - previously was exported in an invalid format sorry - because I think it's still solves the problem, also solves the contextual links - except on Android 4 native browser.

So testing this patch:

Menu scroll:
iPad iOS 8: OK
iPhone iOS 8: OK
Sony Xperia Ray Android 4 native browser: OK
Sony Xperia Ray Android 4 Chrome: OK
Desktop Chrome (OSX): OK
Desktop Firefox (OSX): OK

Contextual links:
iPad iOS 8: OK
iPhone iOS 8: OK
Sony Xperia Ray Android 4 native browser: Buggy: tapping on the edit icon cause to show and immediately hide the contextual icons. Need tapping+holding to work.
Sony Xperia Ray Android 4 Chrome: OK
Desktop Chrome (OSX): OK
Desktop Firefox (OSX): OK

So this patch may not ready yet but maybe help identify what's going on.

I played with this for a couple of hours and I found that the problem #11 patch causes (have to double-tap on menu items) exists elsewhere too: using Bartik, I have to double-tap on the secondary links. The issue is with secondary-menu.css, using a:hover - will report it as a separate issue after further investigation.
So looks like the toolbar menu css causes this double-tap behavior. Will investigate it further.

I did manual testing on iOS and Android devices. I couldn't test on 4.0 native Android browser. But I think that it's better if its working on the major, newer devices but not working on some old Android browsers, then not working on any of them.
Please review this patch.

Update version to 8.1.x-dev

Fix works, tested on iPhone. Current Android version is 5.x so not a big problem that it has issues on 4.x

More importantly the menu hard to use on iPhones as is.

Android native browser on 4.x has pretty low usage now according to caniuse.com via Statcounter. Looks like 4.4+ ships with Chrome instead of the native browser too.

From my testing, this seems to improve the native browser behaviour and certainly doesn't regress it. I'd like to commit this but want to do a bit more testing and just getting off the train. Will return to this! :)

Committed and pushed 85aea4e to 8.3.x and 1fc59bc to 8.2.x and 8b401c2 to 8.1.x. Thanks!

This is high impact and minimal risk for disruption so it can go into the 8.2.x beta.

I tested HEAD following this commit -- this is so much better! Thank you.

Needed testing `touchend`, I think this is unnecessary, we can remove it.

mobile browsers like iOS Safari has no responses on DIV so that above code added simulate function to touch events.

Also,

Strictly, this is an API change than bug fixing. (I thought event system is API in JS.)
can it land in 8.1.x - 8.2.x?

.trigger is not the mandatory class for toolbar items before patch. Any custom toolbar from other modules would be broken.

Maybe create a change notification?

@droplet thanks, retroactively adding missing JS tag!

I'm not clear on what is meant in #32 for the followup.

As for #33, my assumption (and I don't think this is documented anywhere) was that changing a backbone view in this way was not API breaking. Can you elaborate on how the event mapping would be used as API?

As for .trigger that is a part of the ToolbarItem element which contains that structure, so it doesn't seem like a risk at all to me but I may be missing something.

@Cottser,

In Drupal we don't have a solid pattern lib and example like Bootstrap and we don't have a toolbar-item API to add new buttons. Any custom module able to adding buttons in following way in old day and it should work:
<div class="toolbar-tab">Module X</div>

Or say, we may register a different event handler to replace 'click .toolbar-bar .toolbar-tab': 'onTabClick',, after change, the custom handler may broken.

another example,
https://www.drupal.org/project/toolbar_anti_flicker

Even said it hacked the Core Toolbar hardly, if all core modules updates, this module will not work as expected.

@droplet: The API to add toolbar buttons (they're actually "tabs") is not in JS, it's hook_toolbar().

PHP API + JS API is a combination to get the Toolbar works.

It's a long time after Drupal 8 Releases, someone may code their workaround. I think this patch still okay for D8.2 but not so suitable for D8.1

--------

For example, someone also developed a outside_in module and have similar coding:
#2782899: Outside In is broken on iOS

-      $('.contextual-toolbar-tab.toolbar-tab button').once('toggle-edit-mode').on('click', function () {
+      $('.contextual-toolbar-tab.toolbar-tab button').once('toggle-edit-mode').on('touchstart click', function () {

This part of code actually triggered the event handler twice. After this commit, it may trigger triple times, or still twice. Or once but now trigger TWO different handler. I don't know but anything is possible.

Unfortunately, if someone also developed another Turbolinks-like module or using React doing frontend rendering. Or they may also attach the same Event handler to ".trigger". Now the new handler from current patch will be replaced the contributed one..etc

--------

We able to change code from: (Not a suggestion to current patch. It's bad)

<div class="toolbar-tab"><a class="trigger">TEXT</a></div>

TO:

<div class="toolbar-tab"><a class="toolbar-tab trigger">TEXT</a></div>

But not point Event to another Target: .trigger.

--------

Let's thinking the same logic in PHP side,

If our node path pointed to node/toolbar-tab, you now change path to node/trigger. All contributed code pointed to node/toolbar-tab will stop working immediately. Or say, we changed any public "Symfony EventDispatcher" in Drupal Core.

We also won't allow this less harmful patch get in: #1663622: Change directory structure for JavaScript files

--------

We able to extend JS events. In another side, we modify the hook_toolbar also.
Someone keen on CLEAN CODING. They will simplify this awful code:

class="toolbar-icon toolbar-icon-menu trigger toolbar-item is-active"

TO

class="toolbar-item"

In this case, picky coder still coding CORE Compatible Code for CORE API & Patterns. However, the CORE change the default behaviors.

--------

not trying to be picky but it really has the difference, hehe.

It's why I'm keen on the API design pattern, we can't go back usually.
https://www.drupal.org/node/2786459#comment-11551215

I'd be fine with reverting this from 8.1.x if you think that's the way to go @droplet. But I think the breaking scenarios aren't very likely or are not possible, for example I don't think you could do anything in hook_toolbar() to break this.

I'll leave the maintainers to make the decision. It seems the Core Policy allowed to do everything case by case. I just trying to tell the problem that will happen. Most of my sites are benefited from this changes really :) And I agreed this is the simplest way to fix this bug.

for example I don't think you could do anything in hook_toolbar() to break this.

Why? Is it because `hook_toolbar` calling `Drupal\toolbar\Element\ToolbarItem` and we counted `Drupal\toolbar\Element\ToolbarItem` as private API? Hmm. It makes sense to me if I think the PHP alone.

Well, give a simple example:

      jQuery('#toolbar-administration').on('click', '.toolbar-bar .toolbar-tab .trigger', function (e) {
          e.preventDefault();
          jQuery('.toolbar-tray.is-active').html('IT WILL SHOWN ON TOOLBAR. DO NOT COLLAPSED');
          e.stopPropagation();
        });

Trying to click the same toolbar few times.

Before Patch: NOT COLLAPSED
After Patch: COLLAPSED

Behind the scene, the bubbling: http://javascript.info/tutorial/bubbling-and-capturing

I will be surprised if we won't count it as a problem.

Reverted from 8.1.x since it sounds like there is a small amount of risk there. If people do have custom and conflicting things going on they can spot them when upgrading to 8.2.x. I would have preferred this fix make it into 8.1.x but 8.2.x is around the corner.

Thanks for the feedback @droplet. Do we still need a followup for #32?

@Cottser,

Do we still need a followup for #32?

Leave as it for now. I will post it around 8.4.x LTS version when the iOS 8/9 usage down to ZERO.

I attached a patch for 8.1.x