654916 - To/Cc Popups are uneven for first and last rows

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[David Ascher (:davida)]

Attachment: screenshot

See attached screenshot.  Notice different size of both Cc lines

Comment 1

[David Ascher (:davida)]

this is with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:5.0a2) Gecko/20110504 Thunderbird/3.3a4pre

Comment 3

[rsx11m]

Richard, I'm reopening this to treat it separately from the menu-width issue in bug 654426. After having a closer look, I could reproduce this in both Linux and Windows 7, the first and last row have buttons with a reduced height. Also, if you move the splitter down to increase the number of rows, then you will notice that the previously shrunk button expands after a certain height of the listbox has been reached. Also, spacing of the rows changes in the same way, thus those two effects are somehow connected.

Comment 4

[rsx11m]

Attachment: on Windows 7

I've selected the "Reply-to:" heading here on purpose as you can nicely see the different distances between the lowest pixel of the 'p' and the lower borders of the menu buttons. This is Mozilla/5.0 (Windows NT 6.1; WOW64; rv:5.0a2) Gecko/20110506 Thunderbird/3.3a4pre on Windows Classic.

Comment 5

[David Ascher (:davida)]

While we're tweaking css here, on the mac at least the focus rings on the To/Cc menu are clipped to the left. (shift-tab to select it, likely requires telling OS X to enable tabbing to non-text-fields in the system prefs).

Comment 6

[Richard Marti (:Paenglab)]

Please can you attach a screenshot with the focus ring?

Comment 7

[rsx11m]

Maybe move any Mac-related issues to the platform-specific bug 654426 and keep this bug here for the cross-platform issues with that listbox?

Comment 8

[Jim Porter (:squib)]

The entire row for the first and last rows is one pixel short, which I'm guessing is because of the listcols element having a border. One way to fix this would be to add:

#addressingWidget listboxbody {
  margin: 1px;
}

Comment 9

[Richard Marti (:Paenglab)]

I found, the problem was the removing of the listitem borders. I don't know, if this then triggers rounding errors.

Your proposal works, but makes the listitem smaller (22px instead 24px height). I had already an other solution. I'll check, if I can combine both.

Comment 10

[rsx11m]

Have a look at the comparison in bug 654686 attachment 529968 [details].
The row spacing increased between the old and the new implementation, thus if you'd go with 22px, this would restore the previous row height which should be sufficient (thus saving some vertical space in the process).

Comment 11

[David Ascher (:davida)]

Attachment: screenshot of focus ring

screenshot w/ focus ring.  i don't know if it's platform specific.

Comment 12

[Richard Marti (:Paenglab)]

Attachment: Patch fixing this issue (hopefully on OSX)

#addressingWidget listboxbody {
  margin: 1px;
}
did the fix. But why does a margin on the listboxbody solve a problem on childs (listitem, listcell)?

This patch needs Bug 643262 and Bug 654686 applied. If this patch is earlier ready to land, I can update.

I gave for OSX 1px margin also for left and right in the hope, this solves the focus ring problem.

OSX isn't tested because I have no Mac.

Comment 13

[Nomis101]

(In reply to comment #12)
> OSX isn't tested because I have no Mac.

I'll do that, because I soon have a Mac build with this patch.

Comment 14

[Nomis101]

Attachment: Screenshot, Mac with patch

With this patch and the patch from Bug 654426, it looks now like this on Mac.

Comment 15

[Richard Marti (:Paenglab)]

Nomis101 thank you for testing. It looks the menulists have now the same height, but are aligned on top and not centered. I hope :andreasn can this and the still appearing truncating address in Bug 654426.

Comment 16

[Blake Winton (:bwinton) (:☕️)]

Attachment: OSX with the patch (I think).

And for completion, here's the focus ring on the top, middle, and bottom dropdowns.

Comment 17

[Richard Marti (:Paenglab)]

Regarding the focus ring: I checked the images again. It looks like the menulists itself have a glow effect. Is this focus ring still needed?

Comment 18

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 530826 [details] [diff] [review]
Patch fixing this issue (hopefully on OSX)

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

I don't see any glow effect on OS X.  And I'm not sure whether we're adding the focus ring, or whether the OS is…  Either way, it might be solved if we messed with the padding instead of the margin…  But handling it in bug 654426 makes some sense, since it seems to be Mac-specific.

Other than that, this definitely fixes the problem (at least on OS X), so I think going with this, and doing the other work in followup bugs makes sense.  r=me.

::: mail/themes/gnomestripe/mail/compose/messengercompose.css
@@ +442,5 @@
>    background-color: transparent;
>  }
>  
> +#addressingWidget listboxbody {
> +  margin: 1px;

Why do all the other themes use "1px 0", but pinstripe uses "1px"?  Is it a typo, or intentional?

Comment 19

[Richard Marti (:Paenglab)]

(In reply to comment #18)
> I don't see any glow effect on OS X.  And I'm not sure whether we're adding
> the focus ring, or whether the OS is…

As :davida in Comment 5 wrote: (shift-tab to select it, likely requires telling OS X to enable tabbing to non-text-fields in the system prefs) it's not enabled as standard. I found in the css file this:

.aw-menulist:focus {
  outline: 2px solid -moz-mac-focusring;
  outline-offset: -2px;
}

This looks as the creator of this focus rings, which now are no more used, because the aw-menulist has no more none appearance.

> ::: mail/themes/gnomestripe/mail/compose/messengercompose.css
> @@ +442,5 @@
> >    background-color: transparent;
> >  }
> >  
> > +#addressingWidget listboxbody {
> > +  margin: 1px;
> 
> Why do all the other themes use "1px 0", but pinstripe uses "1px"?  Is it a
> typo, or intentional?

I hoped this solves the problem with the focus ring being cut on the left, but didn't work. Because the other patches are not reviewed, I'm creating a new patch without this dependency and give the same margin as the other themes. Additionally I remove the focus ring, if it's okay.

Does this patch need a new review because of the changes?

Comment 20

[Richard Marti (:Paenglab)]

Attachment: Patch for check-in

Because of the changes in Comment 19 mentioned asking again review.

Comment 21

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 530925 [details] [diff] [review]
Patch for check-in

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

Yes, I see what you mean now.  I like the glow _way_ more.  It's still cut off a little on the left (and the top of the first one), but that sounds like it can get handled in the other bug.

r=me, and thanks!  :)

Comment 22

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/9be6c29bb2f8