|
|
Created:
8 years, 5 months ago by Alexei Svitkine (slow) Modified:
8 years, 5 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionFix 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 #Messages
Total messages: 29 (0 generated)
This LGTM, but please: 1) Add a unit test (maybe okay in followup) 2) add 134746 to BUG= 3) fix the "LTER" typo in TEST= Another question & note I'll dup on the Issue itself: So this worked correctly in M19? This needs testing to avoid further regressions before considering merge.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/asvitkine@chromium.org/10689013/1
Sigh, this causes some cursor test failures: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu... +cc xji
On 2012/06/27 21:49:26, Alexei Svitkine wrote: those tests probably assumed TextDirectionality in Windows. I need to take a closer look at them. Besides the tests, we do not have anywhere in the source code that depends on GetTextDirection()?
xji, any ideas about the cursor failures? It seems the native_textfield_views_unittest.cc failures can be made to pass by getting rid of the ifdef OS_WIN blocks and make it use the same path as Linux. This is not the case for all failing RenderText tests - MoveCursorLeftRightInRtlLtr and MoveCursorLeftRightInRtlLtrRtl fail even after making the code use the Linux ifdef'd paths. But is it even correct to do this? I do not understand those tests. Xiaomei, any suggestions? If the GetTextDirection() change is not correct for cursor movement, I am leaning towards changing fade_head/fade_tail to fade_left/fade_right which would be unambiguous and would not require swapping them in the case of RTL. We would also be able to set them correctly using the same criteria as are used for choosing the alignment direction in canvas_skia.cc. Mike, what do you think?
(To be clear, the ifdef'd code paths I refer to are in the test cases themselves.)
On Wed, Jun 27, 2012 at 6:30 PM, <xji@chromium.org> wrote: > > On 2012/06/27 21:49:26, Alexei Svitkine wrote: > > those tests probably assumed TextDirectionality in Windows. I need to take a > closer look at them. > > Besides the tests, we do not have anywhere in the source code that depends on > GetTextDirection()? It does seem that we do in: RenderText::GetCursorBounds(), RenderText::SelectAll(), RenderText::GetVisualDirectionOfLogicalEnd() ... which is used by RenderText::EdgeSelectionModel() which is used by RenderText::GetAdjacentSelectionModel() which is used by RenderText::MoveCursor() ... so yes.
Using the first strong directionality char (and removing win-specific test code) is probably the correct long-term fix. Can you post the latest patch set with test updates and run trybots to show the remaining failures? It might not be too tough to fix the RenderTextWin cursor movement code/tests (I can help too). Otherwise, fade_[head|tail] -> fade_[left|right] seems lame. Can't canvas_skia use the alignment logic to determine head from tail? If neither of these approaches work, fade_[left|right] seems okay with a TODO to fix the underlying direction and cursor movement problem on Windows. Sorry for the mess, thanks for looking into it...
I do not have build handy. If GetTextDirection() is the same for both platforms, I think the cursor movement should be the same. Maybe the code has some assumption somewhere. (I have not pinpoint anything by looking at the code). If you type the following in Windows "ABCabc" (capital stands for Hebrew), does it display as "abcCBA"? If you move cursor to far right by pressing HOME, then continue press left arrow, does the cursor move to left by one character?
Ah, I think I got it theoritically. We can not simply change GetTextDirection() in RenderText() without changing the underlying text layout. The underlying text direction is still always left-to-right. We will need to set script embedding bidi level according to 1st strong character as well.
On Wed, Jun 27, 2012 at 7:49 PM, <xji@chromium.org> wrote: > Ah, I think I got it theoritically. > > We can not simply change GetTextDirection() in RenderText() without changing > the > underlying text layout. > > The underlying text direction is still always left-to-right. > We will need to set script embedding bidi level according to 1st strong > character as well. Okay, so it sounds like the right short term solution is to base the fading on the text alignment property that we set from canvas_skia.cc until we properly add support for manipulating the bidi level property in Uniscribe. I'll try this tomorrow. -Alexei
> Okay, so it sounds like the right short term solution is to base the > fading on the text alignment property that we set from canvas_skia.cc To be clear: "fade location" based on "text alignment direction". -Alexei
update
I've updated the CL to just base the fade location on the horizontal alignment instead of on GetTextDirection(), with a TODO to change this back once we fix GetTextDirection() in RenderTextWin. I've verified that this works correctly per the TEST= line instructions. Please take another look.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/asvitkine@chromium.org/10689013/18001
On 2012/06/28 17:45:33, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/asvitkine%40chromium.org/10689013/18001 This will *not* work for linux. in linux RTL UI, if the title is pure English, its directionality is ltr, you want both truncation and fading on the right.
On 2012/06/28 18:44:37, xji wrote: > On 2012/06/28 17:45:33, I haz the power (commit-bot) wrote: > > CQ is trying da patch. Follow status at > > https://chromium-status.appspot.com/cq/asvitkine%2540chromium.org/10689013/18001 > > This will *not* work for linux. > in linux RTL UI, if the title is pure English, > its directionality is ltr, you want both truncation and fading on the right. It will work because the canvas_skia.cc makes sure to set the correct directionality.
Sorry, I meant to say canvas_skia.cc sets the text alignment correctly in a platform-specific manner already - and this patch just makes sure to use the same end for fading as was set for the alignment.
On 2012/06/28 18:47:31, Alexei Svitkine wrote: > Sorry, I meant to say canvas_skia.cc sets the text alignment correctly in a > platform-specific manner already - and this patch just makes sure to use the > same end for fading as was set for the alignment. could you please point me the code? thanks.
On 2012/06/28 18:56:25, xji wrote: > On 2012/06/28 18:47:31, Alexei Svitkine wrote: > > Sorry, I meant to say canvas_skia.cc sets the text alignment correctly in a > > platform-specific manner already - and this patch just makes sure to use the > > same end for fading as was set for the alignment. > > could you please point me the code? thanks. So on Windows, tab titles get drawn via DrawFadeTruncatingString() in canvas_skia.cc. On Linux, the regular DrawString() code does fading when eliding is requested, so the code actually ends up going through DrawStringWithShadows() in canvas_skia.cc and there's the following block: #if defined(OS_LINUX) // On Linux, eliding really means fading the end of the string. But only // for LTR text. RTL text is still elided (on the left) with "...". if (elide_text) { render_text->SetText(adjusted_text); if (render_text->GetTextDirection() == base::i18n::LEFT_TO_RIGHT) { render_text->set_fade_tail(true); elide_text = false; } } #endif Which was written to match the previos canvas_linux.cc behavior. Note that in the DrawStringWithShadows(), alignment can be set separately by the caller. But since alignment controls truncation (i.e. the text that overflows will get truncated), the fading should always be on that same side. To complicate things further, the Linux GTK UI (as opposed to the ChromeOS views UI), doesn't always necessarily go through this text drawing code - sometimes it draws code purely using GTK. (In which case it wouldn't be affected by this change.) But this definitely applies to the ChromeOS tab drawing.
On 2012/06/28 19:04:52, Alexei Svitkine wrote: > On 2012/06/28 18:56:25, xji wrote: > > On 2012/06/28 18:47:31, Alexei Svitkine wrote: > > > Sorry, I meant to say canvas_skia.cc sets the text alignment correctly in a > > > platform-specific manner already - and this patch just makes sure to use the > > > same end for fading as was set for the alignment. > > > > could you please point me the code? thanks. > > So on Windows, tab titles get drawn via DrawFadeTruncatingString() in > canvas_skia.cc. > > On Linux, the regular DrawString() code does fading when eliding is requested, > so the code actually ends up going through DrawStringWithShadows() in > canvas_skia.cc and there's the following block: > > #if defined(OS_LINUX) > // On Linux, eliding really means fading the end of the string. But only > // for LTR text. RTL text is still elided (on the left) with "...". > if (elide_text) { > render_text->SetText(adjusted_text); > if (render_text->GetTextDirection() == base::i18n::LEFT_TO_RIGHT) { > render_text->set_fade_tail(true); > elide_text = false; > } > } > #endif > > Which was written to match the previos canvas_linux.cc behavior. > > Note that in the DrawStringWithShadows(), alignment can be set separately by the > caller. But since alignment controls truncation (i.e. the text that overflows > will get truncated), the fading should always be on that same side. > > To complicate things further, the Linux GTK UI (as opposed to the ChromeOS views > UI), doesn't always necessarily go through this text drawing code - sometimes it > draws code purely using GTK. (In which case it wouldn't be affected by this > change.) But this definitely applies to the ChromeOS tab drawing. Thanks for the detailed explanation. A separate note: I just tried tab title of 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.
> 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?
Change committed as 144791
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.
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. |