249109 - Double-quotes (both bar and >) in display

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Ben Bucksch (:BenB)]

Bug 245196 broke various cases of quote display, e.g. with certain prefs set,
maybe even in default config. You need to consider these cases in the stylesheet
rules.

We're having a number of combinations here,
controlled by prefs and stylesheets, making this quite complicated.
See
<http://lxr.mozilla.org/seamonkey/source/themes/modern/messenger/messageBody.css#114>.

See bug 245196 comment 10 for problem description and screenshot.

Comment 1

[Ben Bucksch (:BenB)]

*Probably* You'll only need to add
.moz-text-plain[graphical-quote="true"]
in front of the rules. But I'm not sure, you'll need to test all cases (all
combinations of msg types, prefs etc.).

Pref:
The way to add a pref for it would be to add
[colored-quotes="true"]
and then set that attribute in mimetpla/mimetfl.cpp depending on the pref.

Style:
FWIW, my styling rules in userContent.css (no prefs etc. considered) are:

blockquote[type=cite] {
  border-color: gray ! important;
  color: red ! important;
}
blockquote[type=cite] blockquote {
  border-color: red ! important;
  color: green ! important;
}
blockquote[type=cite] blockquote blockquote {
  border-color: green ! important;
  color: blue ! important;
}
blockquote[type=cite] blockquote blockquote a {
  color: fuchsia;
}
blockquote[type=cite] blockquote blockquote blockquote {
  border-color: blue ! important;
  color: gray ! important;
}
blockquote[type=cite] blockquote blockquote blockquote blockquote {
  border-color: gray ! important;
}
blockquote[type=cite] pre, blockquote[type=cite] div {
  color: inherit ! important;
}

This colors the text and the bars (according to who added them), but not the
background to not reduce contrast too much.

Comment 2

[Ben Bucksch (:BenB)]

And please test, if the existing quote coloring pref UI (4.x-heritage) still
works. I personally find it relatively useless, but I don't know about others.

Sorry for overloading the bug, but it's all the same code. We can still break it
out.

Comment 3

[Peter Lairo]

Since this bug _may_ also be addressing the bar/text/background colors, here's a
quote from bug 90315 comment 34 by a colorblind person who names some imortant
criteria for color selection. Particularly: 

"... not putting green and red next to each other (blue could be inserted in
between) - that might help the extreme cases." and "Still even for a completely
colourblind person (monocromatic vision) the _shades of grey on the background_
will indicate the difference in quotelevel .. as will the quotebars."

I hope colors for the bars, text _and_ background will be the default.

Comment 4

[parish]

As a temporary workround you can disable the quote bars by adding this to
userContent.css:

blockquote[type=cite] {
         padding: 0 ! important;
         border-left: none ! important;
         border-right: none ! important;
}

Comment 5

[Mike Cowperthwaite]

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

Comment 6

[Mike Cowperthwaite]

I would use this CSS rather than comment 4:

.moz-text-plain blockquote[type=cite]
{
  border-left: none !important;
  border-right: none !important;
  padding-left: 0 !important;
}

The .moz-text-plain is necessary to keep the styling from being applied to format=flowed messages, which always strip the '>' characters in the quotes -- you want to have the vertical bars in that case.

I note that in the default skin (classic.jar), messageQuotes.css has several rules which set the border and the padding -- and these rules are all flagged with !important.  However, I'm seeing the rules from the above CSS being applied as expected with no interference.

Comment 7

[Mike Cowperthwaite]

Actually, let me amend that further!  Try this:

.moz-text-plain[graphical-quote=false] blockquote
{
  border-left: none !important;
  border-right: none !important;
  padding: 0 !important;
}

This automatically turns off borders only when graphical-quote=false, so you can turn the pref on and off as desired.

Boy, figuring this stuff out would be a lot easier if you didn't have to restart the program for every userContent change.

Comment 8

[Mike Cowperthwaite]

Attachment: Overhaul messageQuotes.css (diff -uw vs. 2b1-0116 distro)

The problem seen here was due to all the |!important| flags in messageQuotes.css.  The reason this was used, I guess, is that Scott hadn't figured out a better way to override the  |border-color:gray|  imposed for the various plain-text quotebars in messageBody.css.

I got rid of all the |!important|s; the way I force the color override is to use a more-specific selector.  I also compacted the redundant styles to a single occurrence; the only thing that varies going from level to level is the border color, so that's what the rules change.  This has the added advantage of making the behavior much easier to override in userContent.css (if, say, the right-hand border should be turned off).

Altho this file is ostensibly only for coloring, it does in fact turn borders and padding on for quotes -- this is already done (somewhat differently) for the various types of plain-text messages in messageBody.css, but the quote bordering is also desired for HTML messages and the editor.  I think this could be done more efficiently, but I was trying to limit the patch to a single file.

