Håkan Waara Reporter
	Description • 24 years ago

This is in a trunk build 3 mins old:

When I try to push the GetMsgs button, the button doesn't depress. However, it
still works. Another thing is that when I click its little arrow, the whole
button (plus the arrow) depresses. Only the arrow should be depressed.

This is critical for 0.8 IMHO as it breaks a very widely used UI!

cc'ing some mailnews people to make sure this is being looked at.

	(not reading, please use seth@sspitzer.org instead)
	Comment 1 • 24 years ago

over to racham.

	Sheela Ravindran
	Comment 2 • 24 years ago

change qa contact->myself, cc esther

	Asa Dotzler [:asa]
	Comment 3 • 24 years ago

do we know when this regressed? is this fallout from tuesday night? putting on 
my 0.8 radar.

	Håkan Waara Reporter
	Comment 4 • 24 years ago

I suspect that this is a regression of bug 12165.

	scottputterman
	Comment 5 • 24 years ago

I agree.  If this is an easy fix and it can get approved, let's go for it.  The
other button menus seem to be working ok so hopefully this will be easy to find.

	racham
	Comment 6 • 24 years ago

I am thinking that it is the matter having some more style rules that I might
have missed in bug fix for 12165. I will investigate and post the patch.

	R.K.Aa.
	Comment 7 • 24 years ago

Additional observations:
-When mousing over the button from top or left, it doesn't highlight
-Mousing over the history-arrow on it highlights whole button though
and
-the area the history-arrow covers "feels" bigger than it used to be:
I suddenly find i repeatedly click the arrow rather than the button. Never used
to "slip" like that.

	Joe Hewitt (gone) Assignee
	Comment 8 • 24 years ago

I'm noticing the mail toolbar is also taller than it used to be, which is
probably related to the get message button's transition to becoming a menubutton. 

	Fabian Guisset
	Comment 9 • 24 years ago

part of the fix is easy : add
-moz-binding :
url("chrome://communicator/skin/menubuttonBindings.xml#menubutton-dual-standard");
to the #get-msg {} style list. You forgot that one and obviously a menubutton
without the xbl is useless ;-) so that fixes some issues, but not all :
You still have to hover over the arrow to highlight the menubutton.. i have no
idea why this is, as it works for other menubuttons like print and mark..
let's get this baby fixed asap

	Fabian Guisset
	Comment 11 • 24 years ago

r=fabian
i did exactly the same patch separately so it has to be the correct fix..
by the way, the "mark" button is coded in the same way as the getmsg button was,
so it should be fixed to use the same style properties, i'll open a new bug for that

	Håkan Waara Reporter
	Comment 12 • 24 years ago

does this patch also solve the problems R.K.Aa listed?

	Fabian Guisset
	Comment 13 • 24 years ago

yes

	Håkan Waara Reporter
	Comment 14 • 24 years ago

nice! let's get this in 0.8 then

	Håkan Waara Reporter
	Comment 15 • 24 years ago

The patch works like a charm. r=hwaara

	racham
	Comment 16 • 24 years ago

Hewitt just showed how this patch works on his machine. It looks great. There is
one more patch coming for Classic also to do the right thing with dropdown
marker clicks. I missed binding to the dropdown marker in the original fix for
12165. 

Reassigning to Hewitt. He has patches for both modern and classic. Thanks.

	Joe Hewitt (gone) Assignee
	Comment 17 • 24 years ago

The mark button suffers from the same exact problem as the getmsg button (in
both modern and classic) so the forthcoming patch fixes both mark and getmsg.

	Fabian Guisset
	Comment 19 • 24 years ago

The patch is a hell to review, with lines inserted etc.
The code is correct and I applied the patch and it works fine.

	racham
	Comment 20 • 24 years ago

I applied patch for the modern skin to test it out. I noticed that when I click 
on the drop marker of GetMsg button (just GetMsg button only, Mark [which is 
also changed] and Print dropmarkers are behaving normally) , the the toolbar 
height seemed to have affected (become shorter) and there by causing a jerky 
movement of toolbar and other panes...did anyone else see this..?

	Christopher Blizzard (:blizzard)
	Comment 21 • 24 years ago

What's the deal here?  This is a 0.8 bug so we're trying to make sure that we
still have traction.

	racham
	Comment 22 • 24 years ago

I sent a mail to Hewitt. He will get to this. 

