Closed Bug 495593 Opened 15 years ago Closed 10 years ago

video controls should match the size of the scaled video inside the video element

Categories

(Toolkit :: Video/Audio Controls, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
firefox31 --- wontfix
firefox32 --- fixed

People

(Reporter: Dolske, Assigned: DChen)

References

Details

(Whiteboard: [mentor=jaws][lang=js])

Attachments

(6 files, 11 obsolete files)

62.88 KB, image/png
Details
5.90 KB, patch
jaws
: review+
jaws
: checkin+
Details | Diff | Splinter Review
1.36 KB, patch
MattN
: review+
jaws
: checkin+
Details | Diff | Splinter Review
5.87 KB, patch
Details | Diff | Splinter Review
5.64 KB, patch
jaws
: feedback-
Details | Diff | Splinter Review
102.07 KB, image/png
Details
Attached image Screenshot
Currently, the size of the video controls matches the size of the video element, not the size of the video region within. For example, a 100x100 video inside <video style="width: 300px; height: 200px"> will result in a video region that's scaled to 200x200 (preserving aspect ratio), but the controls are 300 pixels wide.

This ends up looking ugly, and people will probably thing the videocontrols sizing is broken. I've noticed this happening twice on our own video demo sites, so it's fair to assume the rest of the web will get this wrong too. We talked about doing this early on; iirc the thinking then was that this wouldn't be a big deal, but now I think we need to fix it.

One approach to fix this would be to have the native anonymous content frame the controls are in scaled to fit the video region. Or the controls could adjust their own left/right padding (adjusting any time loadedmetadata fires). ISTR roc saying the former would be hard to do, so I'll try the latter.

Note that this effect can also happen in the vertical direction, eg a 100x100 video inside <video style="width: 100px; height: 200px">. I think the horizontal sizing is more important, but maybe we should set bottom-padding too when needed.
Component: Video/Audio → Video/Audio Controls
Product: Core → Toolkit
QA Contact: video.audio → video.audio
Target Milestone: --- → mozilla1.9.2a1
Version: Trunk → unspecified
Target Milestone: mozilla1.9.2a1 → ---
I have a patch for this, but what can we do if the video is resized programmatically after loadedmetadata was fired ?
Here is the implementation, with a test case here : http://paul.cx/test/controls-width.html. You can click on the "resize" button to trigger the problem I mentioned in the previous comment. I tried to work around this by recomputing the width when the controls are showing again after having been hidden.
Assignee: dolske → paul
Status: NEW → ASSIGNED
Attachment #549368 - Flags: review?(dolske)
(In reply to comment #1)
> I have a patch for this, but what can we do if the video is resized
> programmatically after loadedmetadata was fired ?

Not much directly, see also bug 462117 comment 6.

But I think just recomputing when showing the controls is probably good enough until we support resize events for arbitrary elements.
Has there been any progress on this bug?
Comment on attachment 549368 [details] [diff] [review]
Patch v0 - initial implementation

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

Also, it would sure be nice to have some reftests to ensure that the controls are sized/placed correctly in the various combinations of scaled/non-scaled, padding/border, rounding.

Probably just need to steal one of the existing OGG files that's just a flat black or white field (so that it looks the same scaled), and compare a test with the weird stuff to a reference with the video properly scaled (1:1) and positioned so it looks the same.

::: toolkit/content/widgets/videocontrols.xml
@@ +407,5 @@
> +                resizeControls : function() {
> +                  // Adapt the size of the controls to the size of the video
> +                  if (this.video.readyState >= this.video.HAVE_METADATA) {
> +                     if (!this.isAudioOnly && this.video.videoWidth != 0 && this.video.videoHeight != 0) {
> +                       var rect = this.video.getBoundingClientRect();

Hmm. I'm not sure if this is correct, offhand... Did you experiment with setting large borders/padding? ISTR the native anonymous content frame is the same size as the <video>'s content (ie, no border/padding), in which case getBoundingClientRect() would result in mis-sized controls.

Bug 470596 dealt with a similar issue.

I wonder if it would be useful to expose a .videoScaledHeight / .videoScaledWidth to help with cases like these...

@@ +410,5 @@
> +                     if (!this.isAudioOnly && this.video.videoWidth != 0 && this.video.videoHeight != 0) {
> +                       var rect = this.video.getBoundingClientRect();
> +                       var widthRatio = rect.width / this.video.videoWidth;
> +                       var heightRatio = rect.height / this.video.videoHeight;
> +                       var width = this.video.videoWidth * Math.min(widthRatio, heightRatio);

This will give you a fractional width in some cases... The actual video rectangle is going to be rounded to a whole-pixel value, but I'm not sure if that means rounding up or down.

@@ +414,5 @@
> +                       var width = this.video.videoWidth * Math.min(widthRatio, heightRatio);
> +                       this.controlBar.style.width = width + "px";
> +                       this.log("Resizing controls to "+ width + "px wide.");
> +
> +                       this.controlsOverlay.align = "center";

Should probably just set this in the CSS unconditionally, even if the controls don't need their size adjusted having it center-aligned will still look the same.

Also, seems like we should just go ahead and deal with the the same issue for videos are are scaled inside a <video> that's too tall.

Oh, and we should just do this for the entire binding, eg so that the error/status overlay is also scaled to fit the actual video.

@@ +415,5 @@
> +                       this.controlBar.style.width = width + "px";
> +                       this.log("Resizing controls to "+ width + "px wide.");
> +
> +                       this.controlsOverlay.align = "center";
> +                    }

There needs to be an |else| block so that if we're not explicitly setting .width, then it needs to be reset (.width = "") do that it's not sized to, say, the previous media loaded in the element.

@@ +696,3 @@
>                      // If the controls are static, don't change anything.
>                      if (!this.dynamicControls)
>                          return;

Nit: I'd slightly prefer to have the resize call after the dynamicControls check. Least-surprises principle. (Though currently I can't think of a case where this would actually matter.)
Attachment #549368 - Flags: review?(dolske) → review-
It would be great to get this picked back up and landed before the Firefox 10 cut-off (November 8th).

If Paul doesn't pick this bug back up, then someone else can pick up from where he left off.
Assignee: paul → nobody
Status: ASSIGNED → NEW
Whiteboard: [good first bug][mentor=jwein]
I'm waiting for my new machine to be shipped to me (as my current machine takes hours to compile), and I'm rebasing and continuing work on all my bugs. It should be feasible before the 8th.
Whiteboard: [good first bug][mentor=jwein] → [good first bug][mentor=jwein][lang=js]
Hello people. I am new around here. I would like to work on this bug. Please guide me on what to do.
Hi Pallani! As a start, can you use what Paul implemented and apply the suggestions that Justin gave in comment #5?

Let me know if you would like more guidance. I'm happy to help :)
Assignee: nobody → pallanikumaran
Status: NEW → ASSIGNED
Hey Jared, I have looked through the patch. Would you be able to guide me on applying the patch? I am having problem applying the patch.
Hi Pallani: I'd be glad to help, although I may be a little slow to respond this week.

To apply the patch, you can use Mercurial Queues (MQ). See http://hgbook.red-bean.com/read/mercurial-queues-reference.html and https://developer.mozilla.org/en/Mercurial_Queues for more information about MQ.

Using MQ:
1) hg qimport https://bugzilla.mozilla.org/attachment.cgi?id=549368 -n 495593.patch
2) hg qpush 495593.patch
3) Look at the reject file (with file extension .rej) and see what lines Mercurial had trouble applying. These should be easily fixable by hand, but may require reading through the surrounding code.
4) Once the changes are applied, you can do: hg qref

