Page MenuHomePhabricator

Mouse cannot expand/contract headers
Closed, ResolvedPublic

Description

Author: chutten

Description:
When visiting Wikipedia articles with expandable sections, mouse events (from a bluetooth mouse or using a device that supports mouse-like interactions like the BlackBerry 9900) do not expand or contract the sections.

Checking the event listeners, there appears to be a key listener for spacebar and a listener for jquery's "tap" event. None for DOM Events Level 0's "click".

Can be reproduced using a BlackBerry device with a pointer, a bluetooth-mouse-equipped smartphone, or Google Chrome's Developer Tools' mobile emulator with touch screen emulation turned off (on the console under Emulation>Sensors).


Version: unspecified
Severity: normal
Platform: Smartphone

Details

Reference
bz72566

Event Timeline

bzimport raised the priority of this task from to Needs Triage.Nov 22 2014, 3:53 AM
bzimport set Reference to bz72566.
bzimport added a subscriber: Unknown Object (MLST).

bingle-admin wrote:

Prioritization and scheduling of this bug is tracked on Trello card https://trello.com/c/iJcO2LLD

Hi Chris H-C thanks for the heads up. I'll look to get this fixed asap. I can't replicate with Chrome developer tools but I will take a look on a Blackberry device.

FYI, I notice the @blackberry email address - it would be great to have you as one of our regular beta testers at http://en.m.wikipedia.org/wiki/Special:MobileOptions - Blackberry doesn't get that many bug reports around our features, so things like this slip through the net. We tend to put features like this in their for at least a month (this one was actually a year :-)) - all you have to do is opt in and you'll be able to trial new features before they hit a large audience.

Fix soon!

chutten wrote:

You have to enable mobile emulation for viewport and user-agent string, but disable the touch screen emulation, so it's a bit tricky to finagle Chrome into place... I wasn't sure if you had BlackBerry devices kicking around to test with, though, so I tried to find cross-platform-reproducible steps.

That beta setting, looks like it's implemented using Cookies? I'll need to remember to check that every time I load a new test build. Thanks for that tip!

We have a couple but right now we are struggling to even power them on to investigate :)

chutten wrote:

Not my department :) (if you're running into a wall, let me know what you're seeing and I might be able to help. I'm Browser dev, but I've picked up a few things over the years.)

Chris, I've tried replicating the issue using Chrome and your instructions, but I'm able to successfully collapse and expand sections. What OS and Chrome version should I try? Thanks!

chutten wrote:

I was running Chrome 38 on Linux Mint 16.

Ah-ha, there's a trick!

Look for a line like "if ('ontouchstart' in window) {"

