233791 - toolbar button jumps when marking a message as junk (or not junk)

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Scott MacGregor]

toolbar button jumps when marking a message as junk (or not junk)

to reproduce

1)  make sure the toolbar has the "junk" button, and buttons to the right of it.
2)  make sure you are showing text or pictures and text in the toolbar buttons
3)  make sure you aren't moving messages automatically when you mark them as junk
4)  select a message, mark it as junk

because the button changes from "junk" to "not junk" the buttons to the right of
the junk buttons will jump over to the right.

a possible solution would be to pad the button enough to make the button wide
enough to hold "not junk", so that when it says "junk" it won't be narrower.

perhaps this width would be in ems and specified in the dtd file, as it would
vary per language, as the length of "not junk" varies.

Comment 1

[Scott MacGregor]

bug #233790 covers the tbird case.

Comment 2

[Mike Cowperthwaite]

Dupe of bug 208122?  May be theme-specific.

Comment 3

[Stefan Borggraefe]

The modern theme has the same issue when either the font size is large enough or
the translation of "Not Junk" is longer.

Comment 4

[Stefan Borggraefe]

*** Bug 208122 has been marked as a duplicate of this bug. ***

Comment 5

[Stefan Borggraefe]

Attachment: Patch

Since this is not theme specific but localization-specific, I chose to fix this
bug directly in the XUL-file by adding a min-width and then externalizing it.

This is ugly, but I can't think of a better way to fix this. :-( Ideally we
would calculate the widths of the strings and then use the maximum of these two
values. AFAIK this is not possible with XUL.

Comment 6

[Stefan Borggraefe]

Comment on attachment 143933 [details] [diff] [review]
Patch

Neil, do you think this is acceptable?

Comment 7

[Scott MacGregor]

I actually like this approach. The minimum width of this button really is driven
by localization and not theme style, so allowing it to be controlled by the
localizers (as a style attribute in xul) instead of in a css theme file seems
like a good thing.

At least for me, I was able to shrink this width all the way down to 4.2em
before the button ran out of horizontal room. This could be because I am using
large fonts or because I was trying it directly in thunderbird which may have
some other padding rules on toolbar buttons that influenced this. I need to
figure out why that is.

Comment 8

[Stefan Borggraefe]

Attachment: Patch V2

I found a way to get Mozilla to calculate the correct width for us. :-)

Scott: The needed width for the previous patch was dependent on the used font.
Before I found the new solution in this attachment I tested more and found that
if you use a monospace font you nearly need a width of <number of
characters>em. 
The new patch doesn't have this issue, so let us all forget these worries of
the past. ;-)

Comment 9

[Scott MacGregor]

Yup, good detective work. I was using a monospace font. That's why I was able to
shrink the number so much. 

Comment 10

[Scott MacGregor]

Attachment: thunderbird version of the patch

had to make some tweaks to work nicely with custom toolbars

Comment 11

[Stephen Walker]

Can this be fixed to not spew js console output when the junk button isn't on
the toolbar?

Error: junkButtonDeck has no properties
Source File: chrome://messenger/content/mailWindowOverlay.js
Line: 594

Comment 12

[Scott MacGregor]

sorry about that Stephen. I fixed that issue (which was specific to the
thunderbird patch due to customizeable toolbars).

Comment 13

[Scott MacGregor]

Comment on attachment 144024 [details] [diff] [review]
Patch V2

I like this approach a lot because it minimizes the work localizers will have
to do compared to the previous solution.  Plussing this...pending Neil's r of
course.

Comment 14

[Adam Becevello]

I have filed bug 238141 which deals with a regression in Thunderbird that this
patch has caused.  The junk toolbar icon displays the wrong icon when it is in
the disabled state.

Comment 15

[Stefan Borggraefe]

Attachment: Patch V2.1

Updated patch for Seamonkey that fixes the problem Adam described. Thanks for
reporting this Adam! :-)

