Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in mozilla24

Status

()

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

People

(Reporter: jaws, Assigned: Bao Duong)

Tracking

Trunk
mozilla24
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=jaws][lang=js], URL)

Attachments

(1 attachment, 15 obsolete attachments)

4.76 KB, patch
Dolske
: review+
Details | Diff | Splinter Review
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.
Assignee: nobody → bharath_ves
Status: NEW → ASSIGNED
bharath: How do you think we should fix this bug?
The link that is in the URL field can be used to test the different error cases.
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
(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.
Yes ,I have added the necessary changes what you have mentioned earlier , but statusOverlay doesn't receive any click event
(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?
Created attachment 603567 [details] [diff] [review]
added the event click listener to statusOverlay
Attachment #603567 - Flags: review+
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 "+".
Attachment #603567 - Flags: review+ → feedback?(jwein)
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 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.
Attachment #603567 - Flags: feedback?(jwein) → feedback+
Created attachment 603879 [details] [diff] [review]
patch that is close but not all the way there
Created attachment 603880 [details] [diff] [review]
patch that is closer but the events don't seem to fire yet
Attachment #603879 - Attachment is obsolete: true
Created attachment 603924 [details] [diff] [review]
Formatted Patch to the previous one
Attachment #603924 - Flags: review?
Attachment #603567 - Attachment is obsolete: true
Attachment #603880 - Attachment is obsolete: true
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.
Attachment #603924 - Flags: review?
Created attachment 604954 [details] [diff] [review]
Added Click Event Listener to the StatusOverlay in clickToPlayClickHandler
Attachment #604954 - Flags: review?
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.
Attachment #604954 - Flags: review?
Attachment #603924 - Attachment is obsolete: true
Whiteboard: [good first bug][mentor=jwein][lang=js][lang=css] → [good first bug][mentor=jwein][lang=js]
Created attachment 605278 [details] [diff] [review]
Modified the clickToPlayClickHandler
Attachment #605278 - Flags: review?
Attachment #605278 - Flags: review? → review?(jwein)
Attachment #604954 - Attachment is obsolete: true
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'.
Attachment #605278 - Flags: review?(jwein) → feedback+
Created attachment 605367 [details] [diff] [review]
Modified the clickToPlayClickHandler and added the Event click Listener to the statusOverlay
Attachment #605278 - Attachment is obsolete: true
Attachment #605367 - Flags: review?(jwein)
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.
Attachment #605367 - Flags: review?(jwein) → feedback+
https://hg.mozilla.org/mozilla-central/rev/edf115968196 which claimed to be for this bug was actually for bug 730636
bharath: Can you upload a new patch that addresses the feedback in comment #20?
Whiteboard: [good first bug][mentor=jwein][lang=js] → [good first bug][mentor=jaws][lang=js]
Assignee: bharath_ves → nobody
Status: ASSIGNED → NEW

Comment 23

4 years ago
Hey, We would like to work on this bug. Thank you!
Assignee: nobody → seanchen
Status: NEW → ASSIGNED
(Assignee)

Comment 24

4 years ago
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 :)
(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.
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.
(Assignee)

Comment 27

4 years ago
Created attachment 743430 [details] [diff] [review]
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?
Attachment #743430 - Flags: review?
Created attachment 743444 [details] [diff] [review]
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.
Attachment #605367 - Attachment is obsolete: true
Attachment #743430 - Attachment is obsolete: true
Attachment #743430 - Flags: review?
Flags: needinfo?(seanchen)
(Assignee)

Comment 29

4 years ago
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?
(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).
Assignee: seanchen → bduong
Flags: needinfo?(seanchen)
Version: unspecified → Trunk
(Assignee)

Comment 31

4 years ago
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.
Flags: needinfo?(jaws)
(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.
Flags: needinfo?(jaws)
(Assignee)

Comment 33

4 years ago
Created attachment 748997 [details] [diff] [review]
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!
Attachment #748997 - Flags: review?
Attachment #748997 - Flags: review? → review?(jaws)
(Assignee)

Comment 34

4 years ago
Created attachment 749158 [details] [diff] [review]
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?
Attachment #749158 - Flags: review?(jaws)
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.
Attachment #748997 - Flags: review?(jaws) → feedback+
Attachment #749158 - Flags: review?(jaws)
(Assignee)

Comment 36

4 years ago
Created attachment 749601 [details] [diff] [review]
Combined patches and added an X icon to the dismiss button.
Attachment #748997 - Attachment is obsolete: true
Attachment #749158 - Attachment is obsolete: true
Attachment #749601 - Flags: review?(jaws)
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.
Attachment #743444 - Attachment is obsolete: true
Attachment #749601 - Attachment is obsolete: true
Attachment #749601 - Flags: review?(jaws)
Attachment #749637 - Flags: feedback?(bduong)
(Assignee)

Comment 38

4 years ago
(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?
(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?
(Assignee)

Comment 40

4 years ago
Created attachment 750183 [details] [diff] [review]
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?
Attachment #750183 - Flags: review?(jaws)
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.
Attachment #750183 - Flags: review?(jaws)
Attachment #749637 - Flags: feedback?(bduong)
(Assignee)

Comment 42

4 years ago
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.
Created attachment 750450 [details] [diff] [review]
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.
Attachment #749637 - Attachment is obsolete: true
Attachment #750183 - Attachment is obsolete: true
Flags: needinfo?(bduong)
(Assignee)

Comment 44

4 years ago
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!
Flags: needinfo?(bduong)
Created attachment 750673 [details] [diff] [review]
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?
Attachment #750450 - Attachment is obsolete: true
Attachment #750673 - Flags: review?(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.
Flags: needinfo?(jaws)
(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.
Flags: needinfo?(jaws)
(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 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.
Attachment #750673 - Flags: review?(dolske) → review+
(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.
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!
https://hg.mozilla.org/mozilla-central/rev/d7c6d6061ab5
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
(Assignee)

Comment 53

4 years ago
(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.
You need to log in before you can comment on or make changes to this bug.