Closed Bug 565071 Opened 14 years ago Closed 14 years ago

Odd connection between disk space elements in the Migration Assistant window / fix RTL in migration assistant window

Categories

(Thunderbird :: Migration, defect)

Product:
Thunderbird
Component:
Migration
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-thunderbird3.1 rc1+, thunderbird3.1 rc1-fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 3.3a1
Tracking Flags:
Tracking Status
blocking-thunderbird3.1 --- rc1+
thunderbird3.1 --- rc1-fixed

People

(Reporter: andreasn, Assigned: andreasn)

Details

Attachments

(4 files, 1 obsolete file)

current layout
73.69 KB, image/png
Details
patch to move them to new location
2.93 KB, patch
bwinton
: review+
clarkbw
: ui-review+
Details | Diff | Splinter Review
and here is the patch in action
75.98 KB, image/png
Details
The previous patch, but with RTL working.
13.05 KB, patch
bwinton
: review+
andreasn
: review+
Details | Diff | Splinter Review
Attached image 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.
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. :)
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!
Assignee: nobody → nisses.mail
Flags: wanted-thunderbird+
Comment on attachment 444711 [details] [diff] [review]
patch to move them to new location

Setting for review
Attachment #444711 - Flags: ui-review?(clarkbw)
Attachment #444711 - Flags: review?(bwinton)
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.
Attachment #444711 - Flags: review?(bwinton) → review+
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/
Attachment #444711 - Flags: ui-review?(clarkbw) → ui-review+
If someone could test for RTL and nominate for 3.1 approval, that would be great.  Thanks!
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.
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!
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.
philor knows stuff.
<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 ...;
}
Attached patch not a finished solution (obsolete) — — Splinter Review 
rtl in css refuses to work with me... :(
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.
blocking-thunderbird3.1: --- → rc1+
Summary: Odd connection between disk space elements in the Migration Assistant window → Odd connection between disk space elements in the Migration Assistant window / fix RTL in migration assistant window
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.
Attachment #446235 - Attachment is obsolete: true
Attachment #446269 - Flags: review+
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.
Attachment #446269 - Flags: review?(nisses.mail)
(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.
Attachment #446269 - Flags: review?(nisses.mail) → review+
(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.
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.
Status: NEW → ASSIGNED
Whiteboard: [needs landing]
(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.
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.
status-thunderbird3.1: --- → rc1-fixed
OS: Linux → All
Hardware: x86 → All
Whiteboard: [needs landing] → [needs landing on trunk]
Target Milestone: --- → Thunderbird 3.2a1
Landed on trunk:
http://hg.mozilla.org/comm-central/rev/6228d5f54df9
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
status-thunderbird3.1: rc1-fixed → ---
OS: All → Linux
Hardware: All → x86
Resolution: --- → FIXED
Whiteboard: [needs landing on trunk]
Target Milestone: Thunderbird 3.2a1 → ---
status-thunderbird3.1: --- → rc1-fixed
Target Milestone: --- → Thunderbird 3.2a1
For better hygiene, the dir attribute can be removed from body elements.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: