Closed Bug 1151925 Opened 9 years ago Closed 9 years ago

[RTL][Browser] URL(title) should support bidirectional text

Categories

(Firefox OS Graveyard :: Gaia::System::Browser Chrome, defect)

Product:
Firefox OS Graveyard
Component:
Gaia::System::Browser Chrome
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-b2g:2.2+, b2g-v2.2 verified, b2g-master verified)

Status:
VERIFIED FIXED
Milestone:
2.2 S10 (17apr)
Project Flags:
blocking-b2g 2.2+
Tracking Flags:
Tracking Status
b2g-v2.2 --- verified
b2g-master --- verified

People

(Reporter: pcheng, Assigned: sfoster)

References

Details

(Whiteboard: [3.0-Daily-Testing][systemsfe])

Attachments

(4 files)

screenshots.png
267.95 KB, image/png
Details
[gaia] sfoster:bidi-urlbar-bug-1151925 > mozilla-b2g:master
46 bytes, text/x-github-pull-request
kgrandon
: review+
bajaj
: approval-gaia-v2.2+
Details | Review
v3.0_verify.png
461.24 KB, image/png
Details
v2.2_verify.png
329.07 KB, image/png
Details
Attached image screenshots.png — 
This bug is filed to separate the issue that remains to be fixed in bug 1149448.

I'm actually not quite sure what is expected here, as browser titles/URL bar could display both RTL and LTR languages. Currently when in RTL environment, it displays all text RTL regardless of content, but when there's no RTL text on there I think it should somehow display the text in LTR and truncate/show ellipsis to the right.

STR:
0) Set device language to Arabic
1) Open browser and access www.youtube.com
2) Tap to view any English video, and observe the URL bar/title
3) Enter card view, and observe the title

Expected: When there's no RTL text, it should display text LTR and show ellipsis on the right

Actual: Browser URL bar/title shows all text RTL regardless of text language. See attached screenshots for problematic areas.

Device: Flame 3.0
BuildID: 20150406010204
Gaia: ef61ebbe5de8c2c9fc2a8f74a12455044c3b82e9
Gecko: 4fe763cbe196
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 40.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0
This issue also occurs on 2.2.

Device: Flame 2.2
BuildID: 20150406002503
Gaia: a6351e1197d54f8624523c2db9ba1418f2aa046f
Gecko: c3335a5d3063
Gonk: ebad7da532429a6f5efadc00bf6ad8a41288a429
Version: 37.0 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0
QA Whiteboard: [rtl-impact][QAnalyst-Triage?]
status-b2g-v2.2: --- → affected
status-b2g-master: --- → affected
Flags: needinfo?(ktucker)
Whiteboard: [3.0-Daily-Testing]
Whiteboard: [3.0-Daily-Testing] → [3.0-Daily-Testing][systemsfe]
I think we need to separate out the directionality of the containing element - which includes the lock icon and browser chrome - from the title itself. In an RTL language, the UI should present the lock icon on the right. But the direction and truncation of the title should reflect *its* language. This is true for the browser as well as card view. 

Stephanie, can you confirm? Should this block 2.2?
blocking-b2g: --- → 2.2?
Flags: needinfo?(swilkes)
Whiteboard: [3.0-Daily-Testing][systemsfe] → [3.0-Daily-Testing][systemsfe][systemsfe]
Whiteboard: [3.0-Daily-Testing][systemsfe][systemsfe] → [3.0-Daily-Testing][systemsfe]
I'll work on this this week
QA Contact: sfoster
Depends on: 1149448
Blocks: 1149448
No longer depends on: 1149448
Sam, you may want to take a look at this issue as well bug 1151067 I wrote up search history results are truncated on the wrong side.
QA Whiteboard: [rtl-impact][QAnalyst-Triage?] → [rtl-impact][QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Julien, (picking on you as you were last in chrome.css :) it looks like we still don't have this quite right. The UI direction and content direction may differ. When the UI is RTL, we want the lock icon on the right, but the title itself could be in a latin language. That means we need dir="auto" for the .urlbar element, but this has the side-effect that the -moz-padding-* properties for that element will be reversed. html[dir="rtl"] .urlbar, but .urlbar:-moz-dir(ltr). Layout comedy ensues :)

We could stack up the CSS rules for all the permutations, but ISTM we just need to avoid overloading elements to act as both UI containers and content containers. For the .urlbar, we use it to contain the title or URL as well as position the lock icon - that's not going to work. I'm going to look at options - we could probably flexbox that whole thing, but I'd like a simpler, less radical fix to avoid a load of churn this late into 2.2.
Flags: needinfo?(felash)
Sam, I'm planning to do something that will make this task easier.

Have a look at Bug 1152082.
Depends on: 1152082
(In reply to Ted Clancy [:tedders1] from comment #6)
> Sam, I'm planning to do something that will make this task easier.
> 
> Have a look at Bug 1152082.

So take an example like 

    <html dir="rtl"><div class="title">En title</div></html> 

With your patch landed, this would get truncated appropriately and the ellipsis would be on the right, without my having to add an explicit dir="auto"? That still leaves the issue where CSS rules assuming .title has a direction matching the documentElement will get tripped up when .title { -moz-padding-start: 1rem } puts the padding on the left in this case, not the right as the author expected. I don't think this is a platform problem, its one we need to be aware of when writting markup & CSS (and reviewing those). 
I'll attach my WIP, which seems to be working out so far.
Sam, you can have a look at [1] that we use in the SMS app (do a "git grep ellipsis-dir-fix" in the apps/sms directory). I agree with your analysis of adding yet another container.

I've seen the issue in both the rocketbar and the "plain" chrome window (such as when we import gmail contacts). I planned to look at it but it's even better if you do :D

[1] https://github.com/mozilla-b2g/gaia/blob/edb97837286128eb705f852533e593bf0ec435f4/apps/sms/style/sms.css#L1062-L1070
Flags: needinfo?(felash)
Comment on attachment 8589341 [details] [review]
[gaia] sfoster:bidi-urlbar-bug-1151925 > mozilla-b2g:master

I played around with CSS fixes for this, but the unicode-bidi: -moz-plaintext kills the ellipsis, and embed etc. don't do the same job. So, dir="auto" it is - for now at least. 
In this patch I just want to fix the combined-chrome titles, and those in card view, to contain the scope a bit and make QA and uplift practical. There are a bunch of similar issue around Gaia like Julien mentions, but they seem to have their own bugs filed and can be patch and uplifted case by case. 

To test this, I found it necessary to build Gaia with GAIA_DEFAULT_LOCALE=ar to ensure the system app was properly localized. Just selecting the language in Settings doesn't do it(?) Once all the UI is in ar or some other RTL language, you can test by browsing both Arabic language sites (e.g. http://www.aljazeera.net/), as well as en-US sites like youtube.com to get nice mixed-language, long-enough-to-truncate bidi titles. Obviously we also need to not regress LTR too.
Attachment #8589341 - Flags: review?(kgrandon)
We're not blocked by bug 1152082, so moving it to see also. We should also track bug 883884 for these kind of issues.
No longer depends on: 1152082
See Also: → 1152082, 883884
I think this should block. Sam, it sounds like you've described the spec expectations well - i.e. each language should behave according to its own, etc.
Flags: needinfo?(swilkes)
Sam - I took a quick look, but the immediate reason behind nesting the .title element inside the div was not immediately clear. Can you tell me the reason we need to add this? This area is a bit of a hotspot and things often cause regressions as we don't have very good tests for layout currently - so I just want to make sure we have a very good reason of making such a modification.
Flags: needinfo?(sfoster)
Kevin, see comment #2; also, this is basically the same fix as SMS use. The UI elements (lock icon in this case) need to follow the system app's document direction, but the title itself should be allowed to be ltr, rtl or bidi and truncated appropriately.

Agreed with the need for care. That why I wanted to take this one step at a time - with this patch for the just the combined chrome. That's a much smaller subset of app titles the check - basically the browser (search app) and hosted apps if grep isn't lying to me?
Flags: needinfo?(sfoster) → needinfo?(kgrandon)
Thanks - that makes sense then. I'll start looking at this asap.
Flags: needinfo?(kgrandon)
Assignee: nobody → sfoster
QA Contact: sfoster
blocking-b2g: 2.2? → 2.2+
Comment on attachment 8589341 [details] [review]
[gaia] sfoster:bidi-urlbar-bug-1151925 > mozilla-b2g:master

I left a question about the styles on github, but I couldn't find any regressions with this. Nice work.
Attachment #8589341 - Flags: review?(kgrandon) → review+
Kevin, I addressed the comment in the PR, but I'll add it here as well: 

There's no padding on .chrome-combined .title at all now, its all on .chrome-title-container. If I could make it display:inline I would, but we need block for the text-overflow ellipsis to work. For this rule, I changed the selector from chrome-plain.maximized .title to chrome-plain.maximized .title - those are exclusive - chrome-combined is never chrome-plain and vice-versa. 

Naoki, flagging you to be aware of this change and any potential fallout. I've narrowed this right down so we should only see impact in the browser (and apps the request full chrome). There are similar bugs out there for the non-browser chrome. Not the same thing!
Flags: needinfo?(nhirata.bugzilla)
Keywords: checkin-needed
Thanks Sam.  I'll try to look at this later today.
So the things I looked at were, Rocketbar, Browser and homescreen links.

1) URL doesn't show after tapping on a history link in browser; not sure if that's specific to this patch, I have to investigate more.  ( tap browser -> tap a history link)

2) Tapping on the URL, the ellipse shows on the wrong side for browser; I'm pretty sure this was reported somewhere...  it's similar to this one, except it has to do with the URL itself.

