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

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Tomer Cohen :tomer]

Attachment: Screenshot (Firefox Beta 10)

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.

Comment 1

[Philipp von Weitershausen [:philikon]]

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.

Comment 2

[Tomer Cohen :tomer]

(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. ☺

Comment 4

[Gregory Szorc [:gps]]

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

Comment 5

[Tomer Cohen :tomer]

Elad stepped in to help. 

$ hg clone http://hg.mozilla.org/mozilla-central
update file browser/base/content/syncProgress.xhtml
$ hg diff

Comment 6

[Tomer Cohen :tomer]

Attachment: Another screenshot (Firefox Aurora 12)

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.

Comment 7

[Elad Alfassa]

Attachment: Patch

Comment 8

[Richard Newman [:rnewman]]

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.

Comment 9

[Philipp von Weitershausen [:philikon]]

Comment on attachment 600630 [details] [diff] [review]
Patch

Apologies for the delay. This looks good. Thanks, Elad!

Comment 10

[Richard Newman [:rnewman]]

https://hg.mozilla.org/services/services-central/rev/ba7ce48a3f13

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/665891e3f09e

Comment 12

[Richard Newman [:rnewman]]

(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.

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/665891e3f09e

Comment 14

[Tomer Cohen :tomer]

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 15

[Richard Newman [:rnewman]]

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.

Comment 16

[Gregory Szorc [:gps]]

Landed again.

https://hg.mozilla.org/mozilla-central/rev/ba7ce48a3f13

Comment 17

[Alex Keybl [:akeybl]]

Comment on attachment 600630 [details] [diff] [review]
Patch

[Triage Comment]
Low risk and fixes locale compatibility - approved for Aurora 12.

Comment 18

[Gregory Szorc [:gps]]

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.

Comment 19

[Virgil Dicu [:virgil] [QA]]

Attachment: Screenshot with 12 beta 6

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.

Comment 20

[Tomer Cohen :tomer]

(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.

Comment 21

[Virgil Dicu [:virgil] [QA]]

Thank You.