Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(245)

Issue 10689013: Fix tab title fading direction in RTL locales. (Closed)

Created:
8 years, 5 months ago by Alexei Svitkine (slow)
Modified:
8 years, 5 months ago
Reviewers:
msw, xji
CC:
chromium-reviews
Visibility:
Public.

Description

Fix tab title fading direction in RTL locales. BUG=134759, 134746 TEST=On Windows, launch chrome.exe with --lang=he and go to http://en.wikipedia.org/wiki/Main_Page. Observe that the text is faded on the right. Open another tab and go to http://www.bbc.co.uk/arabic/topics/syria/ and observe that the text is faded on the left. Observe the same is true with LTR locales. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=144791

Patch Set 1 #

Patch Set 2 : update #

Unified diffs Side-by-side diffs Delta from patch set Stats (+4 lines, -1 line) Patch
M ui/gfx/render_text.cc View 1 1 chunk +4 lines, -1 line 0 comments Download

Messages

Total messages: 29 (0 generated)
Alexei Svitkine (slow)
8 years, 5 months ago (2012-06-27 20:24:15 UTC) #1
msw
This LGTM, but please: 1) Add a unit test (maybe okay in followup) 2) add ...
8 years, 5 months ago (2012-06-27 20:36:43 UTC) #2
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/asvitkine@chromium.org/10689013/1
8 years, 5 months ago (2012-06-27 20:46:52 UTC) #3
Alexei Svitkine (slow)
Sigh, this causes some cursor test failures: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&number=41046 +cc xji
8 years, 5 months ago (2012-06-27 21:49:12 UTC) #4
Alexei Svitkine (slow)
8 years, 5 months ago (2012-06-27 21:49:26 UTC) #5
xji
On 2012/06/27 21:49:26, Alexei Svitkine wrote: those tests probably assumed TextDirectionality in Windows. I need ...
8 years, 5 months ago (2012-06-27 22:30:20 UTC) #6
Alexei Svitkine (slow)
xji, any ideas about the cursor failures? It seems the native_textfield_views_unittest.cc failures can be made ...
8 years, 5 months ago (2012-06-27 22:33:45 UTC) #7
Alexei Svitkine (slow)
(To be clear, the ifdef'd code paths I refer to are in the test cases ...
8 years, 5 months ago (2012-06-27 22:34:24 UTC) #8
Alexei Svitkine (slow)
On Wed, Jun 27, 2012 at 6:30 PM, <xji@chromium.org> wrote: > > On 2012/06/27 21:49:26, ...
8 years, 5 months ago (2012-06-27 22:38:47 UTC) #9
msw
Using the first strong directionality char (and removing win-specific test code) is probably the correct ...
8 years, 5 months ago (2012-06-27 22:58:26 UTC) #10
xji
8 years, 5 months ago (2012-06-27 23:04:29 UTC) #11
xji
I do not have build handy. If GetTextDirection() is the same for both platforms, I ...
8 years, 5 months ago (2012-06-27 23:08:44 UTC) #12
xji
Ah, I think I got it theoritically. We can not simply change GetTextDirection() in RenderText() ...
8 years, 5 months ago (2012-06-27 23:49:55 UTC) #13
Alexei Svitkine (slow)
On Wed, Jun 27, 2012 at 7:49 PM, <xji@chromium.org> wrote: > Ah, I think I ...
8 years, 5 months ago (2012-06-28 00:09:46 UTC) #14
Alexei Svitkine (slow)
> Okay, so it sounds like the right short term solution is to base the ...
8 years, 5 months ago (2012-06-28 00:20:12 UTC) #15
Alexei Svitkine (slow)
update
8 years, 5 months ago (2012-06-28 15:42:02 UTC) #16
Alexei Svitkine (slow)
I've updated the CL to just base the fade location on the horizontal alignment instead ...
8 years, 5 months ago (2012-06-28 15:44:10 UTC) #17
msw
LGTM
8 years, 5 months ago (2012-06-28 17:43:48 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/asvitkine@chromium.org/10689013/18001
8 years, 5 months ago (2012-06-28 17:45:33 UTC) #19
xji
On 2012/06/28 17:45:33, I haz the power (commit-bot) wrote: > CQ is trying da patch. ...
8 years, 5 months ago (2012-06-28 18:44:37 UTC) #20
Alexei Svitkine (slow)
On 2012/06/28 18:44:37, xji wrote: > On 2012/06/28 17:45:33, I haz the power (commit-bot) wrote: ...
8 years, 5 months ago (2012-06-28 18:45:52 UTC) #21
Alexei Svitkine (slow)
Sorry, I meant to say canvas_skia.cc sets the text alignment correctly in a platform-specific manner ...
8 years, 5 months ago (2012-06-28 18:47:31 UTC) #22
xji
On 2012/06/28 18:47:31, Alexei Svitkine wrote: > Sorry, I meant to say canvas_skia.cc sets the ...
8 years, 5 months ago (2012-06-28 18:56:25 UTC) #23
Alexei Svitkine (slow)
On 2012/06/28 18:56:25, xji wrote: > On 2012/06/28 18:47:31, Alexei Svitkine wrote: > > Sorry, ...
8 years, 5 months ago (2012-06-28 19:04:52 UTC) #24
xji
On 2012/06/28 19:04:52, Alexei Svitkine wrote: > On 2012/06/28 18:56:25, xji wrote: > > On ...
8 years, 5 months ago (2012-06-28 19:12:48 UTC) #25
Alexei Svitkine (slow)
> A separate note: I just tried tab title of http://amazon.com in my chromeos (not ...
8 years, 5 months ago (2012-06-28 19:16:57 UTC) #26
commit-bot: I haz the power
Change committed as 144791
8 years, 5 months ago (2012-06-28 20:30:38 UTC) #27
Alexei Svitkine (slow)
On 2012/06/28 19:16:57, Alexei Svitkine wrote: > > A separate note: I just tried tab ...
8 years, 5 months ago (2012-06-28 20:44:35 UTC) #28
xji
8 years, 5 months ago (2012-06-28 20:57:33 UTC) #29
On 2012/06/28 20:44:35, Alexei Svitkine wrote:
> On 2012/06/28 19:16:57, Alexei Svitkine wrote:
> > > A separate note: I just tried tab title of http://amazon.com in my
chromeos
> > (not
> > > up-to-date). it seems that the title is correctly faded (on right) but it
> was
> > > truncated at left.
> > 
> > Can you file a bug?
> 
> I've gone ahead and filed it myself: http://crbug.com/135058
> 
> That problem is caused by a different issue.

thanks for filing the bug.
I was building an up-to-date one to test.

Powered by Google App Engine
This is Rietveld 408576698