Closed Bug 713634 Opened 8 years ago Closed 8 years ago

[RTL] Firefox Sync completion message doesn't contain dir=rtl

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla13
Tracking Status
firefox12 --- verified

People

(Reporter: tomer, Assigned: elad)

References

()

Details

(Keywords: rtl, Whiteboard: [qa+])

Attachments

(4 files)

After setting up Sync for the first time, I got the message text garbled because of missing dir=rtl on the document. Screenshot attached. 

see also bug 652341.
Good catch! Should be pretty easy to fix. Perhaps Ally would care to do so as she implemented this page. If not, I'll be happy to mentor anybody who wants to give it a shot.
Hardware: x86 → All
Whiteboard: [good first bug][lang=html][mentor=philikon]
(In reply to Philipp von Weitershausen [:philikon] from comment #1)
> Good catch! Should be pretty easy to fix. Perhaps Ally would care to do so
> as she implemented this page. If not, I'll be happy to mentor anybody who
> wants to give it a shot.

In case the content of the page is (x)html, is could be easily done by watching the patch we made on bug 652341. In other words, this fix should be done in less than two lines of code. ☺
Duplicate of this bug: 721815
I'm pretty sure the document in question is browser/base/content/syncProgress.xhtml. The change will look a lot like https://hg.mozilla.org/mozilla-central/rev/0e12d3b6027e
Elad stepped in to help. 

$ hg clone http://hg.mozilla.org/mozilla-central
update file browser/base/content/syncProgress.xhtml
$ hg diff
Assignee: nobody → elad
chrome://browser/content/syncProgress.xhtml

The word Aurora should be the last word in the sentence, i.e., at the left side in RTL. After fixing this bug it should look correctly.
Attached patch PatchSplinter Review
Attachment #600630 - Flags: review?
Comment on attachment 600630 [details] [diff] [review]
Patch

Hey Elad,

Thanks for the patch! I'm pointing your review flag at the mentor for this bug.

For future reference, you should follow the instructions here:

  https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

when preparing a patch to attach to a bug. There are a bunch of headers that need to be present before it can be checked in, and the more you put in in advance, the easier time we'll have.
Attachment #600630 - Flags: review? → review?(philipp)
Comment on attachment 600630 [details] [diff] [review]
Patch

Apologies for the delay. This looks good. Thanks, Elad!
Attachment #600630 - Flags: review?(philipp) → review+
https://hg.mozilla.org/services/services-central/rev/ba7ce48a3f13
Whiteboard: [good first bug][lang=html][mentor=philikon] → [fixed in services][qa+]
(In reply to Ryan VanderMeulen from comment #11)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/665891e3f09e

Oh, dear, I forgot to clear the checkin-needed flag. This already landed in services-central.
https://hg.mozilla.org/mozilla-central/rev/665891e3f09e
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
Is it possible to push this fix to Aurora/Beta as well? As far as I know there was no difference in the affected file recently.
Comment on attachment 600630 [details] [diff] [review]
Patch

Probably not enough of a showstopper to even consider beta approval, but Aurora for sure.

[Approval Request Comment]
Regression caused by (bug #):
  Long-term.

User impact if declined:
  Degraded setup completion experience for RTL users.

Testing completed (on m-c, etc.):
  Landed on m-c; routine QA pass will cover this next week.

Risk to taking this patch (and alternatives if risky):
  Minimal.

String changes made by this patch:
  None.
Attachment #600630 - Flags: approval-mozilla-aurora?
Landed again.

https://hg.mozilla.org/mozilla-central/rev/ba7ce48a3f13
Whiteboard: [fixed in services][qa+] → [qa+]
Comment on attachment 600630 [details] [diff] [review]
Patch

[Triage Comment]
Low risk and fixes locale compatibility - approved for Aurora 12.
Attachment #600630 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/c0bc135b7c61

Patch didn't apply cleanly because of bug 730271. But, that was trivial to work around.
The Aurora word is now displayed in the left in 12 beta 6, as the sync button. I assume this is correct, but will appreciate a confirmation from someone who understands Hebrew before setting it to verified.
(In reply to Virgil Dicu [:virgil] [QA] from comment #19)
> I assume this is correct, but will appreciate a confirmation from
> someone who understands Hebrew before setting it to verified.

I can confirm this is correct.
Component: Firefox Sync: UI → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.