Last Comment Bug 722758 - Print button is misaligned in the Mail & News toolbar with the Modern theme
: Print button is misaligned in the Mail & News toolbar with the Modern theme
Status: VERIFIED FIXED
: regression
Product: SeaMonkey
Classification: Client Software
Component: Themes (show other bugs)
: Trunk
: All All
: -- trivial (vote)
: seamonkey2.8
Assigned To: Ian Neal
:
Mentors:
Depends on:
Blocks: 676991
  Show dependency treegraph
 
Reported: 2012-01-31 09:56 PST by rsx11m
Modified: 2013-03-08 11:58 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
wontfix
fixed
fixed
fixed


Attachments
Appearance on Windows 7 (5.31 KB, image/png)
2012-01-31 09:58 PST, rsx11m
no flags Details
Temporary workaround until new icons arrive [Checked in:comm-central Comment 16 comm-aurora/beta Comment 17] (1.52 KB, patch)
2012-01-31 16:08 PST, Ian Neal
philip.chee: review+
philip.chee: approval‑comm‑aurora+
philip.chee: approval‑comm‑beta+
Details | Diff | Splinter Review

Description rsx11m 2012-01-31 09:56:16 PST
Steps to reproduce:

 - open the Mail/News window (new or existing profile)
 - customize the toolbar to add the "Print" button anywhere
 - this looks fine while using the Default theme
 - switch to Modern theme, now it is too low and misaligned

> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0.1) Gecko/20111221 SeaMonkey/2.6.1
> Mozilla/5.0 (X11; Linux x86_64; rv:12.0a1) Gecko/20120131 SeaMonkey/2.9a1
Comment 1 rsx11m 2012-01-31 09:58:38 PST
Created attachment 593138 [details]
Appearance on Windows 7

with Windows Classic desktop theme.
Comment 2 rsx11m 2012-01-31 11:06:45 PST
That's actually a quite recent regression. I've created a new profile with 2.1 and added the Print button to the Mail/News toolbar - looks fine even with the Modern theme. From there I went to 2.5b4 and the Print button was still there and well aligned.

Now, when going from 2.5b4 to 2.6.1, the Print button disappeared from the previously customized toolbar, and when adding it back from the customization palette, it shows the misalignment as described. Thus, the regression happened somewhere between 2.5 and 2.6.
Comment 3 rsx11m 2012-01-31 11:20:46 PST
Ian, possibly related to bug 676991 "Move toolbar print button to utilityOverlay"? The patch in attachment 553967 [details] [diff] [review] made quite a few changes to the style files and would fit the time frame.
Comment 4 Philip Chee 2012-01-31 11:41:42 PST
We switched from using:
#button-print {
  list-style-image: url("chrome://communicator/skin/icons/btn1.gif");
  -moz-image-region: rect(34px 49px 67px 0);
}

To
#print-button {
  list-style-image: url("chrome://communicator/skin/icons/common.png");
  -moz-image-region: rect(0 42px 39px 0);
}

The height changed from 33px to 39px.

Hg Blame says:
http://hg.mozilla.org/comm-central/rev/a508ab7862d2
author	Ian Neal <iann_cvs@blueyonder.co.uk>
Bug 676991 - Move toolbar print button to utilityOverlay r=neil/standard8
Comment 5 rsx11m 2012-01-31 12:38:56 PST
I'm measuring six pixels off in the screen shot, so this would match that difference. Bottom line, the image is too big now, or could it just be cropped?

On a side note, it looks (at least visually) correct in the main toolbar of the navigator window.
Comment 6 rsx11m 2012-01-31 14:06:53 PST
In a naive approach based on Phil's comment #4, I've changed all -moz-image-region: rect(0 * 39px *); to rect(3px * 36px *); in communicator.css, which indeed resolves the problem for the mail/news toolbar. However, given that print-button is shared among windows now (the aim of bug 676991 to start with) it looks horribly misaligned in the main-window toolbar with that change. Thus, adjusting this globally doesn't seem to be the right way to go... :-\
Comment 7 Ian Neal 2012-01-31 16:08:40 PST
Created attachment 593257 [details] [diff] [review]
Temporary workaround until new icons arrive [Checked in:comm-central Comment 16 comm-aurora/beta Comment 17]

This patch:
* overrides the print icon for mailnews windows