Comment 16

[Stefan Borggraefe]

Comment on attachment 144443 [details] [diff] [review]
Patch V2.1

Transferring Scrotts sr+ and rerequesting review from Neil.

Comment 17

[Adam Becevello]

I wasn't sure if Seamonkey also experienced the problem I described.  I hope you
will be making a thunderbird version of the patch so that it will be fixed as
well.  I just patched my version of Thunderbird with the updated seamonkey patch
V2.1 (with a little fixing up to make it work with Thunderbird) and it works.

Comment 18

[[e-mail]]

Unfortunately themes will assume that button-junk refers to a toolbarbutton and
design their style rules appropriately... at the very least this patch slightly
breaks classic and modern, and probably most (cough!) 3rd party themes too.

An alternative that did occur to me would be to highlight the button if the
message is junk, in much the same way as Composer's Bold button, JS Console's
All button or DOM Inspector's Select Element button highlights. Thoughts?

Comment 19

[Stefan Borggraefe]

(In reply to comment #18)
> Unfortunately themes will assume that button-junk refers to a toolbarbutton and
> design their style rules appropriately...

It also works when I give both buttons inside the deck the same id
"button-junk"... but this isn't exactly what an id is meant to be and I'm not
sure what ill effects this could have (surely one shouldn't use getElementById()
to do something with these buttons). This was the reason why I moved the id to
the deck.

> at the very least this patch slightly breaks classic and modern

What kind of breakage have you seen? I didn't notice anything... but then again
I didn't even notice the problem Adam spotted. ;-( I tested with classic, modern
and retro.

> An alternative that did occur to me would be to highlight the button if the
> message is junk, in much the same way as Composer's Bold button, JS Console's
> All button or DOM Inspector's Select Element button highlights. Thoughts?

This would avoid this bug, but I think such a button would look out-of-place in
the MailNews/TB-Toolbar, because it would be the only button with a depressed look.

When my <deck>-idea is not acceptable and there is no other way to automatically
determine the needed space, what about the first patch in this bug?

Comment 20

[[e-mail]]

Trying to reproduce the problem I was worried about, I found a tree on which I
could apply the patch (unfortunately my main tree has other toolbarbuttons
patched, in case anyone cares about changing them to menubuttons (spots airborne
pig)) and discovered a rather serious regression, which may be a bug in the deck
code; if you mouse down the junk button, you can't change your mind :-(

Comment 21

[Stefan Borggraefe]

(In reply to comment #20)
> if you mouse down the junk button, you can't change your mind :-(

I just filed bug 238492 about this issue.

Comment 22

[Stefan Borggraefe]

Neil, with bug 238492 fixed now, do you still think this is the wrong way to fix
this bug? Patch V2.1 still applies with a few lines offset.

Comment 23

[[e-mail]]

Actually it's bug 65917 that makes this possible.

Comment 24

[[e-mail]]

Comment on attachment 144443 [details] [diff] [review]
Patch V2.1

r=me if you share the oncommand somewhere.

Comment 25

[Stefan Borggraefe]

(In reply to comment #24)
> (From update of attachment 144443 [details] [diff] [review])
> r=me if you share the oncommand somewhere.

I moved the oncommand handler up to the <deck> element before checkin.

Marking FIXED.

Comment 26

[[e-mail]]

Comment on attachment 144443 [details] [diff] [review]
Patch V2.1

Why did you fiddle with the ids on the patch you checked in?

Comment 27

[Stefan Borggraefe]

(In reply to comment #26)
> (From update of attachment 144443 [details] [diff] [review])
> Why did you fiddle with the ids on the patch you checked in?

I don't know how that happened. :-( Sorry! I just corrected my mistake with a
follow-up checkin:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=SeaMonkeyAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=07%2F28%2F2004+23%3A50%3A00&maxdate=07%2F28%2F2004+23%3A55%3A00&cvsroot=%2Fcvsroot