Last Comment Bug 565071 - Odd connection between disk space elements in the Migration Assistant window / fix RTL in migration assistant window
: Odd connection between disk space elements in the Migration Assistant window ...
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Migration (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: Thunderbird 3.3a1
Assigned To: Andreas Nilsson (:andreasn)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-05-11 10:27 PDT by Andreas Nilsson (:andreasn)
Modified: 2010-05-19 15:56 PDT (History)
9 users (show)
dmose: wanted‑thunderbird+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
rc1+
rc1-fixed


Attachments
current layout (73.69 KB, image/png)
2010-05-11 10:27 PDT, Andreas Nilsson (:andreasn)
no flags Details
patch to move them to new location (2.93 KB, patch)
2010-05-11 12:12 PDT, Andreas Nilsson (:andreasn)
bwinton: review+
clarkbw: ui‑review+
Details | Diff | Splinter Review
and here is the patch in action (75.98 KB, image/png)
2010-05-11 12:14 PDT, Andreas Nilsson (:andreasn)
no flags Details
not a finished solution (5.69 KB, patch)
2010-05-19 09:09 PDT, Andreas Nilsson (:andreasn)
no flags Details | Diff | Splinter Review
The previous patch, but with RTL working. (13.05 KB, patch)
2010-05-19 12:23 PDT, Blake Winton (:bwinton) (:☕️)
bwinton: review+
bugs: review+
Details | Diff | Splinter Review

Description Andreas Nilsson (:andreasn) 2010-05-11 10:27:42 PDT
Created attachment 444689 [details]
current layout

Talking about the migration assistant over the phone, we realized that the disk space elements in the migration assistant was a bit off from the actual bits they were affecting.
More specifically, the required disk space info actually only applies to the Synchronize option, but it's way off from that.

Bryan suggested to have the required space next to the Synchronize option and the free disk space down at the bottom with the current disk space usage info.
Comment 1 Andreas Nilsson (:andreasn) 2010-05-11 12:12:25 PDT
Created attachment 444711 [details] [diff] [review]
patch to move them to new location

The xhtml and css is not in a really good state (all the names are very odd), but this will work until I tackle it again tomorrow. :)
Comment 2 Andreas Nilsson (:andreasn) 2010-05-11 12:14:41 PDT
Created attachment 444715 [details]
and here is the patch in action
Comment 3 Dan Mosedale (:dmose) 2010-05-11 12:30:24 PDT
This doesn't quite rise to the level of blocking-thunderbird 3.1, but we'd _really_ like to get this, so marking as wanted+.  Thanks for working on this, Andreas!
Comment 4 Andreas Nilsson (:andreasn) 2010-05-13 01:52:52 PDT
Comment on attachment 444711 [details] [diff] [review]
patch to move them to new location

Setting for review
Comment 5 Blake Winton (:bwinton) (:☕️) 2010-05-14 09:04:11 PDT
Comment on attachment 444711 [details] [diff] [review]
patch to move them to new location

>-          <div>&featureConfigurator.autoSync.on;</div>
>+          <div>&featureConfigurator.autoSync.on; <span class="disk-space-required" id="disk-space-free">&featureConfigurator.diskSpace.requireSpace; <span id="size-difference"/></span></div>

I don't think this is going to do the right thing in right-to-left languages…

Although having said that, I think
>-        <div class="disk-space" id="disk-space-used">&featureConfigurator.diskSpace.currentSize; <span id="current-size"/></div>

might already be wrong, so perhaps we're okay.

The only other thing I would suggest is to add a tiny bit of space on the bottom.

I will say r=me, if you either get a signoff from a right-to-left user, or post a screenshot showing that it'll be okay.  ;)

Later,
Blake.
Comment 6 Bryan Clark (DevTools PM) [@clarkbw] 2010-05-14 15:50:38 PDT
Comment on attachment 444711 [details] [diff] [review]
patch to move them to new location

looks good though we need that padding back at the bottom.  7px should be good enough.  ui-r+ with that.

If you want to test RTL in Thunderbird, here are some instructions:
http://clarkbw.net/blog/2009/04/02/testing-rtl-in-thunderbird/
Comment 7 Dan Mosedale (:dmose) 2010-05-17 12:01:11 PDT
If someone could test for RTL and nominate for 3.1 approval, that would be great.  Thanks!
Comment 8 Dan Mosedale (:dmose) 2010-05-17 16:45:55 PDT
I've just uploaded this patch to the try server, so, with luck, there will be testable builds at <http://tinderbox.mozilla.org/showbuilds.cgi?tree=ThunderbirdTry> in a number of hours.  Note that we still need an updated patch with the padding tweak for approval, however.
Comment 9 Bryan Clark (DevTools PM) [@clarkbw] 2010-05-18 19:27:20 PDT
I've started testing this with the RTL force add-on however I'm not really sure if things are working out as they should be.  i.e. I have no idea if this is near correct.
http://i.imgur.com/x5t9P.png

The image is a background image so I don't see how we can get it to swap sides.  We might have to create an actual image entry instead to fix that.

Is there any help we could get from the l10n community?  Thanks!
Comment 10 Dan Mosedale (:dmose) 2010-05-18 19:42:47 PDT
I've just added thunderbird@localization.bugs as well; I'm hoping there will be a sympathetic RTL hacker there who can help us out.  If we were to get a working patch for this soon enough, we'd take it as a ridealong for 3.1RC1.
Comment 11 David Ascher (:davida) 2010-05-18 20:28:35 PDT
philor knows stuff.
Comment 12 Phil Ringnalda (:philor) 2010-05-18 20:56:45 PDT
<body dir="&locale.dir;"> (making triple-sure you are including chrome://global/locale/global.dtd anywhere you use it), then for the things that doesn't fix,

html {
  background ... top right ...;
}

html:-moz-locale-dir(rtl) {
  background ... top left ...;
}
Comment 13 Andreas Nilsson (:andreasn) 2010-05-19 09:09:11 PDT
Created attachment 446235 [details] [diff] [review]
not a finished solution

rtl in css refuses to work with me... :(
Comment 14 Mark Banner (:standard8) 2010-05-19 10:24:54 PDT
After a bit of discussion, we're going to block 3.1rc1 on the RTL parts of this as non of the migration assistant is in RTL.
Comment 15 Blake Winton (:bwinton) (:☕️) 2010-05-19 12:23:39 PDT
Created attachment 446269 [details] [diff] [review]
The previous patch, but with RTL working.

Re this part:
+html[dir="rtl"] {
+  background-position: -850px 0px;
+}

That could be "top left", except our image has a billion pixels of whitespace on the left, so it doesn't work.  Ehsan didn't really want to change all the images, but if we do, we can change that position to something that makes more sense.

r=ehsan, in person.

Later,
Blake.
Comment 16 Blake Winton (:bwinton) (:☕️) 2010-05-19 12:39:07 PDT
Comment on attachment 446269 [details] [diff] [review]
The previous patch, but with RTL working.

I'm not sure who else should be reviewing this, but I figured you'ld want a look at it.
Comment 17 Andreas Nilsson (:andreasn) 2010-05-19 13:04:18 PDT
(In reply to comment #15)
> Created an attachment (id=446269) [details]
> The previous patch, but with RTL working.

Can't find anything wrong in this one, but if someone else wants to take a peek as well, that would be great.
r+ from me.
Comment 18 Andreas Nilsson (:andreasn) 2010-05-19 13:06:37 PDT
(In reply to comment #15)
> Created an attachment (id=446269) [details]
 
> Re this part:
> +html[dir="rtl"] {
> +  background-position: -850px 0px;
> +}

We can open a new bug about that if it makes the code more understandable, but I think this approach works for now.
Comment 19 Blake Winton (:bwinton) (:☕️) 2010-05-19 13:52:32 PDT
Okay, I'm heading home now.  If no-one has and objections, and if no-one else has landed this by the time I get there, I'll do it then.

Thanks,
Blake.
Comment 20 :Ehsan Akhgari 2010-05-19 14:47:50 PDT
(In reply to comment #12)
> html {
>   background ... top right ...;
> }
> 
> html:-moz-locale-dir(rtl) {
>   background ... top left ...;
> }

FWIW, :-moz-locale-dir *only* works in XUL *documents*.  That means that in the following situations, you cannot use it:

* HTML elements in HTML documents.
* XUL elements in HTML documents.
Comment 21 Mark Banner (:standard8) 2010-05-19 14:52:21 PDT
Checked in on 1.9.2 branch:
http://hg.mozilla.org/releases/comm-1.9.2/rev/3f51b3c1799e

Please can someone do landing on trunk at some stage in the next 24 hours.
Comment 22 Blake Winton (:bwinton) (:☕️) 2010-05-19 15:13:40 PDT
Landed on trunk:
http://hg.mozilla.org/comm-central/rev/6228d5f54df9
Comment 23 :Ehsan Akhgari 2010-05-19 15:56:26 PDT
For better hygiene, the dir attribute can be removed from body elements.

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