This also happens for Rocektbar search and looks really wierd ( ... .aljazeera.net/portal )

3) did we remove truncation of URL in the history for Rocketbar?

Ran across:
1) Bug 1129234 - [RTL][Rocket Bar] Browser History and Google Search results are left-aligned.
( note: this also occurs in the browser app; same database?  not sure if that would be the same bug )
(oh, I hadn't flattened the commits so this didn't land, trying again as I dont see anything in your comments to cause alarm)

(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #19)
> So the things I looked at were, Rocketbar, Browser and homescreen links.
> 
> 1) URL doesn't show after tapping on a history link in browser; not sure if
> that's specific to this patch, I have to investigate more.  ( tap browser ->
> tap a history link)

I'm not sure what you are expecting to see. But doing side-by-side comparison with master vs. with this patch I see the page title as expected in the rocketbar when I've tapped a history entry. 

> 2) Tapping on the URL, the ellipse shows on the wrong side for browser; I'm
> pretty sure this was reported somewhere...  it's similar to this one, except
> it has to do with the URL itself.
> This also happens for Rocektbar search and looks really wierd ( ...
> .aljazeera.net/portal )

Yes, that sounds like bug 883884, I also just found bug 1151067 which notes a similar/same issue with history links. No doubt we have dupes out there, hopefully with this landed we can start to whittle them down. 
 
> 3) did we remove truncation of URL in the history for Rocketbar?
> 
> Ran across:
> 1) Bug 1129234 - [RTL][Rocket Bar] Browser History and Google Search results
> are left-aligned.
> ( note: this also occurs in the browser app; same database?  not sure if
> that would be the same bug )

The history links are a similar issue in a different place. We should fix it there - this bug and patch is specifically concerned with how the title (which can be URL or page title) is represented in the urlbar in the browser chrome.
Keywords: checkin-needed
Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.
Component: Gaia::Browser → Gaia::System::Browser Chrome
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
(In reply to Sam Foster [:sfoster] from comment #20)
> (oh, I hadn't flattened the commits so this didn't land, trying again as I
> dont see anything in your comments to cause alarm)

I pulled from your branch, so I should have both pull requests that differ from master.

> (In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from
> comment #19)
> > So the things I looked at were, Rocketbar, Browser and homescreen links.
> > 
> > 1) URL doesn't show after tapping on a history link in browser; not sure if
> > that's specific to this patch, I have to investigate more.  ( tap browser ->
> > tap a history link)
> 
> I'm not sure what you are expecting to see. But doing side-by-side
> comparison with master vs. with this patch I see the page title as expected
> in the rocketbar when I've tapped a history entry. 

Oh oops!  It's because I had set my device to proxy server in Puerto Rico.  So the issue I'm seeing isn't really supported.  Oops.  retested, and the comparison is fine.
 
> > 2) Tapping on the URL, the ellipse shows on the wrong side for browser; I'm
> > pretty sure this was reported somewhere...  it's similar to this one, except
> > it has to do with the URL itself.
> > This also happens for Rocektbar search and looks really wierd ( ...
> > .aljazeera.net/portal )
> 
> Yes, that sounds like bug 883884, I also just found bug 1151067 which notes
> a similar/same issue with history links. No doubt we have dupes out there,
> hopefully with this landed we can start to whittle them down. 

Ok, sounds good.  I'll let one of those two bugs take care of that issue then.

