Last Comment Bug 711968 - Buttons in Account Provisioner do not wrap long text
: Buttons in Account Provisioner do not wrap long text
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Mail Window Front End (show other bugs)
: 11 Branch
: x86 All
: -- normal (vote)
: Thunderbird 11.0
Assigned To: Mike Conley (:mconley)
:
:
Mentors:
Depends on: 700536
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-19 06:30 PST by Mike Conley (:mconley)
Modified: 2011-12-28 10:13 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
+
fixed


Attachments
Screenshot of problem in action (65.67 KB, image/png)
2011-12-19 06:30 PST, Mike Conley (:mconley)
no flags Details
Patch v1 (2.82 KB, patch)
2011-12-19 06:41 PST, Mike Conley (:mconley)
bwinton: review+
bwinton: ui‑review-
bugs: feedback+
Details | Diff | Splinter Review
Updated patch (2.79 KB, patch)
2011-12-19 14:32 PST, Andreas Nilsson (:andreasn)
mconley: feedback+
Details | Diff | Splinter Review
patch v3 (2.82 KB, patch)
2011-12-19 15:35 PST, Andreas Nilsson (:andreasn)
mconley: feedback+
Details | Diff | Splinter Review
patch v4 (3.83 KB, patch)
2011-12-19 16:24 PST, Andreas Nilsson (:andreasn)
bwinton: review+
bwinton: ui‑review+
Details | Diff | Splinter Review
screenshot of patch v4 to ease review (47.00 KB, image/png)
2011-12-19 17:08 PST, Andreas Nilsson (:andreasn)
no flags Details
updated patch with just width instead of max and min-width (3.74 KB, patch)
2011-12-20 07:19 PST, Andreas Nilsson (:andreasn)
bugs: review+
bugs: ui‑review+
standard8: approval‑comm‑aurora+
Details | Diff | Splinter Review

Description Mike Conley (:mconley) 2011-12-19 06:30:37 PST
Created attachment 582813 [details]
Screenshot of problem in action

The text inside the Account Provisioner buttons is not wrapped for long text.  See screenshot.
Comment 1 Mike Conley (:mconley) 2011-12-19 06:41:04 PST
Created attachment 582815 [details] [diff] [review]
Patch v1

Andreas:

Does this make sense, or will I be introducing a regression?

-Mike
Comment 2 Andreas Nilsson (:andreasn) 2011-12-19 10:55:22 PST
Comment on attachment 582815 [details] [diff] [review]
Patch v1

I'm unable to test with a german locale, but I don't see any regressions to the english version, so I think we should be good. We really want to fit german and dutch.
Comment 3 Mike Conley (:mconley) 2011-12-19 10:56:39 PST
Comment on attachment 582815 [details] [diff] [review]
Patch v1

Okie doke, let's see what bwinton thinks.
Comment 4 Blake Winton (:bwinton) (:☕️) 2011-12-19 12:26:03 PST
Comment on attachment 582815 [details] [diff] [review]
Patch v1

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

Cool, r=me.

