1441857 - Add a comment in browser_parsable_css.js to explain why the video controls CSS custom properties are defined but not referenced by CSS

Status: RESOLVED FIXED

(Toolkit :: Video/Audio Controls, enhancement, P5)

Description

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

--muteButton-width,
--closedCaptionButton-width,
--fullscreenButton-width,
--positionDurationBox-width,
--durationSpan-width,
--positionDurationBox-width-long,
--durationSpan-width-long

These were introduced by bug 1373537 but never referenced.

Comment 1

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

Bug 1441882 added a test that catches these types of errors. When fixing this, the entry for this bug will need to be removed from the whitelist in browser_parsable_css.js.

Comment 2

[Sean Reeise [:sreeise]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #1)
> Bug 1441882 added a test that catches these types of errors. When fixing
> this, the entry for this bug will need to be removed from the whitelist in
> browser_parsable_css.js.

Hey Jared, wanted to comment and say I can try to take a look. Wanted to ask a couple of questions just so I am sure, I am still getting used to the large repository. The css custom properties are not referenced anywhere so they can be removed from what I understand. And they are under video controls and the tests for whitelisting them are in browser_parsable_css.js?

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

They are being referenced in JavaScript in videocontrols.xml.

https://dxr.mozilla.org/mozilla-central/rev/a6f5fb18e6bcc9bffe4a0209a22d8a25510936be/toolkit/content/widgets/videocontrols.xml#384-393

Unfortunately, it's not very obvious; we should either add a comment there or find another way to keep these variables.

Comment 4

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

Thanks for catching that Tim! I should have dug a little deeper. Sean, let's find a new bug for you to work on as this one is mostly invalid. Though to "fix" this bug we can just add a comment to browser_parsable_css.js to say why these are not referenced by any CSS file.

Can you put up a patch with the added comment? This will still give you experience with submitting patches for review and seeing how patches make their way in to mozilla-central. I'll find you another bug that is more meaningful in the meantime.

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

A comment in the CSS file will be very helpful too.

Comment 6

[Sean Reeise [:sreeise]]

Yes I definitely can. I will start on it now. It might take me a little longer, as in a few minutes, since I have not done this before. Besides submitting a patch, is their anything particular I am suppose to do on Bugzilla?

Comment 7

[Sean Reeise [:sreeise]]

Attachment: Patch - adds comment to browser_parsable_css.js

Possible patch or partial patch. Added comment to browser_parsable_css.js that explains why custom properties were not being referenced. I separated the custom properties for Bug 144857 from the others and added the comment. Will something like this work? Ill be glad to change what ever is needed.

I was not sure where to add comments in the CSS file since they are not referenced, would a small line below the license work? Just to be sure, this is in videocontrols.css that the comment needs to be added as well?

Comment 8

[Sean Reeise [:sreeise]]

I meant to reference this bug, I am sorry for the inconvenience.

Comment 9

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

Comment on attachment 8958688 [details] [diff] [review]
Patch - adds comment to browser_parsable_css.js

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

The CSS comment that Tim is referring to should go in the /toolkit/themes/shared/media/videocontrols.css file on the line above the definitions for these properties.

::: browser/base/content/test/static/browser_parsable_css.js
@@ +114,5 @@
>    // Bug 1442300
>    {propName: "--in-content-category-background",
>     isFromDevTools: false},
>  
> +  // Bug 1441857 These custom properties are retrieved directly from CSSOM 

can you remove the trailing whitespace on this line and the one below? as well, you can remove the bug number here.

Comment 10

[Sean Reeise [:sreeise]]

Attachment: Comments for why CSS is defined but never referenced and removed whitespace

Possible patch

Removed Bug # and white space from browser_parsable_css.js. Added comment to videocontrols.css to explain custom properties as well.

Comment 11

[Sean Reeise [:sreeise]]

Attachment: Comments for why CSS is defined but never referenced and removed whitespace

Updated from last patch

Adds comments on why CSS custom properties is defined but never referenced.
Removed the extra white space, except for at the bottom on the patch. Not sure how to get rid of that last one because the white space is created when the patch is done. Any ideas?

Comment 12

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

It looks like this new patch you've requested review on is a commit on top of your previous commit. You should roll those two commits together. You can use the `hg histedit` command to "roll" the two commits together, discarding the commit message of the most recent one. Then when you make any more changes you can run the `hg amend` command to amend your new changes on top of the previous commit.

Comment 13

[Sean Reeise [:sreeise]]

Attachment: Patch - adds comment to browser_parsable_css.js

I did not realize it was spread across two commits, sorry about that. This patch should only be 1 commit with using hg histedit to roll the last commit.

Comment 14

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

Comment on attachment 8960355 [details] [diff] [review]
Patch - adds comment to browser_parsable_css.js

Thanks! I'll get this landed for you now :)

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I will rebase bug 1444489 on top of this one.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1e6f19dffce
Added comment explaining custom properties are directly from CSSOM in order to get predefined style which is why they are not referenced by CSS. r=jaws

Comment 17

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/a1e6f19dffce