Closed Bug 896415 Opened 11 years ago Closed 11 years ago

Remove mouse_event_shim.js from video app

Categories

(Firefox OS Graveyard :: Gaia::Video, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: gduan, Assigned: johnhu)

References

Details

(Keywords: perf, Whiteboard: [c= p= s=2013.08.23 u=])

Attachments

(1 file)

Refer to bug 861735 ,

We'd better remove dependency of this mouse_event_shim.js for below reasons.
1. prevent hidden bug in the future.
2. improve load time and responsiveness
3. gecko already dispatch events by itself.
Assignee: nobody → johu
Blocks: 861735
No longer blocks: 861735
This patch does:
1. remove mouse event shim
2. use touch event to handle all events
Attachment #783703 - Flags: review?(dflanagan)
David, 

Before reviewing, I found a strange behavior on touch event while making the patch. When I replace all mouse events as touch events, some buttons use click event, like delete or share in video controls. When video controls are not displayed and user touches the place where delete or share button are, the video controls are displayed and delete or share is triggered. This is because delete and share use click event and not handled inside playerTouchStart (the original playerMousedown). If we calls the preventDefault at the code showing video controls, those buttons will not triggered.

And the most strange behavior is the same symptom happened to back button of player page. When player is shown and user touches "back" button which is handled inside playerTouchStart. The device fires a click event to the underlying thumbnail, so that video app plays it. I also add a preventDefault to playerTouchStart to prevent it.

To test this case, I had create three page for testing different mobile browsers:
1. http://huchengtw-moz.github.io/test.html : hide blue when touchstart, and show alert when click at green.
2. http://huchengtw-moz.github.io/test2.html : hide blue when touchstart, and show alert when touchstart at green.
3. http://huchengtw-moz.github.io/test3.html : hide blue when touchstart, and show alert when touchend at green.

All mobile browsers shows alert in the case 1, but not case 2 and 3.
(In reply to John Hu [:johnhu] from comment #2)
> David, 
> 
> Before reviewing, I found a strange behavior on touch event while making the
> patch. When I replace all mouse events as touch events, some buttons use
> click event, like delete or share in video controls. When video controls are
> not displayed and user touches the place where delete or share button are,
> the video controls are displayed and delete or share is triggered. This is
> because delete and share use click event and not handled inside
> playerTouchStart (the original playerMousedown). If we calls the
> preventDefault at the code showing video controls, those buttons will not
> triggered.
> 

Thanks for testing carefully and noticing this.  Another way to deal with this would be to convert everything to use click handlers and only listen for touchstart/move/end over the slider.

> And the most strange behavior is the same symptom happened to back button of
> player page. When player is shown and user touches "back" button which is
> handled inside playerTouchStart. The device fires a click event to the
> underlying thumbnail, so that video app plays it. I also add a
> preventDefault to playerTouchStart to prevent it.
> 
> To test this case, I had create three page for testing different mobile
> browsers:
> 1. http://huchengtw-moz.github.io/test.html : hide blue when touchstart, and
> show alert when click at green.
> 2. http://huchengtw-moz.github.io/test2.html : hide blue when touchstart,
> and show alert when touchstart at green.
> 3. http://huchengtw-moz.github.io/test3.html : hide blue when touchstart,
> and show alert when touchend at green.
> 
> All mobile browsers shows alert in the case 1, but not case 2 and 3.

Interesting tests! I have no idea what the specs say is the right thing here.
Comment on attachment 783703 [details]
remove mouse_event_shim

John,

Thanks for taking this bug. Its needed to be fixed for a long time.

Your code looks good, but I think it could be simpler and better.  I have a number of suggestions on github. You don't have to do all of them, but I would like, at least, for you to register the touchmove and touchend handlers statically rather than registering them in touchstart and removing them in touchend.  That is needed for mouse events, but not for touch events, because touchmove and touchend events are always delivered to the same element as the initial touchstart event.
Attachment #783703 - Flags: review?(dflanagan) → review-
Keywords: perf
Whiteboard: [c= p=]
Comment on attachment 783703 [details]
remove mouse_event_shim

David, 

I had changed the patch to use statical event hooking. But I keep the code to prevent multiple touch. I had tried to remove it. But the usage looks strange while user dragging slider and touch others places, including buttons and slider. The result will make player play video while user dragging.
Attachment #783703 - Flags: review- → review?(dflanagan)
Comment on attachment 783703 [details]
remove mouse_event_shim

John,

Thanks for making these changes. The code seems cleaner now. 

I don't like the style of handling event for many UI elements in one handler, but I know that is the code you inherited. I think that if you were registering event handlers just on the slider wrapper element then you wouldn't have to worry about the multi-touch case and the code could be even simpler.  But this isn't the time to make that change.

I have not tested your patch.  I'm assuming that you have.
Attachment #783703 - Flags: review?(dflanagan) → review+
landed to master:
https://github.com/mozilla-b2g/gaia/commit/703eef4ed4cba808c0a5690eb35873e897d8d815
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
Whiteboard: [c= p=] → [c= p= s=2013.08.23 u=]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: