Closed Bug 732052 Opened 8 years ago Closed 7 years ago

XUL Scale (video scrubber) elements should support touch events

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 16
Tracking Status
firefox18 --- verified
blocking-fennec1.0 --- -
fennec + ---

People

(Reporter: xti, Assigned: wesj)

References

Details

Attachments

(4 files, 7 obsolete files)

Firefox 13.0a1 (2012-03-01)
Device: Samsung Galaxy S2
OS: Android 2.3.4

Steps to reproduce:
1. Open Fennec
2. Go to http://people.mozilla.com/~nhirata/html_tp/big_buck_bunny_480p.webm
3. Tap on screen for video controls
4. Tap on the video scrubber and drag it to a certain point

Expected result:
The scrubber will move accordingly to finger movement. When the finger is released, the video will start playing for that point forward.

Actual result:
The scrubber jumps automatically to the end of the video.
The video listed in the description is incorrectly encrypted and will not allow seeking. It doesn't work on desktop Firefox either. We should do this. Do NOT that video for testing this behavior.
That video was taken from : http://www.webmfiles.org/demo-files/

Try using : http://introducinghtml5.com/examples/ch04/ch4-full-fallback.html for testing the scrubber.

If that works then please update the litmus test with that example and mark this bug as invalid.  I thought I had, I must have missed some tests.
(In reply to Naoki Hirata :nhirata from comment #2)
> That video was taken from : http://www.webmfiles.org/demo-files/
> 
> Try using : http://introducinghtml5.com/examples/ch04/ch4-full-fallback.html
> for testing the scrubber.
> 
> If that works then please update the litmus test with that example and mark
> this bug as invalid.  I thought I had, I must have missed some tests.

Indeed, it's working this video, but Wesley suggested to keep this bug because we should add touch events for video controls. 

Wesley could you add some more details about that? Perhaps we should change the summary as well
blocking-fennec1.0: --- → ?
Flags: in-litmus?
Sure. I made it a bit more general. Video's just use a XUL scale element, and so its probably best to just add support for touch events there.

On the surface, that doesn't sound to hard, just add some handlers to the binding:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/scale.xml#187

Scale elements currently use some special paths for dragging with the mouse though, and it might be better to see if we can just use touchevents in the same place.
Summary: Dragging the video scrubber just moves it to the end of the video → XUL Scale elements should support touch events
having scrubbers work is a good thing.  Wes, is this yours to work on?
blocking-fennec1.0: ? → +
I'd like to do this, so yeah, its mine.
Assignee: nobody → wjohnston
Attached patch Patch v1 (obsolete) — Splinter Review
So there's much harder (maybe better?) ways to do this by digging into scrollable frames and stuff, but that will require bigger changes to nsPresShell to support gCapturingMouse for touch events.... I didn't want to do that. This is the easy way out.
Attachment #608440 - Flags: review?
Attachment #608440 - Flags: review? → review?(gavin.sharp)
Attachment #608441 - Flags: review?(dolske)
Comment on attachment 608440 [details] [diff] [review]
Patch v1

>diff --git a/toolkit/content/widgets/scale.xml b/toolkit/content/widgets/scale.xml

>+      <method name="moveThumbTo">

>+              percent = aY/this.boxObject.height;

I thought uses of boxObject were frowned upon now - can you use getBoundingClientRect?

>+      <handler event="touchstart" preventdefault="true">

>+          // if a second touch starts, cancel the drag
>+          if (event.touches.length > 1) {
>+            this.dragStateChanged(false);
>+          }
>+          this.dragStateChanged(true);

I don't understand this - dragStateChanged seems to only set a property that has no setter, so why bother setting it to false if you're going to unconditionally set it to true immediately after?

Neil Deakin may be a better reviewer for this.
Attachment #608440 - Flags: review?(gavin.sharp)
Comment on attachment 608440 [details] [diff] [review]
Patch v1

dragStateChanged and valueChanged are both is overridden in videocontrols.xml and pauses the video while we're seeking (and other stuff). We don't NEED it here, but I figured since it was there, we might as well use it.
Attachment #608440 - Flags: review?(enndeakin)
I'm fine with the getBoundingClientRect change too. Would rather get any other comments rather than having a bunch of iterations on the patch, but I can do that first if you want Neil.
Although I didn't test to be sure, it looks like this patch doesn't handle various things, for example, disabled scales and the movetoclick attribute. It would be better I think to handle the touch events in nsSliderFrame. The three touch events look to map almost directly to the three related mouse events except the coordinates are retrieved differently, so could the touch events not just be handled similarly to the mouse events in nsSliderFrame::HandleEvent?
I played with doing this in nsSliderFrame once, and after a bit felt like it was just growing unnecessarily complex. I'll give it a go again today and post the patch so that we can at least compare the two approaches.
Grr... I just don't know my way through this event dispatching code very well. Currently, I can pick up TOUCH_START events in nsSliderFrame (both in HandleEvent, and by registering an event listener in the SliderFrameMediator), but I don't receive TOUCH_MOVE or TOUCH_END events in HandleEvent. From some printf/gdb debugging we're entering nsEventDispatcher::HandleEventTargetChain, but never calling the callback here:

http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsEventDispatcher.cpp#394

which would then send us the events.

I'm also worried that I'm (currently) letting us use the mouse capturing code in nsPresShell, which does some magic to retarget mouse events that we probably don't want/need here (touch event's crazy targeting is actually useful in this case!). So we may have to write some trickery to step around that. Figured I'd cc Smaug in here to see if he has any strong opinions about the right way to do this. I still think xbl is a much cleaner solution.
Comment on attachment 608440 [details] [diff] [review]
Patch v1

