730626 - Add the ability to hide the error message overlay on HTML5 videos

Status: RESOLVED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

Some errors, such as the download aborted error, should be dismissable as the user may still view the video as normal up to the point where the download was stopped.

We should add the ability to dismiss or hide these error messages.

To fix this, the changes will need to be made in /toolkit/content/widgets/videocontrols.xml and any styling changes will need to be made in /toolkit/themes/winstripe/global/media/videocontrols.css as well as /toolkit/themes/pinstripe/global/media/videocontrols.css

Some ideas I have are to fade out the error after a period of time, add something to click on to dismiss the error, or update the display of errors so they aren't so obtrusive. I'm interested to hear what other ideas people have for fixing this.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

bharath: How do you think we should fix this bug?

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

The link that is in the URL field can be used to test the different error cases.

Comment 3

[Bharath Thiruveedula[:tbharath]]

jaws: I want to display the error in the player when it opens unsupported file type  and when the users clicks on the statusOverlay , I want to make hidden property to false and the error will disappear


But I have added this.statusOverlay.addEventListener("click",function(){this.setAttribute("hidden",false);},false); after the http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml#621 , but statusOverlay doen't receive any click event

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to bharath from comment #3)
> But I have added
> this.statusOverlay.addEventListener("click",function(){this.
> setAttribute("hidden",false);},false); after the
> http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/
> videocontrols.xml#621 , but statusOverlay doen't receive any click event

Sorry for not getting back to you sooner.

The statusOverlay is currently beneath the controlsSpacer, and so it won't receive click events due to this. One way we could try to fix this is by setting the CSS property of 'pointer-events: none' on the controlsSpacer if there is an error.

This should allow pointer-events to go through the controlsSpacer and reach the statusOverlay.

You might be able to add a CSS rule like so:

> .statusOverlay:not([hidden]) + .statsOverlay + .controlsOverlay > .controlsSpacer {
>   pointer-events: none;
> }

Try that out and let me know if the click events work. Note that if the statistics are showing, that the statsOverlay will probably intercept the clicks too. In case pointer-events isn't already set on the statsOverlay, you can add the following CSS rule:

> .statsOverlay {
>   pointer-events: none;
> }

I don't know of a time where we would want the statsOverlay to receive pointer-events, thus the rule above unconditionally removes pointer-events from the statsOverlay.

Comment 5

[Bharath Thiruveedula[:tbharath]]

Yes ,I have added the necessary changes what you have mentioned earlier , but statusOverlay doesn't receive any click event

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to bharath from comment #5)
> Yes ,I have added the necessary changes what you have mentioned earlier ,
> but statusOverlay doesn't receive any click event

Can you post a patch with those changes to this bug?

Comment 7

[Bharath Thiruveedula[:tbharath]]

Attachment: added the event click listener to statusOverlay

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 603567 [details] [diff] [review]
added the event click listener to statusOverlay

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

Thanks, I'll get you some feedback on this tomorrow. Note: when requesting review, please set the review flag to "?", not "+".

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

The patch that is attached to this bug is not in the normal format for patches (for example, it is missing paths to the directories).

bharath: Can you follow the steps here to fix the format of your patch? https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 603567 [details] [diff] [review]
added the event click listener to statusOverlay

This is a good start, but I also couldn't get the events to work for me. I will attach a patch that I put together that fixes a couple things I said earlier.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: patch that is close but not all the way there

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: patch that is closer but the events don't seem to fire yet

Comment 13

[Bharath Thiruveedula[:tbharath]]

Attachment: Formatted Patch to the previous one

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 603924 [details] [diff] [review]
Formatted Patch to the previous one

bharath: I thought about this more and there is a much simpler way to implement this same behavior.

In the clickToPlayClickHandler, we currently exit early if there is an error. Within that function, we can simply hide the error if the overlay has been clicked, but we should only hide the error for whitelisted error events. The only error that I can think of where we would want to add the ability to hide the message is for MEDIA_ERR_ABORTED.

This should be a much simpler fix and will only require us to update videocontrols.xml and not any other files.

Comment 15

[Bharath Thiruveedula[:tbharath]]

Attachment: Added Click Event Listener to the StatusOverlay in clickToPlayClickHandler

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 604954 [details] [diff] [review]
Added Click Event Listener to the StatusOverlay in clickToPlayClickHandler

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

This is close, but we still need to return early if the error is not MEDIA_ERR_ABORTED.

::: toolkit/content/widgets/videocontrols.xml
@@ +986,2 @@
>                          return;
> +                    if(this.hasError()&&this.video.error.code==this.video.error.MEDIA_ERR_ABORTED)

