225090 - Sanitize Help CSS

Status: VERIFIED FIXED

(SeaMonkey :: Help Documentation, defect)

Description

[Daniel Wang]

We need:

/* for <td><p> and <li><p>, in case we have multiple <p>s */
p:first-child { padding-top: 0; margin-top: 0; }

/* for long lists, bigger separation make items more pleasing
to read and more clear */
li { padding-bottom: 0.5em; }

ul.compact li, ol.compact li { padding-bottom: 0; }

/* UI element labels, e.g. menu items */
.ui { color: #3333cc; }

h1, h2, h3, h4, h5, h6 {
//  color: blah
}

strong { color: blah; }
em { font-style: normal; font-weight: bold; }

kbd { font-family: sans-serif; }

--------

We need to consider not using <kbd>. The reason is that x-height of normal
sans-serif and monospace are different. On Windows (and possibly Linux),
x-height of monospace is especially small, thus creating reading difficulty. The
breakage is especially severe when we have a whole sentence in sans-serif but
some words in monospace (e.g. "to bookmark the current page, type _ctrl+b_. blah
blah). For longer phrases, the eyes can more easily adjust to the type change
(e.g. "To search the Web, type _http://www.google.com_ in the Location Bar).

Comment 1

[R.J. Keller]

sounds good. Let's do this during 1.7alpha.

Comment 2

[R.J. Keller]

> /* for long lists, bigger separation make items more pleasing
> to read and more clear */
> li { padding-bottom: 0.5em; }
>
> ul.compact li, ol.compact li { padding-bottom: 0; }

Let's switch this. Let's have a .expand or something like that that will have
padding-bottom: 0.5em;, while the default is 0.

> h1, h2, h3, h4, h5, h6 {
> //  color: blah
> }

why set this?

> strong { color: blah; }
> em { font-style: normal; font-weight: bold; }

why are we changing <em> from italic to bold?

Comment 3

[R.J. Keller]

Daniel, can you please answer my comments :). I'd like to get this in for 1.6b 
if we can.

Comment 4

[Daniel Wang]

>> h1, h2, h3, h4, h5, h6 {
>> //  color: blah
>> }
>
> why set this?

shrug. sounded good when I filed the bug.

>> strong { color: blah; }
>> em { font-style: normal; font-weight: bold; }
>
> why are we changing <em> from italic to bold?

because italic is ugly

for .ui, background-color: is probably better than color:

Comment 5

[R.J. Keller]

> because italic is ugly

Then maybe we need to add something different than italic.

> for .ui, background-color: is probably better than color:

Think we should highlight UI items that much? Anyway, that color you specified
in the report is too dark to make a decent background color, nobody could see
the text. Maybe #B7F5C7 would be a less painful background color.

Comment 6

[R.J. Keller]

Attachment: Patch

Comment 7

[R.J. Keller]

Comment on attachment 136201 [details] [diff] [review]
Patch

what the heck? Not sure what I attached :). Let's try again.

Comment 8

[R.J. Keller]

Attachment: Real Patch ;)

real patch ;)

Comment 9

[Daniel Wang]

> +.separate li { padding-bottom: 0.5em; }

What is this for? Please comment the source ;-)

Comment 10

[R.J. Keller]

> What is this for? Please comment the source ;-)

That spacing was the opposite that you reported in the bug. Instead of having a
class that would make the lists not have spacing, I made one so that they did so
that the old docs wouldn't get messed up with the new spacing.

Comment 11

[R.J. Keller]

Comment on attachment 136317 [details] [diff] [review]
Real Patch ;)

r=timeless over IRC, BTW.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 136317 [details] [diff] [review]
Real Patch ;)

>+.separate li { padding-bottom: 0.5em; }

Why not margin-bottom?

>+.ui { background-color: #B7F5C7 }

You could use CSS2 system colors...

Comment 13

[Asa Dotzler [:asa]]

Comment on attachment 136317 [details] [diff] [review]
Real Patch ;)

a=asa (on behalf of drivers) for checkin to 1.6beta

Comment 14

[R.J. Keller]

> You could use CSS2 system colors...

dbaron, do you mean using rgb(x,y,z);?

Changing Target Milestone to 1.6, I'd like to debug this more on other help
files before checking this in.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

no.  http://www.w3.org/TR/CSS21/ui.html#system-colors

Comment 16

[Daniel Wang]

which is deprecated in favour of http://www.w3.org/TR/css3-ui/#system

Do UI labels have to look like real UI elements? If we want this, maybe we
should also style the text and background to window text and window background
colours. but that may look ugly.

>>+.separate li { padding-bottom: 0.5em; }
> 
> Why not margin-bottom?

yeah, padding will be a problem if we ever want to use border or background. We
should change this.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The fact that it's deprecated is irrelevant.  (I objected to the deprecation,
actually, and I should do so more forcefully.)  Mozilla implements them, and
they're useful.

Comment 18

[R.J. Keller]

Attachment: Patch with dbaron's comments

> >+.ui { background-color: #B7F5C7 }
> 
> You could use CSS2 system colors...

I disagree. I would like consistency in help and not have different colors on
different OS's or different OS settings.

I definetely agree on the padding being changed to margin.

Comment 19

[R.J. Keller]

Comment on attachment 137773 [details] [diff] [review]
Patch with dbaron's comments

dbaron, since you offered feedback on this, can you review this?

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 137773 [details] [diff] [review]
Patch with dbaron's comments

>+.separate li { margin-bottom: 0.5em; }

r=dbaron, although this should probably be:

.separate > li { ...

Comment 21

[R.J. Keller]

Fix checked in.

dbaron, I changed ".separate li" to ".separate > li" before checkin.