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)
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)
289.79 KB,
image/gif
|
Details | |
46 bytes,
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
|
Details | Review |
1.63 KB,
patch
|
birtles
:
review+
pascalc
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
[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.
Reporter | ||
Comment 1•6 years ago
|
||
Last good revision: 84450fef88b2fa9c6696bf26cc5c211a0ce98b97 First bad revision: a1c1b16370154dfd42563f057fb722b37aa34112 Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=84450fef88b2fa9c6696bf26cc5c211a0ce98b97&tochange=a1c1b16370154dfd42563f057fb722b37aa34112
Blocks: 1473209
Keywords: regressionwindow-wanted
Comment 2•6 years ago
|
||
Mantaroh, can you please have a look at this? Thanks.
Flags: needinfo?(mantaroh)
Assignee | ||
Comment 3•6 years ago
|
||
(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.
Assignee | ||
Comment 4•6 years ago
|
||
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.
Comment 5•6 years ago
|
||
> 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.
Assignee | ||
Comment 6•6 years ago
|
||
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
Assignee | ||
Comment 7•6 years ago
|
||
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)
Comment 8•6 years ago
|
||
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.
tracking-firefox63:
--- → +
Assignee | ||
Comment 9•6 years ago
|
||
(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.
Comment 10•6 years ago
|
||
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)
Comment 11•6 years ago
|
||
FYI, this has been identified as a potential blocker for the feature in 63.
Flags: needinfo?(pbrosset)
Comment 12•6 years ago
|
||
(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.
Assignee | ||
Comment 13•6 years ago
|
||
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.
Assignee | ||
Comment 14•6 years ago
|
||
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)
Reporter | ||
Comment 15•6 years ago
|
||
This issue is not reproducible using the provided try build (comment 14) on Windows 10 64bit (Dell XPS 12).
Comment 16•6 years ago
|
||
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)
Assignee | ||
Comment 17•6 years ago
|
||
Emil, Thank you for the your confirmation. I'll request the reveiw after brushing up the patch.
Comment 18•6 years ago
|
||
(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)
Comment 19•6 years ago
|
||
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)
Assignee | ||
Comment 20•6 years ago
|
||
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.
Comment 21•6 years ago
|
||
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
Comment 22•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f34e414cd0aa
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Comment 23•6 years ago
|
||
Mantaroh, could you request beta uplift please? Thanks
Flags: needinfo?(mantaroh)
Assignee | ||
Comment 24•6 years ago
|
||
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 25•6 years ago
|
||
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+
Updated•6 years ago
|
Flags: qe-verify+
Reporter | ||
Comment 26•6 years ago
|
||
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.
Comment 27•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/f997cf948ca0
Reporter | ||
Comment 28•6 years ago
|
||
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)
Assignee | ||
Comment 29•6 years ago
|
||
(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.
Assignee | ||
Comment 30•6 years ago
|
||
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
Assignee | ||
Comment 31•6 years ago
|
||
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)
Assignee | ||
Comment 32•6 years ago
|
||
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)
Updated•6 years ago
|
Attachment #9016153 -
Flags: review?(bbirtles) → review+
Assignee | ||
Comment 33•6 years ago
|
||
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 34•6 years ago
|
||
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+
Comment 35•6 years ago
|
||
Previous uplift for part1 means this wasn't on anyone's radar for uplift.
Comment 36•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-release/rev/bcfe1d1d13b5 https://hg.mozilla.org/releases/mozilla-beta/rev/e19d6a029c18b1b36bc66522cfac6590ff844d62 (FIREFOX_63b_RELBRANCH)
Reporter | ||
Comment 37•6 years ago
|
||
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.
Description
•