Basically, if there's touch, listen for touch. If there isn't touch, listen for mouse. Either way, consume the mouse click (if the mouse click weren't consumed, we could do something on our end).

So the trick is loading the page with touch enabled, then disable the touch screen to use mouse to click the header.

Here are full instructions:

Start a new incognito session, start inspector. Click on the little device icon second from the top-left. Your view will change into mobile mode.

Under Device select BlackBerry Z30 (not sure whether this matters, but it's what I've reproduced using).

Load any article page. The touch screen should be emulated at this time.

Click on "Show Drawer" (fourth from the top-right of the inspector) then select "Emulation" tab. Select "Sensors". Uncheck "Emulate touch screen".

Click a header in the page.

We do fall back to click. I'm pretty sure this was working in desktop IE8 which is what I find so strange. Will take a closer look today.

I'd rather we used a real device to debug this one Baha so let me take a look:)

I've checked on version 4 (8900) and version 6 (9800) and am not seeing the issue. The version 4 doesn't run JavaScript from Wikipedia and the 9800 has touch support.

I can replicate this on IE8 reliably though so I guess the fix there will help this!

Seems related to window.addEventListener on IE8 (guess we need to use attachEvent).
Does BlackBerry 9900 support window.addEventListener ? Is it the same problem? I don't know much about the browser this runs...

gerritadmin wrote:

Change 169553 had a related patch set uploaded by Jdlrobson:
WIP: Get tap event to work without window.addEventListener

https://gerrit.wikimedia.org/r/169553

chutten wrote:

Since BlackBerry 6, BlackBerry devices ran a version of WebKit. addEventListener's just fine.

The problem is the touch detector. I don't have the source, but Chrome prettifies it fairly well for me. It looks something like:

(function($) {
    var $window = $(window), moved, tapEv;
    function handleTap(ev) {
        tapEv = $.Event('tap');
        if (!moved)
            $(ev.target).trigger(tapEv);
    }
    window.addEventListener('click', function(ev) {
        if (tapEv && tapEv.isDefaultPrevented()) {
            ev.stopPropagation();
            ev.preventDefault();
        }
    }, true);
    if ('ontouchstart' in window) {
        $window.on('touchstart', function(ev) {
            moved = false;
        }).on('touchmove', function() {
            moved = true;
        }).on('touchend', handleTap);
    } else {
        window.addEventListener('mouseup', handleTap, true);
    }
}(jQuery));

We have both touch and mouse. So the flow is thus: the page loads with ontouchstart being defined on window. So click, touchstart, touchmove, and touchend are listening on window.

We click a header. mousdown, mouseup, click on the header node. The click is captured, but ignored.

That's it.

Generally speaking, we try to convert mouse to touch when the mouse event isn't handled, but only if there is a touch event listener on the node under the cursor. Which there isn't.

I must confess I'm baffled at the window-wide listeners for something that 'click' provides much more efficiently. But maybe there's something IE-like that makes this necessary.

gerritadmin wrote:

Change 169569 had a related patch set uploaded by Jdlrobson:
Restore the place on Nearby for back button

https://gerrit.wikimedia.org/r/169569

Mm.. this leaves me even more confused. I followed your steps completely in the emulator and am unable to replicate it there.

According to the code it should fallback to mousedown if 'ontouchstart' in window is not true. To be clear you are saying that on this Blackberry device 'ontouchstart' in window is true even though touch events are enabled and you are seeing this issue on a physical device? I am not noticing an issue on a WebKit Blackberry Torch is this too modern?

Sorry for all the questions, I'm just a little confused with what's going on here.

FYI the code that we are running is here https://github.com/jgonera/micro.js

chutten wrote:

That's really odd. The code is quite explicit: if the touch screen is on (window.ontouchstart is truthy) when it's registering listeners, it'll forego adding a mouseup listener and instead register for the three touch events (start, move, end).

If a user then tries to use a pointer device to interact with the code, it'll not "tap".

The in-market device I'm using is a little newer than your Torch (it is a BlackBerry Bold 9900 running BlackBerry OS 7.1), but I think the mouse events from the trackpad should be similar enough.

When you follow my steps using Chrome, hit the pause button in the Scripts window before you click a header. Which listener in micro.tap.js are you triggering? I'm getting the click listener (micro.tap.js:16). Are you getting mouseup? That listener isn't registered if the touch screen emulation is on when you load the page.

Okay yeh I think I see what's going on. The Torch is touch screen only - if I understand correctly 9900 supports both touch and mousepad and it's obvious the code hasn't thought about this (I can also now replicate in the emulator I think I just forgot to refresh).

Sorry to have made this more complicated then it needed to be :)

gerritadmin wrote:

Change 169581 had a related patch set uploaded by Jdlrobson:
Trigger tap events on phones where both touch and mouse are supported

https://gerrit.wikimedia.org/r/169581

The above patch fixes it for Blackberry (I think) but breaks it for iOS. I'm actually going to suggest we kill this code. ios8 [1] has no need for it, neither does Chrome [2]

[1] https://github.com/ftlabs/fastclick/issues/262#issuecomment-56023175
[2] http://updates.html5rocks.com/2013/12/300ms-tap-delay-gone-away

gerritadmin wrote:

Change 169582 had a related patch set uploaded by Jdlrobson:
Tap code be gone!

https://gerrit.wikimedia.org/r/169582

gerritadmin wrote:

Change 169581 abandoned by Jdlrobson:
Trigger tap events on phones where both touch and mouse are supported

Reason:
Use https://gerrit.wikimedia.org/r/169582 instead

https://gerrit.wikimedia.org/r/169581

gerritadmin wrote:

Change 169582 merged by jenkins-bot:
Tap code be gone!

https://gerrit.wikimedia.org/r/169582

chutten wrote:

Yay!

So.. when will I see this in "Experimental" or "Beta" (or in the normal, every-day view)?

Or, more directly, when/where/how will I be able to test it?

Hi Chris, can you verify this works on http://en.m.wikipedia.beta.wmflabs.org/wiki/Main_Page ? Sadly I was not able to find a physical advice or emulator to verify this. Wikimedia definitely needs to expand its BlackBerry test device list!

chutten wrote:

Well, the Random button doesn't work. (the beta slide out menu works better with mouse, though, than the public one.)

I eventually found an article page (Wikipedia in culture) whose headers now work flawlessly with the pointer interaction.

Oddly, it's now experiencing the click touch-to-mouse conversion delay when you use touch interaction.

Oh, you don't have a width=device-width in your viewport. Why not? That's the switch that turns off the delay (as per http://updates.html5rocks.com/2013/12/300ms-tap-delay-gone-away )

Mmm I could have sworn we had width=device-width I will have to look at the git history and see where and why that went.

Not sure why random wouldn't work. I'll take another look at code today to see if I missed something.

I've raised bug 72820. No idea why it's not set. Never has been apparently :-S

chutten wrote:

I don't know your integration schedule or policy... could you give me an idea when we'll start seeing this patch in Beta or on the public site?