Hopefully when there is a new modern theme the icons in all components will be the same size as they are for classic plus there will be small versions for mailnews.
Comment 8 Philip Chee 2012-01-31 21:44:31 PST
Comment on attachment 593257 [details] [diff] [review]
Temporary workaround until new icons arrive [Checked in:comm-central Comment 16 comm-aurora/beta Comment 17]

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

Cancelling review request for the time being.

::: suite/themes/modern/messenger/primaryToolbar.css
@@ +264,5 @@
>  #button-junk[disabled="true"] {
>    -moz-image-region: rect(544px 199px 577px 150px) !important;
>  }
>  
> +/* To workaround the mailnews icons are 33px tall and have no small versions */

We *were* using the small print buttons from chrome://communicator/skin/icons/common-small.png !
http://hg.mozilla.org/comm-central/rev/a508ab7862d2#l20.46

Could you put these back? Thanks.
Comment 9 Philip Chee 2012-01-31 21:49:18 PST
> +toolbar[iconsize="small"] > #print-button[disabled="true"],
> +#print-button[disabled="true"] {
> +  -moz-image-region: rect(3px 168px 36px 126px) !important;
What style are your overriding with !important here?
Comment 10 Stefan [:stefanh] 2012-01-31 23:56:04 PST
(In reply to Philip Chee from comment #9)
> > +toolbar[iconsize="small"] > #print-button[disabled="true"],
> > +#print-button[disabled="true"] {
> > +  -moz-image-region: rect(3px 168px 36px 126px) !important;
> What style are your overriding with !important here?

iirc it's the hover:active rules (it's like this in all files). We don't use not:([disabled="true"]) etc
Comment 11 rsx11m 2012-02-01 07:21:50 PST
(In reply to Philip Chee from comment #8)
> We *were* using the small print buttons

Other than the "Stop" button (apparently due to the same global definition as the "Print" button now) and changes in the spacing, the icons stay the same in the main Mail & News as well as the composition window when switching to small icons, so it seems to be more consistent to not use small icons for print-button either (and likely so for the stop-button, but that may be a separate bug).
Comment 12 Philip Chee 2012-02-01 07:43:31 PST
Comment on attachment 593257 [details] [diff] [review]
Temporary workaround until new icons arrive [Checked in:comm-central Comment 16 comm-aurora/beta Comment 17]

OK. I'll give the patch a spin.
Comment 13 Philip Chee 2012-02-02 03:18:28 PST
Comment on attachment 593257 [details] [diff] [review]
Temporary workaround until new icons arrive [Checked in:comm-central Comment 16 comm-aurora/beta Comment 17]

The print button is much better aligned. Compared to 2.0.14 it's about a couple of pixels lower. Moving the -moz-image-region 2 px down seems to work better. e.g.

-  -moz-image-region: rect(3px 42px 36px 0); 
+  -moz-image-region: rect(5px 42px 38px 0);

r=me with the 2px shift at your discretion.
Comment 14 rsx11m 2012-02-02 11:49:55 PST
Comment on attachment 593257 [details] [diff] [review]
Temporary workaround until new icons arrive [Checked in:comm-central Comment 16 comm-aurora/beta Comment 17]

(In reply to Philip Chee from comment #13)
> +  -moz-image-region: rect(5px 42px 38px 0);

I've patched 2.7 with this modification and it looks good to me as well.
Comment 15 Ian Neal 2012-02-05 13:49:36 PST
Comment on attachment 593257 [details] [diff] [review]
Temporary workaround until new icons arrive [Checked in:comm-central Comment 16 comm-aurora/beta Comment 17]

It would be good to get this into 2.8 and 2.9 too
Comment 16 Ian Neal 2012-02-05 13:52:34 PST
Comment on attachment 593257 [details] [diff] [review]
Temporary workaround until new icons arrive [Checked in:comm-central Comment 16 comm-aurora/beta Comment 17]

http://hg.mozilla.org/comm-central/rev/2896ec0b8c06
Comment 17 Ian Neal 2012-02-06 15:42:14 PST
Comment on attachment 593257 [details] [diff] [review]
Temporary workaround until new icons arrive [Checked in:comm-central Comment 16 comm-aurora/beta Comment 17]

http://hg.mozilla.org/releases/comm-aurora/rev/a4536456e92a
http://hg.mozilla.org/releases/comm-beta/rev/458dad0ffd8f
Comment 18 rsx11m 2012-02-08 09:31:35 PST
Verified fixed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a2) Gecko/20120208 SeaMonkey/2.9a2, thanks!

Note You need to log in before you can comment on or make changes to this bug.