Closed Bug 1398196 Opened 2 years ago Closed 2 years ago

On MacBook, on Nightly, "Add to Firefox" button doesn't detect trackpad tap, only click

Categories

(Core :: User events and focus handling, defect)

57 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- wontfix
firefox56 - wontfix
firefox57 + fixed

People

(Reporter: haik, Assigned: smaug)

References

Details

(Keywords: regression)

Attachments

(1 file)

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.
Blocks: 1369140
Flags: needinfo?(mconley)
Flags: needinfo?(ehsan)
Flags: needinfo?(bzbarsky)
Keywords: regression
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.
Flags: needinfo?(bzbarsky) → needinfo?(bugs)
Thank you for taking a first look at this, Boris! Moving to Core:DOM to match the component of bug 1369140.
Component: Widget: Cocoa → DOM
Component: DOM → DOM: Events
I don't think I have much of value to add here, sorry!
Flags: needinfo?(mconley)
So we do flush already when handling mouseup, but that just happens after hit testing.

I wonder where to flush...
Component: DOM: Events → Event Handling
Flags: needinfo?(bugs)
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?
Flags: needinfo?(bzbarsky)
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.
Flags: needinfo?(bzbarsky)
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.
Flags: needinfo?(bzbarsky)
thanks bz and smaug for covering while I was out!
Flags: needinfo?(ehsan)
Attachment #8907555 - Flags: review+
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3820db371ae
flush layout before hittesting when handling mousedown/up, r=ehsan
Assignee: nobody → bugs
Flags: needinfo?(bzbarsky)
https://hg.mozilla.org/mozilla-central/rev/b3820db371ae
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
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.
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.