Closed Bug 931484 Opened 7 years ago Closed 7 years ago

Apply transparency rule to signatures only once even if they are nested

Categories

(SeaMonkey :: Themes, defect)

defect
Not set
minor

Tracking

(seamonkey2.23 fixed, seamonkey2.24 fixed, seamonkey2.25 fixed)

RESOLVED FIXED
seamonkey2.24
Tracking Status
seamonkey2.23 --- fixed
seamonkey2.24 --- fixed
seamonkey2.25 --- fixed

People

(Reporter: rsx11m.pub, Assigned: rsx11m.pub)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

This is another followup to bug 855684 which changed the signature style from fixed colors to transparency, thus allowing signature colors to follow the default colors selected in the preferences.

Turns out that in contrast to "color" the "opacity" rule can be nested, thus increasing the transparency with each subsequent level until the text becomes virtually invisible.

See bug 929518 (nested HTML signatures after re-editing the file) and bug 929006 (digest-type mailing list) for testcases.

Thunderbird has removed any style from HTML signatures per bug 917906 (no equivalent reports were filed for SeaMonkey, thus I assume we want to retain that style), hence eliminating the bug 929518 issue for them. A solution template was provided in bug 917906 comment #10. Those fixes should be added to the SeaMonkey themes as well to prevent such cases from making parts of the signature >50% transparent.
(In reply to rsx11m from comment #0)

> Turns out that in contrast to "color" the "opacity" rule can be nested, thus
> increasing the transparency with each subsequent level until the text
> becomes virtually invisible.

Hm. According to the docs, opacity isn't inherited.
Based on the testcases from the other bugs, it evidently is.

https://developer.mozilla.org/en-US/docs/Web/CSS/opacity
> Using this property with a value different than 1 places the element
> in a new stacking context.

So, apparently this is implicitly given by the stacking with different z-index levels applying their own opacity, thus ending up with a multiplicative effect.
Attached patch Possible patch (obsolete) — Splinter Review
So, this is a direct merge of bug 929006 attachment 821953 [details] [diff] [review] and bug 917906 attachment 813327 [details] [diff] [review], following the analyses in bug 917906 comment #10 and bug 929006 comment #4.

This works as advertised but doesn't look very nice with six rules, and the bottom three could be consolidated to something more generic like

  div.moz-text-html > [ :not(.moz-signature) ]* > div.moz-signature

which doesn't seem to be directly possible in CSS.

On the other hand, unlike Thunderbird, we don't need to distinguish between HTML vs. plain-text messages and signatures. Thus, I'll work on simplifying this to get a more concise set of rules.
Attached patch Better solution (obsolete) — Splinter Review
This patch solves the problem in a more general way. Rather than listing the combinations of nodes to get to the top-most signature node to apply the opacity, this leaves 0.5 intact as the general rule for all such nodes but creates more specific rules to catch the nested-<div> instances.

Thus, the second rule matches all

  [ div.moz-txt-sig ]+ div.moz-txt-sig  (plain-text)
  [ div.moz-signature ]+ div.moz-signature  (HTML)

and applies an opacity value of 1.0 to it by overriding the more general rule. Consequently, something like

  div.moz-signature > div.moz-signature > div.moz-signature

would generate opacity: 0.5*0.5*0.5 = 0.125 with the current set of rules, but 0.5*1.0*1.0 = 0.5 with this patch. Note that "opacity: inherit;" didn't work here, apparently it's inheriting the 0.5 value and applies it anyway, thus an explicit 1.0 value is given.

Tested with both default and modern themes, with additional cases from plain-text and HTML messages along with the two testcases.
Attachment #822951 - Attachment is obsolete: true
Attachment #823024 - Flags: review?(neil)
Comment on attachment 823024 [details] [diff] [review]
Better solution

> @media not print {
>   .moz-txt-sig,
>   .moz-signature {
>     opacity: 0.5;
>   }
>+  div.moz-txt-sig div.moz-txt-sig,
>+  div.moz-signature div.moz-signature {
>+    opacity: 1.0;
>+  }
> }
Why the extra "div" in each selector?
Nit: blank line between style blocks please.
Right, it works with just the class names as well and the new rules are still specific enough. I figured that the pre.moz-signature case isn't necessary as those can't nest, but dropping the div makes the code more compact of course.

Blank line added as requested (I wasn't sure due to the @media).
Attachment #823024 - Attachment is obsolete: true
Attachment #823024 - Flags: review?(neil)
Attachment #823040 - Flags: review?(neil)
Comment on attachment 823040 [details] [diff] [review]
Proposed patch (v3)

So far I have been unable to come up with a plausible scenario whereby .moz-txt-sig and .moz-signature classes occur in the same message body.
Attachment #823040 - Flags: review?(neil) → review+
That shouldn't happen, the .moz-sig-txt class is only use in plain-text messages whereas plain-text signatures in HTML messages always use pre.moz-signature instead.

Thanks for the quick review, push for comm-central please.
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/bf3379636c3d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.24
Comment on attachment 823040 [details] [diff] [review]
Proposed patch (v3)

[Approval Request Comment]
Regression caused by (bug #): bug 855684
User impact if declined: impaired visibility of signature in certain cases
Testing completed (on m-c, etc.): tested with 2.23a2 build
Risk to taking this patch (and alternatives if risky): low
String changes made by this patch: none
Attachment #823040 - Flags: approval-comm-beta?
Attachment #823040 - Flags: approval-comm-beta? → approval-comm-beta+
Thanks Ian, push for comm-beta please.
Keywords: checkin-needed
Whiteboard: [c-n: comm-beta]
Keywords: checkin-needed
Whiteboard: [c-n: comm-beta]
You need to log in before you can comment on or make changes to this bug.