Closed
Bug 730626
Opened 13 years ago
Closed 12 years ago
Add the ability to hide the error message overlay on HTML5 videos
Categories
(Toolkit :: Video/Audio Controls, defect)
Toolkit
Video/Audio Controls
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: jaws, Assigned: bduong)
References
()
Details
(Whiteboard: [good first bug][mentor=jaws][lang=js])
Attachments
(1 file, 15 obsolete files)
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.
Reporter | ||
Updated•13 years ago
|
Assignee: nobody → bharath_ves
Status: NEW → ASSIGNED
Reporter | ||
Comment 1•13 years ago
|
||
bharath: How do you think we should fix this bug?
Reporter | ||
Comment 2•13 years ago
|
||
The link that is in the URL field can be used to test the different error cases.
Comment 3•13 years ago
|
||
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
Reporter | ||
Comment 4•13 years ago
|
||
(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•13 years ago
|
||
Yes ,I have added the necessary changes what you have mentioned earlier , but statusOverlay doesn't receive any click event
Reporter | ||
Comment 6•13 years ago
|
||
(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•13 years ago
|
||
Attachment #603567 -
Flags: review+
Reporter | ||
Comment 8•13 years ago
|
||
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)
Reporter | ||
Comment 9•13 years ago
|
||
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
Reporter | ||
Comment 10•13 years ago
|
||
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+
Reporter | ||
Comment 11•13 years ago
|
||
Reporter | ||
Comment 12•13 years ago
|
||
Attachment #603879 -
Attachment is obsolete: true
Comment 13•13 years ago
|
||
Attachment #603924 -
Flags: review?
Reporter | ||
Updated•13 years ago
|
Attachment #603567 -
Attachment is obsolete: true
Reporter | ||
Updated•13 years ago
|
Attachment #603880 -
Attachment is obsolete: true
Reporter | ||
Comment 14•13 years ago
|
||
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?
Comment 15•13 years ago
|
||
Attachment #604954 -
Flags: review?
Reporter | ||
Comment 16•13 years ago
|
||
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?
Reporter | ||
Updated•13 years ago
|
Attachment #603924 -
Attachment is obsolete: true
Reporter | ||
Updated•13 years ago
|
Whiteboard: [good first bug][mentor=jwein][lang=js][lang=css] → [good first bug][mentor=jwein][lang=js]
Comment 17•13 years ago
|
||
Attachment #605278 -
Flags: review?
Updated•13 years ago
|
Attachment #605278 -
Flags: review? → review?(jwein)
Reporter | ||
Updated•13 years ago
|
Attachment #604954 -
Attachment is obsolete: true
Reporter | ||
Comment 18•13 years ago
|
||
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+
Comment 19•13 years ago
|
||
Attachment #605278 -
Attachment is obsolete: true
Attachment #605367 -
Flags: review?(jwein)
Reporter | ||
Comment 20•13 years ago
|
||
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+
Comment 21•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/edf115968196 which claimed to be for this bug was actually for bug 730636
Reporter | ||
Comment 22•13 years ago
|
||
bharath: Can you upload a new patch that addresses the feedback in comment #20?
Reporter | ||
Updated•13 years ago
|
Whiteboard: [good first bug][mentor=jwein][lang=js] → [good first bug][mentor=jaws][lang=js]
Reporter | ||
Updated•12 years ago
|
Assignee: bharath_ves → nobody
Status: ASSIGNED → NEW
Comment 23•12 years ago
|
||
Hey, We would like to work on this bug. Thank you!
Reporter | ||
Updated•12 years ago
|
Assignee: nobody → seanchen
Status: NEW → ASSIGNED
Assignee | ||
Comment 24•12 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 :)
Reporter | ||
Comment 25•12 years ago
|
||
(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.
Reporter | ||
Comment 26•12 years ago
|
||
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•12 years ago
|
||
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?
Reporter | ||
Comment 28•12 years ago
|
||
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•12 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?
Reporter | ||
Comment 30•12 years ago
|
||
(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•12 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.
Reporter | ||
Updated•12 years ago
|
Flags: needinfo?(jaws)
Reporter | ||
Comment 32•12 years ago
|
||
(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•12 years ago
|
||
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?
Reporter | ||
Updated•12 years ago
|
Attachment #748997 -
Flags: review? → review?(jaws)
Assignee | ||
Comment 34•12 years ago
|
||
I have changed the position of the dismiss button. Does this look okay?
Attachment #749158 -
Flags: review?(jaws)
Reporter | ||
Comment 35•12 years ago
|
||
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+
Reporter | ||
Updated•12 years ago
|
Attachment #749158 -
Flags: review?(jaws)
Assignee | ||
Comment 36•12 years ago
|
||
Attachment #748997 -
Attachment is obsolete: true
Attachment #749158 -
Attachment is obsolete: true
Attachment #749601 -
Flags: review?(jaws)
Reporter | ||
Comment 37•12 years ago
|
||
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•12 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?
Reporter | ||
Comment 39•12 years ago
|
||
(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•12 years ago
|
||
Added what you suggested. Is there anything else we need to do for this bug?
Attachment #750183 -
Flags: review?(jaws)
Reporter | ||
Comment 41•12 years ago
|
||
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.
Reporter | ||
Updated•12 years ago
|
Attachment #750183 -
Flags: review?(jaws)
Reporter | ||
Updated•12 years ago
|
Attachment #749637 -
Flags: feedback?(bduong)
Assignee | ||
Comment 42•12 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.
Reporter | ||
Comment 43•12 years ago
|
||
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•12 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)
Reporter | ||
Comment 45•12 years ago
|
||
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 46•12 years ago
|
||
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.
Updated•12 years ago
|
Flags: needinfo?(jaws)
Reporter | ||
Comment 47•12 years ago
|
||
(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)
Comment 48•12 years ago
|
||
(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•12 years ago
|
||
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+
Reporter | ||
Comment 50•12 years ago
|
||
(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.
Reporter | ||
Comment 51•12 years ago
|
||
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•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Assignee | ||
Comment 53•12 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.
Description
•