Closed Bug 1146843 Opened 5 years ago Closed 5 years ago

Tapping on seek bar in videos will open the context menu

Categories

(Firefox for Android :: Toolbar, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 40
Tracking Status
firefox36 --- unaffected
firefox37 --- unaffected
firefox38 --- unaffected
firefox39 + fixed
firefox40 + verified
fennec 39+ ---

People

(Reporter: cos_flaviu, Assigned: kats)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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.
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
Keywords: regression
tracking-fennec: --- → ?
Flaviu - Any chance you could get a regression range in fx-team or inbound to narrow things down?
bug 1142336 looks intresting
(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
[Tracking Requested - why for this release]: regression from Bug 1144650 or Bug 1144324, both Kats ni to look at this bug.
Assignee: nobody → bugmail.mozilla
Flags: needinfo?(bugmail.mozilla)
Blocks: 1144324
Component: Audio/Video → Graphics, Panning and Zooming
Flags: needinfo?(bugmail.mozilla)
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
Attached patch Patch (obsolete) — Splinter Review
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
Attachment #8585086 - Flags: review?(snorp)
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.
Attachment #8585086 - Flags: review?(snorp) → review-
Tracking for 39+ since this is a recent regression.
Flags: qe-verify+
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.
Attachment #8585086 - Attachment is obsolete: true
Attachment #8587286 - Flags: review?(snorp)
tracking-fennec: ? → 39+
Attachment #8587286 - Flags: review?(snorp) → review+
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
Attachment #8587286 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/19d96ad34466
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Verified as fixed in build 40.0a1 2015-04-06;
Device: Asus Transformer Tab (Android 4.2.1).
Duplicate of this bug: 1150193
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.
Attachment #8587286 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.