130728 - messageBody.css should use namespaces correctly

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[David Baron :dbaron:]

messageBody.css should use namespaces correctly -- it's current use has
performance problems.  I think this was done because someone wanted to use
"html|.class" instead of the correct "html|*.class".

Note that the one existing selector that does have a namespace, the html|html
rule that gives a background color, should probably be **removed** to avoid
regressing bug 69436.

There's a patch for this already, within attachment 62713 [details] [diff] [review] on bug 91662.

Comment 1

[David Baron :dbaron:]

Attachment: (Av1-MAS) patch

Comment 2

[Ben Bucksch (:BenB)]

David, see my comments in bug 69436 about the background color. Let's discuss
this there.

I also already reviewed this patch in the other bug and had several comments,
which you seem to ignore here. Please see
<http://bugzilla.mozilla.org/show_bug.cgi?id=91662#c58> and following.

You say there:
> Using the default value rather than 'inherit' would make much more sense if
> that's what you want.

No, that's explicitly *not* what I want. For example, blockquote has a left
margin (and a bar with type=cite) by default, which in some cases is not wanted
(e.g. when just the original ">"s are shown).

Please make sure that everything still works exactly the same way as before
(modulo the decision in bug 69436), in all cases. See the *quote* prefs and
related prefs in default/pref/mailnews.js. Also try both format=flowed and
normal plaintext.

Comment 3

[David Baron :dbaron:]

I'm not planning to fix this anytime soon.  And anyway, 'inherit' still doesn't
make any sense.  'inherit' means inheritance from the parent content node.  What
if that's an H1?  Or a P?  Or a DIV?  You get different results, and that's not
what you want.

Comment 4

[Ben Bucksch (:BenB)]

A blockquote within an h1???

What I get is that the element does not change anything. And that's what I want.

Comment 5

[David Baron :dbaron:]

I think what you mean by that is that you want it to have 0 margin, padding, and
border.  These are the initial values for the property.  This is conceptually
very different from 'inherit' and the implementation is quite different (and
more expensive in memory use and time).  (Note that CSS3 will probably have
special keyword 'initial' in addition to 'inherit' to represent this in general.)

Comment 6

[Ben Bucksch (:BenB)]

> I think what you mean by that is that you want it to have 0 margin, padding, and
> border.

No. If the default is a blue bar, I don't want to set it to a blue bar. I want
to use the same bar color that the wrapping blockquote uses, which is ultimately
sat by a pref (generating HTML).

For these elements, I don't want them to have any effect on style. That's what
inherit does and initial wouldn't.

Comment 7

[David Baron :dbaron:]

No, 'inherit' says that if the parent has a blue border, you want *two* blue
borders.  That's definitely an effect on style.

Comment 8

[Ben Bucksch (:BenB)]

Yes, in that case I wanted to have the one bar for each blockquote. Probably a
bad example.
You commented on the margin/border rules for blockquote/pre only. In that case,
in the outer blockquote has been sat to a margin: 1em, you probably want the
nested ones to have the same margin, not?

Note: I fiddled quite a lot with the rules when I wrote them, and they now work
in all cases I know of, even with user stylesheets.
If you have an concrete alternative proposal, test it with all the edge cases
and if the result is the same or better, feel free to submit a patch and we can
discuss about it.

Comment 9

[David Baron :dbaron:]

All my patch has is a comment asking why.  I think any use of inherit needs to
be explained clearly, and you certainly have the opportunity to do that after
this patch goes in, unless you want to give the explanations now.

Comment 10

[Ben Bucksch (:BenB)]

> All my patch has is a comment asking why.

If you want to ask something, you know my email address. "XXX" means "fix me",
while I don't think that there's anything to fix. Please remove the comments.

> unless you want to give the explanations now.

Didn't I?

If it's just the margin/border widths of the pre/blockquotes, I have no strong
objections, if you set them to 0, *if* that breaks nothing.


You didn't yet say, why you don't use html as the default namespace. The message
body is mostly or even completely HTML (ordinary Transitional HTML, not XHTML, BTW).

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

