Stop preprocessing mobile/android/themes

RESOLVED FIXED in Firefox 45

Status

()

RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: nalexander, Assigned: martianwars, Mentored)

Tracking

(Depends on: 1 bug)

Trunk
Firefox 45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 affected, firefox45 fixed)

Details

(Whiteboard: [lang=css][good next bug])

Attachments

(1 attachment, 2 obsolete attachments)

Bob Dylan famously sang, "The times, they are a changin'."  Indeed, the CSS times have changed: CSS has grown features that should allow us to stop preprocessing mobile/android/themes!  Huzzah!

Right now we have a defines.inc included in several CSS files, and we then use @variable@ to text substitute.  Let's do that in CSS.

Part 1: The actual number of consumed variables is low; we can purge most of defines.inc before moving forward.

Part 2: Then we can convert the defines.inc into a regular CSS file and use :root { --variable: value } to set the variables.  See https://developer.mozilla.org/en-US/docs/Web/CSS/Using_CSS_variables, and the existing examples in tree.  I see some non-standard "mozmm" units; I hope those can all be purged in Part 1.

Part 3: Then we @import the new defines.css and update all consumers to use var(--variable) in the consuming CSS.  See https://developer.mozilla.org/en/docs/Web/CSS/@import.

Part 4: test and profit!
Hi Kalpesh,

Are you in getting your hands dirty with some CSS stuff.
Please let me know if you prefer some Java tasks
Flags: needinfo?(kalpeshk2011)
(Assignee)

Comment 2

3 years ago
Yeah I'll be interested, though I'll prefer java stuff. Where do I start?
Flags: needinfo?(kalpeshk2011) → needinfo?(vivekb.balakrishnan)
Please see the bug description on how to solve this.
Feel free to NI me if you need some help.

Thanks for helping with this Kalpesh.
Flags: needinfo?(vivekb.balakrishnan)
(Assignee)

Comment 4

3 years ago
Well I'll go in the order Nick says. If I'm not wrong, I have to remove the unused variables in defines.inc right?
Flags: needinfo?(vivekb.balakrishnan)
Yes. you can use http://mxr.mozilla.org/mozilla-central/ or "search in Path" feature in your favorite IDE to find used reference and then prune those that are not used.
Flags: needinfo?(vivekb.balakrishnan)
(Assignee)

Comment 6

3 years ago
Alright I'll work on this bug :)
(Assignee)

Updated

3 years ago
Assignee: nobody → kalpeshk2011
(Assignee)

Comment 7

3 years ago
Created attachment 8685549 [details] [diff] [review]
cssfix.patch

It was quite cumbersome. I hope its correct
Attachment #8685549 - Flags: review?(vivekb.balakrishnan)
(Assignee)

Comment 8

3 years ago
How do I generate the correct command here ;- http://trychooser.pub.build.mozilla.org/ for this case?
I find it quite confusing.
Flags: needinfo?(vivekb.balakrishnan)
Comment on attachment 8685549 [details] [diff] [review]
cssfix.patch

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

@Kalpesh,

Thanks for going through the unused definitions and cleaning it up.
Can you start working on the 'part 2' mentioned in bug description which involves converting defines.inc to a css file.
While you are at it, use mm instead mozmm for the units.
Attachment #8685549 - Flags: review?(vivekb.balakrishnan) → a11y-review+
Flags: needinfo?(vivekb.balakrishnan)
Attachment #8685549 - Flags: a11y-review+ → feedback+
(Assignee)

Comment 10

3 years ago
Created attachment 8687407 [details] [diff] [review]
cssfix.patch

Hope this solves part 2. 
@vivek,
I wasn't very sure about how to work on the "filter" field so left it as a property. Do let me know if it needs to be changed. Also, could you answer my question regarding the try server in my previous comment?
Attachment #8685549 - Attachment is obsolete: true
Flags: needinfo?(vivekb.balakrishnan)
Attachment #8687407 - Flags: review?(vivekb.balakrishnan)
Comment on attachment 8687407 [details] [diff] [review]
cssfix.patch

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

Thanks Kalpesh,

That looks good. 
I am not sure about the filter definition but I am guessing that we can safely skip that.
Can you continue along and work on Part 3 of bug description?

As for as the try server, I usually select all the unit tests under Android.
For this case, I am not sure what is the right try configuration and unfortunately, this is not well documented.

::: mobile/android/themes/core/defines.css
@@ +20,2 @@
>  
> +	--margin_snormal: 0.64mm;	

nit: trailing space
Attachment #8687407 - Flags: review?(vivekb.balakrishnan) → feedback+
@Nick: I assumed that we can safely skip the filter definition in the above review. 
Am I correct in my assumption. 
Also, can you please point me to some docs if any related to this.
Flags: needinfo?(vivekb.balakrishnan) → needinfo?(nalexander)
(Assignee)

Comment 13

3 years ago
Created attachment 8687718 [details] [diff] [review]
cssfix.patch

Done with part 3, do let me know the errors
Attachment #8687407 - Attachment is obsolete: true
Attachment #8687718 - Flags: review?(vivekb.balakrishnan)
There were some significant issues with this patch, which I addressed and pushed.  I think I've addressed these, and I tested about:logins (the input edit box, especially) and about:addons to make sure they looked reasonable.  

Kalpesh, thanks for digging into this issue, but your testing of this wasn't sufficient: it didn't compile (due to includes that needed to be removed), and as written, it didn't work (due to errors in the defines.css syntax).  You could compare the patch I pushed to the patch you uploaded to see the issues (some in jar.mn, some in the actual code).
Status: NEW → ASSIGNED
Flags: needinfo?(nalexander)
(Assignee)

Comment 16

3 years ago
@nalexander :- I guess I completely messed up the filter field. Wasn't too sure about that :( I'll try to take more care next time.

@vivek, @nalexander :- What could I work on next?
Flags: needinfo?(vivekb.balakrishnan)
Flags: needinfo?(nalexander)

Comment 17

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2eef9ad06663
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Flags: needinfo?(vivekb.balakrishnan)
Flags: needinfo?(nalexander)
Comment on attachment 8687718 [details] [diff] [review]
cssfix.patch

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

Already landed
Attachment #8687718 - Flags: review?(vivekb.balakrishnan)
This change was pretty wrong... These things are exposed to content, see bug 1470709 for an example.

Why was this done without any kind of style system peer review? Who owns these stylesheets?
Blocks: 1470709
No longer blocks: 1470709
Depends on: 1470709
(In reply to Emilio Cobos Álvarez (:emilio) from comment #19)
> This change was pretty wrong... These things are exposed to content, see bug
> 1470709 for an example.

I had no idea.  That's pretty counter-intuitive.

> Why was this done without any kind of style system peer review?

Because this is really counter-intuitive?

 Who owns
> these stylesheets?

At this point, nobody, really.  sdaswani, I guess (Fennec engineering manager).  Do whatever is necessary to fix this obviously busted situation, preferably without re-introducing pre-processing to the build system.
You need to log in before you can comment on or make changes to this bug.