1146843 - Tapping on seek bar in videos will open the context menu

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Flaviu Cos]

Environment: 
Device: Asus Transformer Tab (Android 4.2.1);
Build: Nightly 39.0a1 (2015-03-23);

Steps to reproduce:
1. Load a video (e.g.: http://people.mozilla.com/~nhirata/html_tp/elephants-dream.webm);
2. Tap anywhere on the seek bar.

Expected result:
The video jumps to that specific time.

Actual result:
The context menu is displayed.

Comment 1

[Flaviu Cos]

Regression range:

Last good build: 2015-03-19
First bad build: 2015-03-20

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cf1060d8ce9f&tochange=4d2d97b3ba34

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Flaviu - Any chance you could get a regression range in fx-team or inbound to narrow things down?

Comment 3

[Kevin Brosnan [Ex-Mozilla]]

bug 1142336 looks intresting

Comment 4

[Flaviu Cos]

(In reply to Mark Finkle (:mfinkle) from comment #2)
> Flaviu - Any chance you could get a regression range in fx-team or inbound
> to narrow things down?

Inbound regression window:

Last good revision: 01abf38a6a4d
First bad revision: 83c5d1c58506
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=01abf38a6a4d&tochange=83c5d1c58506

Comment 5

[Kevin Brosnan [Ex-Mozilla]]

[Tracking Requested - why for this release]: regression from Bug 1144650 or Bug 1144324, both Kats ni to look at this bug.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

In bug 1144324 I removed a bunch of code including [1] which in hindsight was a little too much. However I'd rather fix this by ripping out even more code than by putting it back. This basically disables a small optimization which makes the Java pan/zoom code not wait for touch listeners (i.e. now it will always wait for initial touch event dispatch, even if there are no touch listeners on the page). I don't expect there to be much perf impact from this since in the case that there are no touch listeners dispatching a touch event will be super fast anyway, and it's a lot of code that we can remove. All this will be useless anyway once we switch to APZ since we have a different mechanism to deal with this stuff there.

[1] https://hg.mozilla.org/integration/mozilla-inbound/rev/33c30e283fa8#l3.49

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

The critical change here is setting mHoldInQueue to true instead of mWaitForTouchListeners in TouchEventHandler. All the rest is just code removal.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0626102adbe1

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8585086 [details] [diff] [review]
Patch

Review of attachment 8585086 [details] [diff] [review]:
-----------------------------------------------------------------

Kats and I talked on IRC, and my concern is that this forces us to wait on Gecko to judge every touch event. That means relying on the main thread replying in a reasonable amount of time, which will frequently be a problem. r- because of this.

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 39+ since this is a recent regression.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Attachment: Partial backout of bug 1144324 part 2

smaug's taking a break from reviews so I don't wanna ask him. It's a fairly straightforward change; partial backout of the part 2 patch from bug 1144324, just enough to get the observer notification firing again.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Try https://treeherder.mozilla.org/#/jobs?repo=try&revision=d244b7c68be6 includes this change and looks green.

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/19d96ad34466

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8587286 [details] [diff] [review]
Partial backout of bug 1144324 part 2

Approval Request Comment
[Feature/regressing bug #]: bug 1144324
[User impact if declined]: preventDefault notifications on touch events might get ignored, resulting in unexpected user-visible behavior. This bug is an example of such behavior but there are likely other effects as well.
[Describe test coverage new/current, TreeHerder]: tested locally
[Risks and why]: low risk, is a partial backout of over-aggressive code removal
[String/UUID change made/needed]: yes, had to bump NS_PIDOMWINDOW_IID

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/19d96ad34466

Comment 14

[Flaviu Cos]

Verified as fixed in build 40.0a1 2015-04-06;
Device: Asus Transformer Tab (Android 4.2.1).

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8587286 [details] [diff] [review]
Partial backout of bug 1144324 part 2

Approving for uplift to 39; sounds like we need this fix, it's been verified, and has had some time to bake on Nightly.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1574cebc9661