Comment 9

[Ben Bucksch (:BenB)]

> I think this could be done more efficiently, but I was trying to
> limit the patch to a single file. 

Feel free to fix messageBody.css at the same time. You are the only person on earth I'd trust to do that properly :).

Comment 10

[Mike Cowperthwaite]

Attachment: v.1.1: Same patch plus one small tweak

I realized that the previous patch was showing the quotebar on the right for 
quoted-graphical=false,  I had to add an extra rule to turn it back off.

> Feel free to fix messageBody.css at the same time.

I'm disinclined to touch that file because there are four versions of it in the repository -- one for each of the supported themes (suite: classic & modern, TB: qute [a.k.a. classic] & pinstripe), while qute is the only theme that makes use of messageQuotes.css.  In the area that I'd need to touch, those four are all identical -- conceivably, a core-level messageBody.css is possible.

Comment 11

[Scott MacGregor]

mmm, I really like where this patch is going. Good stuff Mike.

Comment 12

[Mike Cowperthwaite]

Attachment: v.1.2: another tweak

I realized I also had to turn off padding -- specifically, I wanted to cancel padding-bottom -- for non-graphical quotes.  This is due to the pathological test case at bug 367223 -- if someone sends a text/plain message using a string of '>' chars as a separator, the padding would increment once for each '>'.

Comment 13

[Mike Cowperthwaite]

Attachment: v.1.3: another tweak

The previous versions resulted in the first & second borders being blue for the text/plain, graphical-quoted case.  This fixes that.

Comment 14

[Mike Cowperthwaite]

Attachment: v.1.4: another tweak

Dang, another problem: the '2px' border width wasn't getting used for any of the plain-text quoting.  I think this is the last tweak; I've done a comparison of the unpatched vs. patched, and the only difference I see now is the fix for this bug.

Comment 15

[Mike Cowperthwaite]

Attachment: v2: Alternate patch, includes messageBody.css tweaks

All of the thrashing around for the last several versions is exactly because of the difficulty of avoiding conflicts between this file and some of the unnecessary, or unnecessarily-specific, rules in messageBody.css.  So, here's a patch for comparison: it strips down the rules in messageBody.css, thereby allowing all the special-case stuff from messageQuotes.css to be removed entirely in my other patch.

Note the commented-out styles in this patch: those styles are used in the other themes to apply a medium-width grey border to quotes in plaintext messages.  That is, the comment itself would not be included if porting this patch to one of those themes; but the other messageBody.css changes here would apply to all the other themes, and similarly make overriding the styling easier, either within the theme or via userContent.css.


Incidentally: All of these patches are diff'd against 2b1, so they don't incorporate the trunk fix to messageQuotes.css from bug 339818.

Comment 16

[Ben Bucksch (:BenB)]

Last patch: Are you sure that margin: 0 instead of inherit (same maybe for padding) works in all cases? I remember when writing these rules (6 years ago *cough*) I first used absolute values, but found that won't work for a certain case, and I was forced to use inherit, which made several other cases more complex, but still solvable.

Comment 17

[Ben Bucksch (:BenB)]

FWIW, v1.4 looks good to me (r=BenB), although I haven't run it neither do I know whether it breaks anything, I'll rely on you there.
An alternate patch which includes messageBody.css is also good, though, if it doesn't break some cases.

Comment 18

[Ben Bucksch (:BenB)]

Comment on attachment 252180 [details] [diff] [review]
v.1.4: another tweak

I quickly tested, plain, flowed, html, but only with the default prefs, and found nothing breaking. I'll have to rescue my old testcase folder from my archives to test properly.

BTW: I noticed that - with and without patch - be gab between 2 quote bars is different left and right. That doesn't look good. (But it's not this bug.)

Comment 19

[Mike Cowperthwaite]