> > 3) did we remove truncation of URL in the history for Rocketbar?
> > 
> > Ran across:
> > 1) Bug 1129234 - [RTL][Rocket Bar] Browser History and Google Search results
> > are left-aligned.
> > ( note: this also occurs in the browser app; same database?  not sure if
> > that would be the same bug )
> 
> The history links are a similar issue in a different place. We should fix it
> there - this bug and patch is specifically concerned with how the title
> (which can be URL or page title) is represented in the urlbar in the browser
> chrome.

Ok, then since we have side bugs to address all the other stuff, I think I'm good with the patch.  Thanks for your time looking at my comments!
Flags: needinfo?(nhirata.bugzilla)
Target Milestone: --- → 2.2 S10 (17apr)
Note we also have the issue with "chrome-plain"-style chrome windows too. Although this likely happens in a lot less places in our OS. Do we have a bug about this?
(In reply to Julien Wajsberg [:julienw] (PTO April 6th) from comment #24)
> Note we also have the issue with "chrome-plain"-style chrome windows too.
> Although this likely happens in a lot less places in our OS. Do we have a
> bug about this?

We dont have a single bug for it, we should probably go ahead and make one - as the fix belongs in the system app. Is there one in particular which has a good reproducible test case that we can co-opt?
Flags: needinfo?(felash)
I found the issue when I accidentally broke the "font fit" algorithm in gaia-header (so that some titles end up too long to fit). But I don't have a good STR without modifying the code.

Do you know all the places where chrome-plain is used? Maybe in FTU?
Flags: needinfo?(felash)
(In reply to Julien Wajsberg [:julienw] (PTO April 6th) from comment #26)
> I found the issue when I accidentally broke the "font fit" algorithm in
> gaia-header (so that some titles end up too long to fit). But I don't have a
> good STR without modifying the code.
> 
> Do you know all the places where chrome-plain is used? Maybe in FTU?

.chrome-plain seems to be used for popup windows and trusted windows So yes, FTU is one place we'll see it with the import contacts windows.  

Note, my comment #14 about the relative frequency of combined-chrome vs. not is misleading. We use combined-chrome everywhere you see the rocketbar - which is most apps, including the browser. However, we only show the lock icon when the SSL state is either broken or secure (and the state for app:// is insecure, so we never see if for built-in apps currently)

That was bugging me so wanted to clarify. But it doesnt impact the efficacy and value of the patch so I'll request uplift.
Comment on attachment 8589341 [details] [review]
[gaia] sfoster:bidi-urlbar-bug-1151925 > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Browser/Window Management, RTL
[User impact] if declined: Bidi and RTL title are mishandled and incorrectly truncated in secure hosted apps and browser windows
[Testing completed]: Testing by patch author, reviewer and QA (Naoki, see comment #23) on device
[Risk to taking this patch] (and alternatives if risky): Low but not negligible. The patch touches shared app chrome code that is used by most apps, however the changes are well contained in the AppChrome submodule and its CSS and the testing we've done hasnt thrown up any hint of problems
[String changes made]: None
Attachment #8589341 - Flags: approval-gaia-v2.2?
Attached image v3.0_verify.png — 
This issue has been verified passed on latest build of Flame 3.0 with the same steps in comment 0.
See attachment:v3.0_verify.png
Rate:0/3

Device: Flame 3.0 (pass)
Build ID               20150414160204
Gaia Revision          8e28588496f82f8f069c171c65842d622b9d8d7d
Gaia Date              2015-04-14 18:43:50
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/de27ac2ab94f
Gecko Version          40.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150414.194002
Firmware Date          Tue Apr 14 19:40:12 EDT 2015
Bootloader             L1TC000118D0
QA Whiteboard: [rtl-impact][QAnalyst-Triage+] → [rtl-impact][QAnalyst-Triage+][MGSEI-Triage+]
status-b2g-master: affected → verified
Attachment #8589341 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
Attached image v2.2_verify.png — 
This issue has been verified passed on latest build of Flame 2.2 with the same steps in comment 0.
See attachment:v2.2_verify.png
Rate:0/3

Device: Flame 2.2 (pass)
Build ID               20150419002502
Gaia Revision          c15a2b6d3a783813959c2b3bffd2a131f4270b9e
Gaia Date              2015-04-17 17:49:32
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/cc02ee38b252
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150419.040848
Firmware Date          Sun Apr 19 04:08:59 EDT 2015
Bootloader             L1TC000118D0
Status: RESOLVED → VERIFIED
status-b2g-v2.2: fixed → verified
Keywords: verifyme
Test case has been added in moztrap:
https://moztrap.mozilla.org/manage/case/15466/
Flags: in-moztrap+
Depends on: 1183373
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: