1291013 - Closed-caption button spacing is asymetric

Status: VERIFIED FIXED

(Toolkit :: Video/Audio Controls, defect, P2)

Description

[Justin Dolske [:Dolske]]

Attachment: Screenshot

The closed-caption button on the default video control has strange spacing -- there are 7 pixels of space between it and the volume control on the left, and 20 pixels between it and the full-screen control on the right. The volume control has 13 pixels between it and the mute button on its left. (Those are hidpi pixels at 2x, so 1/2 that for normal CSS pixels).

Screenshot attached, using https://www.iandevlin.com/html5test/webvtt/html5-video-webvtt-sample.html as a demo page

Comment 1

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

Ray, can you take this bug? We should get this fixed in Firefox 50 since it should be a simple fix.

[Tracking Requested - why for this release]: Visible UI blurriness in new feature shipping in v50.

Comment 2

[Ray Lin[:ralin]]

Attachment: Bug 1291013 - Adjust closed caption button spacing.

Review commit: https://reviewboard.mozilla.org/r/68542/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68542/

Comment 3

[Ray Lin[:ralin]]

btw, fullscreen button is no longer the only button on the volume control's right, so move the margin from volumeStack to controlBar's padding.

Comment 4

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

Comment on attachment 8776877 [details]
Bug 1291013 - Adjust closed caption button spacing.

Please rebase this on top of the patch that I put in bug 1291268

Comment 5

[Ray Lin[:ralin]]

Comment on attachment 8776877 [details]
Bug 1291013 - Adjust closed caption button spacing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68542/diff/1-2/

Comment 6

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

Comment on attachment 8776877 [details]
Bug 1291013 - Adjust closed caption button spacing.

https://reviewboard.mozilla.org/r/68542/#review68100

::: toolkit/themes/shared/media/videocontrols.css:70
(Diff revision 2)
>  .muteButton[noAudio] + .volumeStack {
>    display: none;
>  }
>  
>  .closedCaptionButton {
> -  background-image: url(chrome://global/skin/media/closeCaptionButton.png);
> +  background: url(chrome://global/skin/media/closeCaptionButton.png) 4px;

Please don't use shorthand here. Keep the background-image property and then use a separate property for any other values you want to set.

::: toolkit/themes/shared/media/videocontrols.css:419
(Diff revision 2)
>    .muteButton[noAudio] {
>      background-image: url(chrome://global/skin/media/[e-mail]);
>      background-size: 33px 28px;
>    }
> -  .closeCaptionButton {
> -    background-image: url(chrome://global/skin/media/[e-mail]);
> +  .closedCaptionButton {
> +    background: url(chrome://global/skin/media/[e-mail]) 4px;

Same comment here about shorthand properties.

Comment 7

[Ray Lin[:ralin]]

Comment on attachment 8776877 [details]
Bug 1291013 - Adjust closed caption button spacing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68542/diff/2-3/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0e721493391d
Adjust closed caption button spacing. r=jaws

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0e721493391d

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Ray, since this is a regression in Fx50 and fixed in Nightly, could you please nominate a patch for uplift to Aurora? Thanks!

Comment 11

[Ray Lin[:ralin]]

Comment on attachment 8776877 [details]
Bug 1291013 - Adjust closed caption button spacing.

Approval Request Comment
[Feature/regressing bug #]: fix wrong spacing of closed caption button 
[User impact if declined]: weird spacing between controls' buttons
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: low risk. adjusting style only.
[String/UUID change made/needed]: none

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8776877 [details]
Bug 1291013 - Adjust closed caption button spacing.

Low risk fix for a recent regression, Aurora50+

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Justin, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/baaef0504ff7