Is this bug also about this css error, or should I file a new bug for that?
CSS Error
(jar:resource:///chrome/classic.jar!/skin/classic/messenger/messageBody.css
:52.6): Unknown namespace prefix 'html'.  Selector expected.  Ruleset ignored
due to bad selector.

Comment 12

[David Baron :dbaron:]

Yes, the patch in this bug fixes that CSS error.

Comment 13

[Bob Clary [:bc] (inactive)]

Attachment: (Av1b-MAS) version of patch addressing only the namespace issues

I thought it would be helpful to separate the invalid html namespace issue from
the other questions related to inherited etc.

Comment 14

[David Baron :dbaron:]

Comment on attachment 139082 [details] [diff] [review]
(Av1b-MAS) version of patch addressing only the namespace issues

r=dbaron, although there are probably more copies of this file now

Comment 15

[[e-mail]]

Comment on attachment 139082 [details] [diff] [review]
(Av1b-MAS) version of patch addressing only the namespace issues

I've trawled through the code, and found that this stylesheet is referenced
twice, once obviously in the HTML message display, and once in
nsMimeXmlEmitter. However as this is only used when mHeadersOnly is true, the
output is promptly discarded by QuotingOutputStreamListener::OnDataAvailable().
Therefore I'd want to see the default namespace should set to HTML instead,
removing XUL styles.

Comment 16

[Justin Wood (:Callek)]

Attachment: (Bv1-MAS) Patch to address review comments

This address the review comments earlier (makes html default namespace, xul is
the other one).

Comment 17

[[e-mail]]

Comment on attachment 159996 [details] [diff] [review]
(Bv1-MAS) Patch to address review comments

I'll have to check when I get back, but I think some of those classes are used
when printing the message header table.

Comment 18

[[e-mail]]

Comment on attachment 159996 [details] [diff] [review]
(Bv1-MAS) Patch to address review comments

window isn't used at all; mailattachcount, header and headerdisplayname are not
XUL, only "used" by the XML emitter while .header-partN are all HTML not XUL.

Comment 19

[Justin Wood (:Callek)]

>(From update of attachment 159996 [details] [diff] [review])
>window isn't used at all; mailattachcount, header and headerdisplayname are not
>XUL, only "used" by the XML emitter while .header-partN are all HTML not XUL.

had no clue I should remove window...
mailattachcount, header, and headerdisplayname  were not touched by original
patch, so I assumed they were xul (since there are no other namespaces).

classes I can fix, can you please advise me on what to do with |window| and with
the other entities?

Comment 20

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8a4) Gecko/20040927] (release) (W98SE)

(line changed)
{{
Error: Unknown namespace prefix 'html'.  Selector expected.  Ruleset ignored due
to bad selector.
Source File: chrome://messenger/skin/messageBody.css
Line: 54
}}

This error is now reported in the JS.C. [new feature];
I hope it can be fixed before v1.8a5.

Comment 21

[Serge Gautherie (:sgautherie)]

Neil:
Could you answer the question(s) in comment 19 ?
(I guess you missed that comment since you were not added yet in the CC list...)

Comment 22

[Serge Gautherie (:sgautherie)]

Comment on attachment 159996 [details] [diff] [review]
(Bv1-MAS) Patch to address review comments

>Index: ./themes/classic/messenger/messageBody.css
>===================================================================
> 
>+/* set default namespace to HTML */
>+@namespace xul url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
>+@namespace url("http://www.w3.org/1999/xhtml"); 
> 
> 
>Index: ./themes/modern/messenger/messageBody.css
>===================================================================
> 
>+/* set default namespace to HTML */
> @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
> @namespace html url("http://www.w3.org/1999/xhtml"); 

I guess the 2 modern lines should be changed like the classic ones...

Comment 23

[Ben Bucksch (:BenB)]

