Closed Bug 1493659 Opened 6 years ago Closed 6 years ago

touchscreen: The "meatball" and "chevron" menus break after being double tapped

Categories

(DevTools :: General, defect)

x86_64
Windows 10
defect
Not set
normal

Tracking

(firefox-esr60 unaffected, firefox62 unaffected, firefox63+ verified, firefox64 verified)

VERIFIED FIXED
Firefox 64
Tracking Status
firefox-esr60 --- unaffected
firefox62 --- unaffected
firefox63 + verified
firefox64 --- verified

People

(Reporter: emilghitta, Assigned: mantaroh)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

Attached image issue.gif
[Affected versions]:
Firefox 64.0a1 (BuildId:20180924100354).
Firefox 63.0b8 (BuildId:20180920135444).

[Unaffected versions]:
Firefox 62.0.2 (BuildId:20180920131237).
Firefox 60.2.1esr (BuildId:20180920175354).

[Affected platforms]:
Windows 10 64bit (Dell XPS 12)

[Steps to reproduce]:
1. Launch Firefox.
2. Open the developer tools.
3. Tap the "meatball" or "chevron" menu button.
4. Tap the "meatball" or "chevron" menu button again.

[Expected result]:
The "meatball" or "chevron" menu opens successfully.

[Actual result]:
It seems that the "meatball" or the "chevron" menu no longer opens (via touch or click). In order to open the meatball menu the user has to reopen the Developer tools.

[Regression range]

This seems to be a regression.

I will get back with a regression range as soon as time permits.

[Notes]
For further information regarding this issue please observe the attached screencast.
Last good revision: 84450fef88b2fa9c6696bf26cc5c211a0ce98b97
First bad revision: a1c1b16370154dfd42563f057fb722b37aa34112

Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=84450fef88b2fa9c6696bf26cc5c211a0ce98b97&tochange=a1c1b16370154dfd42563f057fb722b37aa34112
Mantaroh, can you please have a look at this? Thanks.
Flags: needinfo?(mantaroh)
(In reply to Brian Birtles (:birtles) from comment #2)
> Mantaroh, can you please have a look at this? Thanks.

Sure. I'll look into this.
This seems like that the XUL popup doesn't eat the click event when using the screen touch. I'm not sure why XUL doesn't eat the click event in this case in spite of the 'consumeoutsideclick' is false. However, MenuButton will call the Panel.openPopupAtScreen again, then this popup will not open and HTMLTooltip waits for popupShown forever.

Mechanism of this phenomenon:
1) Open popup by touching the menu and the 'pointer-events' of button will be 'none'. (i.e., Button prohibits the mouse click)
2) Panel hide the popup by touching the menu again. (This is the XUL Popup behavior).
3) The 'popuphidden' event is occurred, then MenuButton will set 'pointer-events' to 'auto'. (i.e., Button allow the mouse click)
4) The 'click' event is occurred in spite of specifying the 'consumeoutsideclicks' to false.
5) HTMLTooltip call the Panel.openPopupAtScree, then popup doesn't show.
> I'm not sure why XUL doesn't eat the click event in this case in spite of the 'consumeoutsideclick' is false.

That would probably be worth investigating.
OK. I'll continue to investigate this further.

I can reproduce this on Linux environment. (The behavior is different from this report, but the bug mechanism is same.)

Note to self:
We should set the "MOZ_USE_XINPUT2" environment variable to "1", to enable touch event on Linux environment.
Assignee: nobody → mantaroh
It seems to me that both of  the click and touch will occur the mouse click event internally, however, EventStateManager skip the EventStateManager::InitAndDispatchClickEvent() due to mClickCount is zero in the case of clicking the button.[1]  But the touch event is different, the mClickCount is "1". I need to investigate this reason furthermore..

masayuki,
I have a question about this touch event. It seems to me that the "mousedown" event and the "mouseup" event is happened if touch the button. Is this correct behavior? (I complete thought that the mouse event will happen in the case of mouse operation)t


