New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 134746 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2012
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug

Restricted
  • Only users with Commit permission may comment.



Sign in to add a comment

NativeTextfieldViews/RenderText (--enable-views-textfield) should support arbitrary base text directions.

Project Member Reported by msw@chromium.org, Jun 26 2012

Issue description

RenderTextWin::script_state_ should be initialized properly; see:
http://msdn.microsoft.com/en-us/library/windows/desktop/dd374043

"SCRIPT_STATE.uBidiLevel - Embedding level associated with all characters in the associated run according to the Unicode bidirectional algorithm. When the application passes this structure to ScriptItemize, this member should be initialized to 0 for a left-to-right base embedding level, or to 1 for a right-to-left base embedding level."

Currently, this value is always initialized to zero :(
This causes odd input symptoms, like (uppercase == RTL, '|' == cursor):
Typing [A] yields "|A", then [space] yields "A |", then [B] yields "|B A".

Use the directionality of the UI or the 1st strong LTR/RTL character (like Linux/CrOS).
 
Also see AdjustStringDirection() in canvas_skia.cc.

Ideally, this should be exposed as a setter on the RenderText as well (to override the default, if desired), which could allow us to remove the text directionality character wrapping code in AdjustStringDirection() in favor of the setter.

Comment 2 by msw@chromium.org, Jun 26 2012

Yup, and exposing a setter will likely be required to fix related Issue 134752.
Project Member

Comment 3 by bugdroid1@chromium.org, Jun 28 2012

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=144791

------------------------------------------------------------------------
r144791 | asvitkine@chromium.org | Thu Jun 28 13:30:36 PDT 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/trunk/src/ui/gfx/render_text.cc?r1=144791&r2=144790&pathrev=144791

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.

Review URL: https://chromiumcodereview.appspot.com/10689013
------------------------------------------------------------------------
Project Member

Comment 4 by bugdroid1@chromium.org, Jun 29 2012

Labels: merge-merged-1180
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=144983

------------------------------------------------------------------------
r144983 | asvitkine@chromium.org | Fri Jun 29 13:55:42 PDT 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/branches/1180/src/ui/gfx/render_text.cc?r1=144983&r2=144982&pathrev=144983

Merge 144791 - 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.

Review URL: https://chromiumcodereview.appspot.com/10689013

TBR=asvitkine@chromium.org
Review URL: https://chromiumcodereview.appspot.com/10702047
------------------------------------------------------------------------
Labels: -merge-merged-1180
Removing merge label from this bug since that CL is only tangetially related and does not actually fix the issue.
Project Member

Comment 6 by bugdroid1@chromium.org, Jul 3 2012

Labels: merge-merged-1132
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=145353

------------------------------------------------------------------------
r145353 | asvitkine@chromium.org | Tue Jul 03 12:19:28 PDT 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/branches/1132/src/ui/gfx/render_text.cc?r1=145353&r2=145352&pathrev=145353

Merge 144791 - 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.

Review URL: https://chromiumcodereview.appspot.com/10689013

TBR=asvitkine@chromium.org
Review URL: https://chromiumcodereview.appspot.com/10694073
------------------------------------------------------------------------

Comment 7 by msw@chromium.org, Jul 3 2012

Labels: -merge-merged-1132
Status: Started
Removing merged label for a tangentially related fix.
Project Member

Comment 8 by bugdroid1@chromium.org, Jul 4 2012

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=145476

------------------------------------------------------------------------
r145476 | msw@chromium.org | Wed Jul 04 11:28:59 PDT 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/trunk/src/ui/views/controls/textfield/native_textfield_views_unittest.cc?r1=145476&r2=145475&pathrev=145476
 M http://src.chromium.org/viewvc/chrome/trunk/src/ui/gfx/canvas_skia.cc?r1=145476&r2=145475&pathrev=145476
 M http://src.chromium.org/viewvc/chrome/trunk/src/ui/gfx/render_text_unittest.cc?r1=145476&r2=145475&pathrev=145476
 M http://src.chromium.org/viewvc/chrome/trunk/src/ui/gfx/render_text_win.cc?r1=145476&r2=145475&pathrev=145476
 M http://src.chromium.org/viewvc/chrome/trunk/src/ui/views/controls/textfield/textfield_views_model_unittest.cc?r1=145476&r2=145475&pathrev=145476

Fix RenderTextWin base direction; remove test exceptions; etc.

Use the first strong character's direction to set SCRIPT_STATE.uBidiLevel.
RenderTextWin::GetTextDirection returns this calculated base direction.

Fix RenderTextWin::AdjacentCharSelectionModel:
Handle all out-of-bounds run indicies as invalid runs.
Use the visual ordering of runs (implicit on Linux) to determine edges.

Add new RenderTextTest.GetTextDirection unit test.
Add SetRTL test utility function which works with GTK.
Fix RenderTextTest.DisplayRectShowsCursorRTL on linux with SetRTL.
Remove relevant Windows-specific unit test exceptions.

TODO(msw): Cleanup canvas_skia.cc in a followup CL.
TODO(msw): Use UI dir by default in GetFirstStrongCharacterDirection.

BUG= 134746 , 134009 
TEST=Updated unit tests; no RTL/LTR regressions.
TBR=sky@chromium.org

Review URL: https://chromiumcodereview.appspot.com/10693061
------------------------------------------------------------------------

Comment 9 by msw@chromium.org, Jul 10 2012

I believe that we should remove all of our code that forces directionality.
Text now has a base directionality matching its first strong directional character.
We should only force alignment, fading, and eliding as needed, not directionality.

I've prepared a CL which does this at http://codereview.chromium.org/10695101
The attached pictures shows the effects in LTR locales (RTL locales are unchanged).
My change seems beneficial, showing the beginning of RTL text that was previously cut off weird.
Feel free to weigh in if you think this is a negative change.
force_direction_b_rtl.png
98.6 KB View Download
force_direction_b_ltr.png
113 KB View Download

Comment 10 by xji@chromium.org, Jul 10 2012

Cc: jeremy@chromium.org aharon.l...@gmail.com
++jeremy, aharon.

I think it is a good approach to auto-detect text's directionality based on first strong directionality character, instead of always using UI's dir.
But this changes our UI, and it would be better to have our native speakers to take a look.

A couple of notes:
1. FORCE_RTL/LTR_DIRECTIONALITY were originally introduced to write JS messages based on first strong directionality character. Those messages' dir are auto-detected, and they align based on *first* paragraph's dir (http://code.google.com/p/chromium/issues/detail?id=70806#c1). If we remove force dir and auto dir flag, we need to introduce an alignment flag so that JS message's alignment is not flipped around like UI strings.
(in Linux, JS message's dir and alignment are in sync, if we'd like to have the same behavior, we will be fine.)

2. the change does not affect RTL UI is because in RTL UI, we always wrap text with unicode bidi marks to force them to display as RTL (except there is not RTL characters inside the text). I think we should remove this part of the code as well (not necessarily in the same patch).

3. We have a bug (http://code.google.com/p/chromium/issues/detail?id=27094) about display page title according to page's directionality (not auto-detect). If I remember correctly, we were adding unicode bidi marks to page title to force its directionality. It broke several components (such as extension) which require the original page title. We might could keep the title text unchanged but using force directionality flag for display purpose. Then, the force dir flag will be needed again.

4. about attachment force_direction_b_ltr.png, why the title "ABCDEFG -- google search" truncated to be "ch -- GFEDCBA"? should it be "go -- GFEDCBA"? What is the url for last tab?

Comment 11 Deleted

Sorry, I need to look at this more closely, please allow me another day or so to respond...
Note being fluent with Chromum or WebKit code, I am not sure where 	NativeTextfieldViews/RenderText is used. The screenshots indicate page titles, but there are probably other places. What are they?

The right approach to use is to set the direction when it is known or has been specified, and to use estimation (first strong) otherwise.

For page titles, the direction is specified by the <title> element's dir attribute (set explicitly or inherited from <head> or <html>; if not specified on any, it is LTR). Of course, it can be <title dir=auto>, in which we have to estimate. This is what the HTML5 spec says and it is what should get implemented.
I don't have anything to add past what Aharon wrote.

Comment 15 by msw@chromium.org, Aug 1 2012

Here are screenshots showing the impact on the tab strip and bookmark bar of:
http://codereview.chromium.org/10807082/ Patch Set 9
Note the difference: LTR Chrome UI shows the leading text of RTL tab titles (seems good)!
base_direction_j_cros_ltr.png
140 KB View Download
base_direction_j_win_ltr.png
50.9 KB View Download
base_direction_j_cros_rtl.png
131 KB View Download
base_direction_j_win_rtl.png
45.0 KB View Download

Comment 16 by msw@chromium.org, Aug 1 2012

Labels: OS-Chrome Internals-Views
Mike: I think the changes in comment 15 make sense, but let's file a separate bug to track that behavior change and make sure we get buy-in from i18n folks.

I believe the reason we didn't do this before is the concern that wrapping with directionality chars may add more boxes to the titles - i.e. we were limited by the implementation at the time. But let's double check with the i18n folks.

Comment 18 by msw@chromium.org, Aug 1 2012

Summary: NativeTextfieldViews/RenderText (--enable-views-textfield) should support arbitrary base text directions.
Ok, this bug now just tracks the underlying support for arbitrary base text directions. 
I'll file a follow-up issue for behavioral changes and hold off on canvas_skia changes for now.

Comment 19 by xji@chromium.org, Aug 1 2012

Jeremy, Aharon,

Could you please take a look at snapshots Mike attached in comment #15?

Comment 20 by msw@chromium.org, Aug 1 2012

Don't bother, I'm backing off on any behavioral changes within this issue...
I'll file another issue to match the HTML5 spec as Aharon mentioned in comment #13.
Project Member

Comment 21 by bugdroid1@chromium.org, Aug 1 2012

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=149503

------------------------------------------------------------------------
r149503 | msw@chromium.org | 2012-08-01T21:57:31.016063Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/ui/gfx/render_text_unittest.cc?r1=149503&r2=149502&pathrev=149503
   A http://src.chromium.org/viewvc/chrome/trunk/src/ui/gfx/text_constants.h?r1=149503&r2=149502&pathrev=149503
   M http://src.chromium.org/viewvc/chrome/trunk/src/ui/gfx/render_text.cc?r1=149503&r2=149502&pathrev=149503
   M http://src.chromium.org/viewvc/chrome/trunk/src/ui/gfx/render_text.h?r1=149503&r2=149502&pathrev=149503
   M http://src.chromium.org/viewvc/chrome/trunk/src/ui/views/controls/textfield/textfield_views_model.cc?r1=149503&r2=149502&pathrev=149503
   M http://src.chromium.org/viewvc/chrome/trunk/src/ui/views/controls/textfield/textfield_views_model.h?r1=149503&r2=149502&pathrev=149503
   M http://src.chromium.org/viewvc/chrome/trunk/src/ui/gfx/render_text_win.cc?r1=149503&r2=149502&pathrev=149503
   M http://src.chromium.org/viewvc/chrome/trunk/src/ui/gfx/pango_util.cc?r1=149503&r2=149502&pathrev=149503
   M http://src.chromium.org/viewvc/chrome/trunk/src/ui/gfx/render_text_linux.cc?r1=149503&r2=149502&pathrev=149503
   M http://src.chromium.org/viewvc/chrome/trunk/src/ui/gfx/render_text_win.h?r1=149503&r2=149502&pathrev=149503
   M http://src.chromium.org/viewvc/chrome/trunk/src/ui/gfx/pango_util.h?r1=149503&r2=149502&pathrev=149503
   M http://src.chromium.org/viewvc/chrome/trunk/src/ui/gfx/render_text_mac.cc?r1=149503&r2=149502&pathrev=149503
   M http://src.chromium.org/viewvc/chrome/trunk/src/ui/ui.gyp?r1=149503&r2=149502&pathrev=149503
   M http://src.chromium.org/viewvc/chrome/trunk/src/ui/gfx/render_text_linux.h?r1=149503&r2=149502&pathrev=149503
   M http://src.chromium.org/viewvc/chrome/trunk/src/ui/gfx/render_text_mac.h?r1=149503&r2=149502&pathrev=149503
   M http://src.chromium.org/viewvc/chrome/trunk/src/ui/views/controls/textfield/native_textfield_views.cc?r1=149503&r2=149502&pathrev=149503

Add RenderText DirectionalityMode enum and support; etc.

Add gfx::DirectionalityMode enum.
Add RenderText::SetDirectionalityMode and member.
Revise/consolidate RenderText::GetTextDirection.
Expand on existing unit tests; minor cleanup.

Consume GetTextDirection in layout initialization:
-Windows: RenderTextWin::ItemizeLogicalText()
-Linux: SetupPangoLayoutWithoutFont()
-Mac: Add TODO in RenderTextMac::EnsureLayout()

BUG= 134746 
TEST=Existing/updated unit tests, no behavioral changes!

Review URL: https://chromiumcodereview.appspot.com/10807082
------------------------------------------------------------------------

Comment 22 by msw@chromium.org, Aug 28 2012

Status: Fixed
The underlying support for arbitrary base text directions is in place. Behavioral changes are tracked separately.
Do the changes made include the side on which the visual truncation of tab titles is done. Currently, the tabs get truncated on the end side relative to the Chromium UI direction. They need to get truncated on the end side relative to the tab title direction.

Comment 24 by msw@chromium.org, Aug 29 2012

No, this change was purely providing some underlying capabilities.
I filed  Issue 145418  to track that change, please correct/add details there.
Labels: -OS-Chrome
Verified for 2723.82.0 dev
Project Member

Comment 26 by bugdroid1@chromium.org, Oct 15 2012

Labels: Restrict-AddIssueComment-Commit
This issue has been closed for some time. No one will pay attention to new comments.
If you are seeing this bug or have new data, please click New Issue to start a new bug.
Project Member

Comment 27 by bugdroid1@chromium.org, Mar 10 2013

Labels: -Area-UI -Feature-TextInput -Feature-Omnibox -Internals-Views Cr-UI Cr-Internals-Views Cr-UI-Browser-Omnibox Cr-UI-Input-Text-IME

Sign in to add a comment