321708 - Don't inline CSS styles in print dialogs

Status: RESOLVED INCOMPLETE

(Core :: Printing: Output, defect)

Description

[Vidar Haarr (not reading bugmail)]

From bug 277296 comment 14, timeless said:

"+                <label style="visibility: hidden;"
value="&marginLeft.label;"/>

+                  <box style="border: 1px;
+                              border-style: dashed;
+                              border-color: gray;"
+                       flex="1"/>

i'm not particularly fond of inline style, i wonder if these should have ids to
make it easier for other things to poke."

Comment 1

[Vidar Haarr (not reading bugmail)]

This is simply about eliminating most/some of the inline styles in
<http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpfe/global/resources/content/unix/printjoboptions.xul&rev=1.11&mark=143,147,151,155>
<http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpfe/global/resources/content/printdialog.xul&rev=1.10&mark=100,102,112>
<http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpfe/global/resources/content/printPageSetup.xul&rev=1.14&mark=113,122,129,130,132,133,134,136,142,151>
(and their toolkit counterparts), and move it to their respective CSS files.

Remember to fix all mozilla.org-themes.

Comment 2

[Vidar Haarr (not reading bugmail)]

AFAIK, you would add the CSS styles to these files:
http://lxr.mozilla.org/mozilla/find?string=printing.css

And you'd also need to add the following stylesheet to printjoboptions.xul and printdialog.xul:
<?xml-stylesheet href="chrome://global/skin/printing.css" type="text/css"?>

I think that would be OK - timeless would know better.

Comment 3

[(no longer active)]

Attachment: Move inline styles to printing.css

Comment 4

[timeless]

Comment on attachment 235885 [details] [diff] [review]
Move inline styles to printing.css

> This invisible label (with same content as the visible one!) is used to ensure that the <textbox> is centered above the page. The same technique is deployed for the bottom/left/right input fields, below.

Adding an id is good, but please also add a class since the rule is going to be the same in general for all of them class="hiddenspacing" or something

>+              <label id="marginTopLabel" value="&marginTop.label;"/>

this means that average themes would use a single css rule .hiddenspacing {visibility:hidden}, but if some strange instance wanted to do something else per id'd node, it could.

Comment 5

[(no longer active)]

Attachment: Move inline styles to printing.css (revised)

In this revised patch, I've addressed the issues raised in comment #4, by adding a hiddenspacing class to the four elements, and also adding their CSS rules to the printing.css files.

Comment 6

[timeless]

Comment on attachment 236611 [details] [diff] [review]
Move inline styles to printing.css (revised)

>Index: themes/classic/global/printing.css
i was hoping that this:
>+#fromPageInput, #toPageInput, #numCopiesInput, #topInput, #bottomInput, #leftInput, #rightInput {

could have been replaced with a class :(
>+  width: 5em;
>+}

and similarly:
>+#marginRightLabel, #marginBottomLabel {
>+  visibility: hidden;
>+}

with this:
>+.hiddenspacing {
>+  visibility: hidden;
>+}

>Index: xpfe/global/resources/content/printPageSetup.xul

:(

Comment 7

[(no longer active)]

Attachment: Move inline styles to printing.css (revised again)

(In reply to comment #6)
> (From update of attachment 236611 [details] [diff] [review] [edit])
> >Index: themes/classic/global/printing.css
> i was hoping that this:
> >+#fromPageInput, #toPageInput, #numCopiesInput, #topInput, #bottomInput, #leftInput, #rightInput {
> 
> could have been replaced with a class :(
> >+  width: 5em;
> >+}
> 
> and similarly:
> >+#marginRightLabel, #marginBottomLabel {
> >+  visibility: hidden;
> >+}
> 
> with this:
> >+.hiddenspacing {
> >+  visibility: hidden;
> >+}
> 
> >Index: xpfe/global/resources/content/printPageSetup.xul
> 
> :(
> 

Done.

Comment 8

[[e-mail]]

Comment on attachment 238884 [details] [diff] [review]
Move inline styles to printing.css (revised again)

>+#marginPage {
>+  height: 29.7mm;
>+}
>+
>+#marginTop {
>+  height: 0.05in;
>+}
>+
>+#marginLeft {
>+  width: 0.025in;
>+}
These values are all overridden by inline style created by the preview generator. There's no point setting them here.

>+#marginCenter {
>+  border: 1px;
>+  border-style: dashed;
>+  border-color: gray;
>+}
The background of the preview box is, unfortunately, still set in code...

>+#marginRight {
>+  width: 0.025in;
>+}
>+
>+#marginBottom {
>+  height: 0.05in;
>+}
As per page/top/left.

>-              <textbox id="topInput" size="5"
>+              <textbox id="topInput" size="5" class="margin-input-box"
size="5" != style="width: 5em;"
In fact those textboxes using a width should use a size but unfortunately I don't have a build with a unix print dialog to verify what size to use.

Comment 9

[[e-mail]]

Comment on attachment 238884 [details] [diff] [review]
Move inline styles to printing.css (revised again)

>+  border-color: gray;
And since we're printing on paper do we want a white background or should we use the system background colour (and therefore border colour)?

Comment 10

[(no longer active)]

Leaving this for those new to mozilla as a good first bug.  I've passed that stage... :-)

Comment 11

[Tyler Downer [He/Him]]

Does this bug even apply anymore?
For example, mozilla/themes/modern/global/printing.css doesn't exist.

Comment 12

[MD. AL-AMIN]

Hi, I want to fix this bug. can I get assigned to it?

Comment 13

[Mike Hoye [:mhoye]]

Hi, MD. Al-Amin - We can do that - have you set up your development environment, as per here?

https://developer.mozilla.org/en-US/docs/Simple_Firefox_build

Comment 14

[MD. AL-AMIN]

Hi Mike, Yes I have set up my development enviroment.

Comment 15

[Felipe César]

Hi, I would like to work on this bug. I already built firefox on my computer.

Comment 16

[Mike Hoye [:mhoye]]

We generally assign bugs to new contributors after their first patch is submitted. Please give it a try, and let us know if you have any questions!

Comment 17

[Preeti[:preeti]]

Hello everyone! This is Preeti and I have just  recently started contributing to open source. Could someone please guide me on how to proceed on this bug? Thanks in advance!

Comment 18

[Lam]

Hi folks, I would like to work on this as my first contribution! Do you guys know how I should approach this?

Comment 19

[Mike Hoye [:mhoye]]

Hi, folks; Can you email me directly? I'd like to help you get set up to get started, but in-bug isn't the right place for that.

Comment 20

[Temisan Iwere]

Hi, I'm a new contributor and would like to take on this bug? Can I proceed? or is anyone else still working on it?

Comment 21

[samueljselvarajah]

hi, i'd like to contribute to this bug?