There is a few different things that looks broken with APZC. One is related to the fact that we don't use the Kinetic helper and so it never reset it's values and it ends up clearing the highlight instantly. The other is related to how we dispatch the mouse events right after a touchend. This does not let the time for the content to highlight itself.

Comment on attachment 8375185 [details] [diff] [review] highlight.fixes.patch Review of attachment 8375185 [details] [diff] [review]: ----------------------------------------------------------------- I'm not too familiar with the BEP.js changes so you might want another reviewer for that. The changes look sane to me though. Some comments on TabChild changes - I'd like to see an updated version of the patch before r+'ing. ::: dom/ipc/TabChild.cpp @@ +292,5 @@ > , mUpdateHitRegion(false) > , mContextMenuHandled(false) > , mWaitingTouchListeners(false) > { > + Preferences::AddIntVarCache(&sActiveDurationMs, This uses sActiveDurationMs instead of mActiveDurationMs. I think I would prefer making it static, in case TabChild instances can be destroyed on non-B2G platforms. We don't want to leave a instance variable from a destroyed object in the IntVarCache. Alternatively, if there's some way to clear the intvarcache in the TabChild destructor that's fine too. @@ +1628,3 @@ > > + mSingleTapTimer = NewRunnableMethod(this, > + &TabChild::FireSingleTapEvent); You can pass currentPoint as an additional argument to NewRunnableMethod, so that you don't need mSingleTapPoint at all. See for instance the NewRunnableMethod invocation in AsyncPanZoomController::OnSingleTapUp. Also, mSingleTapTimer doesn't look needed; it's assigned here but never cleared which seems like an unncessary footgun. Just use a local var instead.

Thanks for the quick feedback. I will addressed your comments and ask to fabrice for the related BEC changes. I'm pretty sure he will be more than happy to kill the event loop spinning code.

Patch v2.

And voila the v3!

Comment on attachment 8375829 [details] [diff] [review] highlight.fixes.patch Review of attachment 8375829 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, thanks!

nominating per https://bugzilla.mozilla.org/show_bug.cgi?id=975923#c30

When can we land it on v1.3T?

1.3T+ James, can your team uplift this to resolve Bug 975923? thanks

(In reply to Joe Cheng [:jcheng] from comment #12) > 1.3T+ > James, can your team uplift this to resolve Bug 975923? thanks Ying will.

patch for 1.3t, there are some conflicts and I fixed them Please review it again

Fabrice - Did you intend this blocking decision to be 1.3+ or 1.3T+?

Comment on attachment 8394685 [details] [diff] [review] 972081_v1.3t.patch Review of attachment 8394685 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Ying!

Verified fixed against the original STR in 975923.

...although we should probably do a testpass around APZC to verify this functionality on Tarako, as it's blocking us there. Let's investigate this some more.

QA Wanted isn't the right flag here - this patch is already landed here.