Closed Bug 1291013 Opened 3 years ago Closed 3 years ago

Closed-caption button spacing is asymetric


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




Tracking Status
firefox50 + fixed
firefox51 --- verified


(Reporter: Dolske, Assigned: ralin)




(2 files)

Attached image 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 as a demo page
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.
Flags: needinfo?(ralin)
Assignee: nobody → ralin
Flags: needinfo?(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 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
Attachment #8776877 - Flags: review?(jaws)
Priority: -- → P2
Comment on attachment 8776877 [details]
Bug 1291013 - Adjust closed caption button spacing.

::: 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/noAudio@2x.png);
>      background-size: 33px 28px;
>    }
> -  .closeCaptionButton {
> -    background-image: url(chrome://global/skin/media/closeCaptionButton@2x.png);
> +  .closedCaptionButton {
> +    background: url(chrome://global/skin/media/closeCaptionButton@2x.png) 4px;

Same comment here about shorthand properties.
Attachment #8776877 - Flags: review?(jaws) → review+
Keywords: checkin-needed
Pushed by
Adjust closed caption button spacing. r=jaws
Keywords: checkin-needed
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Hi Ray, since this is a regression in Fx50 and fixed in Nightly, could you please nominate a patch for uplift to Aurora? Thanks!
Flags: needinfo?(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
Flags: needinfo?(ralin)
Attachment #8776877 - Flags: approval-mozilla-aurora?
Comment on attachment 8776877 [details]
Bug 1291013 - Adjust closed caption button spacing.

Low risk fix for a recent regression, Aurora50+
Attachment #8776877 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Hi Justin, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(dolske)
Flags: needinfo?(dolske)
You need to log in before you can comment on or make changes to this bug.