1217784 - [RTL][Browser]The URL is left-aligned.

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Browser, defect, P2)

Description

[Lancy(Leave from Mozilla)]

Attachment: Browser_URL_v2.5.png

[1.Description]:
[RTL][AriesKK&FlameKK v2.5][Browser]Go to a website, the URL is left-aligned.
See attachment:Browser_URL_v2.5.png

[2.Testing Steps]: 
1. Set system language as Arabic. 
2. Click on the Browser app. 
3. Tap the address bar. 
4. Input website address and tap Search button on keyboard. 
>> 4.1. The "..." (Menu) icon should be left-aligned. 
4.2. The URL should be right-aligned but read left to right. 

[3.Expected Result]: 
4.The URL should be right-aligned and read left to right.

[4.Actual Result]: 
4.The URL is left-aligned.

[5.Reproduction build]: 
Device: Flame KK 2.5 build (Affected)
Build ID               20151022150207
Gaia Revision          29ce8ec8606e59f582375234440812b046346513
Gaia Date              2015-10-22 05:31:38
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/76bd0c01d72e64ca4f261ffdb2652a91f961e930
Gecko Version          44.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20151022.185000
Firmware Date          Thu Oct 22 18:50:13 EDT 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0

Device: Aries KK 2.5 build (Affected)
Build ID               20151023005002
Gaia Revision          29ce8ec8606e59f582375234440812b046346513
Gaia Date              2015-10-22 05:31:38
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/1f03a14106e59280761ac53904340f389674337f
Gecko Version          44.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20151023.001128
Firmware Date          Fri Oct 23 00:11:35 UTC 2015
Bootloader             s1

[6.Reproduction Frequency]: 
Always Recurrence,10/10

[7.TCID]: 
15466

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] Phoxygen:bug1217784-browser_url_alignment > mozilla-b2g:master

Comment 2

[Augustin Trancart [:autra]]

Comment on attachment 8679965 [details] [review]
[gaia] Phoxygen:bug1217784-browser_url_alignment > mozilla-b2g:master

Hi Ben,

Please r?

Comment 3

[Ben Francis [:benfrancis]]

Comment on attachment 8679965 [details] [review]
[gaia] Phoxygen:bug1217784-browser_url_alignment > mozilla-b2g:master

Thanks for the patch,

This patch seems very drastic for the issue described in this bug, it would have effect on the whole system app.

If you're just trying to fix the problem described in this bug I would suggest a solution more isolated to the browser chrome. If you are trying to change the default for the whole system app I would suggest maybe this isn't the right bug or the right CSS file to do that in.

What are you trying to achieve?

Comment 4

[Augustin Trancart [:autra]]

Hi Ben, thanks for the review!

Yes, it looks a bit drastic. However, I don't think we are taking a big risk there: 
1/ for elements that have neither direction nor text-align specified, this is the default anyway (the default is 'start', but in this case, that's the same). 
2/ for elements that have a dir="auto" (added for RTL most probably), this is what we want in most of the case : the calculated direction of the content of an element should not change the alignment. We are restoring what happens without the dir="auto". I'm saying "most of", but the only case where this is not true is for multiline contenteditable elements.
3/ for elements (either with or without dir) that have a text-align specified, their rule will always override the inherited text-align.

We used this to cleanly fix alignment problems (what I describe in 2/ actually) in other apps, with success. That being said, chrome.css is clearly not the right place to put this, indeed. 

Do you have a suggestion for another place where I can put that? Or I can restrict that to .chrome or other class if you still think this change is too risky of course. Thanks!

Comment 5

[Augustin Trancart [:autra]]

(In reply to Ben Francis [:benfrancis] from comment #3)
> Comment on attachment 8679965 [details] [review]
> [gaia] Phoxygen:bug1217784-browser_url_alignment > mozilla-b2g:master
> What are you trying to achieve?

I didn't answer this: I'm trying to fix all alignment issues once and for all :-) I might be too ambitious though...

Comment 6

[Ben Francis [:benfrancis]]

Sam or Tim, do you have a suggestion for a better place to put this global CSS rule?

Comment 7

[Sam Foster [:sfoster] (he/him)]

Thinking about this. I get how the rule works and it maybe a succinct way to solve a lot of problems. But I really think a patch like this should be attached to a bug that describes the problem it is solving. On a good day this would make me a bit fidgety. On the day before RA I think its reckless. Lets make a chrome-specific patch and file a follow-up for any further alignment issues.

Comment 8

[Augustin Trancart [:autra]]

Comment on attachment 8679965 [details] [review]
[gaia] Phoxygen:bug1217784-browser_url_alignment > mozilla-b2g:master

Hey Ben,

Here is a more reasonable patch :-) Please r? again.

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

What you asked in comment 6 is probably rebased and gone in comment 8.

The current patch in the pull request (changes the .urlbar only) looks to me like a right fix (and in the right place).

