545353 - some polish in about.css, config.css, neterror.css and dirListings.css for Gecko 1.9.x

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[philippe (part-time)]

Putting a pretty face on about:buildconfig, about:license.

Comment 1

[philippe (part-time)]

Attachment: like this

The font-size on table is needed as about:buildconfig is in quirksmode
(see core bug 456756)

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

1) The box-shadow seems to make scrolling the longer pages very slow. I think this is bug 509052, bug 535412, and maybe some others.  I don't know if we care enough about scrolling speed on these pages or not.

2) If we do add the box-shadow, I think we should port it to dirListing, config, netError CSS files, too, so that all of the pages have a consistent look.  dirListing is the real-world case where having box-shadow would actually affect users.

I'll ask today what we want to do; whatever it is, we should take the patch on 1.9.0 just to keep the "huge diff" size down, since box-shadow and other changes that depend on 1.9.x will just be ignored.

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #2)
> 1) The box-shadow seems to make scrolling the longer pages very slow. I think
> this is bug 509052, bug 535412, and maybe some others.  I don't know if we care
> enough about scrolling speed on these pages or not.

Actually, I just tried it with my release build to see how much of the slowness was debug-related, and the slowdown/lag is not really noticeable (on license, credits, or the Firefox nightly ftpdir listing), so I think we're OK to add it.

Comment 4

[philippe (part-time)]

(In reply to comment #2) 
> 2) If we do add the box-shadow, I think we should port it to dirListing,
> config, netError CSS files, too, so that all of the pages have a consistent
> look.  dirListing is the real-world case where having box-shadow would actually
> affect users.

Yeah, I intend to sync the style of those, but haven't gotten around to check those.

Did you really notice a lag when scrolling - with a release build ? I have no problems on my hardware. That said, most of my more recent experience with box-shadow is with Minefield builds, which are faster (or better optimised) for this.

> I'll ask today what we want to do; whatever it is, we should take the patch on
> 1.9.0 just to keep the "huge diff" size down, since box-shadow and other
> changes that depend on 1.9.x will just be ignored.

Bear in mind that having box-shadow in the 1.9.0 stylesheet will flag a warning in console.

Comment 5

[philippe (part-time)]

Attachment: patch 1

patch to the various .css files

in all 3 files: adding a box shadow around the box, simulates a little bit Finder windows.

about.css:
- prettify about:buildconfig and about:license (see comment 1 above)

config.css
- box shadow around the warning box

neterror.css
- box-shadow 
- for the phishing overlay, I made the shadow slightly darker taking int account the dark background.

Comment 6

[philippe (part-time)]

Attachment: patch 2 - dirListings.css

