372826 - Update about:license, about:buildconfig to the new style/look of about:

Status: VERIFIED FIXED

(Firefox :: General, enhancement)

Description

[François Gagné]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.3pre) Gecko/20070305 BonEcho/2.0.0.3pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.3pre) Gecko/20070305 BonEcho/2.0.0.3pre

Simply update about:credits, about:license, about:buildconfig use the same style as about:

See the attached file for a quick merge I have done from about:credits and about: .

This would bring those pages in the 21st century ;)

Reproducible: Always

Comment 1

[François Gagné]

Attachment: What a updated about:credits would looks like.

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

Wanted, but won't block release.

Comment 3

[Reed Loden [:reed] (use needinfo?)]

I took care of about:credits in bug 391911.

Comment 4

[:ehsan akhgari]

Attachment: Migrate about:buildconfig to the new style

Not sure whom to ask for review here...

Note that the buildconfig.html.in files in toolkit and xpfe are identical.

Comment 5

[:ehsan akhgari]

Attachment: Screenshot of the new about:buildconfig page

Comment 6

[:ehsan akhgari]

Attachment: Migrate about:license to the new style

Comment 7

[:ehsan akhgari]

Attachment: Screenshot of the new about:license page

Comment 8

[:ehsan akhgari]

Comment on attachment 295280 [details] [diff] [review]
Migrate about:buildconfig to the new style

Hmmm, maybe gerv is the correct reviewer here?

Comment 9

[Reed Loden [:reed] (use needinfo?)]

(In reply to comment #8)
> (From update of attachment 295280 [details] [diff] [review])
> Hmmm, maybe gerv is the correct reviewer here?

No, you had it right the first time. Mano or gavin would both work fine as a reviewer.

Comment 10

[Steffen Wilberg]

about:buildconfig is too ugly for the new style, it needs some heavy beating ;-)

Comment 11

[:ehsan akhgari]

Comment on attachment 295280 [details] [diff] [review]
Migrate about:buildconfig to the new style

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 295280 [details] [diff] [review] [details])
> > Hmmm, maybe gerv is the correct reviewer here?
> 
> No, you had it right the first time. Mano or gavin would both work fine as a
> reviewer.

Oops!  :">

Comment 12

[:ehsan akhgari]

(In reply to comment #10)
> about:buildconfig is too ugly for the new style, it needs some heavy beating
> ;-)

about:license isn't that pretty as well, if you ask me!  ;-)

Comment 13

[Gervase Markham [:gerv] (not reading bugmail)]

Fine by me.

Gerv

Comment 14

[François Gagné]

hmm, by comment #9, ask gavin to review?

Comment 15

[:Gavin Sharp [email: [e-mail]]]

This makes about:buildconfig pretty hard to read, with the build options all squished together into that little center box. I'm not really sure this is worth fixing, the buildconfig page isn't really meant to be pretty...

Comment 16

[:ehsan akhgari]

What about about:license?

Comment 17

[:Gavin Sharp [email: [e-mail]]]

I don't know, seems like about:license would have the same problem. In your screenshot the "bubble" is almost as wide as the viewport, but increasing the window width doesn't increase the bubble width, so the problem is worse with wide windows. I have my window maximized on a widescreen display, for example.

I'm pretty ambivalent about "prettying up" these pages.

Comment 18

[:ehsan akhgari]

Hmm, yes, but that's the same that other pages, such as about:credits do...  And since they're all using the same CSS, that problem can be addressed for all of them by editing the CSS.  However, I doubt that would apply to about: for example.

Comment 19

[:ehsan akhgari]

Hmm, any new thoughts on this, gavin?  :-)

Comment 20

[:Gavin Sharp [email: [e-mail]]]

I don't think we should fix this until the about: page CSS is fixed to be less restrictive for wide windows.

Comment 21

[:ehsan akhgari]

Attachment: Wide layout for about:license and about:buildconfig

Wide layout for about:license and about:buildconfig based on the patch in bug 416891.  If this gets r+, it should be checked in after that bug.

Comment 22

[:ehsan akhgari]

gavin: Now that bug 416891 is in, would you please take a look at the new patch (attachment 303817 [details] [diff] [review])?  Thanks!

Comment 23

[François Gagné]

So, is the patch good?

Comment 24

[:ehsan akhgari]

(In reply to comment #23)
> So, is the patch good?

Yeah!

Gavin: ping...

Comment 25

[:ehsan akhgari]

Gavin: re-ping!

Comment 26

[Reed Loden [:reed] (use needinfo?)]

dao isn't a valid reviewer for this, fyi. He can do an unofficial review, but you still need a toolkit reviewer to review this. :)

Comment 27

[Dão Gottwald [::dao]]

Comment on attachment 303817 [details] [diff] [review]
Wide layout for about:license and about:buildconfig

>   <title>about:buildconfig</title>
>+  <link rel="stylesheet" href="chrome://global/skin/about.css" type="text/css">
>+  <style type="text/css">
>+    th, td {
>+      padding: 0 5px;
>+    }
>+  </style>

Can you add that to the style sheet?

> </head>
> <body>
>+<div id="aboutPageContainer" class="aboutPageWideContainer">

What do we need the extra container for? <body> should be enough... see ftp listings for example.

Comment 28

[:ehsan akhgari]

(In reply to comment #26)
> dao isn't a valid reviewer for this, fyi. He can do an unofficial review, but
> you still need a toolkit reviewer to review this. :)

Thanks for the note, Reed.  I'll ask Gavin as soon as I get Dão's r+.  :-)

