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

RESOLVED FIXED in Firefox 61

Status

()

P5
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jaws, Assigned: sreeise, Mentored)

Tracking

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

--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.
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.
(Assignee)

Comment 2

a year ago
(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?
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.
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.
Flags: needinfo?(reeisesean)
Mentor: jaws
Summary: Multiple CSS custom properties for button widths in video controls are defined but never referenced → Add a comment in browser_parsable_css.js to explain why the video controls CSS custom properties are defined but not referenced by CSS
Assignee: nobody → reeisesean
Status: NEW → ASSIGNED
A comment in the CSS file will be very helpful too.
(Assignee)

Comment 6

a year ago
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?
Flags: needinfo?(reeisesean)
(Assignee)

Comment 7

a year ago
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?
Flags: needinfo?(jaws)
(Assignee)

Comment 8

a year ago
I meant to reference this bug, I am sorry for the inconvenience.
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.
(Assignee)

Comment 10

a year ago
Possible patch

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

Comment 11

a year ago
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?
Attachment #8959239 - Attachment is obsolete: true
Flags: needinfo?(jaws)
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.
Flags: needinfo?(jaws)
(Assignee)

Comment 13

a year ago
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.
Attachment #8958688 - Attachment is obsolete: true
Attachment #8959262 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Flags: needinfo?(jaws)
Comment on attachment 8960355 [details] [diff] [review]
Patch - adds comment to browser_parsable_css.js

Thanks! I'll get this landed for you now :)
Flags: needinfo?(jaws)
Attachment #8960355 - Flags: review+

Comment 16

a year ago
Pushed by jwein@mozilla.com:
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a1e6f19dffce
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.