Callek, please carefully test in all the combination of cases (see end of
comment 2). This is the main work to be done in this bug. Comment 22 makes me
wonder, if this is tested at all. I reserve the right to demand a backout or
fast fix, should anybody find regressions. This bug sounds innocent, but has the
potential for hard-to-find regressions. That said, I'd really like this bug to
be fixed (was thinking of doing it myself).

Comment 24

[[e-mail]]

(In reply to comment #19)
>classes I can fix, can you please advise me on what to do with |window| and
>with the other entities?
Just removing those four rogue tags (and html|html) should suffice, I think.

Comment 25

[Justin Wood (:Callek)]

Ben or Serge, feel free to finish the patch for this bug (or someone else) I
wont have time for a while, and I imagine that at least some people want this
fixed ;-)

No need to credit me by the way ;-)

Comment 26

[Serge Gautherie (:sgautherie)]

(In reply to comment #23) [Ben Bucksch]
> That said, I'd really like this bug to
> be fixed (was thinking of doing it myself).

(In reply to comment #25) [Justin Wood]
> Ben or Serge, feel free to finish the patch for this bug (or someone else) I
> wont have time for a while

I could update the patch, but my guess is that fully testing it is beyond my skill.
Ben, would you be ready to assign this bug to yourself and deal with it ?

Comment 27

[Ben Bucksch (:BenB)]

> Ben, would you be ready to ... deal with it ?

Probably not at the moment. Not sure.

Comment 28

[Serge Gautherie (:sgautherie)]

Attachment: (Bv2-MAS) <messageBody.css>

Bv1, with comment 24 suggestion(s).

Is |@namespace url("http://www.w3.org/1999/xhtml");| wanted in
<messageBody.css> ?
If yes, should that same line be added to <smileys.css> (in a separate patch) ?


Implicitly, I removed |div.headerdisplayname| as a "fifth rogue tag"...

I noticed that these 2 theme files are (now) identical, but I guess that's
wanted as is.

As I warned, this patch is untested: helpwanted !

Comment 29

[[e-mail]]

Comment on attachment 171384 [details] [diff] [review]
(Bv2-MAS) <messageBody.css>

(In reply to comment #28)
>Is |@namespace url("http://www.w3.org/1999/xhtml");| wanted in
><messageBody.css> ?
That's the whole point of this bug.

>If yes, should that same line be added to <smileys.css> (in a separate patch)?
I'd accept them both in a single patch.

>Implicitly, I removed |div.headerdisplayname| as a "fifth rogue tag"...
That's not how CSS works. div is the tag, and headerdisplayname is actually a
class name. The generated HTML looks like <div class="headerdisplayname">.

>I noticed that these 2 theme files are (now) identical, but I guess that's
>wanted as is.
That sounds about right to me for now, though at some point scott/bienvenu
might want to move them to a single file in content.

Comment 30

[Serge Gautherie (:sgautherie)]

Attachment: (Bv3-MAS) <messageBody.css> ++ [Checked in: Comment 57]

Bv2-MAS, with comment 29 suggestion(s).

(These 2 theme <smileys.css> are identical too.)

As I warned, this patch is untested...

Comment 31

[David Baron :dbaron:]

Comment on attachment 171462 [details] [diff] [review]
(Bv3-MAS) <messageBody.css> ++
[Checked in: Comment 57]

clearing review request since previous reviewers have requested specific
testing

Comment 32

[Ben Bucksch (:BenB)]

This is so not a blocker.

Comment 33

[Robert Kaiser]

We (or better MoFo) have been shipping a few releases with that without
problems, so we can also ship an alpha with it. That doesn't mean it can't go
in, just that we won't hold alpha for it :)

Comment 34

[Alex Vincent [:WeirdAl]]

(In reply to comment #31)
> clearing review request since previous reviewers have requested specific
> testing

dbaron, I'm willing to test this if it's still needed.

Comment 35

[Serge Gautherie (:sgautherie)]

(In reply to comment #34)
> dbaron, I'm willing to test this if it's still needed.

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050705 SeaMonkey/1.0a]
(nightly) (W98SE)

Bug still there ... your help would be welcomed !

Comment 36

[Paul Tomlin]

Thunderbird version 1.5 Beta 2 (20051006) (WinXP)

Error: Unknown namespace prefix 'html'.  Ruleset ignored due to bad selector.
Source File: chrome://messenger/skin/messageBody.css
Line: 55

Comment 37

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8) Gecko/20051215 SeaMonkey/1.0b] (nightly) (W98SE)

(Someday maybe.)

Comment 38

[Alex Vincent [:WeirdAl]]

Requesting blocking-seamonkey1.0.  A prior version of this patch has r=dbaron, and I believe it's SeaMonkey only.  It does not affect UI, and predates the SeaMonkey project.  At best, the goal is to silence an error.

Comment 39

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

This hardly seems severe enough to block for.  However, it also sounds low-risk enough that a patch may be acceptable post-beta.

Comment 40

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20051230 SeaMonkey/1.5a] (nightly) (W98SE)

The error was reduced to a warning
{{
Warning: Unknown namespace prefix 'html'.  Ruleset ignored due to bad selector.
Source File: chrome://messenger/skin/messageBody.css
Line: 54
}}
by a checkin in the last week.

Comment 41

[Karsten Düsterloh]

*** Bug 322653 has been marked as a duplicate of this bug. ***

Comment 42

[Eyal Rozenberg]

(In reply to comment #40)

And I get this warning all the time. can't someone change that 

html|html {

line?

Comment 43

[Ilja Sekler]

(In reply to comment #30)

> As I warned, this patch is untested...

So far no problems with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.1) Gecko/20060127 SeaMonkey/1.0, patch applied. The error/warning mentioned above doesn't appear any more, can't find any regressions from the patch.

Comment 44

[Eyal Rozenberg]

Serge: It seems people report the patch is working - how about a review?

Comment 45

[Serge Gautherie (:sgautherie)]

(In reply to comment #44)
> Serge: It seems people report the patch is working - how about a review?

Comment 43 doesn't seem enough to me, compared to what comment 23 asks for.

Ben, Alex:
Would one of you have time now to do the extended test part ?

Comment 46

[Serge Gautherie (:sgautherie)]

Comment on attachment 171462 [details] [diff] [review]
(Bv3-MAS) <messageBody.css> ++
[Checked in: Comment 57]

From a Neil's email:
{{
I did a quick test on it, e.g. smileys still work.
}}

Comment 47

[David Baron :dbaron:]

What's all the removed stuff?  Why is it not needed anymore?

Comment 48

[Karsten Düsterloh]

> What's all the removed stuff?  Why is it not needed anymore?

See comment #18.

Comment 49

[David Baron :dbaron:]

(In reply to comment #18)
> mailattachcount, header and headerdisplayname are not
> XUL, only "used" by the XML emitter

Does this mean there's no need to style them?

Comment 50

[Mike Cowperthwaite]

(In reply to comment #47)
> What's all the removed stuff?  Why is it not needed anymore?

FWIW, using Seamonkey 1.0, I loaded a message window with a forwarded message (invoking the messageBody.css file) into the DOM Inspector and checked the 'window' element and the 'html' element, neither which load any rules from 
messageBody.css.  I also loaded a .EML file into the browser and checked the 'window' -- same thing; but for some reason, I couldn't drill down to the display of the message; the tabbrowser node (id = content) showed no subnodes 
in DOMi.  At any rate, the sorts of subtle regressions Ben was worrying about seem unlikely as the rules for those elements don't seem to be applied 
anywhere.


Somewhat OT, I note that the rule in messageBody.css for 
  |div.headerdisplayname| setting |white-space:pre;| 
is superfluous: that's being applied only to the header name in the table of headers, and the header name is always one word (altho, sometimes with 
hyphens).  There is no whitespace to be controlled; and since that .headername <div> has |display:inline;| the 'white-space' rule doesn't apply anyway.
(Why a <div> instead of a <span>?)

Comment 51

[David Baron :dbaron:]

Comment on attachment 171462 [details] [diff] [review]
(Bv3-MAS) <messageBody.css> ++
[Checked in: Comment 57]

OK, it's ok with me but I'd to double-check with mscott that those rules aren't needed.

Comment 52

[Serge Gautherie (:sgautherie)]

(In reply to comment #50)
> Somewhat OT, I note that the rule in messageBody.css for 
>   |div.headerdisplayname| setting |white-space:pre;| 
> is superfluous

David, Neil, Scott:
Then, should I remove that |white-space:pre;| line too ?

Comment 53

[[e-mail]]

(In reply to comment #50)
>I also loaded a .EML file into the browser and checked the 'window'
Easier is to press Ctrl+Shift+I which loads the current tab into a new DOMI window. Otherwise you have to remember to view anonymous content.

(In reply to comment #52)
>should I remove that |white-space:pre;| line too ?
You never know, someone might implement hyphenation...

Comment 54

[Scott MacGregor]

Are we really really sure these rules aren't used anymore? We still use the classnames in the various mime emitters including the base emitter which uses headerdisplayname:

http://lxr.mozilla.org/mozilla/source/mailnews/mime/emitters/src/nsMimeBaseEmitter.cpp#737

Comment 55

[[e-mail]]

(In reply to comment #54)
>the base emitter [snip] uses headerdisplayname:

We actually declare it twice, so we're removing the unused version:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/themes/classic/messenger/messageBody.css&rev=1.13&mark=69,83#68

Comment 56

[Scott MacGregor]

Comment on attachment 171462 [details] [diff] [review]
(Bv3-MAS) <messageBody.css> ++
[Checked in: Comment 57]

thanks for the explanation Neil.

Comment 57

[[e-mail]]

Fix checked in to the trunk and branch.

Comment 58

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20060315 SeaMonkey/1.5a] (nightly) (W98SE)

V.Fixed.

Comment 59

[Serge Gautherie (:sgautherie)]

(In reply to comment #36)
> Thunderbird version 1.5 Beta 2 (20051006) (WinXP)
> 
> Error: Unknown namespace prefix 'html'.  Ruleset ignored due to bad selector.
> Source File: chrome://messenger/skin/messageBody.css
> Line: 55

I had missed this comment :-(

[Mozilla Thunderbird, version 3 alpha 1 (20060403)] (nightly) (W98SE)

This bug is still present in TB :-/

Comment 60

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) </Toolkit> port

Comment 61

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1) </mail> ThunderBird port [Checkin: Comment 65]

I left alone 'mailattachcount' in both files, as it has a color in pinstripe...

Untested patch; comment welcomed.

Comment 62

[David :Bienvenu]

I'll try out the TB patch.

Comment 63

[Serge Gautherie (:sgautherie)]

Comment on attachment 171462 [details] [diff] [review]
(Bv3-MAS) <messageBody.css> ++
[Checked in: Comment 57]

(In reply to comment #39)
> This hardly seems severe enough to block for.  However, it also sounds low-risk
> enough that a patch may be acceptable post-beta.

Bug is still present in
[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.0.1) Gecko/20060130 SeaMonkey/1.0] (release) (W98SE)

'approval&#8209;seamonkey1.0.2=?': 
Trivial U.I. code cleanup, no risk.

Comment 64

[David :Bienvenu]

I'll land this -I've been running with it.

Comment 65

[David :Bienvenu]

TB fix checked in, thx very much, Serge.

Comment 66

[Serge Gautherie (:sgautherie)]

Comment on attachment 217348 [details] [diff] [review]
(Dv1) </mail> ThunderBird port
[Checkin: Comment 65]

'approval-branch-1.8.1=?': (ThunderBird only)
Simple U.I. code cleanup, no risk.
(You previously a+ the SM part.)

Comment 67

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8) Gecko/20060414 SeaMonkey/1.1a] (nightly) (W98SE)

V.Fixed for SMv1.1a.

Comment 68

[Serge Gautherie (:sgautherie)]

Comment on attachment 217348 [details] [diff] [review]
(Dv1) </mail> ThunderBird port
[Checkin: Comment 65]

'approval1.8.0.3=?': (ThunderBird only)
Simple U.I. code cleanup, no risk.

Comment 69

[David Baron :dbaron:]

This doesn't seem anywhere remotely close to the criteria for security+stability releases.  Please stop nominating bugs that don't fit the criteria.

http://developer.mozilla.org/devnews/index.php/2005/12/16/whats-next/

Comment 70

[Serge Gautherie (:sgautherie)]

(In reply to comment #69)
> This doesn't seem anywhere remotely close to the criteria for
> security+stability releases.  Please stop nominating bugs that don't fit the
> criteria.
> 
> http://developer.mozilla.org/devnews/index.php/2005/12/16/whats-next/

It may not be close enough (meaning approval will be minused), but I don't see it as far as you say: let me explain.

1) Security: it's clearly not.

2) "reliability": it may not be a "major performance issues", but you did add the 'perf' keyword. (see comment 0)

3) Then:
"Has been on the trunk with no regressions reported in 2 weeks": yes, first checkin happened a month ago.
"Must have a clear explanation in the bug of": 
 "Summary of the changes in the fix": cleanup, change namespaces between html and xul.
 "Risk assessment": none/low, already commented on between you and others.
 "A reproducible testcase": displaying an email.
 "l10n impact": none.
"Does not change any public API": (N/A).
"Does not change website compatibility or rendering": (N/A).

And it was said:
"This hardly seems severe enough to block for.  However, it also sounds low-risk
enough that a patch may be acceptable post-beta."

This is what led me to ask for Gv1.8.0 branch too.

Comment 71

[David Baron :dbaron:]

(In reply to comment #70)
> 3) Then:

Not relevant, since that part only applies if something from the first list applies.

But how about not trying to twist the text to fit your intent and instead applying some common sense?

Comment 72

[Serge Gautherie (:sgautherie)]

(In reply to comment #71)
> But how about not trying to twist the text to fit your intent and instead
> applying some common sense?

My intent was to improve stable releases too, but I stand corrected.

*****

(In reply to comment #65)
> TB fix checked in, thx very much, Serge.

Thanks to you (and others) too !

Did anyone have a chance to check whether I was right when "doing":
"I left alone 'mailattachcount' in both files, as it has a color in pinstripe...",
since it was removed in the xpfe patch( see comment 18) ?

*****

(In reply to comment #60)
> Created an attachment (id=217344) [edit]
> (Cv1) </Toolkit> port

Now, I read what scott wrote in bug 321389 comment 3, which probably applies to my patch too:
"Im pretty sure this patch isn't going to do anything. Thunderbid no longer
builds tookit\themes\qute, instead we use winstripe now. I haven't checked in
the code to cvs remove the files yet."

Scott, what should we do about this ?

Comment 73

[Serge Gautherie (:sgautherie)]

Comment on attachment 217344 [details] [diff] [review]
(Cv1) </Toolkit> port

(See comment 72)

Comment 74

[Serge Gautherie (:sgautherie)]

Comment on attachment 217348 [details] [diff] [review]
(Dv1) </mail> ThunderBird port
[Checkin: Comment 65]


Checkin: { 2006-04-23 02:19	bugzilla%standard8.demon.co.uk 	MOZILLA_1_8_BRANCH }

Comment 75

[Serge Gautherie (:sgautherie)]

[Mozilla Thunderbird, version 2 alpha 1 (20060425)] (nightly) (W98SE)

V.Fixed for TBv2.0a1

Comment 76

[Serge Gautherie (:sgautherie)]

Comment on attachment 217344 [details] [diff] [review]
(Cv1) </Toolkit> port

(This file does not exist anymore.)

Comment 77

[Serge Gautherie (:sgautherie)]

Whether I was right to keep the 'mailattachcount' rules in the <mail/> patch was not answered,
but I'm resolving this bug anyway.