clearing review. bnicholson i looking into the scrollframe stuff.
Attachment #608440 - Flags: review?(enndeakin)
Comment on attachment 608441 [details] [diff] [review]
Patch for videocontrols

clearing review.

I'm still not sold that doing this in C is the way to go though.
Attachment #608441 - Flags: review?(dolske)
Assignee: wjohnston → bnicholson
Summary: XUL Scale elements should support touch events → XUL Scale (video scrubber) elements should support touch events
I'm having the same problem wesj mentioned in comment 14. I've spent some time trying to figure out why the touchmove events aren't making it to nsSliderFrame, but it's been a pretty big timesink since I'm not at all familiar with this code. I'll let someone more experienced pick this up.
Assignee: bnicholson → nobody
An alternate approach would be to get rid of our <scale> usage, and switch to something more HTMLish... Like bug 726240 does with <progress>.

Looks like HTML5 has <input type="range">, but we don't support it yet (bug 344618) and I'm not sure if our special time-in-thumb UI would work with that. We could just roll our own, judging from http://frankyan.com/labs/html5slider/ it might not be all that complex.

I'd eventually like to do this anyway to stop using XUL in content, and to allow videocontrols to work when content JS is disabled. [There are ways to move the JS guts over to chrome code -- see the pluginProblem code -- but that's not really possible when using XUL that's inheriting other bindings+JS.]
(In reply to Justin Dolske [:Dolske] from comment #18)
>  but
> that's not really possible when using XUL that's inheriting other
> bindings+JS.]
Nothing to do with XUL, but using XBL, and scripts in XBL
Neil, this patch has bounced owners a few times - can you and wes work together to make something go, particularly now that it's a Fennec blocker?
Assignee: nobody → enndeakin
Wes is in Toronto this week, have you guys been able to connect?
Talked with Neil, who said that Wes is looking into whether this was related to mouse capture, and whether disabling might work. Tempted to infer from that that this should be re-assigned to Wes, but iirc Wes has several blockers on the go. Are there alternates here?
I'd like Wes to chime in first, but we have one or two people this could fallback to.
I talked with Neil about this a bit too. He's got issues with Fennec builds that I didn't have time to help him with (we should fix that!).

I'm going to try and look at this this week (hopefully today or tomorrow with enn's help) so I'll steal it back.
Assignee: enndeakin → wjohnston
Attached patch ScrollFrame Patch (obsolete) — Splinter Review
This turned out to not be bad once I figured out what the problem was. We aren't setting mCurrentEventFrame for touchmove and touchend events. This sets them right before we dispatch.

The rest just uses basically the same code paths that mouse events already did with some special logic to get the coordinates of the touch.

If there are two touch points we just give up until there's one. We could do something more advanced so that if you grabbed to sliders in the same presShell, we would scroll them both. A better solution would let you scroll two sliders in the same document at the same time. That's going to be... difficult here since we don't have access to targetTouches I think?
Attachment #608440 - Attachment is obsolete: true
Attachment #620097 - Flags: review?(bugs)
Attachment #620097 - Flags: review?(enndeakin)
Comment on attachment 608441 [details] [diff] [review]
Patch for videocontrols

This will still be needed with this patch.
Attachment #608441 - Flags: review?(dolske)
Comment on attachment 620097 [details] [diff] [review]
ScrollFrame Patch

