Closed Bug 130728 Opened 23 years ago Closed 19 years ago

messageBody.css should use namespaces correctly

Categories

(SeaMonkey :: MailNews: Message Display, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
seamonkey1.1alpha

People

(Reporter: dbaron, Assigned: sgautherie)

References

Details

(Keywords: fixed-seamonkey1.1a, perf, verified1.8.1, Whiteboard: [verified-seamonkey1.1a])

Attachments

(7 obsolete files)

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.
Severity: normal → trivial
Status: NEW → ASSIGNED
Priority: -- → P5
Target Milestone: --- → Future
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.
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.
A blockquote within an h1??? What I get is that the element does not change anything. And that's what I want.
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.)
> 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.
No, 'inherit' says that if the parent has a blue border, you want *two* blue borders. That's definitely an effect on style.
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.
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.
> 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).
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.
Yes, the patch in this bug fixes that CSS error.
I thought it would be helpful to separate the invalid html namespace issue from the other questions related to inherited etc.
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
Attachment #139082 - Flags: review+
Attachment #139082 - Flags: superreview?(neil.parkwaycc.co.uk)
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.
Attachment #139082 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview-
This address the review comments earlier (makes html default namespace, xul is the other one).
Attachment #139082 - Attachment is obsolete: true
Attachment #159996 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #159996 - Flags: review?(dbaron)
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 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.
Attachment #159996 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview-
>(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?
[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.
Product: Browser → Seamonkey
Flags: blocking1.8a6?
Flags: blocking1.8a6? → blocking1.8a6-
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...)
Flags: blocking1.8b?
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...
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).
(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.
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 ;-)
(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 ?
Severity: trivial → blocker
Severity: blocker → normal
> Ben, would you be ready to ... deal with it ? Probably not at the moment. Not sure.
Attachment #73978 - Attachment description: patch → (Av1-MAS) patch
Attachment #73978 - Attachment is obsolete: true
Attachment #139082 - Attachment description: version of patch addressing only the namespace issues → (Av1b-MAS) version of patch addressing only the namespace issues
Attachment #159996 - Attachment description: Patch to address review comments → (Bv1-MAS) Patch to address review comments
Attachment #159996 - Attachment is obsolete: true
Attached patch (Bv2-MAS) <messageBody.css> (obsolete) — Splinter Review
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 !
Attachment #171384 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #171384 - Flags: review?(dbaron)
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.
Attachment #171384 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview-
Attachment #171384 - Attachment is obsolete: true
Attachment #171384 - Flags: review?(dbaron)
Bv2-MAS, with comment 29 suggestion(s). (These 2 theme <smileys.css> are identical too.) As I warned, this patch is untested...
Assignee: dbaron → gautheri
Attachment #171462 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #171462 - Flags: review?(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
Attachment #171462 - Flags: review?(dbaron)
Priority: P5 → --
Whiteboard: [helpwanted: looking for a patch tester...]
Target Milestone: Future → mozilla1.8beta
Flags: blocking1.8b? → blocking1.8b-
Flags: blocking1.8b2?
This is so not a blocker.
Flags: blocking-seamonkey1.0a?
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 :)
Flags: blocking-seamonkey1.0a? → blocking-seamonkey1.0a-
(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.
(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 !
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
[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8) Gecko/20051215 SeaMonkey/1.0b] (nightly) (W98SE) (Someday maybe.)
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.
Flags: blocking-seamonkey1.0?
This hardly seems severe enough to block for. However, it also sounds low-risk enough that a patch may be acceptable post-beta.
Flags: blocking-seamonkey1.0? → blocking-seamonkey1.0-
[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.
*** Bug 322653 has been marked as a duplicate of this bug. ***
(In reply to comment #40) And I get this warning all the time. can't someone change that html|html { line?
(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.
Serge: It seems people report the patch is working - how about a review?
(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 ?
Attachment #171462 - Flags: superreview?(neil)
Attachment #171462 - Flags: superreview+
Attachment #171462 - Flags: branch-1.8.1?(mscott)
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. }}
Attachment #171462 - Flags: review?(dbaron)
What's all the removed stuff? Why is it not needed anymore?
> What's all the removed stuff? Why is it not needed anymore? See comment #18.
(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?
(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 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.
Attachment #171462 - Flags: review?(mscott)
Attachment #171462 - Flags: review?(dbaron)
Attachment #171462 - Flags: review+
(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 ?
(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...
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
(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 on attachment 171462 [details] [diff] [review] (Bv3-MAS) <messageBody.css> ++ [Checked in: Comment 57] thanks for the explanation Neil.
Attachment #171462 - Flags: review?(mscott)
Attachment #171462 - Flags: review+
Attachment #171462 - Flags: approval-branch-1.8.1?(mscott)
Attachment #171462 - Flags: approval-branch-1.8.1+
Fix checked in to the trunk and branch.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Attachment #171462 - Attachment description: (Bv3-MAS) <messageBody.css> ++ → (Bv3-MAS) <messageBody.css> ++ [Checked in: Comment 57]
Attachment #171462 - Attachment is obsolete: true
[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20060315 SeaMonkey/1.5a] (nightly) (W98SE) V.Fixed.
Status: RESOLVED → VERIFIED
Whiteboard: [helpwanted: looking for a patch tester...]
Target Milestone: mozilla1.8beta1 → seamonkey1.1alpha
(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 :-/
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Attached patch (Cv1) </Toolkit> port (obsolete) — Splinter Review
Attachment #217344 - Flags: review?(bryner)
I left alone 'mailattachcount' in both files, as it has a color in pinstripe... Untested patch; comment welcomed.
Attachment #217348 - Flags: review?
Attachment #217348 - Flags: review? → review?(mscott)
I'll try out the TB patch.
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.
Attachment #171462 - Flags: approval-seamonkey1.0.2?
Attachment #217348 - Flags: review?(mscott) → review+
I'll land this -I've been running with it.
TB fix checked in, thx very much, Serge.
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.)
Attachment #217348 - Attachment description: (Dv1) </mail> ThunderBird port → (Dv1) </mail> ThunderBird port [Checkin: Comment 65]
Attachment #217348 - Flags: approval-branch-1.8.1?(mscott)
[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8) Gecko/20060414 SeaMonkey/1.1a] (nightly) (W98SE) V.Fixed for SMv1.1a.
Whiteboard: ['fixed1.8.1' applies to Bv2-MAS patch only] [verified-seamonkey1.1a]
Attachment #217348 - Flags: approval-branch-1.8.1?(mscott) → approval-branch-1.8.1+
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.
Attachment #217348 - Flags: approval1.8.0.3?
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/
(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.
(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?
Attachment #171462 - Flags: approval-seamonkey1.0.2?
Attachment #217348 - Flags: approval1.8.0.3?
(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 on attachment 217344 [details] [diff] [review] (Cv1) </Toolkit> port (See comment 72)
Attachment #217344 - Flags: review?(mscott)
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 }
Attachment #217348 - Attachment is obsolete: true
Whiteboard: ['fixed1.8.1' applies to Bv2-MAS patch only] [verified-seamonkey1.1a] → [verified-seamonkey1.1a]
[Mozilla Thunderbird, version 2 alpha 1 (20060425)] (nightly) (W98SE) V.Fixed for TBv2.0a1
Comment on attachment 217344 [details] [diff] [review] (Cv1) </Toolkit> port (This file does not exist anymore.)
Attachment #217344 - Attachment is obsolete: true
Attachment #217344 - Flags: review?(mscott)
Attachment #217344 - Flags: review?(bryner)
Whether I was right to keep the 'mailattachcount' rules in the <mail/> patch was not answered, but I'm resolving this bug anyway.
Status: REOPENED → RESOLVED
Closed: 19 years ago19 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
Blocks: 316222
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: