Video controls don't hide if mouse leaves controls via left/right sides

RESOLVED FIXED in mozilla35

Status

()

Toolkit
Video/Audio Controls
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Ben Craddock, Assigned: Ben Craddock)

Tracking

Trunk
mozilla35
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0
Build ID: 20140917194002

Steps to reproduce:

1) Move mouse onto video controls
2) Move mouse directly right or directly left until it has left the video element


Actual results:

Video controls remain visible


Expected results:

Video controls should hide after mouse leaves
(this works if the mouse leaves the controls via top or bottom)
(Assignee)

Comment 1

4 years ago
Created attachment 8494409 [details] [diff] [review]
patch1 - add left and right checks to onMouseInOut

Adding a patch for it, looks to be a simple fix.
Attachment #8494409 - Flags: review?(dolske)
(Assignee)

Comment 2

4 years ago
First patch, also.  Hopefully I haven't misunderstood the process.
Comment on attachment 8494409 [details] [diff] [review]
patch1 - add left and right checks to onMouseInOut

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

My first thought was that this bug was crazy, and the controls clearly disappear when moving outside the video! But then I noticed you're talking about moving left/right from the actual control bar, not anywhere else on the video... Holy cow, that is indeed broken! Nice catch!

Looks like bug 757625 (which added this code) should have considered this case, but was only thinking about the specific issue there (scrubbing, not leaving the video).

This patch looks fine, but could I ask for you to do a little code cleanup while you're here? I think it would greatly improve readability, especially with the extra lines added here, to use an intermediate variable for |this.controlBar.getBoundingClientRect()|, so we don't keep repeating that.

eg:
  var controlRect = this.controlBar.getBoundingClientRect();
  if (event.clientY > controlRect.top && ....)
(Assignee)

Comment 4

4 years ago
Created attachment 8494661 [details] [diff] [review]
patch 2 - checkAllBoundsOnHover + code cleanup

No problem, changes added.
Attachment #8494409 - Attachment is obsolete: true
Attachment #8494409 - Flags: review?(dolske)
Attachment #8494661 - Flags: review?(dolske)
Comment on attachment 8494661 [details] [diff] [review]
patch 2 - checkAllBoundsOnHover + code cleanup

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

Awesome! Thanks for the patch!
Attachment #8494661 - Flags: review?(dolske) → review+
Assignee: nobody → bcc
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Landed in https://hg.mozilla.org/integration/fx-team/rev/36f08e5cfab0, if all goes well it should be in tomorrow's Nightly build. There will be another comment here when the code is merged and then the bug will be closed.

If you haven't already, check out https://developer.mozilla.org/en-US/docs/Introduction -- although I suspect you have, since everything here was flawless. Looking forward to your next patch! :)
https://hg.mozilla.org/mozilla-central/rev/36f08e5cfab0
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.