At this point the patch is applied and you can do your work on this patch.
This is my patch which made the changes mentioned in the review of Justin's patch.
Comment on attachment 597732 [details] [diff] [review]
Modifications to Justin's initial implementation

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

Thanks for writing a patch. Here are some issues that need to be addressed in a new revision of the patch before we can approve it.

::: toolkit/content/widgets/videocontrols.css
@@ +27,5 @@
>  }
>  
> +.controlsOverlay{
> +	align = center;
> +}

This is invalid CSS. It should be:

.controlsOverlay {
  -moz-box-align: center;
}

::: toolkit/content/widgets/videocontrols.xml
@@ +484,5 @@
>                         var width = this.video.videoWidth * Math.min(widthRatio, heightRatio);
>                         this.controlBar.style.width = width + "px";
>                         this.log("Resizing controls to "+ width + "px wide.");
> +                    }
> +                    else{

This should be:

} else {

@@ +848,5 @@
>                      }
>                  },
>  
>                  onMouseInOut : function (event) {
> +                   

Please remove the whitespace from this line.

@@ +853,5 @@
>                      // If the controls are static, don't change anything.
>                      if (!this.dynamicControls)
>                          return;
> +                     // Resize the controls, in case their size have changed.
> +                  	this.resizeControls();

The indentation is incorrect here.
Please remove one space before // and three spaces before this.resizeControls();
Attachment #597732 - Attachment is obsolete: true
Comment on attachment 598178 [details] [diff] [review]
Modified as per :fyrn suggestions

I'll try to give feedback on this today or tomorrow.

On the next patch, please request feedback or review since it is easy to miss patches without pending requests attached to them.
Attachment #598178 - Attachment is patch: true
Attachment #598178 - Flags: feedback?(jwein)
Comment on attachment 598178 [details] [diff] [review]
Modified as per :fyrn suggestions

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

Have you checked to see what the video statistics and error overlays look like with these patches applied? We should include some reftests for those.

The next steps of this bug:
1. Fix the nit mentioned below.
2. Test this out manually to see if this is doing what we want.
3. Create some reftests to make sure that nobody in the future breaks the code that you've been working on :)

I am happy to help with any/all of the above if you would like my help. Let me know if you have any questions.

::: toolkit/content/widgets/videocontrols.css
@@ +26,5 @@
>    text-decoration: none !important;
>  }
>  
> +.controlsOverlay{
> +	-moz-box-align: center;

nit: this line should start with two space characters and no tab characters.
Attachment #598178 - Flags: feedback?(jwein) → feedback+
Does nit mean the object of nitpicking? If it is, that's cool. I will go find out what reftest.
(In reply to Pallani Kumaran from comment #17)
> Does nit mean the object of nitpicking? If it is, that's cool. I will go
> find out what reftest.

Yeah, it means nitpicking :)
Attached patch Removed nit (obsolete) — Splinter Review
Attachment #598178 - Attachment is obsolete: true
Attachment #599274 - Flags: review+
Comment on attachment 599274 [details] [diff] [review]
Removed nit

Thanks for fixing the nit. I've removed the r+ since there is still some more work to do and this hasn't been through an official review yet.

Pallani: Can you work on those other two points that I mentioned above?
Attachment #599274 - Flags: review+ → feedback+
Attached image Screenshot of current patchset (obsolete) —
This is a screenshot of the current patch. I've applied it and the background of the clickToPlay overlay no longer covers the video. Further, the controlsSpacer is no longer wide enough to hit with the mouse to toggle playback.

I've tried investigating this further, and I found that removing the |-moz-box-align: center| fixed the issue. I tried tweaking the |background-size| of the clickToPlay overlay to |contain| and |cover|, but neither made a difference.

Pallani: Can you investigate a different way to achieve the goal of this bug without using -moz-box-align?
Whiteboard: [good first bug][mentor=jwein][lang=js] → [good first bug][mentor=jaws][lang=js]
Can you confirm that you're still working on this bug?
Flags: needinfo?(pallanikumaran)
Status: ASSIGNED → NEW
I'm reopening this bug up for others due to a lack of response and my interpretation of the ASSIGNED -> NEW movement two days ago.

Pallani, please retake the bug if you are still working on it.
Assignee: pallanikumaran → nobody
Flags: needinfo?(pallanikumaran)
Whiteboard: [good first bug][mentor=jaws][lang=js] → [mentor=jaws][lang=js]
Attached patch Patch 2 (obsolete) — Splinter Review
Do we have any other examples of videos contained in elements with mismatched aspect ratios?
Attachment #8400858 - Flags: feedback?(jaws)
The spacer error is still apparent when my page is loading but I think once the video metadata(?) has loaded, the size adjustments are called and everything appears scaled.
Attached patch Patch 3 (obsolete) — Splinter Review
I realized attachment 8400858 [details] [diff] [review] doesn't ever correct controlsSpacer's size for audio-only video elements, so I changed -moz-box-align to be applied conditionally.
Attachment #8400858 - Attachment is obsolete: true
Attachment #8400858 - Flags: feedback?(jaws)
Attachment #8400948 - Flags: feedback?(jaws)
Comment on attachment 8400948 [details] [diff] [review]
Patch 3

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

Just some drive-by feedback first. Thanks for putting together a patch!

::: toolkit/content/widgets/videocontrols.xml
@@ +921,5 @@
>                      if (!this.dynamicControls)
>                          return;
>  
> +                    // Resize controls in case size has changed
> +                    this.adjustControlSize();

We now have a resizevideocontrols event that is used to adjust the control size when the media element is resized.

@@ +1405,5 @@
> +                        this.controlsOverlay.style.width = width + "px";
> +                        this.controlsSpacer.style.width = width + "px";
> +                        this.controlBar.style.width = width + "px";
> +                      } else {
> +                        this.controlsOverlay.removeAttribute("alignCenter");

We should refrain from using attribute names that are closely tied with visual outcomes.
Attachment #8400948 - Flags: feedback?(jaws) → feedback+
Assignee: nobody → dannychen210
Status: NEW → ASSIGNED
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #27)
> We should refrain from using attribute names that are closely tied with
> visual outcomes.

What would be more suitable? Something like 'scaleControlsToVideo' or 'notFullWidth'?
Flags: needinfo?(jaws)
(In reply to Danny Chen [:DChen] from comment #28)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #27)
> > We should refrain from using attribute names that are closely tied with
> > visual outcomes.
> 
> What would be more suitable? Something like 'scaleControlsToVideo' or
> 'notFullWidth'?

Let's go with 'scaled'
Flags: needinfo?(jaws)
Attached patch Patch 4 (obsolete) — Splinter Review
I renamed the attribute and removed some changes left over from exploring alternatives to mox-boz-align.
Attachment #8400861 - Attachment is obsolete: true
Attachment #8400948 - Attachment is obsolete: true
Attachment #8403513 - Flags: feedback?(jaws)
Comment on attachment 8403513 [details] [diff] [review]
Patch 4

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

This looks good! Sorry about the slow response here.

You can make the following change and re-upload the patch with an updated commit message, adding 'r=jaws' to the end of it.

::: toolkit/content/widgets/videocontrols.xml
@@ +1391,5 @@
>                      let videoWidth = isAudioOnly ? minWidthAllControls : this.video.clientWidth;
>  
> +                    // Adapt the size of the controls to the size of the video
> +                    if (this.video.readyState >= this.video.HAVE_METADATA) {
> +                      if (!this.isAudioOnly && this.video.videoWidth != 0 && this.video.videoHeight != 0) {

We can simplify this to:
if (!this.isAudioOnly && this.video.videoWidth && this.video.videoHeight) {
Attachment #8403513 - Flags: feedback?(jaws) → review+
Comment on attachment 8403513 [details] [diff] [review]
Patch 4

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

::: toolkit/content/widgets/videocontrols.xml
@@ +1441,5 @@
>                      this.durationLabel = document.getAnonymousElementByAttribute(binding, "class", "durationLabel");
>                      this.positionLabel = document.getAnonymousElementByAttribute(binding, "class", "positionLabel");
>                      this.statusOverlay = document.getAnonymousElementByAttribute(binding, "class", "statusOverlay");
>                      this.statsOverlay  = document.getAnonymousElementByAttribute(binding, "class", "statsOverlay");
> +                    this.controlsOverlay = document.getAnonymousElementByAttribute(binding, "class", "controlsOverlay");

Oh, you'll also need to add `controlsOverlay` as a property in the list at the beginning of `this.Utils = {`
Attached patch Patch 4.1Splinter Review
Attachment #8403513 - Attachment is obsolete: true
Attachment #8408644 - Flags: review?(jaws)
Attachment #549368 - Attachment is obsolete: true
Attachment #599274 - Attachment is obsolete: true
Attachment #599274 - Attachment is patch: true
Attachment #600474 - Attachment is obsolete: true
Comment on attachment 8408644 [details] [diff] [review]
Patch 4.1

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

looks good, thanks!
Attachment #8408644 - Flags: review?(jaws) → review+
https://hg.mozilla.org/integration/fx-team/rev/f8118ec22ed9
Keywords: checkin-needed
Whiteboard: [mentor=jaws][lang=js] → [mentor=jaws][lang=js][fixed-in-fx-team]
This typo caused test_videocontrols_standalone.html to fail, which makes a lot of sense.
Attachment #8409141 - Flags: review?(MattN+bmo)
Attachment #8409141 - Flags: review?(MattN+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/f8118ec22ed9
https://hg.mozilla.org/mozilla-central/rev/667b8d9d4c7b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=jaws][lang=js][fixed-in-fx-team] → [mentor=jaws][lang=js]
Target Milestone: --- → mozilla31
Depends on: 1005031
We should back out this patch since it caused bug 1005031, which doesn't appear to be getting any attention.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): backout, the patch caused bug 1005031
User impact if declined: B2G and mobile users will see controls in the middle of their video
Testing completed (on m-c, etc.): backout of the patches in this bug
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none
Attachment #8424851 - Flags: approval-mozilla-aurora?
Attachment #8424851 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attached patch Android Patch v1 (obsolete) — Splinter Review
I noticed the width changes don't affect the Android video controls while working on Bug 1005031. The padding doesn't appear on the left/right when resized so the width is recalculated assuming 20 pixels of open space on both sides.
Attachment #8432171 - Flags: feedback?(jaws)
Attached patch Android patch v2 (obsolete) — Splinter Review
A closing bracket was moved into the wrong patch while splitting.
Attachment #8432171 - Attachment is obsolete: true
Attachment #8432171 - Flags: feedback?(jaws)
Attachment #8432175 - Flags: feedback?(jaws)
Comment on attachment 8432175 [details] [diff] [review]
Android patch v2

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

::: toolkit/content/widgets/videocontrols.xml
@@ +1399,5 @@
>                          var heightRatio = rect.height / this.video.videoHeight;
>                          var width = this.video.videoWidth * Math.min(widthRatio, heightRatio);
> +                        
> +                        if (this.videocontrols.isTouchControl) {
> +                          var touchControlsPadding = 40;

is there a way that we can have this computed instead of hardcoded?
Attachment #8432175 - Flags: feedback?(jaws)
Attached patch Android patch v3Splinter Review
Disabled moz-box-flex to enable scaling. I can't seem to affect controlBar using width:100% or width:auto though. Currently re-sizing each touch control whether it's for a video, video element with audio-only media, or audio element.
Attachment #8432175 - Attachment is obsolete: true
Attachment #8436274 - Flags: feedback?(jaws)
Comment on attachment 8436274 [details] [diff] [review]
Android patch v3

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

The previous patch was a lot simpler. Why did it need to change so much between versions?

::: toolkit/content/widgets/videocontrols.xml
@@ +1378,5 @@
>                                                this._muteButtonWidth +
>                                                this._volumeControlWidth +
>                                                this._fullscreenButtonWidth;
>  
> +                    let isTouchControl = this.videocontrols.isTouchControl;                          

nit, trailing whitespace here and elsewhere
Attachment #8436274 - Flags: feedback?(jaws) → feedback-
The previous patch resizes videos but disabling -moz-box-flex to do so means the audio-only <video> / <audio> elements need to be changed. I tried using width:auto or width:100% in the previous patch but the controlBar becomes a default size.
Flags: needinfo?(jaws)
Do you have ideas on how to set the original width but allowing the controls to resize?
(In reply to Danny Chen [:DChen] from comment #47)
> Do you have ideas on how to set the original width but allowing the controls
> to resize?

I've tried thinking of ideas here but I couldn't come up with anything.
Flags: needinfo?(jaws)
You need to log in before you can comment on or make changes to this bug.