( I split this out separately, I'm not sure about landing this on Gecko 1.9.0)

from bug comment 11 (smokey)
> I wonder if we want to do something else with the :hover rules and/or if it's
> possible to simulate the alternating row colors as in the Finder.

- the patch creates alternate row colors (nth-child())
- hover over rows uses a background-color instead of that border/outline
- add some padding to et left and right of each row
- box-shadow

Comment 7

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #5)
> patch 1

> about.css:

One thing I noticed here (it's not a new problem, but it's worse in about:license) is that the spacing between the preceding text and the <hr>s is pretty limited and it can look cramped (also where the subsequent text is an <H1>).

Related, the plain <hr>s look pretty crappy (Netscape 1 called and wants its <hr> back) compared to the rule that we have with the <h1>s.  Maybe a 1px solid black rule?  Also, maybe we should consider dropping the rule from any <h1> beyond the first one in order to combat rule proliferation in about:license (where there's the <hr> and then our <h1>-with-rule for each license)?

> neterror.css
> - for the phishing overlay, I made the shadow slightly darker taking int
> account the dark background.

The shadow is more subtle here, but it's there enough to make the box "pop out" like the others, I think.

(In reply to comment #6)
> patch 2 - dirListings.css
> 
> ( I split this out separately, I'm not sure about landing this on Gecko 1.9.0)

It looks OK in about:crashes and a FTP listing, I think.

> - add some padding to et left and right of each row

The one thing I noticed here is that in the Finder, there is more padding to the left of the icon and to the right of the text in the right-most column.  The right side doesn't look so bad, but to me the left side looks like there's not enough padding to the left of the icon before the highlight/bgcolor ends.

Overall, though, these look good and degrade well to 1.9.0 so that we can land there and keep the 1.9.x megadiff down.

Comment 8

[philippe (part-time)]

Attachment: combined patch v2

This should cover all comments

about.css

- only the first <h1> has a rule below.
- the <hr> have a larger margin (top and bottom)
- the <hr> are displayed as 1px dark grey line
- the <ul> (in about:license) now use a disc as list-marker (bug 376690 makes square list-markers look crappy at some font-sizes)

dirListings.css

- more padding to the left of the first cell and to the right of the last cell
- switch to use -moz-padding-* for better RTL support
- aligned the text of the first column header 'Name' with the text in the cells below (Finder like)

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

The only negative thing I've really noticed about this patch is that the leading/line spacing seems a lot tighter in about:crashes (and maybe directory listings, but I think about:crashes is worse because there are no icons) than it did when the border/outline rules were used; it definitely seems more cramped to me.

Comment 10

[philippe (part-time)]

(In reply to comment #9)
> The only negative thing I've really noticed about this patch is that the
> leading/line spacing seems a lot tighter in about:crashes (and maybe directory
> listings, but I think about:crashes is worse because there are no icons) than
> it did when the border/outline rules were used; it definitely seems more
> cramped to me.

That is to be expected, with the border-collapse set to to collapse (that is what provides the additional spacing, when set to separate - the default).

Updated patch coming soon with some more vertical breathing space. That should also make all rows having more equal height, independently of the presence of icons (for some reason beyond our control the folder.png takes up slightly more space than all other icons I can come up with).

Comment 11

[philippe (part-time)]

Attachment: patch v2.1

some more vertical spacing on the rows in directory listings & about crashes

Comment 12

[philippe (part-time)]

Attachment: aboutMemory v0.3

Stylesheet for about:memory (new on Gecko 1.9.2), loosely based on about.css

Comment 13

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 426945 [details] [diff] [review]
patch v2.1

This looks good, both on 1.9.0 and 1.9.2, for all relevant files.

Comment 14

[philippe (part-time)]

Attachment: aboutMemory v0.4

Forgot the margin declaration on the h1 in previous patch

Comment 15

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 427050 [details] [diff] [review]
aboutMemory v0.4

This looks good, too.

Note to self: this requires an addition in the 1.9.x pinstripe/jar.mn, toolkit.jar section:

# Uh, yeah.  See bug 520364.
+  content/global/aboutMemory.css                          (../embed-replacements/skin/classic/global/aboutMemory.css)

Comment 16

[Stuart Morgan]

Comment on attachment 426945 [details] [diff] [review]
patch v2.1

> body {
>   margin: 0;
>   padding: 0 1em;
>   color: -moz-FieldText;
>   font: message-box;
>-}
>-
>-#aboutPageContainer {
...

Bwuh? Why do we want all the style rules to apply directly to the body? Does putting something like position:relative on the body even work?

Comment 17

[philippe (part-time)]

(In reply to comment #16)
> (From update of attachment 426945 [details] [diff] [review])
> > body {
> >   margin: 0;
> >   padding: 0 1em;
> >   color: -moz-FieldText;
> >   font: message-box;
> >-}
> >-
> >-#aboutPageContainer {
> ...
> 
> Bwuh? Why do we want all the style rules to apply directly to the body? Does
> putting something like position:relative on the body even work?

Yes. You can apply {position: relative} to the body no problem.
The #aboutPageContainer is actually attached to body.
about:license, about:buildconfig don't have any ID for body, though. Hence I moved the whole block.
Firefox 3.5+ does the same thing, btw.

From about:credits
<body class="aboutPageWideContainer" id="aboutPageContainer">

Comment 18

[Stuart Morgan]

Comment on attachment 426945 [details] [diff] [review]
patch v2.1

>+  -moz-box-shadow:  -4px 4px 8px rgba(0,0,0,0.12), 4px 4px 8px rgba(0,0,0,0.12);

All the box-shadow lines have two spaces after the : instead of one, which isn't consistent with anything else.

>+  }

Fix indentation

sr=smorgan otherwise

Comment 19

[Stuart Morgan]

Comment on attachment 427050 [details] [diff] [review]
aboutMemory v0.4

>+  /*-moz-padding-start: 30px;*/

Why check this in?

>+  -moz-box-shadow:  -4px 4px 8px rgba(0,0,0,0.12), 4px 4px 8px rgba(0,0,0,0.12);

Same spacing issue.

sr=smorgan

Comment 20

[philippe (part-time)]

Attachment: combined patch for checkin

Combined patch for checkin addressing review comments.

Comment 21

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Landed on cvs trunk.

As our aboutMemory.css landed on 1.9.0 (it does nothing there, but landing it keeps the whole patch together), the pinstripe/jar.mn change from comment 15 will be picked up on 1.9.x as part of the big "fix embed-replacements/pinstripe packaging for 1.9.x" bug/patch, whenever I tease that out of my 1.9.x tree.