849359 - Print button is misaligned in the Composer and Message Composition toolbars with the Modern theme

Status: RESOLVED FIXED

(SeaMonkey :: Themes, defect)

Description

[rsx11m]

+++ This bug was initially created as a clone of Bug #722758 +++

I'm rarely using Composer, thus I didn't notice that the same problem applies here as well (solved for Mail & News before):

 - open the Composer window (new or existing profile)
 - make sure that the toolbar has the "Print" button somewhere
 - this looks fine while using the Default theme
 - switch to Modern theme, now it is too low and misaligned

Seen in urrent Windows 7 and Linux builds.

Comment 1

[rsx11m]

Attachment: Appearance on Windows 7

Comment 2

[rsx11m]

Attachment: Proposed patch

Great, Ian's workaround for Mail & News does it directly for Composer as well!

This takes the additions from bug 722758 attachment 593257 [details] [diff] [review] and puts them into the spot where bug 676991 attachment 553967 [details] [diff] [review] removed the old printButton code.

BTW: I've double-check the Address Book toolbar, but it doesn't have a Print button in its toolbar palette and thus isn't affected.

Comment 3

[rsx11m]

Attachment: Alternative patch

It appears that the Composer mail toolbar doesn't have any small toolbar icons, which would simplify the rules quite a bit. I'm posting this as an alternative version, thus please r+ either one or the other. If you want to ensure that this won't regress again if and when Composer gets a customizable toolbar, then attachment 722967 [details] [diff] [review] would be the better choice.

Comment 4

[rsx11m]

(In reply to rsx11m from comment #2)
> BTW: I've double-check the Address Book toolbar, but it doesn't have a Print
> button in its toolbar palette and thus isn't affected.

Well, I've failed to look into the Mail/News Message Composition toolbar, which shows the same issue as the other toolbars despite not having been touched by the change in bug 676991. Thus, I'll amend the patch to that window as well.

I've also added a dependency to bug 606683 on customization of the Composer main toolbar as part of that bug. Right now I tend to go with the simpler code for the editor/editorPrimaryToolbar.css fix, given that it will have to be revisited when completing the Composer toolbar customization with small icons anyway.

Comment 5

[rsx11m]

Attachment: Proposed fix (v2)

This is attachment 722984 [details] [diff] [review] for the Composer plus code for the Message Composition window. The latter needs the "long" version given that the toolbar is already fully configurable.

Comment 6

[rsx11m]

This may be too close to the next scheduled merge (in about to weeks) to consider the fix for 2.17b, but I'd definitely try to get it in for 2.18a2 once approved.

Comment 7

[Philip Chee]

Comment on attachment 725888 [details] [diff] [review]
Proposed fix (v2)

> +/* To workaround the composer icons are 33px tall */
The grammar looks a bit off. How about:
"To workaround that the composer icons are all 33px tall"

> +/* To workaround the mailnews icons are 33px tall and have no small versions */
--ditto--

+++ b/suite/themes/modern/messenger/messengercompose/messengercompose.css

+toolbar[iconsize="small"] > toolbarpaletteitem > #print-button,
+toolbar[iconsize="small"] > #print-button {
+  list-style-image: url("chrome://communicator/skin/icons/common.png");
+}
+
+toolbar[iconsize="small"] > toolbarpaletteitem > #print-button,
+toolbar[iconsize="small"] > #print-button,
+#print-button {
+  -moz-image-region: rect(5px 42px 38px 0);
+}

You can consolidate the above two rules:

toolbar[iconsize="small"] > toolbarpaletteitem > #print-button,
toolbar[iconsize="small"] > #print-button,
#print-button {
  list-style-image: url("chrome://communicator/skin/icons/common.png");
  -moz-image-region: rect(5px 42px 38px 0);
}

r=me with the above fixed.

Comment 8

[rsx11m]