As a side note, this is probably a really really slow CSS selector -- I wonder if there has ever been a discussion on this somewhere. If not we need to talk to the platform team and ask if there is a better way to specify rtl style.

Comment 10

[Augustin Trancart [:autra]]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #9)
> 
> As a side note, this is probably a really really slow CSS selector -- I
> wonder if there has ever been a discussion on this somewhere. If not we need
> to talk to the platform team and ask if there is a better way to specify rtl
> style.

Are you talking about html[dir="ltr"] alone? Doesn't the engine try to find all html elements first ? That would be very fast. But maybe it tries to match all [dir="ltr"]... I'd like to hear from platform engineer indeed :-) That being said, I'm not sure we really have an alternative: usually we really want to test the document direction, not the element's inherited direction (they are different is some cases).

For this particular case, we match 3 classes before. Even though the child selector is slow, I don't expect many elements will be matched after the first class .urlbar anyway, so walking up the tree should not be a real problem, WDYT?

Comment 11

[Sam Foster [:sfoster] (he/him)]

> For this particular case, we match 3 classes before. Even though the child
> selector is slow, I don't expect many elements will be matched after the
> first class .urlbar anyway, so walking up the tree should not be a real
> problem, WDYT?

Its a pattern we use throughout Gaia, I think Tim was just saying we should investigate (post 2.5). We're just waiting for Ben's review here to land.

Comment 12

[Ben Francis [:benfrancis]]

Comment on attachment 8679965 [details] [review]
[gaia] Phoxygen:bug1217784-browser_url_alignment > mozilla-b2g:master

Thank you!

Comment 13

[Augustin Trancart [:autra]]

NI myself to land this when tests are green

Comment 14

[Carsten Book [:Tomcat]]

https://github.com/mozilla-b2g/gaia/commit/3dff75eb3c4bd59fe1862eae7d893783381218a4

Comment 15

[Augustin Trancart [:autra]]

Comment on attachment 8679965 [details] [review]
[gaia] Phoxygen:bug1217784-browser_url_alignment > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):None
[User impact] if declined: Bad RTL support of the browser app
[Testing completed]: on Flame master
[Risk to taking this patch] (and alternatives if risky): Very low risk, only a text-alignment in CSS
[String changes made]: None

Comment 16

[Lancy(Leave from Mozilla)]

Attachment: Verify1_URL_master.png

This issue is verified as pass on latest FlameKK&AriesKK master build by the same STR in comment 0.
Actual result:The URL is right-aligned and read left to right.
See attachment:Verify1_URL_master.png.
Reproducing rate:0/10

Device:FlameKK master build(512mb) (Pass)
Build ID               20151103150203
Gaia Revision          61918ddd9ccce104c009e873e34a0791e125753a
Gaia Date              2015-11-03 17:22:30
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/f742b9412ed5aace90ad863b276faae0641090a8
Gecko Version          45.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20151103.182550
Firmware Date          Tue Nov  3 18:26:03 EST 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0

Device: AriesKK master build (Pass)
Build ID               20151104004249
Gaia Revision          61918ddd9ccce104c009e873e34a0791e125753a
Gaia Date              2015-11-03 17:22:30
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/f742b9412ed5aace90ad863b276faae0641090a8
Gecko Version          45.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20151104.000116
Firmware Date          Wed Nov  4 00:01:24 UTC 2015
Bootloader             s1

Comment 17

[Augustin Trancart [:autra]]

Comment on attachment 8679965 [details] [review]
[gaia] Phoxygen:bug1217784-browser_url_alignment > mozilla-b2g:master

Already in 2.5

Comment 18

[Autumn.Qiu(Leave from Mozilla)]

This bug has been verified as "pass" on the latest build of Flame KK v2.5 512mb and Aries KK v2.5 by the STR in comment 0 .

Actual result: The URL is right-aligned and read left to right.

See attachment: verified_Aries_KK_v2.5_URL.png
Reproduce rate: 0/10.

Device: Aries KK v2.5(Pass)
Build ID               20151110094357
Gaia Revision          07baf613699fa6225359c7f04825c5caeb71d424
Gaia Date              2015-11-09 21:32:50
Gecko Revision         http://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/e14287b00a514a15418dfaa89287030c588ad19d
Gecko Version          44.0a2
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20151110.090331
Firmware Date          Tue Nov 10 09:03:39 UTC 2015
Bootloader             s1

Device: Flame KK v2.5 512mb(Pass)
Build ID               20151109004552
Gaia Revision          cf646c52bb947af28329b0a100df91d1b1f2a907
Gaia Date              2015-11-09 02:55:50
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/4eafef5b80f8985c94c4a067f130d37513e1a581
Gecko Version          44.0a2
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20151109.041411
Firmware Date          Mon Nov  9 04:14:26 EST 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0

Comment 19

[Autumn.Qiu(Leave from Mozilla)]

Attachment: verified_Aries_KK_v2.5_URL.png