># HG changeset patch
># Parent 8a1c2368b434a6eb8cedb8642f5c9de0feb9c858
>
>diff --git a/layout/base/nsPresShell.cpp b/layout/base/nsPresShell.cpp
>--- a/layout/base/nsPresShell.cpp
>+++ b/layout/base/nsPresShell.cpp
>@@ -6681,16 +6681,19 @@ PresShell::DispatchTouchEvent(nsEvent *a
>       }
> 
>       nsTouchEvent newEvent(NS_IS_TRUSTED_EVENT(touchEvent) ?
>                               true : false,
>                             touchEvent);
>       newEvent.target = targetPtr;
> 
>       nsCOMPtr<nsIContent> content(do_QueryInterface(targetPtr));
>+      if (content) {
>+        mCurrentEventFrame = content->GetPrimaryFrame();
>+      }
Is it guaranteed here that content->OwnerDoc() == mDocument ?


>@@ -976,16 +1030,19 @@ nsSliderFrame::AddListener()
> 
>   nsIFrame* thumbFrame = mFrames.FirstChild();
>   if (!thumbFrame) {
>     return;
>   }
>   thumbFrame->GetContent()->
>     AddSystemEventListener(NS_LITERAL_STRING("mousedown"), mMediator,
>                            false, false);
>+  thumbFrame->GetContent()->
>+    AddSystemEventListener(NS_LITERAL_STRING("touchstart"), mMediator,
>+                           false, false);
You don't remove the listener ever.



this all needs tests.
Attachment #620097 - Flags: review?(bugs) → review-
Comment on attachment 620097 [details] [diff] [review]
ScrollFrame Patch

>@@ -509,27 +514,29 @@ nsSliderFrame::HandleEvent(nsPresContext
>         else {
>           // vertical scrollbar - check if mouse is left or right of thumb
>           if (eventPoint.x < -gSnapMultiplier * thumbSize.width ||
>               eventPoint.x > thumbSize.width +
>                                gSnapMultiplier * thumbSize.width)
>             isMouseOutsideThumb = true;
>         }
>       }
>+      *aEventStatus = nsEventStatus_eConsumeNoDefault;

I'm not sure if we should change this for mouse events. Why is it needed?

>     // set it
>     nsWeakFrame weakFrame(this);
>     // should aMaySnap be true here?
>     SetCurrentThumbPosition(scrollbar, pos - thumbLength/2, false, false, false);
>     NS_ENSURE_TRUE(weakFrame.IsAlive(), NS_OK);
> 
>     DragThumb(true);
>+    *aEventStatus = nsEventStatus_eConsumeNoDefault;

Same here, and mousedown has various default behaviour that might be used currently.