Attachment: Final patch (v3) [c-c: comment #10]

(In reply to Philip Chee from comment #7)
> > +/* To workaround the composer icons are 33px tall */
> The grammar looks a bit off. How about:
> "To workaround that the composer icons are all 33px tall"

Verbatim copy of Ian's comment from bug 722758 (except for "composer") thus I assumed that it's correct even though it sounded funny to me as well.  ;-)

> You can consolidate the above two rules:

I wasn't sure about that due to the extra #print selector for the second rule, but it works fine with both large and small settings in all states.

Both fixed, ready to be pushed on trunk.

Comment 9

[rsx11m]

Comment on attachment 728629 [details] [diff] [review]
Final patch (v3) [c-c: comment #10]

(In reply to rsx11m from comment #6)
> This may be too close to the next scheduled merge (in about to weeks) to
> consider the fix for 2.17b, but I'd definitely try to get it in for 2.18a2
> once approved.

I don't know if I have to effectively wait until the patch is checked in before being able to request approval, but I'm doing so now thus you can do so as you see fit and possibly check it in on all channels at the same time.

If I'm not mistaken, there should be one beta left next week, thus trying for comm-beta as well unless it's considered too close to the release by now.

[Approval Request Comment]
Regression caused by (bug #): bug 676991
User impact if declined: misalignment in modern theme, no impact on function
Testing completed (on m-c, etc.): c-c and c-a on Windows and Linux
Risk to taking this patch (and alternatives if risky): none anticipated
String changes made by this patch: none

Comment 10

[Philip Chee]

Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/b5f136f16770

Thanks rsx11m

Comment 11

[rsx11m]

Attachment: Backport to bug 722758

Phil, thanks for checking in the main patch. Meanwhile, I've also modified primaryToolbar.css according to your comment #7, adjusting the comment and consolidating the "small" rules. In this way, both versions are identical.

Here the quick follow-up patch in case you want this for Ian's version too.

I'm not envisioning this for the aurora/beta channel as the existing code works fine for those, so this would be for trunk only.

Comment 12

[Philip Chee]

I just remembered why we did http://hg.mozilla.org/comm-central/rev/b5f136f16770#l2.13
in that order of rules:

  2.13 +toolbar[iconsize="small"] > toolbarpaletteitem > #print-button,
  2.14 +toolbar[iconsize="small"] > #print-button,
  2.15 +#print-button {

Which was to minimize churn in Hg Blame. However we are writing new styles here so we might as well have rearranged the order to:

#print-button,
toolbar[iconsize="small"] > #print-button,
toolbar[iconsize="small"] > toolbarpaletteitem > #print-button { ...

And:

#print-button:hover,
toolbar[iconsize="small"] > #print-button:hover { ...
...etc...

Sorry for not spotting this earlier.

> Meanwhile, I've also modified primaryToolbar.css according to your comment #7
Since it's working prefectly well now I'd leave it until there's need to rewrite. This code would need a rewrite anyway if/when we get small icons working.

Comment 13

[rsx11m]

Ok, so we don't want attachment 728725 [details] [diff] [review] if I understand you correctly, but do you want me to post a new follow-up patch relative to comm-central changeset b5f136f16770 addressing comment #12 on the order of the rules?

Now I'm a bit confused...

Comment 14

[rsx11m]

(frankly, I'd prefer to have the primary-toolbar and the composition-toolbar versions not to differ too much as they'd likely be addressed at the same time when small toolbar buttons are coming, thus flipping the order in messagecompose compared to the primary toolbar may be rather confusing in this regard.)

Comment 15

[rsx11m]

Attachment: Follow-up patch (v2, trunk only)

Ok, so here comes the compromise. This patch reorders the rules according to your comment #12 as desired. To also satisfy my comment #14, I'm making the rules in primaryToolbar.css match those in messengercompose.css, thus to be unambiguous that those are indeed identical rules. I still think this should make it easier for anybody creating new icons to recognize that those actually match without any confusion by slight difference between those implementations.

(In reply to Philip Chee from comment #12)
> Which was to minimize churn in Hg Blame.

Looking at bug 722758's comm-central changeset 2896ec0b8c06, these were all new rules from scratch as well, thus there is in fact no difference to the situation for messagecompose.css here. 

Anyway, so I'd say either let's stick with what is checked in or change it consistently in both versions.

Comment 16

[Philip Chee]

Comment on attachment 728761 [details] [diff] [review]
Follow-up patch (v2, trunk only)

> Looking at bug 722758's comm-central changeset 2896ec0b8c06, these were all
> new rules from scratch as well, thus there is in fact no difference to the
> situation for messagecompose.css here.

In this case I'd say WONTFIX for this patch.

> Anyway, so I'd say either let's stick with what is checked in or change it
> consistently in both versions

Comment 17

[rsx11m]

Comment on attachment 728761 [details] [diff] [review]
Follow-up patch (v2, trunk only)

Ok, let's be happy with what we have then until a permanent solution is found. Thus, only the branch approval of attachment 728629 [details] [diff] [review] is left to do.

Comment 18

[rsx11m]

Looks and works fine in today's SM 2.19a1 nightly (as tested with Linux x86_64 build).

Comment 19

[rsx11m]

Comment on attachment 728629 [details] [diff] [review]
Final patch (v3) [c-c: comment #10]

This has missed the last 2.17 beta, thus moving the comm-aurora request for 2.18 to comm-beta after today's merge.

[Approval Request Comment]
(see comment #9)

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/comm-beta/rev/2dc8a32e4163