1279230 - Provide RTL support for Pocket component

Status: VERIFIED FIXED

(Firefox :: Pocket, defect)

Description

[Itiel]

Attachment: Screenshot of un-RTL'd Pocket

Firefox should include RTL support for Pocket on RTL'd Firefoxs such as Hebrew or Arabic, same as prodived to Firefox Hello component.

Attached screenshot of current look of the un-RTL'd Pocket on a Hebrew Firefox Developer Edition 48.0a2.

Comment 1

[Tomer Cohen :tomer]

I can't reproduce this issue. It seems that the Hebrew translation have been taken offline. Are you aware of this?

Comment 2

[Itiel]

(In reply to Tomer Cohen :tomer from comment #1)
> I can't reproduce this issue. It seems that the Hebrew translation have been
> taken offline. Are you aware of this?

Yeah, I am. But I'm not sure if the same applies to other RTL languages (AR, FA etc).

Comment 3

[Arash Mousavi [:Arash-M]]

Attachment: Pcoket dropdown not RTL in Persian

The dropdown content is also not RTL in Persian. The sentences are wrongly aligned to left and their position is reversed whenever there's an English word is in the sentence.

Comment 4

[Tomer Cohen :tomer]

Attachment: Bug 1279230 - Provide RTL support for Pocket component

See also https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Internationalization\#Predefined_messages

Review commit: https://reviewboard.mozilla.org/r/141204/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/141204/

Comment 5

[Tomer Cohen :tomer]

Try seems to be busted at the moment. I'll update this bug later with test builds.

Comment 7

[Khaled Hosny]

Is there anyway to test this patch without building Firefox from source?

Comment 8

[Shane Caraveo (:mixedpuppy)]

(In reply to Khaled Hosny from comment #7)
> Is there anyway to test this patch without building Firefox from source?

You can checkout the code from github, apply this patch, then build the xpi.

git clone https://github.com/mozilla-l10n/pocket-l10n
git clone https://github.com/mozilla-partners/pocket
cd pocket make

You should end up with a build directory that contains an xpi.

Comment 9

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8869623 [details]
Bug 1279230 - Provide RTL support for Pocket component

https://reviewboard.mozilla.org/r/141204/#review146152

This looks reasonable, but I have no way to know for sure that the issue is fixed.  How will we know, what steps should QA have?

Comment 10

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8869623 [details]
Bug 1279230 - Provide RTL support for Pocket component

https://reviewboard.mozilla.org/r/141204/#review146188

After some discussion, this is actually not working yet so removing r+

Comment 11

[Itiel]

Tomer is not actively working on this.

Comment 12

[Itiel]

Attachment: Bug 1279230 - Convert Pocket CSS to logical properties to prepare for RTL support

Comment 13

[Itiel]

Gijs, pinging you as requested to take a look at the patch.

Comment 14

[:Gijs (he/him)]

I responded on phabricator. :-)

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/573654d340bc
Convert Pocket CSS to logical properties to prepare for RTL support r=thecount,Gijs

Comment 16

[Arthur Iakab [arthur_iakab]]

Backed out changeset 573654d340bc (bug 1279230) for causing browser-chrome failures on browser_parsable_css.js. CLOSED TREE

Backout revision https://hg.mozilla.org/integration/autoland/rev/41a809838799749458784cdb8a92d32adb9bff90

Failed push https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=os%2Cx%2C10.14%2Cshippable%2Copt%2Cmochitests%2Ctest-macosx1014-64-shippable%2Fopt-mochitest-browser-chrome-e10s-4%2Cm%28bc4%29&revision=573654d340bce73cb4f90b777b86cffc79fe0280

Failure log https://treeherder.mozilla.org/logviewer.html#?job_id=275243940&repo=autoland

Failures https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=275243940&fromchange=6a21e61770a8668cddce4825f9199270c5de35fb&searchStr=os%2Cx%2C10.14%2Cshippable%2Copt%2Cmochitests%2Ctest-macosx1014-64-shippable%2Fopt-mochitest-browser-chrome-e10s-4%2Cm%28bc4%29

Itiel can you please take a look?

These failures started from your push but is not obvious because there was a another failure winch started here https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=275240030&resultStatus=testfailed%2Cbusted%2Cexception&revision=ec5a8a6a37d41c4447eb2e71004c5f07893959a3 and was backed out here https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=275243940&searchStr=os%2Cx%2C10.14%2Cshippable%2Copt%2Cmochitests%2Ctest-macosx1014-64-shippable%2Fopt-mochitest-browser-chrome-e10s-4%2Cm%28bc4%29&revision=bc8aa0201da85c45770c960ade2c97889b076305

Comment 17

[Francesco Lodolo [:flod]]

https://hg.mozilla.org/integration/autoland/rev/573654d340bc#l1.138

text-align: inline-start;

I don't think that's a valid value?
https://developer.mozilla.org/en-US/docs/Web/CSS/text-align

Comment 18

[:Gijs (he/him)]

(In reply to Francesco Lodolo [:flod] from comment #17)

https://hg.mozilla.org/integration/autoland/rev/573654d340bc#l1.138

text-align: inline-start;

I don't think that's a valid value?
https://developer.mozilla.org/en-US/docs/Web/CSS/text-align

Yeah, that should have been start.

Comment 19

[Itiel]

Yes, sorry, my bad. It's easy to get confused sometimes with all the start/end properties...
Fixed on phab.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/52a6fac5e177
Convert Pocket CSS to logical properties to prepare for RTL support r=thecount,Gijs

Comment 21

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/52a6fac5e177