362919 - message pane is too wide due to alert bar's minimum-width

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Tim Maks van den Broek [:mad_maks]]

when i use the preview pane in build version 2 beta 1 (20061205) the pane is to wide. The button "not scam" / show images are off screen. (see screenshot)

Comment 1

[Tim Maks van den Broek [:mad_maks]]

Attachment: screenshot 1

Comment 2

[Tim Maks van den Broek [:mad_maks]]

Attachment: screenshot 2

Comment 3

[Nir]

when you open TB in safe mode , does it give same result ?
http://kb.mozillazine.org/Safe_mode

Comment 4

[Mike Cowperthwaite]

In TB 2, the alert bar no longer wraps its text when it's made narrow; it has a minimum width which (in the English version, anyway) is wide enough for the text "To protect your privacy..." plus the icon and the Load Images button.  If the window is narrowed below that width, the window edge will start to encroach on the bar (and on the message body, too, which never gets narrower than the alert bar, if there is one).

It's true that, with the spam or scam alerts, there is now a lot of blank space in the alert bar at the minimum width.


Please don't set a target when you're filing a bug; that field is for use by the developers.

Comment 5

[Scott MacGregor]

Two things jump out at me here:

1) The "to protect your privacy" text should still wrap, I'm not sure why that broke, I could have sworn it worked when I first landed the UI changes to the privacy bar. 

2) The text shouldn't have a minimum width. It's designed to take as much space as it needs, then there's a spring that keeps the button as far right as possible.  But when I shrink the window now, it sure looks like something is keeping that from working right. 

Comment 6

[Tim Maks van den Broek [:mad_maks]]

(In reply to comment #3)
> when you open TB in safe mode , does it give same result ?
> http://kb.mozillazine.org/Safe_mode
> 

yes if the folderpane is at the same width.

Comment 7

[Tim Maks van den Broek [:mad_maks]]

(In reply to comment #4)
> 
> minimum width which (in the English version, anyway) is wide enough for the
> text "To protect your privacy..." plus the icon and the Load Images button.  

that is very bad for l10n, in i.e. nl the text is much longer.

Comment 8

[Mike Cowperthwaite]

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

Comment 9

[Magnus Melin [:mkmelin]]

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

Comment 10

[Scott MacGregor]

Attachment: agressive fix

This patch fixes two problems:

the containing vbox needed flex in order to allow the remote image description text to wrap.

In order to make things work exactly like they did in 1.5.0.9, I had to also revert the label back to a description element but as Neil pointed out the text-link class is really for labels not descriptions. Not sure which trade off to make here.

One comprise might be to just add the flex to the box, this fixes the initial junk bar problem reported here. But would be nice to get that 2nd line of text for the remote image bar to also wrap.

Partially asking Neil for a review / advice on what to do with the label.

Comment 11

[Scott MacGregor]

Attachment: lower risk, complementary patch

this makes things a bit better as the description text will start wrapping again, but the dynamically generated label still won't wrap until Neil and I figure something out. It's still worth adding the flex to the box as it does make tis problem better.

Comment 12

[[e-mail]]

(In reply to comment #10)
>In order to make things work exactly like they did in 1.5.0.9, I had to also revert
>the label back to a description element but as Neil pointed out the text-link class
>is really for labels not descriptions. Not sure which trade off to make here.
One option is to add the crop attribute to the label.

I tried <label class="text-link">foo</label> on trunk but it didn't underline.

Comment 13

[Scott MacGregor]

Tim, this should be a lot better in beta 2. I landed the "lower risk, complementary fix" in time for beta 2 last night. Can you test today's nightly or beta 2 when it comes out?

Comment 14

[Tim Maks van den Broek [:mad_maks]]

Attachment: screenshot 3

it is a lot better but still not ok.
see this screenshot

Comment 15

[Scott MacGregor]

right, the problem with the remote content bar requires a fix like the more aggressive patch (or at least a version that works better than that one :)). The simple fix that went in addressed the problem for the junk and phishing bar.

Comment 16

[Tim Maks van den Broek [:mad_maks]]

Attachment: screenshot 4

it didn't fix that problem also not totally

Comment 17

[[e-mail]]

Attachment: Suite version

While I was there I thought I'd clean up some of the other flexes too.
I also added the crop="end" on the label as an interim fix.
I think the lack of underlining on <label class="text-link">Click here...<label> might be a Gecko bug.

Comment 18

[[e-mail]]

Attachment: Fixed suite patch

Filed the underlining issue on trunk as bug 367745.

Comment 19

[[e-mail]]

Comment on attachment 251588 [details] [diff] [review]
agressive fix

>-    document.getElementById('allowRemoteContentForAuthorDesc').value = 
>+    document.getElementById('allowRemoteContentForAuthorDesc').childNodes[0].nodeValue = 
>       gMessengerBundle.getFormattedString('alwaysLoadRemoteContentForSender', [emailAddress ? emailAddress : aMsgHdr.author]);
If you use .textContent instead (see attachment 252468 [details] [diff] [review]) you don't need the foo.

>-        <label id="allowRemoteContentForAuthorDesc" class="text-link" flex="1"
>-               onclick="allowRemoteContentForSender();"/>
>+        <description id="allowRemoteContentForAuthorDesc" class="text-link" flex="1"
>+               onclick="allowRemoteContentForSender();">foo</description>
Leave this as a label. r=me with this fixed.

Comment 20

[Ian Neal]

Comment on attachment 252468 [details] [diff] [review]
Fixed suite patch

r=me definite improvement

Comment 21

[Scott MacGregor]

Attachment: updated tbird patch

updated patch based on Neil's review comments and to pick up the flex changes he made to the suite which look good too.

Comment 22

[Scott MacGregor]

Neil, do you want to nominate the suite patch for the branch?

Comment 23

[[e-mail]]

(In reply to comment #22)
>Neil, do you want to nominate the suite patch for the branch?
Hey, let me check it in on trunk first ;-)

As this is suite-only code I'd normally set our approval-seamonkey1.1.1 flag but as this product doesn't have it I'll just poke someone on IRC :-)

Comment 24

[Robert Kaiser]

approval-seamonkey1.1.1=me

Comment 28

[Scott MacGregor]

I think there was an unintended side effect with using .textContent for the text-link label in ths patch. The label is no longer underlined like it should be since it's a link. Neil, do you know why using .textContent would have broken that?