>+  nsPoint pt;
>+  nsresult rv = GetEventPoint(event, pt);
>+  if (NS_FAILED(rv)) {
>+    return rv;

In one case, you return ok when a failure occurs, but here you return rv. It might be better to just have GetTouchPoint return a boolean and return if it returns false.
> I'm not sure if we should change this for mouse events. Why is it needed?

We need it for touch to prevent the page from also panning. But I will special case it only for touch events.

> In one case, you return ok when a failure occurs, but here you return rv. It
> might be better to just have GetTouchPoint return a boolean and return if it
> returns false.

Sounds good. I didn't really like the nsresult way of doing this anyway...
Found some bugs writing tests today too. The mediator here works to catch touches that start on the thumb and redirect them to the underlying sliderFrame. Because we don't use the target set by captureMouse, that isn't working for us, so I'm going to adjust our dispatchTouchEvent to also look if there's a capture target set and use it (alone, or in addition to the other targets we already dispatch to?)

Secondly, there are HandlePress, HandleRelease, and HandleDrag methods called by nsIFrame when it receives mouse events. I'm still digging into exactly how they relate(?), but implementing them for touch events seems to help some test cases pass (and others still fail).
Attached patch Patch v2 (obsolete) — Splinter Review
This includes most of the fixes you asked for and a test.

Smaug, this basically uses the capturing frame if it exists. If not, it falls back to the I don't understand the event target's frame. I tried including a check for if (content && content->OwnerDoc() == mDocument) in the presShell changes, but that turns out to not be true in my tests? I'm not familiar enough with that to understand if it should be or not?
Attachment #620097 - Attachment is obsolete: true
Attachment #620864 - Flags: review?(enndeakin)
Attachment #620864 - Flags: review?(bugs)
Attachment #620097 - Flags: review?(enndeakin)
Comment on attachment 620864 [details] [diff] [review]
Patch v2

>              (gMiddlePref && aEvent->message == NS_MOUSE_BUTTON_DOWN &&
>               static_cast<nsMouseEvent*>(aEvent)->button ==
>-                nsMouseEvent::eMiddleButton)) {
>+                nsMouseEvent::eMiddleButton) ||
>+             (aEvent->message == NS_TOUCH_START && GetScrollToClick())) {

Where is NS_TOUCH_START handled when GetScrollToClick() is false?

>+<window title="XUL resizer tests"

Change this.

>+    function testMouseDragThumb(aId, aDir, aVal1, aVal2, aMods) {
>+      var scale = document.getElementById(aId);
>+      var [x,y] = getOffset(scale, aDir);
>+
>+      sendTouch("mousedown", getThumb(scale).getBoundingClientRect(), 0, 0, aMods);

You could just use synthesizeMouse or synthesizeMouseAtCenter, but I suppose the symmetry with the touch tests make it clearer as is.

> +      is(scale.value, aVal1, "Scale has correct value");

Make each of the comments for the 'is' lines different so that it's easier to determine what part of the test is failing.

Please move the test to toolkit/content/tests/chrome alongside the existing scale test, and name this new test something like test_scaledrag.
Attachment #620864 - Flags: review?(enndeakin) → review+
> Where is NS_TOUCH_START handled when GetScrollToClick() is false?

All of these events get sent back to the frame at the end of this function via. nsIFrame::HandleEvent() which then calls back into the slider frame vis nsSliderFrame::HandlePress, nsSliderFrame::HandleRelease, and nsSliderFrame::HandleDrag. Yes that seems kinda silly to me.

Worth filing a bug to unify all three methods of handling touchstart/mousedown in here?
(In reply to Wesley Johnston (:wesj) from comment #31)I tried
> including a check for if (content && content->OwnerDoc() == mDocument) in
> the presShell changes, but that turns out to not be true in my tests?
Uh, that is not good. You should set the PushCurrentEventInfo only in the right presshell.
Also, where do you call PopCurrentEventInfo ?
Attachment #620864 - Flags: review?(bugs) → review-
Comment on attachment 608441 [details] [diff] [review]
Patch for videocontrols

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

::: toolkit/content/widgets/videocontrols.xml
@@ +128,5 @@
>                case "curpos":
>                  if (this.type == "scrubber") {
>                      // Update the time shown in the thumb.
>                      this.thumb.setTime(newValue);
> +                    this.Utils.positionLabel.setAttribute("value", this.thumb.timeLabel.value);

Is this actually needed?

Scrubbing should be updating the video's position, which should be generating timeupdate events, which would in turn be calling setPosition() to update the positionLabel.

Patch is fine otherwise. I'd like to understand this one bit, but harmless enough that I'll mark r+ anyway. :)
Attachment #608441 - Flags: review?(dolske) → review+
Attached patch Patch v3 (obsolete) — Splinter Review
I've got neal's comments here, and added back in the check for content->OwnerDoc() == mDocument in DispatchTouchEvent. The requires that we're using the target's presShell when we call HandlePositionedEvent, so I added an additional check to get the correct prseShell for touchend, touchmove and touchcancel events.

We look for the first "changed" event in the current list, and dispatch to that presShell. Events can have multiple changed points, but they should all be in the same presShell... I hope? I'm a bit worried about that, so looking for any guidance/advice you have smaug.
Attachment #620864 - Attachment is obsolete: true
Attachment #624499 - Flags: review?(bugs)
(In reply to Justin Dolske [:Dolske] from comment #35)
> Scrubbing should be updating the video's position, which should be
> generating timeupdate events, which would in turn be calling setPosition()
> to update the positionLabel.

I think we bail here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml#579

i.e. we're trying to decouple the scale position from the video time while you're dragging, and avoid a cyclical update spiral of death.
Ah! I see, it all makes sense now. Thanks. :)
Comment on attachment 624499 [details] [diff] [review]
Patch v3


>-      nsCOMPtr<nsIContent> content(do_QueryInterface(targetPtr));
>+      // If someone is capturing, all touch events are filtered to their target
>+      nsCOMPtr<nsIContent> content = GetCapturingContent();
>+
>+      // if no one is capturing, set the capturing target
>+      bool useCapture = false;
Strange name. Could you just call this didPush


>+      if (!content) content = do_QueryInterface(targetPtr);
if (expr) {
  stmt;
}



>+      if (content && content->OwnerDoc() == mDocument) {
So, are there cases when content->OwnerDoc() != mDocument?
Could we call Push/Pop on the content->OwnerDoc()->GetShell() ?


>+        PushCurrentEventInfo(content->GetPrimaryFrame(), content);
>+        useCapture = true;
>+      }
>       nsPresContext *context = nsContentUtils::GetContextForContent(content);
>       if (!context) {
>         context = mPresContext;
>       }
>       tmpStatus = nsEventStatus_eIgnore;
>       nsEventDispatcher::Dispatch(targetPtr, context,
>                                   &newEvent, nsnull, &tmpStatus, aEventCB);
>       if (nsEventStatus_eConsumeNoDefault == tmpStatus) {
>         preventDefault = true;
>       }
>+      if (useCapture) {
>+        PopCurrentEventInfo();
>+      }
Attached patch Patch (obsolete) — Splinter Review
Fixed those nits. Tests still pass fine.
Attachment #624499 - Attachment is obsolete: true
Attachment #624499 - Flags: review?(bugs)
Attachment #625718 - Flags: review?(bugs)
Comment on attachment 625718 [details] [diff] [review]
Patch

># HG changeset patch
># Parent f1b1d737a915b0fc3e8a80acdb56ebc749c6fa87
>
>diff --git a/layout/base/nsPresShell.cpp b/layout/base/nsPresShell.cpp
>--- a/layout/base/nsPresShell.cpp
>+++ b/layout/base/nsPresShell.cpp
>@@ -5882,16 +5882,49 @@ PresShell::HandleEvent(nsIFrame        *
>       }
> 
>       return NS_OK;
>     }
> 
>+          nsCOMPtr<nsPIDOMEventTarget> targetPtr;
>+          oldTouch->GetTarget(getter_AddRefs(targetPtr));
>+          nsCOMPtr<nsIContent> content = do_QueryInterface(targetPtr);
>+          if (content) {
>+            shell = static_cast<PresShell*>(content->GetPrimaryFrame()->PresContext()->PresShell());
Null-check the result of GetPrimaryFrame


>-      nsCOMPtr<nsIContent> content(do_QueryInterface(targetPtr));
>+      // If someone is capturing, all touch events are filtered to their target
>+      nsCOMPtr<nsIContent> content = GetCapturingContent();
>+
>+      // if no one is capturing, set the capturing target
>+      bool didPush = false;
>+      if (!content) {
>+        content = do_QueryInterface(targetPtr);
>+      }
>+      if (content && content->OwnerDoc() == mDocument) {
>+        PresShell* contentPresShell = static_cast<PresShell*>
>+            (content->OwnerDoc()->GetShell());
>+        contentPresShell->PushCurrentEventInfo(content->GetPrimaryFrame(), content);
Null-check the result of GetShell().
Also, if you push here, you want to keep strong ref to the shell and
pop using it.
Attachment #625718 - Flags: review?(bugs) → review+
Attached patch Patch for checkin (obsolete) — Splinter Review
Just want to make sure I did those nits right.
Attachment #625718 - Attachment is obsolete: true
Attachment #626585 - Flags: review?(bugs)
Attachment #626585 - Attachment is patch: true
Whoops. This is what I meant to post.
Attachment #626585 - Attachment is obsolete: true
Attachment #626585 - Flags: review?(bugs)
Attachment #626588 - Flags: review?(bugs)
Comment on attachment 626588 [details] [diff] [review]
Patch for checkin

>+
>+          shell = static_cast<PresShell*>(
>+                      contentFrame->PresContext()->PresShell());
2 space indentation, please

>+      // if no one is capturing, set the capturing target
>+      if (!content) {
>+        content = do_QueryInterface(targetPtr);
>+      }
>+      PresShell* contentPresShell = nsnull;
Make this nsRefPtr<PresShell> contentPresShell;
Attachment #626588 - Flags: review?(bugs) → review+
Dang it. This causes this test to fail:
http://mxr.mozilla.org/mozilla-central/source/content/events/test/test_bug648573.html?force=1#24

i.e. every page with scrollbars now automatically has a touch event listener. Not sure if there's a good way (or we want) to NOT trigger that code from the slider frames... or maybe we can do overflow: hidden on the test somehow. Also need to test that this doesn't cause us to take our slow touch event patch on Fennec for every page, but since we hide scrollbars there, I think we will be fine....
(In reply to Wesley Johnston (:wesj) from comment #45)
> Dang it. This causes this test to fail:
> http://mxr.mozilla.org/mozilla-central/source/content/events/test/
> test_bug648573.html?force=1#24
> 
> i.e. every page with scrollbars now automatically has a touch event
> listener. 
Oh, right. That sounds wrong and breaks gestures on Win7.
Could you actually handle touch events in
nsSliderFrame::HandleEvent(nsPresContext* aPresContext,
                                      nsGUIEvent* aEvent,
                                      nsEventStatus* aEventStatus)

and not use AddSystemEventListener?
I guess no, since that gets called only if nsSliderFrame->GetContent() is the target of the event.

Perhaps we need to change nsEventListenerManager::AddEventListener
so that it doesn't set mMayHaveTouchEventListener to true if 
aFlags & NS_EVENT_FLAG_SYSTEM_EVENT
Attached patch Patch 2Splinter Review
Don't set mayHaveTouchListener if its a system listener. I also saw these new tests having some problems that don't make sense to me.... Looking at them.

old push:
https://tbpl.mozilla.org/?tree=Try&rev=8527620b2c91

push with new patch included:
https://tbpl.mozilla.org/?tree=Try&rev=544998258393
Duplicate of this bug: 760032
Re-triaging to strip out non-OMG-ON-FIRE release blockers - pushing this to .N+ but we'd still love to look at a safe patch.
blocking-fennec1.0: + → .N+
I'm guessing this what you meant?
Blocks: 747910
No longer blocks: 747907
(In reply to Wesley Johnston (:wesj) from comment #51)
> I'm guessing this what you meant?

It is - thanks
Wes, looks like the patches here are reviewed, have they landed? Can this be closed? If so, be sure to request approval for uplift.
Blocks: 751358
tracking-fennec: --- → +
blocking-fennec1.0: .N+ → -
Attached patch Patch (obsolete) — Splinter Review
Sorry for the insane delay. Finally sat down and figured out what was wrong here. Three things in this:
1.) Tests shouldn't run twice. Removed an extra initalization
2.) Unbitrot by removing some private event stuff
3.) Missing piece that kept us for handing touch up correctly

Passed on try once, but pushed again:
https://tbpl.mozilla.org/?tree=Try&rev=aeed66d3ab98
Attachment #638273 - Flags: review?(bugs)
Comment on attachment 638273 [details] [diff] [review]
Patch

Grr. OSX is still giving me headaches. Looks like they think they must move to the click point automatically.
Attachment #638273 - Flags: review?(bugs)
Attached patch Patch 3Splinter Review
Stupid Mac has to do everything backwards. This looks good on try.
Attachment #638273 - Attachment is obsolete: true
Attachment #638488 - Flags: review?(enndeakin)
Attachment #638488 - Flags: review?(bugs)
Comment on attachment 638488 [details] [diff] [review]
Patch 3

>   // test dragging a slider by tapping off the thumb and holding shift
>   // modifiers don't affect touch events
>   var mods = /Mac/.test(navigator.platform) ? Components.interfaces.nsIDOMNSEvent.ALT_MASK :
>                                               Components.interfaces.nsIDOMNSEvent.SHIFT_MASK;

You can also use 'isMac' here.
Attachment #638488 - Flags: review?(enndeakin) → review+
Attachment #638488 - Flags: review?(bugs) → review+
Depends on: 772422
Going to back this out so it doesn't interfere with uplifting text selection to Aurora.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Please reopen when backing something out.
Sorry bout that. Everything looked good on try. Will reland this afternoon.
Target Milestone: --- → Firefox 16
Depends on: 776447
Depends on: 779078
Depends on: 790164
Video scrubber works perfectly for any video. Great work! 

Closing bug as verified fixed on:

Firefox 18.0a1 (2012-09-24)
Device: Galaxy Note
OS: Android 4.0.4
Status: RESOLVED → VERIFIED
Video scrubber seems to refuse to work properly just for http://people.mozilla.com/~nhirata/html_tp/big_buck_bunny_480p.webm. For any other webm or other kind of videos, it works fine. Is it because of this video file or is it a Fennec issue?
I'm pretty sure its the video. The same problems exist with that video on Desktop.
Ya, I'm not sure exactly.  There was a bug on almost a year ago in terms of the scrubber with that video, and it turned out it had something to do with the encoding on that video I believe.
http://people.mozilla.com/~nhirata/html_tp/big_buck_bunny_480p.webm doesn't have Cues (i.e. a keyframe index) which we require to seek. Bug 657791 is on file to track this.
Flags: in-litmus?
You need to log in before you can comment on or make changes to this bug.