Closed Bug 1470733 Opened 3 years ago Closed 3 years ago

Quote side bar incorrectly styled in compose window: Too thin and wrong colour

Categories

(Thunderbird :: Theme, enhancement)

enhancement
Not set
normal

Tracking

(thunderbird_esr60 fixed, thunderbird60 fixed, thunderbird61 wontfix, thunderbird62 fixed)

RESOLVED FIXED
Thunderbird 62.0
Tracking Status
thunderbird_esr60 --- fixed
thunderbird60 --- fixed
thunderbird61 --- wontfix
thunderbird62 --- fixed

People

(Reporter: jorgk-bmo, Assigned: Paenglab)

Details

Attachments

(1 file, 3 obsolete files)

As per PM to assignee ;-) - TB 60 beta already affected.
This fixes the border width for me. Additionally I cleaned the code a bit (removed the border-radius and changed a bit of the spacing between the quote level borders).
Attachment #8987331 - Flags: review?(jorgk)
Comment on attachment 8987331 [details] [diff] [review]
Bug1470733-quoteBorderWidth.patch

Review of attachment 8987331 [details] [diff] [review]:
-----------------------------------------------------------------

This works for me, but I'd like to understand why so many changes are required.

::: mail/themes/windows/mail/messageQuotes.css
@@ +43,4 @@
>    border-color: rgb(114,159,207); /* Sky Blue 1 */
>  }
>  
> +blockquote[type=cite] > blockquote {

Why is it OK to leave off the [type=cite] on the second one? So why not:

blockquote[type=cite] > blockquote[type=cite]

And why change the selector to > ?

Oh, I see, Mac was always like this. But is that correct?
Windows-only with only body prepended. This works, too.
(In reply to Jorg K (GMT+2) from comment #2)
> Comment on attachment 8987331 [details] [diff] [review]
> Bug1470733-quoteBorderWidth.patch
> 
> Review of attachment 8987331 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This works for me, but I'd like to understand why so many changes are
> required.
> 
> ::: mail/themes/windows/mail/messageQuotes.css
> @@ +43,4 @@
> >    border-color: rgb(114,159,207); /* Sky Blue 1 */
> >  }
> >  
> > +blockquote[type=cite] > blockquote {
> 
> Why is it OK to leave off the [type=cite] on the second one? So why not:

I copied it from osx stylesheet and there it worked always. It seems, when we are in a blockquote[type=cite] all blockquote children are then of type="cite".

> blockquote[type=cite] > blockquote[type=cite]
> 
> And why change the selector to > ?

It's faster. Then it checks only the direct parent if it's a blockquote. Without '>' it could be every parent in the DOM tree and the parser checks every parent up to the root if it's a blockquote[type=cite].

> Oh, I see, Mac was always like this. But is that correct?

It worked until now, so I think yes.
(In reply to Jorg K (GMT+2) from comment #3)
> Created attachment 8987336 [details] [diff] [review]
> Bug1470733-quoteBorderWidth.patch - JK
> 
> Windows-only with only body prepended. This works, too.

Only the first blockquote[type=cite] level needs the 'body' added. All others are more specific than the one from html.css.
(In reply to Richard Marti (:Paenglab) from comment #4)
> I copied it from osx stylesheet and there it worked always. It seems, when
> we are in a blockquote[type=cite] all blockquote children are then of
> type="cite".
Well, sure, in anything we produce, but nothing stops people creating blockquotes which aren't cites. It would be more correct to check the way Windows did.

> > And why change the selector to > ?
> It's faster. Then it checks only the direct parent if it's a blockquote.
> Without '>' it could be every parent in the DOM tree and the parser checks
> every parent up to the root if it's a blockquote[type=cite].
Yes, but it's also safer. And in practice, there wouldn't be much to search. For the first level, the parent is the body. And for the level 2+ the parent is another quote. Only weird cases will cause more searching. Again, I prefer the Windows way.

> > Oh, I see, Mac was always like this. But is that correct?
> It worked until now, so I think yes.
I think I can construct something where the Mac code won't do the right thing.

(In reply to Richard Marti (:Paenglab) from comment #5)
> Only the first blockquote[type=cite] level needs the 'body' added. All
> others are more specific than the one from html.css.
I was just an example.
Attached patch AccountManager.patch (obsolete) — Splinter Review
Okay, is this better. Changed osx to be the same as the other two platforms.
Attachment #8987331 - Attachment is obsolete: true
Attachment #8987336 - Attachment is obsolete: true
Attachment #8987331 - Flags: review?(jorgk)
Attachment #8987346 - Flags: review?(jorgk)
Sorry, wrong patch.
Attachment #8987346 - Attachment is obsolete: true
Attachment #8987346 - Flags: review?(jorgk)
Attachment #8987365 - Flags: review?(jorgk)
Comment on attachment 8987365 [details] [diff] [review]
Bug1470733-quoteBorderWidth.patch

Review of attachment 8987365 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks that works for me on Windows and should work on Linux equally. Mac is still mysteriously different.

::: mail/themes/osx/mail/messageQuotes.css
@@ +33,3 @@
>    color: blue !important;
>    border-color: blue !important;
> +  border-inline-start-width: 2px;

Why is the CSS for Mac so different and where does border-inline-start-width: 2px; suddenly come from when it wasn't there before?

@@ +39,5 @@
>    color: green !important;
>    border-color: green !important;
>  }
>  
> +blockquote[type=cite] blockquote[type=cite] blockquote[type=cite] {

Why does Mac only have three levels when the other platforms have five? Should we increase that?
Attachment #8987365 - Flags: review?(jorgk) → review+
(In reply to Jorg K (GMT+2) from comment #9)
> Comment on attachment 8987365 [details] [diff] [review]
> Bug1470733-quoteBorderWidth.patch
> 
> Review of attachment 8987365 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why is the CSS for Mac so different and where does
> border-inline-start-width: 2px; suddenly come from when it wasn't there
> before?

It was always 1px before. Now it's 2px like in message reading. The difference is, that Mac has the border only on the left side.
 
> Why does Mac only have three levels when the other platforms have five?
> Should we increase that?

This is since long time like this. I decided to leave it like it is now. If we want to extend it, we'd need to choose the border colors as Mac uses different to the other platforms.
Keywords: checkin-needed
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/5d4f4fe4d4e6
Fix the quote border width in the compose window. r=jorgk
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 62.0
Comment on attachment 8987365 [details] [diff] [review]
Bug1470733-quoteBorderWidth.patch

I hope that fixes the TB 60 beta problem, too.
Attachment #8987365 - Flags: approval-comm-esr60+
Attachment #8987365 - Flags: approval-comm-beta+
Works fine in the beta.
You need to log in before you can comment on or make changes to this bug.