(In reply to comment #16)
> Last patch: Are you sure that margin: 0 instead of inherit (same maybe for
> padding) works in all cases?

No, I'm not *sure* -- however, setting margin:0 (or padding:0) essentially is reverting the property to the default, as margin and padding do not inherit unless explicitly set to.

Forcing 'inherit' would mean 'use the margin/padding from the containing element' -- that is, the <div>, either "moz-txt-plain" or "moz-txt-flowed".  If somebody were to customize one of these <div>s with margin or padding, the blockquotes would be rendered with that margin or padding re-applied, due to inheritance; that's the case that I'm trying to prevent by changing to an explicit 0.

If you have a testcase that shows a problem, I'd be happy to see it; but 0  seems like the logical value.


(In reply to comment #18)
> I quickly tested, plain, flowed, html, but only with the default prefs, and
> found nothing breaking.

Well, pertinent to this bug, did you at least set quoted_graphical to false while testing?


> BTW: I noticed that - with and without patch - [the gap] between 2 quote
> bars is different left and right. That doesn't look good.

Yeah -- that's bug 339818, referred to in comment 15 and fixed on trunk.

Comment 20

[Mike Cowperthwaite]

Comment on attachment 252180 [details] [diff] [review]
v.1.4: another tweak

Scott, take a look at both this and attachment 252183 [details] [diff] [review] -- assuming Ben doesn't present data that breaks the latter, I guess that would be the way to go -- it yields a cleaner result; a patch to Pinstripe would follow.

How are theme authors alerted when a change like this takes place, so that they can integrate the changes if needed?

Comment 21

[Ben Bucksch (:BenB)]

I digged out my test folders (took these 3 hours). See 133016 comment 80 (accidentally attached there).

Patch 1.4 works fine and fixes bug. I haven't tried the other.

---

I'm wondering: These CSS rules are so complex that hardly anybody can change them without breaking anything. Half of that complexity is due to the multitue of prefs allowed.

The quote_graphical = false pref, and the way it's implemented using CSS, is the worst and a big reason for the complexity.

The idea was that I wanted to be conservative when altering the mail content, so I left the >s in the generated content and removed them via CSS. People who don't like the graphical quotes could just use CSS to remove the bars and have the original characters.

However, as we see here, it makes things very complex. Given that this feature has been broken now for 2.5 years, without a huge outcry, I wonder if we should not simply remove it.

It's only for plaintext mails anyways. HTML mails and format=flowed mails will always get graphical quotes (no matter which pref), and plaintext mails get graphical quotes by default.

Comment 22

[Ben Bucksch (:BenB)]

> See 133016 comment 80

ops. linked: bug 133016 comment 80

Comment 23

[Mike Cowperthwaite]

Comment on attachment 252183 [details] [diff] [review]
v2: Alternate patch, includes messageBody.css tweaks

Thanks for that test data, Ben.  You did have one case I hadn't considered: a Mozilla HTML message quoted by Outlook -- but both versions of the patch work as I would expect for that: the quoted type=cite blocks are rendered with the TB multi-colors inside Outlook's blockquote.

I'm feeling more confident about the two-file patch, so I'd like to request a formal review on this as well.

Comment 24

[Scott MacGregor]

Comment on attachment 252183 [details] [diff] [review]
v2: Alternate patch, includes messageBody.css tweaks

I'm going to jump into the second patch as this one looks better to me. 

I'd prefer to ditch the commented out styles altogether instead of leaving unused code in the style sheet

Comment 25

[Scott MacGregor]

Comment on attachment 252180 [details] [diff] [review]
v.1.4: another tweak

clearing review request, I think we are using your second patch instead.

Comment 26

[Ben Bucksch (:BenB)]

Comment on attachment 252183 [details] [diff] [review]
v2: Alternate patch, includes messageBody.css tweaks

ok, will review v2 once I have a chance.

Comment 27

[Mike Cowperthwaite]

Scott, I don't know what your timeframe is for 2.0.  Should I find another reviewer?  Any suggestions who?

Comment 28

[Ben Bucksch (:BenB)]

Comment on attachment 252183 [details] [diff] [review]
v2: Alternate patch, includes messageBody.css tweaks

Sorry.

Yes, patch looks and works fine. I guess you're right on the inherit - it works fine with all my testcases.

r=BenB

Patch does not apply on trunk (for me).

Comment 29

[Phil Ringnalda (:philor)]

Attachment: For checkin

Assuming I guessed right about which files with those names you did and didn't want to patch, and managed to hand apply it correctly, I can check this in someday soon.

Comment 30

[Phil Ringnalda (:philor)]

mail/themes/qute/mail/messageBody.css 1.7
mail/themes/qute/mail/messageQuotes.css 1.3

Is that it, or does Pinstripe still need a fix?

Comment 31

[Mike Cowperthwaite]

I can verify that this patch is in place for 3a1p-1013, Win2K -- thanks, Phil!

I can't say whether Pinstripe is affected, but I assume it is -- since they don't use multicolored quotes (different messageQuotes.css), they're probably getting solid-black quotebars, a change from the grey ones they were using before.  This is the rule that was commented out in my submitted patch, and this is probably what should be copied into Pinstripe's messageQuotes.css:

.moz-text-flowed blockquote,
.moz-text-plain blockquote {
  border-width: medium;
  border-color: gray;
}

However, I'll leave that up to a Pinstripe user to open a bug for that issue.
The problem described in this bug is fixed in trunk.