[1] https://searchfox.org/mozilla-central/rev/6d1ab84b4b39fbfb9505d4399857239bc15202ef/dom/events/EventStateManager.cpp#5002
Flags: needinfo?(masayuki)
Tracking for 63 as per the Beta Preliminary status report from QA which identified fixing and uplifting this bug as important for the readiness of the feature for release.
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #6)
> OK. I'll continue to investigate this further.
> 
> I can reproduce this on Linux environment. (The behavior is different from
> this report, but the bug mechanism is same.)
> 
> Note to self:
> We should set the "MOZ_USE_XINPUT2" environment variable to "1", to enable
> touch event on Linux environment.

Note to self, again:
We can reproduce this phenomenon in the hamburger menu and bookmark doorhanger menu as well. I added the logging into EventStateManager. I think that this hamburger menu phenomenon and the devtools doorhanger menu phenomenon is the same reason.
I don't know well around touch events implementation though. But I believe that it's expected behavior especially for backward compatibility.

On Windows, only 2 finger tap is handled with WidgetSimpleGesture event here:
https://searchfox.org/mozilla-central/rev/819cd31a93fd50b7167979607371878c4d6f18e8/widget/windows/nsWinGesture.cpp#228-239
Otherwise, the gesture message is ignored (DefaultWndProc of Windows will handle it). Then, probably, Windows generates mouse messages for backward compatibility.

If so, each mouse button message calls nsWindow::DispatchMouseEvent():
https://searchfox.org/mozilla-central/rev/819cd31a93fd50b7167979607371878c4d6f18e8/widget/windows/nsWindow.cpp#4381
and it initializes mClickCount.

Although at least around these links, I don't see any difference between tap and mouse click handling.

"click" event should be fired only when both mousedown and mouseup events are fired in same element (even if on different text node). So, it *might* be that the event target computation is different between tap and mouse button.
Flags: needinfo?(masayuki)
FYI, this has been identified as a potential blocker for the feature in 63.
Flags: needinfo?(pbrosset)
(In reply to David Durst [:ddurst] (Regression Engineering Owner for 63) from comment #11)
> FYI, this has been identified as a potential blocker for the feature in 63.

Out of curiosity as much as anything, who identified it as a potential blocker? QA agreed this was not a blocker (and we couldn't back out the feature in 63 even if we wanted to) so I'm curious to understand how the process works.

In any case, Mantaroh is working on this as his top priority. We discussed strategies yesterday and I expect he will come up with a patch today or tomorrow.
I think we need to consider the case of long tap the button. Usually, the touch event is as follow:

 1. touchstart
 2. touchend
 3. mouseup
 4. mousedown
 5. click

We can distinguish whether should we prevent click or not in the touchstart event handler, then we will skip the click event by using the stored this information. 
But, If we tap the button longer, touch event will happen the touchstart event only. (i.e., the click event doesn't happen) We need to clear the stored information of touchevent in this case.

This long tap will happen when touching longer than preference value. So the MenuButton component might need to have the timer to clear the stored information.
As discussed with Birtles, we distinguish whether is the long tap or not by using touchmove event.
If we receive the touchmove event, menu button behaves as long tap. Otherwise, behave as the tap.

This patch will make the click event handler to be early returned if component's state has the prevent flag. This flag will be set in the touch end event handler. This touchend event handler will be registered when the window receives the touchstart event.

Try:
https://hg.mozilla.org/try/rev/d7fc384e52d31c5ded51f4c3cdb5897e807abb13
Flags: needinfo?(mantaroh)
This issue is not reproducible using the provided try build (comment 14) on Windows 10 64bit (Dell XPS 12).
I'll clear my needinfo request since Brian and Mantaroh got this.
Also, based on the QA status described by Brian in comment 12, I think we should mark firefox63 as fix-optional.
Flags: needinfo?(pbrosset)
Emil, Thank you for the your confirmation.

I'll request the reveiw after brushing up the patch.
(In reply to Brian Birtles (:birtles) from comment #12)
> (In reply to David Durst [:ddurst] (Regression Engineering Owner for 63)
> from comment #11)
> > FYI, this has been identified as a potential blocker for the feature in 63.
> 
> Out of curiosity as much as anything, who identified it as a potential
> blocker? QA agreed this was not a blocker (and we couldn't back out the
> feature in 63 even if we wanted to) so I'm curious to understand how the
> process works.

Relman mentioned in yesterday's regression triage that this was being discussed as a potential blocker. I'll leave the status for 63 to him.
Flags: needinfo?(pascalc)
To clarify, this is not a blocker for the feature or the release but given that there is a patch in the works and we have 2 more betas next week before entering RC week, I am leaving the flag as affected until mid-next week to keep it on relman radar.
Flags: needinfo?(pascalc)
Tapping the menu button during the panel has shown make the menu button to be unopenable. This cause reason is removing the pointer-events property before the mouse event happens.
The tapping event will happen many the event like the following:
1) touchstart
2) touchmove (Long tap only)
3) touchend
4) mousedown
5) mouseup
6) click

