Video controls should listen for |stalled| event

RESOLVED FIXED in mozilla8

Status

()

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

People

(Reporter: Dolske, Assigned: jaws)

Tracking

unspecified
mozilla8
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

8 years ago
Spun off from bug 470983.

When a video fires a |stalled| event (because it's waiting on data from the network, but nothing in coming in), it would be good to show some UI to indicate this state. We should already be showing the throbber by the time |stalled| fires, but it seems like we should differentiate between "data's coming in, sit tight and we'll be right with you" and "aww, crap, nothing's happening, go grab some coffee and a Snicker's bar".

Possible ideas:
 * Add text below the throbber. EG "Waiting", "Server not responding", etc.
 * Replace fast-spinning throbber with a slow-spinning throbber?

Localization of the text will be a bit of a pain, because the unprivileged video control code can't just load up a string bundle (as normal chrome would do). XUL does generated content now (on 1.9.2, at least, via bug 472500), so it might suffice to pull in some localized CSS.
(Reporter)

Updated

8 years ago
Component: Video/Audio → Video/Audio Controls
Product: Core → Toolkit
QA Contact: video.audio → video.audio
Version: Trunk → unspecified
Assignee: nobody → jwein
Status: NEW → ASSIGNED
Created attachment 544878 [details] [diff] [review]
Patch for bug 481082

With this patch, the throbber will spin backwards with the occurrence of the |stalled| event. We may want to change the graphic (or slow down the spinning).
Attachment #544878 - Flags: review?(dolske)
Created attachment 544879 [details] [diff] [review]
Patch for bug 481082

With this patch, the throbber will spin backwards with the occurrence of the |stalled| event. We may want to change the graphic (or slow down the spinning).

[Previous patch was missing updates for pinstripe theme]
Attachment #544878 - Attachment is obsolete: true
Attachment #544879 - Flags: review?(dolske)
Attachment #544878 - Flags: review?(dolske)
Created attachment 545792 [details] [diff] [review]
Patch for bug 481082

Updated the graphic to be a slower spinning throbber moving in the backwards direction.
Attachment #544879 - Attachment is obsolete: true
Attachment #545792 - Flags: review?(dolske)
Attachment #544879 - Flags: review?(dolske)
(Reporter)

Comment 4

6 years ago
Comment on attachment 545792 [details] [diff] [review]
Patch for bug 481082

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

::: toolkit/content/widgets/videocontrols.xml
@@ +315,5 @@
>                  setupStatusFader : function(immediate) {
>                      var show = false;
>                      if (this.video.seeking ||
>                          this.video.error ||
> +                        this.stalled ||

I don't think we want to check for stalled here, since you could be both in a stalled state and currently playing from a previously-buffered region.

We need to track the stalled state, but can just rely on the existing "waiting" event handling to trigger the throbber overlay.

Since there are a lot of other events setting the overlay type to "throbber", I suspect it would be easiest to just set an attribute ("stalled") on the throbber overlay (upon receiving stalled, and clear it upon receiving progress). And let CSS handle which image to show.

EG:

.statusIcon[type="throbber"] {
    background: url(chrome://global/skin/media/throbber.png) no-repeat center;
}
.statusIcon[type="throbber"][stalled] {
    background-image: url(stalled.png);
}
Attachment #545792 - Flags: review?(dolske) → review-
Created attachment 547288 [details] [diff] [review]
Patch for bug 481082 v2

I made the recommended changes and was able to remove the |this.stalled| property since it was redundant.
Attachment #545792 - Attachment is obsolete: true
Attachment #547288 - Flags: review?(dolske)
(Reporter)

Updated

6 years ago
Attachment #547288 - Flags: review?(dolske) → review+
Keywords: checkin-needed
http://hg.mozilla.org/integration/mozilla-inbound/rev/ec66137aed05
Flags: in-testsuite?
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/ec66137aed05
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
Blocks: 697124
(Reporter)

Updated

6 years ago
No longer blocks: 697124
You need to log in before you can comment on or make changes to this bug.