nit: Please place spaces between the parentheses of the conditional, a space after the |if| and spaces before and after the && and ==. See https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#JavaScript_practices for more information on the Mozilla coding style.

Comment 17

[Bharath Thiruveedula[:tbharath]]

Attachment: Modified the clickToPlayClickHandler

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 605278 [details] [diff] [review]
Modified the clickToPlayClickHandler

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

Thanks, this looks good :) It looks like it was created on top of the previous patch though. Can you rebase on top of the tip?

Also, as general feedback: When uploading a new patch, please check the checkbox of the old one to mark it as 'obsolete'.

Comment 19

[Bharath Thiruveedula[:tbharath]]

Attachment: Modified the clickToPlayClickHandler and added the Event click Listener to the statusOverlay

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 605367 [details] [diff] [review]
Modified the clickToPlayClickHandler and added the Event click Listener to the statusOverlay

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

::: toolkit/content/widgets/videocontrols.xml
@@ +981,5 @@
>                          this.fullscreenButton.removeAttribute("fullscreened");
>                  },
>  
>                  clickToPlayClickHandler : function(e) {
> +                    if (e.button != 0 || (this.haserror() && this.video.error.code != this.video.error.MEDIA_ERR_ABORTED))

Did you test this? I don't think there is a function named |this.haserror()|. Also, please try to wrap lines to 80 characters when possible.

@@ +986,3 @@
>                          return;
> +                    if (this.hasError() && this.video.error.code == this.video.error.MEDIA_ERR_ABORTED)
> +                        this.statusOverlay.setAttribute(hidden,"true");    

nit: please place a space after the comma.

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/edf115968196 which claimed to be for this bug was actually for bug 730636

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

bharath: Can you upload a new patch that addresses the feedback in comment #20?

Comment 23

[Sean Chen]

Hey, We would like to work on this bug. Thank you!

Comment 24

[Bao Duong]

Hello Jared Wein, Sean Chen and I will be working on this bug together for our Software Engineer project. We will look into this bug as soon as possible. Thanks :)

Comment 25

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Bao Duong from comment #24)
> Hello Jared Wein, Sean Chen and I will be working on this bug together for
> our Software Engineer project. We will look into this bug as soon as
> possible. Thanks :)

Cool and welcome! Feel free to message me on irc.mozilla.org if you have any questions. My nick is "jaws". You can also join #introduction where you can talk with others who are working on Firefox and happy to help.

Comment 26

[Jared Wein [:jaws] (please needinfo? me)]

A simple way to start to fix this bug could be the following:

At http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml#356 when we check this.video.error, we can modify that to check for
> (this.video.error && !this.suppressError)
`suppressError` would be a new member variable that can be added and defaulted to false. You would need to add a new X button to the overlay, which when clicked on would change suppressError to true.

Comment 27

[Bao Duong]

Attachment: Added a dismiss button to the statusOverlay and an event listener for the button

The button seems to be displaying on the overlay, but the event cannot be triggered. It seems that the controlsOverlay is on top of statusOverlay so we cannot click the button to fire the event. Is there anyway we can click both overlays?

Comment 28

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: WIP patch

Bao, I took your patch and played with it a little bit. I see the issue that you are having with the clicks not being recognized and I think it might have to do with the use of the <stack> here.

Can you try some experiments to see if that is the reason? I added some logging to the Web Console that you can use to see if the clicks are getting handled.

Comment 29

[Bao Duong]

Thanks, Jared Wein. I made your suggested changes and still the button won't seem to trigger anything. There wasn't anything on the console regarding to the logs you added. Perhaps the controlsOverlay is blocking us from clicking the statusOverlay?

Comment 30

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Bao Duong from comment #29)
> Thanks, Jared Wein. I made your suggested changes and still the button won't
> seem to trigger anything. There wasn't anything on the console regarding to
> the logs you added. Perhaps the controlsOverlay is blocking us from clicking
> the statusOverlay?

That is correct. Try experimenting with different ways to fix it. It will be possible to fix this, but I'm not sure how much will need to be changed (and if at the end we will want to accept how much needs to change).

Comment 31

[Bao Duong]

Hi Jared Wein, I got a chance to further test this issue. I added a button on the controlsOverlay, and the button was receiving clicks without any problems. 

I don't know what defines what to be in front and on the back for the overlays. Can you guide me where to look? Maybe we can make the statusOverlay on top of the controlsOverlay when there is an error so we can trigger stuff on it? Thanks.