And ui-r-, based on the demo you gave.  :(

Thanks,
Blake.
Comment 5 Andreas Nilsson (:andreasn) 2011-12-19 14:32:04 PST
Created attachment 582962 [details] [diff] [review]
Updated patch

This should take care of the issue with text overflowing the button itself.
Still needs a tiny bit of polish in the case where one button has one line and one have two lines of text.
Comment 6 Mike Conley (:mconley) 2011-12-19 14:51:59 PST
Comment on attachment 582962 [details] [diff] [review]
Updated patch

So far so good.  Assuming we get things lined up in the case you describe (one button having more lines than the other), and assuming we apply the changes to all themes, I think we're on the right track.
Comment 7 Andreas Nilsson (:andreasn) 2011-12-19 15:35:58 PST
Created attachment 582984 [details] [diff] [review]
patch v3

Ok, this should take care of the buttons looking misaligned if one have more lines of text than the other. Both are now aligned to the top.
Comment 8 Mike Conley (:mconley) 2011-12-19 15:47:11 PST
Comment on attachment 582984 [details] [diff] [review]
patch v3

Works brilliantly!  If you can prep it for Windows / OSX too, we'll get bwinton to r/ui-r it, and we're off to the races.
Comment 9 Andreas Nilsson (:andreasn) 2011-12-19 16:24:24 PST
Created attachment 582995 [details] [diff] [review]
patch v4

Fixes from previous patch carried over to Qute and Pinstripe as well.
Comment 10 Andreas Nilsson (:andreasn) 2011-12-19 17:08:06 PST
Created attachment 583023 [details]
screenshot of patch v4 to ease review
Comment 11 Blake Winton (:bwinton) (:☕️) 2011-12-19 17:12:24 PST
Comment on attachment 582995 [details] [diff] [review]
patch v4

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

Aside from the question below, I like it, so r=me.

And ui-r=me based on the screenshot.

Thanks,
Blake.

::: mail/themes/gnomestripe/mail/newmailaccount/accountProvisioner.css
@@ +518,5 @@
>    margin-left: 7px;
>    margin-right: 8px;
> +  display: inline;
> +  max-width: 260px;
> +  min-width: 260px;

If we're setting the max and min width to the same thing, can't we just set the width?
Comment 12 Andreas Nilsson (:andreasn) 2011-12-20 07:19:11 PST
Created attachment 583156 [details] [diff] [review]
updated patch with just width instead of max and min-width

Indeed. Used min-width first and then just added max-width as well and forgot about it. Here is a patch with just width.
Carrying over review+ and ui-review+ from previous patch.
Comment 13 Mike Conley (:mconley) 2011-12-20 07:20:51 PST
Comment on attachment 583156 [details] [diff] [review]
updated patch with just width instead of max and min-width

Our localizers will want this for TB 10.
Comment 14 Mike Conley (:mconley) 2011-12-20 12:25:59 PST
Committed to trunk as http://hg.mozilla.org/comm-central/rev/8b6248df4f24
Comment 15 Mike Conley (:mconley) 2011-12-20 13:26:31 PST
Committed to comm-aurora as http://hg.mozilla.org/releases/comm-aurora/rev/28c353f24518
Comment 16 Tim Maks van den Broek [:mad_maks] 2011-12-22 01:59:58 PST
The latest build of thunderbird 10 is from dec 20 and there it is not fixed yet (same with nightly). but i assume that in the next build it will be....

Thanks for picking up my report in 700536 so quickly.
Comment 17 Mike Conley (:mconley) 2011-12-22 05:54:40 PST
Tim:

Hm - the fix *should* be in our nightly build.  Could you try last night's nightly and see if the fix is there?

-Mike
Comment 18 Tim Maks van den Broek [:mad_maks] 2011-12-25 05:53:45 PST
(In reply to Mike Conley (:mconley) from comment #17)
> Tim:
> 
> Hm - the fix *should* be in our nightly build.  Could you try last night's
> nightly and see if the fix is there?
> 
> -Mike

The build from ftp://ftp.mozilla.org/pub/thunderbird/nightly/latest-comm-central-l10n/ doesn't run on mij sytem and from ftp://ftp.mozilla.org/pub/thunderbird/nightly/latest-comm-aurora-l10n/ doesn't have te fix.
Comment 19 Mike Conley (:mconley) 2011-12-28 06:58:42 PST
Hm...Mark, any idea why this fix wouldn't be in the l10n builds yet?
Comment 20 Tim Maks van den Broek [:mad_maks] 2011-12-28 10:08:59 PST
(In reply to Tim Maks van den Broek from comment #18)
> (In reply to Mike Conley (:mconley) from comment #17)
> > Tim:
> > 
> > Hm - the fix *should* be in our nightly build.  Could you try last night's
> > nightly and see if the fix is there?
> > 
> > -Mike
> 
> The build from
> ftp://ftp.mozilla.org/pub/thunderbird/nightly/latest-comm-central-l10n/
> doesn't run on mij sytem and from
> ftp://ftp.mozilla.org/pub/thunderbird/nightly/latest-comm-aurora-l10n/
> doesn't have te fix.

ftp://ftp.mozilla.org/pub/thunderbird/nightly/latest-comm-aurora-l10n/ 28 dec build is fixed (earlybird 11)

ftp://ftp.mozilla.org/pub/thunderbird/nightly/latest-comm-central-l10n/ i had the wrong architecture :-(, 28 dec build is fixed (daily 12)

ftp://ftp.mozilla.org/pub/thunderbird/nightly/latest-candidate-comm-beta/linux-i686/nl/ is the account provisioner in English?!?
Comment 21 Tim Maks van den Broek [:mad_maks] 2011-12-28 10:13:33 PST
(In reply to Tim Maks van den Broek from comment #20)

> ftp://ftp.mozilla.org/pub/thunderbird/nightly/latest-candidate-comm-beta/
> linux-i686/nl/ is the account provisioner in English?!?

= Buildnaam: Mozilla/5.0 (X11; Linux i686; rv:10.0) Gecko/20111222 Thunderbird/10.0

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