Adding Ben to the cc list.

	Asa Dotzler [:asa]
	Comment 23 • 24 years ago

If a reviewed patch doesn't show up today, this is gonna miss 0.8. What's the scoop?

/be (typing at asa's desk)

	Håkan Waara Reporter
	Comment 24 • 24 years ago

I applied patch, rev2 and it works just fine here!

AFAIK this patch is final (not 100% though) so all we would need is r=/sr=/a= to
get it in.  Is someone up for the challenge? :)

	racham
	Comment 25 • 24 years ago

So, looks like hewitt missed the mac portion of the classic skin. It should 
be the same as the one posted for windows. My mac build is going on. If anyone 
out there has a mac build, please try it on.

It works fine on Windows (the problem I mentioned earlier with Modern skin 
exists, but one can live with it).

Adding mscott and hyatt also to the list.

Seth, Scott, Hyatt and Ben,

Can you do review and super review for this one..? thanks.

	Fabian Guisset
	Comment 26 • 24 years ago

As I said this code simply copies the "print" menubutton that works just fine.
It is no "new" code. Racham, I'm not sure what your problem is but I don't see
it. It might be that the icon is slightly too large? Anyway please review cuz it
would be very bad for the look'n'feel of the mail app.

	racham
	Comment 27 • 24 years ago

The problem I had mentioned was (only in Modern skin), when I click on the
dropmarker of GetMsg button, it is trying to aign with other buttons and there
by causing a slight movement (to the left and the top) in the toolbar and other
panes.

However, that problem is now solved once I did the following (in patch, rev2)...

for  #button-getmsg 

change 

  margin                : 3px 3px 1px 3px;

to

  margin                : 0px;

hewitt, what do you say..?

	racham
	Comment 29 • 24 years ago

In patch rev3, very few things are different from rev2.

Here they are :

1. Had to set the margin-top to 3px for #button-getmsg in modern skin (In my
earlier updates, I have mentioned that as margin : 0px and it was wrong as I
noticed the button was misaligned with others). Setting the top margin to 3px
fixed that.

2. Adjusted margin-top of mark and print buttons (Modern only), so that
dropmarkers do not overlap the text below those buttons.

3. Added mac version patch for classic, same as windows. rev2 has only windows
patch.

Hewitt, please go through the rev3 patch and make sure all the things are covered.

	Håkan Waara Reporter
	Comment 30 • 24 years ago

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

	Joe Hewitt (gone) Assignee
	Comment 31 • 24 years ago

After applying the latest patch, things still aren't right.  By only setting
margin top on the menubuttons, we're now inheriting the bottom margin setting
from .menubutton-dual.toolbar, which is 9px.  This is way too tall and makes the
whole toolbar way too tall.  I'll have to stop by racham's cube to see why it
looks wrong to him with margin: 3px 3px 1px 3px, because it looks fine to me.  I
don't see the jumpiness either when clicking the dropmarker.

This fix is really a hack, anyways.  the xbl binding for these menubuttons is
arranged wrong.  We should not have to tweak the margins of the dropmarker to
fit the button text -- the dropmarker should be in a stack over just the button
icon, not the text.  We can use the current patch for 0.8 if it is so desired,
but for 0.9 I'm filing another but to fix the binding.

	Joe Hewitt (gone) Assignee
	Comment 32 • 24 years ago

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

	racham
	Comment 33 • 24 years ago

Hewitt,

Looks like everything works fine with rev2 itself now. Sorry for all that noise
(I must have had some file corrupted). So, just remember to add the mac portion
of the patch for classic skin..

r=racham.

	(not reading, please use seth@sspitzer.org instead)
	Comment 35 • 24 years ago

sr=sspitzer on patch, rev 4.  thanks joe.

	racham
	Comment 36 • 24 years ago

Joe got a=drivers@mozilla.org.

I Landed fix on the branch. Joe is landing it on the trunk.

	Joe Hewitt (gone) Assignee
	Comment 37 • 24 years ago

fixed on trunk

	Sheela Ravindran
	Comment 38 • 24 years ago

verified this on commercial trunk build only.  Will verify it on trunk when it
is available. For now leaving it as resolved fix.
commercial buildid:2001021909 win98
                   2001021908 linux
                   2001021904 mac

	Sheela Ravindran
	Comment 39 • 24 years ago

According to Paul Wyskoczka marking bug as verified and see above comments for
the builds that were used to verify.