Comment 32

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Bao Duong from comment #31)
> Hi Jared Wein, I got a chance to further test this issue. I added a button
> on the controlsOverlay, and the button was receiving clicks without any
> problems. 
> 
> I don't know what defines what to be in front and on the back for the
> overlays. Can you guide me where to look? Maybe we can make the
> statusOverlay on top of the controlsOverlay when there is an error so we can
> trigger stuff on it? Thanks.

It seems simpler if we just added the button as a child of the controlsOverlay. The button can be hidden until there is an error, and positioned in such a way that it is least likely to overlap with the error text.

Comment 33

[Bao Duong]

Attachment: Clicking on the dismiss button will hide media aborted error

I have set the dismissButton hidden until the video loading is stopped. It will then be next to the fullscreen button where the user can click on that results in hiding the error overlay. If you have a better place to put the button, I'd be happy to change the button location. Thanks!

Comment 34

[Bao Duong]

Attachment: Changed the position of the dismiss button to be out of the control bar.

I have changed the position of the dismiss button. Does this look okay?

Comment 35

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 748997 [details] [diff] [review]
Clicking on the dismiss button will hide media aborted error

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

These two patches don't seem too far off. Please combine the two patches and rebase your patches against a more recent mozilla-central.

Please replace the tabs with spaces (this file uses four spaces for each tab).

After you combine the patches, you can replace the "Dismiss" text with the icon that is being used for the click-to-play overlay. See http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/mozapps/plugins/pluginProblem.css#162 for an example of how you can use the icon.

Comment 36

[Bao Duong]

Attachment: Combined patches and added an X icon to the dismiss button.

Comment 37

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Bao, can you apply this patch and see if it works for you? When you get the aborted error, just clicking on the error text should dismiss it. It won't dismiss other error types, but so far this is the only one that I know of that can occur and the video can still play.

Comment 38

[Bao Duong]