Comment 29

[:ehsan akhgari]

Attachment: Wide layout for about:license and about:buildconfig (v2)

(In reply to comment #27)
> (From update of attachment 303817 [details] [diff] [review])
> Can you add that to the style sheet?

I could, but I prefer not to do it, because in the future some other pages which need about.css could use tables, and they would get the style they did not intend to.  Anyway, if you think that's not an issue, it would be easy for me to modify the stylesheet.

> > </head>
> > <body>
> >+<div id="aboutPageContainer" class="aboutPageWideContainer">
> 
> What do we need the extra container for? <body> should be enough... see ftp
> listings for example.

I merged the buildconfig containers with the body, but couldn't do the same for license, because the body element already had an ID.

Comment 30

[Dão Gottwald [::dao]]

(In reply to comment #29)
> I could, but I prefer not to do it, because in the future some other pages
> which need about.css could use tables, and they would get the style they did
> not intend to.  Anyway, if you think that's not an issue, it would be easy for
> me to modify the stylesheet.

If we ever use that stylesheet for another page with a table that needs a different styling, we should use classes or ids to distinguish between them. So I'd still prefer putting it in the stylesheet.

> I merged the buildconfig containers with the body, but couldn't do the same for
> license, because the body element already had an ID.

Well, the id isn't needed if you modify the stylesheet to style body directly... You could remove the div from /toolkit/content/about.xhtml then, although it doesn't any harm if the stylesheet ignores it.

Comment 31

[:ehsan akhgari]

Attachment: Wide layout for about:license and about:buildconfig (v3)

(In reply to comment #30)
> If we ever use that stylesheet for another page with a table that needs a
> different styling, we should use classes or ids to distinguish between them. So
> I'd still prefer putting it in the stylesheet.

OK, done.

> Well, the id isn't needed if you modify the stylesheet to style body
> directly... You could remove the div from /toolkit/content/about.xhtml then,
> although it doesn't any harm if the stylesheet ignores it.

Done.  I also modified about.xhtml in the patch, because the extra div wasn't really needed any more.

Comment 32

[Dão Gottwald [::dao]]

Comment on attachment 320233 [details] [diff] [review]
Wide layout for about:license and about:buildconfig (v3)

>Index: toolkit/themes/winstripe/global/about.css

> html {
>   background: -moz-Dialog;
> }
> 
> body {
>-  margin: 0;
>-  padding: 0 1em;
>   color: -moz-FieldText;
>   font: message-box;
>-}

That padding is still needed. You need to move it to the html element along with the font (otherwise 1em becomes bigger).

Note that !important in the .aboutPageWideContainer rule isn't needed anymore.

Comment 33

[:ehsan akhgari]

Attachment: Wide layout for about:license and about:buildconfig (v4)

Addressed comment 32.

Comment 34

[:ehsan akhgari]

Comment on attachment 320247 [details] [diff] [review]
Wide layout for about:license and about:buildconfig (v4)

Oops, r+=dao.

Gavin, can you take another look at this?  Dão caught a number of issues, so the patch is cleaner now, and should be easier to review.

Comment 35

[Dão Gottwald [::dao]]

Comment on attachment 320247 [details] [diff] [review]
Wide layout for about:license and about:buildconfig (v4)

> html {
>   background: -moz-Dialog;
>+  padding: 0 1em;
> }
> 
> body {
>-  margin: 0;
>-  padding: 0 1em;
>   color: -moz-FieldText;
>   font: message-box;

As a wrote, the font property needs to be moved to the html element as it affects the 1em padding.

Comment 36

[:ehsan akhgari]

Attachment: Wide layout for about:license and about:buildconfig (v5)

(In reply to comment #35)
> As a wrote, the font property needs to be moved to the html element as it
> affects the 1em padding.

Yes, you're right.  I didn't catch that last time.

Comment 37

[:ehsan akhgari]

Gavin, can you take a look at this one if you get the time?  It's a low-priority but easy review.

Comment 38

[Phil Ringnalda (:philor)]

Ehsan: you'll increase your chances of catching a checkin-needed if you attach a "For checkin" patch that does whatever ought to be done now about the xpfe parts - I was looking for something to push in a hurry, and didn't want to take the time to figure out whether they've moved or moved repos or just disappeared.

Comment 39

[:ehsan akhgari]

Attachment: For checkin

The patch for checkin on mozilla-central.

Comment 40

[Dão Gottwald [::dao]]

http://hg.mozilla.org/mozilla-central/rev/f7795cb888df

Comment 41

[Pardal Freudenthal (:ShareBird)]

(In reply to comment #27)

> What do we need the extra container for? <body> should be enough... see ftp
> listings for example.

Removing this element will brake third party themes which use it to skin the "about:" pages (like mine).

Comment 42

[Alfred Kayser]

Pardal, yes it brakes current themes, but that it is true for more changes that are currently happening for FF3.1 (which is also saddening me...).
But this particular change is easily accommodated for, even in a way that is compatible with the current FF3.0 release (and even FF2.0).
More important is that this change finally allows us themers to also style the other about pages.

Comment 43

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b1pre) Gecko/20081006 Minefield/3.1b1pre.  about: pages have the new layouts.