Closed Bug 689387 Opened 8 years ago Closed 8 years ago

video controls could use new mouseenter / mouseleave events

Categories

(Toolkit :: Video/Audio Controls, defect)

defect
Not set

Tracking

()

RESOLVED WONTFIX
mozilla12

People

(Reporter: Dolske, Assigned: mgerrits)

References

Details

(Whiteboard: [good first bug][mentor=jwein])

Attachments

(1 file, 2 obsolete files)

Bug 432698 just landed, which adds support for the new mouseenter / mouseleave events. Currently, videocontrols.xml has <handler>s for mouseover / mouseout events, but has to muck about with figuring out if the mouse has really left the controls area (or if the events are just bubbling from children in the controls).

AIUI, this is exactly the problem mouseenter/leave are designed to simplify. Simple is good.
Whiteboard: [good first bug] → [good first bug][mentor=jwein]
These changes should be made in /toolkit/content/widgets/videocontrols.xml. See comment 0 for more information about how to fix this bug.
Attached patch Work in progress for 689387 (obsolete) — Splinter Review
I've added handlers for the mouseenter and mouseleave events but these never trigger. AFAICT this patch should at least log a message when the event fires but that never happens. Is there a something wrong with my patch or with the support for mouseenter and mouseleave?
Attachment #585783 - Flags: feedback?(jwein)
(In reply to Martijn Gerrits from comment #2)
> Created attachment 585783 [details] [diff] [review]
> Work in progress for 689387
> 
> I've added handlers for the mouseenter and mouseleave events but these never
> trigger. AFAICT this patch should at least log a message when the event
> fires but that never happens. Is there a something wrong with my patch or
> with the support for mouseenter and mouseleave?

Martijn: Thanks for your first patch, I'm sorry this has been so confusing. I've looked deeper in to the cause of the issue and according to this comment, https://bugzilla.mozilla.org/show_bug.cgi?id=432698#c42, the "native anonymous content" inside of the video controls don't fire the mouseenter/mouseleave events. 

Can you try to add the event listeners to the video element within the setupInitialState function? Such as doing |this.video.addEventListener("mouseenter", function() { ... }, false);|
Attachment #585783 - Flags: feedback?(jwein)
Attached patch Second attempt for bug 689387 (obsolete) — Splinter Review
I've now changed the way the events are added to the video element. I works as expected now. IMO it would be nice to get rid of the isEventWithin function. I couldn't do that because that is used by the mouseover/mouseout events on the muteButton and volumeStack. I tried to use the mouseenter and mouseleave events on these elements too but they don't seem to trigger. I searched if there are any other issues with the mouseenter/leave events but I couldn't find anything, do you maybe have some suggestions?
Attachment #585783 - Attachment is obsolete: true
Attachment #586080 - Flags: feedback?(jwein)
Comment on attachment 586080 [details] [diff] [review]
Second attempt for bug 689387

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

This looks good to me! The mute button and the volume stack are native anonymous content, and so the mouseenter and mouseleave events won't be fired for those elements unfortunately.

I've requested review from Dolske for you.
Attachment #586080 - Flags: review?(dolske)
Attachment #586080 - Flags: feedback?(jwein)
Attachment #586080 - Flags: feedback+
Comment on attachment 586080 [details] [diff] [review]
Second attempt for bug 689387

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

::: toolkit/content/widgets/videocontrols.xml
@@ +443,5 @@
>                      this.video.addEventListener("media-showStatistics", this._handleCustomEventsBound, false, true);
> +
> +                    var self = this;
> +                    this.video.addEventListener("mouseenter", function(e) { self.onMouseInOut(e) }, false, true);
> +                    this.video.addEventListener("mouseleave", function(e) { self.onMouseInOut(e) }, false, true);

Nit: move the addEventListener calls into init(), right above where the mute/volume listeners are added.

And, actually, we should probably add a comment there nothing that we still use mouseover/mouseout due to 432698 comment 42.

@@ -1307,5 @@
>  
>      <handlers>
> -        <handler event="mouseover">
> -            if (!this.isTouchControl)
> -                this.Utils.onMouseInOut(event);

Hmm, I think you'll still need the .isTouchControls check, though now in onMouseInOut.

Not sure if mobile fires these newer events, but it would be safe to keep it anyway.
Attachment #586080 - Flags: review?(dolske) → review-
I've changed the following according to the review comments:
- Moved attaching the event listeners to the video object to the init function
- Added a comment that notes why we are still using mouseover/out for some elements
- Added a check for isTouchControl at the beginning of the onMouseInOut function
Attachment #586080 - Attachment is obsolete: true
Attachment #586705 - Flags: review?(dolske)
Assignee: nobody → mgerrits
Status: NEW → ASSIGNED
Martijn: While we are waiting for review from Justin, can you please follow the steps in http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed to get your patch ready for check-in?
Comment on attachment 586705 [details] [diff] [review]
Third patch for 689387

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

Thanks!
Attachment #586705 - Flags: review?(dolske) → review+
https://hg.mozilla.org/integration/fx-team/rev/4ac407704d74
Whiteboard: [good first bug][mentor=jwein] → [good first bug][mentor=jwein][fixed-in-fx-team]
Congratulations on your first patch Martijn!
Jared: Is this now fixed because I ran into some issues while making the patch ready for checkin-needed? It was bitrotten.
Martijn: It has been fixed in the fx-team repository and will soon be in the mozilla-central repository. The status of this bug will be updated when the fix is merged to mozilla-central.
https://hg.mozilla.org/mozilla-central/rev/4ac407704d74
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=jwein][fixed-in-fx-team] → [good first bug][mentor=jwein]
Target Milestone: --- → mozilla12
Depends on: 718411
Hmm, this is apparently causing some test failures (bug 718411).

Couple of potential fixes:

1) onMouseInOut() may need a check like at the top of handleEvent()...

   if (this.videocontrols.randomID != this.randomID) { /* die */ }

2) terminateEventListeners() may need to call removeEventListener() for these events (I think Jared filed a bug on cleaning up some others, too, since they're not working actually working due to using a function wrapper.)

Bug 699719 might be involved here too, since the test failures in bug 718411 seem to point at code involved from that patch...
(In reply to Justin Dolske [:Dolske] from comment #15)
> terminateEventListeners() may need to call removeEventListener() for
> these events (I think Jared filed a bug on cleaning up some others, too,
> since they're not working actually working due to using a function wrapper.)

This is bug 702161.
Sorry, backed out in https://hg.mozilla.org/mozilla-central/rev/12091526e9d0 - we maybe could have lived with the test failures, but the way the number of shutdown hangs seemed to have gone through the roof at the same time made me nervous that it might be doing something user-visible, too.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Please don't use mouseenter/leave if possible. They are slower than mouseover/out, and if 
there are no listeners for mouseenter/leave, we don't fire those events at all.
mouseenter/leave are handy, but hopefully only used by content, not chrome.
Based on comment #18, I'm thinking this bug should just be marked as WONTFIX. The code cleanup benefits from using mouseenter/mouseleave also don't allow us to remove the |isEventWithin| function since the volume and mute controls still need to use them.
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.