Closed Bug 372826 Opened 13 years ago Closed 11 years ago

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

Categories

(Firefox :: General, enhancement)

enhancement
Not set

Tracking

()

VERIFIED FIXED
Firefox 3.1b1

People

(Reporter: frenchfrog, Assigned: ehsan)

References

()

Details

Attachments

(3 files, 8 obsolete files)

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
Flags: blocking-firefox3?
Keywords: uiwanted
Version: unspecified → Trunk
Status: UNCONFIRMED → NEW
Ever confirmed: true
Wanted, but won't block release.
Flags: blocking-firefox3? → blocking-firefox3-
Whiteboard: [wanted-firefox3]
I took care of about:credits in bug 391911.
Summary: Update about:credits, about:license, about:buildconfig to the new style/look of about: → Update about:license, about:buildconfig to the new style/look of about:
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
Not sure whom to ask for review here...

Note that the buildconfig.html.in files in toolkit and xpfe are identical.
Attachment #295280 - Flags: review?(mano)
Attachment #295283 - Flags: review?(gerv)
Attached image Screenshot of the new about:license page (obsolete) —
Comment on attachment 295280 [details] [diff] [review]
Migrate about:buildconfig to the new style

Hmmm, maybe gerv is the correct reviewer here?
Attachment #295280 - Flags: review?(mano) → review?(gerv)
(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.
about:buildconfig is too ugly for the new style, it needs some heavy beating ;-)
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!  :">
Attachment #295280 - Flags: review?(gerv) → review?(mano)
Attachment #295283 - Flags: review?(gerv) → review?(mano)
(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!  ;-)
Flags: wanted-firefox3+
Whiteboard: [wanted-firefox3]
hmm, by comment #9, ask gavin to review?
Attachment #295280 - Flags: review?(mano) → review?(gavin.sharp)
Attachment #295283 - Flags: review?(mano) → review?(gavin.sharp)
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...
What about about:license?
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.
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.
Hmm, any new thoughts on this, gavin?  :-)
I don't think we should fix this until the about: page CSS is fixed to be less restrictive for wide windows.
Attachment #295280 - Attachment is obsolete: true
Attachment #295280 - Flags: review?(gavin.sharp)
Attachment #295281 - Attachment is obsolete: true
Attachment #295283 - Attachment is obsolete: true
Attachment #295283 - Flags: review?(gavin.sharp)
Attachment #295284 - Attachment is obsolete: true
Depends on: 416891
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.
Attachment #303817 - Flags: review?(gavin.sharp)
Keywords: uiwanted
gavin: Now that bug 416891 is in, would you please take a look at the new patch (attachment 303817 [details] [diff] [review])?  Thanks!
So, is the patch good?
(In reply to comment #23)
> So, is the patch good?

Yeah!

Gavin: ping...
Gavin: re-ping!
Whiteboard: [has patch] [needs review gavin]
Attachment #303817 - Flags: review?(gavin.sharp) → review?(dao)
Whiteboard: [has patch] [needs review gavin] → [has patch] [needs review dao]
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 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.
(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+.  :-)
(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.
Attachment #303817 - Attachment is obsolete: true
Attachment #320131 - Flags: review?(dao)
Attachment #303817 - Flags: review?(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.
(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.
Attachment #320131 - Attachment is obsolete: true
Attachment #320233 - Flags: review?(dao)
Attachment #320131 - Flags: review?(dao)
Attachment #320233 - Attachment is patch: true
Attachment #320233 - Attachment mime type: application/octet-stream → text/plain
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.
Attachment #320233 - Flags: review?(dao) → review+
Addressed comment 32.
Attachment #320233 - Attachment is obsolete: true
Attachment #320247 - Flags: review?(dao)
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.
Attachment #320247 - Flags: review?(gavin.sharp)
Attachment #320247 - Flags: review?(dao)
Attachment #320247 - Flags: review+
Whiteboard: [has patch] [needs review dao] → [has patch] [needs review gavin]
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.
(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.
Attachment #320247 - Attachment is obsolete: true
Attachment #320253 - Flags: review?(gavin.sharp)
Attachment #320247 - Flags: review?(gavin.sharp)
Attachment #320247 - Flags: review+
Gavin, can you take a look at this one if you get the time?  It's a low-priority but easy review.
Attachment #320253 - Flags: review?(gavin.sharp) → review+
Keywords: checkin-needed
Whiteboard: [has patch] [needs review gavin]
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.
Attached patch For checkinSplinter Review
The patch for checkin on mozilla-central.
http://hg.mozilla.org/mozilla-central/rev/f7795cb888df
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.1b1
(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).
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.
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.