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

RESOLVED FIXED in seamonkey2.19

Status

SeaMonkey
Themes
--
trivial
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: rsx11m, Assigned: rsx11m)

Tracking

(Blocks: 1 bug, {modern, regression})

Trunk
seamonkey2.19
modern, regression
Dependency tree / graph

SeaMonkey Tracking Flags

(seamonkey2.17 wontfix, seamonkey2.18 fixed, seamonkey2.19 fixed)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

4 years ago
+++ 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.
(Assignee)

Updated

4 years ago
No longer depends on: 722758
(Assignee)

Comment 1

4 years ago
Created attachment 722897 [details]
Appearance on Windows 7
(Assignee)

Comment 2

4 years ago
Created attachment 722967 [details] [diff] [review]
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.
Attachment #722967 - Flags: review?(philip.chee)
(Assignee)

Comment 3

4 years ago
Created attachment 722984 [details] [diff] [review]
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.
Attachment #722984 - Flags: review?(philip.chee)
(Assignee)

Updated

4 years ago
Blocks: 606683
(Assignee)

Comment 4

4 years ago
(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.
Keywords: modern
Summary: Print button is misaligned in the Composer toolbar with the Modern theme → Print button is misaligned in the Composer and Message Composition toolbars with the Modern theme
(Assignee)

Comment 5

4 years ago
Created attachment 725888 [details] [diff] [review]
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.
Assignee: nobody → rsx11m.pub
Attachment #722967 - Attachment is obsolete: true
Attachment #722984 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #722967 - Flags: review?(philip.chee)
Attachment #722984 - Flags: review?(philip.chee)
Attachment #725888 - Flags: review?(philip.chee)
(Assignee)

Comment 6

4 years ago
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.
status-seamonkey2.17: --- → affected
status-seamonkey2.18: --- → affected
status-seamonkey2.19: --- → affected

Comment 7

4 years ago
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.
Attachment #725888 - Flags: review?(philip.chee) → review+
(Assignee)

Comment 8

4 years ago
Created attachment 728629 [details] [diff] [review]
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.
Attachment #725888 - Attachment is obsolete: true
Attachment #728629 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
Whiteboard: [c-n: comm-central]
(Assignee)

Comment 9

4 years ago
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
Attachment #728629 - Flags: approval-comm-beta?
Attachment #728629 - Flags: approval-comm-aurora?

Comment 10

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

Thanks rsx11m
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [c-n: comm-central]
Target Milestone: --- → seamonkey2.19
(Assignee)

Comment 11

4 years ago
Created attachment 728725 [details] [diff] [review]
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.
Attachment #728725 - Flags: review?(philip.chee)
(Assignee)

Updated

4 years ago
Attachment #728629 - Attachment description: Final patch (v3) → Final patch (v3) [c-c: comment #10]

Comment 12

4 years ago
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.
(Assignee)

Comment 13

4 years ago
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...
(Assignee)

Comment 14

4 years ago
(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.)
(Assignee)

Updated

4 years ago
status-seamonkey2.19: affected → fixed
(Assignee)

Comment 15

4 years ago
Created attachment 728761 [details] [diff] [review]
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.
Attachment #728725 - Attachment is obsolete: true
Attachment #728725 - Flags: review?(philip.chee)
Attachment #728761 - Flags: review?(philip.chee)

Comment 16

4 years ago
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
Attachment #728761 - Flags: review?(philip.chee)
(Assignee)

Comment 17

4 years ago
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.
Attachment #728761 - Attachment is obsolete: true
(Assignee)

Comment 18

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

Updated

4 years ago
Attachment #728629 - Flags: approval-comm-beta?
(Assignee)

Updated

4 years ago
status-seamonkey2.17: affected → wontfix
(Assignee)

Comment 19

4 years ago
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)
Attachment #728629 - Flags: approval-comm-aurora? → approval-comm-beta?

Updated

4 years ago
Attachment #728629 - Flags: approval-comm-beta? → approval-comm-beta+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
Whiteboard: [c-n: comm-beta]
https://hg.mozilla.org/releases/comm-beta/rev/2dc8a32e4163
status-seamonkey2.18: affected → fixed
Keywords: checkin-needed
Whiteboard: [c-n: comm-beta]
You need to log in before you can comment on or make changes to this bug.