This patch will detect the touchend event to eat the click event.
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f34e414cd0aa
Skip the click event handler if touch event has occurred. r=birtles
https://hg.mozilla.org/mozilla-central/rev/f34e414cd0aa
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Mantaroh, could you request beta uplift please? Thanks
Flags: needinfo?(mantaroh)
Comment on attachment 9014652 [details]
Bug 1493659 - Skip the click event handler if touch event has occurred. r?birtles

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1493659

User impact if declined: If the user touches the meatball menu, this popup panel doesn't be hidden.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: Yes

If yes, steps to reproduce: Same to RTS.

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This patch affects the limited area and the touch event only. The touch event is enabled on Windows platform.

String changes made/needed: N/A
Flags: needinfo?(mantaroh)
Attachment #9014652 - Flags: approval-mozilla-beta?
Comment on attachment 9014652 [details]
Bug 1493659 - Skip the click event handler if touch event has occurred. r?birtles

Uplift approved for 63 beta 14, thanks!
Attachment #9014652 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
This issue is verified fixed using Firefox 64.0a1 (BuildId:20181009100040) on Windows 10 64bit (Dell XPS 12).

Leaving a ni? on myself as a reminder to verify this in 63 beta 14 as well.
Status: RESOLVED → VERIFIED
Flags: needinfo?(emil.ghitta)
Hi Mantaroh,

It seems that this issue is still reproducible using the treeherder build provided in Comment 27. 

Can you please have a look?
Flags: needinfo?(emil.ghitta) → needinfo?(mantaroh)
(In reply to Emil Ghitta, QA [:emilghitta] from comment #28)
> Hi Mantaroh,
> 
> It seems that this issue is still reproducible using the treeherder build
> provided in Comment 27. 
> 
> Can you please have a look?

Thanks, Emil.

The Nightly works well. However, beta doesn't work this patch...
In any case, I'll look into this phenomenon on beta.
Ah, beta doesn't contain the creating reference changes of bug 1494162.[1]
So we need to use buttonRef.getClientRects instead of buttonRef.current.getClientRects in beta.

[1] https://hg.mozilla.org/mozilla-central/rev/f12b32a5bb3c#l3.49
Attached patch bug1493659.part2.patch (obsolete) — Splinter Review
Brian,

This patch will fix the problem of beta. The beta doesn't contain the changes of bug 1494162. So this patch will use buttonRef.getClientRects instead of buttonRef.current.getClientRects.

Could you please review it?
Flags: needinfo?(mantaroh)
Attachment #9016151 - Flags: review?(bbirtles)
Sorry, the previous patch contains the unnecessary space. I removed it.
Attachment #9016151 - Attachment is obsolete: true
Attachment #9016151 - Flags: review?(bbirtles)
Attachment #9016153 - Flags: review?(bbirtles)
Attachment #9016153 - Flags: review?(bbirtles) → review+
Comment on attachment 9016153 [details] [diff] [review]
Bug1493659.part2.patch

Thanks, Brian.

This patch is same to the approval comment 24.

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: 

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: Yes

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): 

String changes made/needed:
Attachment #9016153 - Flags: approval-mozilla-beta?
Comment on attachment 9016153 [details] [diff] [review]
Bug1493659.part2.patch

Uplift approved for our last beta, thanks!
Attachment #9016153 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Previous uplift for part1 means this wasn't on anyone's radar for uplift.
This issue is verified fixed using Firefox 63.0 (BuildId:20181015152800) on Windows 10 64bit (Dell XPS 12).
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: