With Nightly (2017-09-08), on a MacBook, the "Add to Firefox" button for installing an addon doesn't detect a trackpad tap. It only detects the physical trackpad click. "Tap" refers to the "Tap to Click" setting in the macOS trackpad preferences. What happened? Taps on the button are ignored. What did you expect to happen? Tapping on the button should trigger the addon to be installed like a standard click does. Anything else we should know? This happens for me for any addon. An example is https://addons.mozilla.org/en-US/firefox/addon/tab-counter-webext/ See also: https://github.com/mozilla/addons/issues/431

I ran mozregression several times because the result seemed suspicious, but it consistently landed on bug 1369140 as the culprit. 10:45.24 INFO: No more inbound revisions, bisection finished. 10:45.24 INFO: Last good revision: 9255719d469c99b4c11cacf6541c66e353518f24 10:45.24 INFO: First bad revision: 87c1327b918d380df584858e28c23d7340c65994 10:45.24 INFO: Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9255719d469c99b4c11cacf6541c66e353518f24&tochange=87c1327b918d380df584858e28c23d7340c65994 Most everybody involved in bug 1369140 seems to be busy or away, so I'm cc-ing a few folks and hopefully someone can take a look.

So what I see with tap-to-click and tapping is that we have a mousedown on this element: <a class="button prominent platform mac add installer" data-hash="sha256:a9f1441f4f51985bdac4bab77c1303996178a042ced964d0cf7b25eb2af55473" href="/firefox/downloads/latest/tab-counter-webext/platform:3/addon-794989-latest.xpi?src=dp-btn-primary"> <b></b> <span>Add to Firefox</span> </a> Then we have a mouseup on the <p class="install-button"> that is the parent of the <a>. The mouseup and mousedown are on different elements, so there is no click event. I don't always get this; sometimes I get the mouseup on the <a> as well, and then things work. The page has a style like so: .button:active::before { content: ""; display: block; height: 2px; position: absolute; top: -2px; left: 0; width: 100%; } So when we go into mousedown that triggers and we post a restyle event. When processed, this restyle event will reframe the <a>. So I think the failure mode is this sequence of events: 1) We go into mousedown/:active, the style starts applying. 2) CheckIfFocusable is called, flushes frames (but not layout, after bug 1369140). 3) We create a new frame for the <a>, sized 0x0, don't do layout so far. 4) We do mouseup, try to determine the target, don't hit the <a> because it's 0x0, end up targeting its parent for mouseup. This used to work becaue CheckIfFocusable flushed layout for the frame as well... Normal clicks presumably work because with a normal click mousedown/up correspond to actual physical motions, as opposed to being auto-generated events. That means there's enough time between the mousedown and mouseup for a refresh tick to happen and flush out layout. The basic questions is whether mouseup targeting should be flushing layout, I guess.

Thank you for taking a first look at this, Boris! Moving to Core:DOM to match the component of bug 1369140.

I don't think I have much of value to add here, sorry!

So we do flush already when handling mouseup, but that just happens after hit testing. I wonder where to flush...

I guess I would put it to http://searchfox.org/mozilla-central/rev/51eae084534f522a502e4b808484cde8591cb502/view/nsViewManager.cpp#803 But I don't have a Mac to play with this right now, and this doesn't seem to happen on Linux. bz, any chance you could try fix this by adding a layout flush?

Bug 1369140 shipped in 55, updating status flags.

Hmm, but nsIFrame may get destroyed when flushing. I wonder how high up in the stack we should put the flush. In child process TabChild would be a good place, but in parent process somewhere in widget level... so widget level in both cases?

I guess we could flush there, assuming Views aren't used too often these days. If a view is destroyed, that is harder to detect. We don't have WeakViews anymore. So need to rely on WeakFrames.

untested patch remote: View the pushlog for these changes here: remote: https://hg.mozilla.org/try/pushloghtml?changeset=1c823cc17498573514086f7afe63eba5856de8a4 remote: remote: Follow the progress of your build on Treeherder: remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c823cc17498573514086f7afe63eba5856de8a4 remote: remote: It looks like this try push has talos jobs. Compare performance against a baseline revision: remote: https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=1c823cc17498573514086f7afe63eba5856de8a4

The "mousedown_up_flush.diff" attachment fixes the STR for me, as far as I can tell.

Hmm, tryserver didn't like the patch too much. Trying again remote: View your change here: remote: https://hg.mozilla.org/try/rev/df37bf085ff9483de7becc18392b2b898efd1b72 remote: remote: Follow the progress of your build on Treeherder: remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=df37bf085ff9483de7becc18392b2b898efd1b72 remote: remote: It looks like this try push has talos jobs. Compare performance against a baseline revision: remote: https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=df37bf085ff9483de7becc18392b2b898efd1b72

Aha, those failures are happening elsewhere too.

bz, so what do you think of the patch. Hopefully we don't end up reflowing too much more than we do currently, just do it earlier.

thanks bz and smaug for covering while I was out!

Pushed by opettay@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b3820db371ae flush layout before hittesting when handling mousedown/up, r=ehsan

As we already shipped 55 with this bug and we are pretty late in the 56 cycle, I don't think we want to uplift that to 56.