(In reply to Jared Wein [:jaws] from comment #37)
> Created attachment 749637 [details] [diff] [review]
> Patch
> 
> Bao, can you apply this patch and see if it works for you? When you get the
> aborted error, just clicking on the error text should dismiss it. It won't
> dismiss other error types, but so far this is the only one that I know of
> that can occur and the video can still play.

It seems to work for me. I had a thought of doing this also, but I wasn't sure if users can automatically find out they can click on the error text to dismiss it. 

When I pressed on the region, the video stopped for me (the controlsOverlay is still in front) so I added this to keep the video playing: http://pastebin.mozilla.org/2405585. Is this fine?

Comment 39

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Bao Duong from comment #38)
> (In reply to Jared Wein [:jaws] from comment #37)
> > Created attachment 749637 [details] [diff] [review]
> > Patch
> > 
> > Bao, can you apply this patch and see if it works for you? When you get the
> > aborted error, just clicking on the error text should dismiss it. It won't
> > dismiss other error types, but so far this is the only one that I know of
> > that can occur and the video can still play.
> 
> It seems to work for me. I had a thought of doing this also, but I wasn't
> sure if users can automatically find out they can click on the error text to
> dismiss it. 
> 
> When I pressed on the region, the video stopped for me (the controlsOverlay
> is still in front) so I added this to keep the video playing:
> http://pastebin.mozilla.org/2405585. Is this fine?

Instead of calling this.Utils.video.play(), we can just return from the function. Would you like to make that change and upload a new patch?

Comment 40

[Bao Duong]

Attachment: When the error text is clicked, the statusOverlay will be hidden and video can still be viewed.

Added what you suggested. Is there anything else we need to do for this bug?

Comment 41

[Jared Wein [:jaws] (please needinfo? me)]

Thanks, yeah that looks good, but this functionality still isn't working for me. The overlay will hide, but the play/pause toggling by clicking on the video doesn't work, and once the video pauses I can't get it to resume.

Sorry for such a headache with this bug. It may not be worth the effort required.

Comment 42

[Bao Duong]

Yes, I see that you cannot call togglePause() since it already returned from the function. 

I went back to your patch attachment 749637 [details] [diff] [review] from comment #37 and tested again and it's fine to click play and pause on the overlay after dismissing the media aborted error. I think we can use that patch instead, but it seems to pause the video at the same time I click on the overlay. I don't think it's a big deal though.

Comment 43

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.1

Ah, perfect, that comment really helped. OK, can you try this patch out? I think it should work pretty well. Now when it notices that it has an error, it also checks to see if that error should be suppressed.

Comment 44

[Bao Duong]

Perfect! It seems to work as expected now :) I still don't know if users are able to realize they can click on the overlay to suppress the media aborted error ;) But other than that, this looks good!

Comment 45

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.2

I've updated the patch to include a comment that we are only choosing to allow MEDIA_ERR_ABORTED to get dismissed for now.

Dolske, I worked on this patch with Bao but have set him as the author. Can you review this?

Comment 46

[Justin Dolske [:Dolske]]

Comment on attachment 750673 [details] [diff] [review]
Patch v1.2

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

::: toolkit/content/widgets/videocontrols.xml
@@ +1043,4 @@
>                          return;
> +                    if (this.hasError() && !this.suppressError) {
> +                        // Errors that can be dismissed should be placed here as we discover them.
> +                        if (this.video.error.code != this.video.error.MEDIA_ERR_ABORTED)

Why only ABORTED? I don't understand why this is the only one that's removable. How would a user get a video into this state (the spec just says the UA aborted the load, but I'm not sure when we would do that).

What makes this one recoverable?

The rest of the patch looks fine, I just don't understand the conditions for when this is shown / removable.

Comment 47

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Justin Dolske [:Dolske] from comment #46)
> Why only ABORTED? I don't understand why this is the only one that's
> removable.

I see this as a possible list of error states that still allow the user to view the parts of the video that have already loaded and decoded successfully. We can add more to it in the future. For now, I only know of ABORTED as one that allows the user to watch part of the video.

> How would a user get a video into this state (the spec just says
> the UA aborted the load, but I'm not sure when we would do that).

If the user starts to navigate the page while the video is still loading but then cancels the navigation, the video can enter the aborted state.

> What makes this one recoverable?

Consider if the video is 4 minutes long, and the first 2 minutes have loaded prior to entering the aborted state. At this point, the user can still watch the video until it reaches the 2-minute mark.

> The rest of the patch looks fine, I just don't understand the conditions for
> when this is shown / removable.

Great! I hope the previous explanation makes it clearer. Perhaps more code comments would help.

Comment 48

[Justin Dolske [:Dolske]]

(In reply to Jared Wein [:jaws] from comment #47)

> > How would a user get a video into this state (the spec just says
> > the UA aborted the load, but I'm not sure when we would do that).
> 
> If the user starts to navigate the page while the video is still loading but
> then cancels the navigation, the video can enter the aborted state.

That seems fairly edgecase-ish.

> > What makes this one recoverable?
> 
> Consider if the video is 4 minutes long, and the first 2 minutes have loaded
> prior to entering the aborted state. At this point, the user can still watch
> the video until it reaches the 2-minute mark.

What happens when it hits the 2-minute mark? Do we show another error or does it just fall over?

Comment 49

[Justin Dolske [:Dolske]]

Comment on attachment 750673 [details] [diff] [review]
Patch v1.2

I guess I don't object strongly to it, so r+. Still curious about questions in last comment though.

Comment 50

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Justin Dolske [:Dolske] from comment #48)
> (In reply to Jared Wein [:jaws] from comment #47)
> 
> > > How would a user get a video into this state (the spec just says
> > > the UA aborted the load, but I'm not sure when we would do that).
> > 
> > If the user starts to navigate the page while the video is still loading but
> > then cancels the navigation, the video can enter the aborted state.
> 
> That seems fairly edgecase-ish.

It can also happen if the page is slow to load and the user hits Stop. But I agree that it is fairly edgecase-ish.
 
> > > What makes this one recoverable?
> > 
> > Consider if the video is 4 minutes long, and the first 2 minutes have loaded
> > prior to entering the aborted state. At this point, the user can still watch
> > the video until it reaches the 2-minute mark.
> 
> What happens when it hits the 2-minute mark? Do we show another error or
> does it just fall over?

The video pauses.

Comment 51

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d7c6d6061ab5

Bao, I have checked the patch in to our mozilla-inbound repository. In a day or two it will be merged to the mozilla-central repository. The day after that merge, it should appear in the Firefox Nightly builds. Thanks for your help!

Comment 52

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d7c6d6061ab5

Comment 53

[Bao Duong]

(In reply to Jared Wein [:jaws] from comment #51)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/d7c6d6061ab5
> 
> Bao, I have checked the patch in to our mozilla-inbound repository. In a day
> or two it will be merged to the mozilla-central repository. The day after
> that merge, it should appear in the Firefox Nightly builds. Thanks for your
> help!

It was my pleasure! I am glad I was given the opportunity to fix this bug and hope to work on more open source projects/bugs in the future.