Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2050583002:
BridgedContentView: Modify IsTextRTL to use GetFirstStrongCharacterDirection. (Closed)
|
Created:
4 years, 6 months ago by karandeepb Modified:
4 years, 6 months ago CC:
chromium-reviews, tfarina, James Su, shuchen+watch_chromium.org, yusukes+watch_chromium.org, nona+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBridgedContentView: Modify IsTextRTL to use GetFirstStrongCharacterDirection.
Currently, IsTextRTL method in BridgedContentView uses
base::i18n::GetStringDirection to get the string text direction. This is
inconsistent with RenderText::GetTextDirection. This CL modifies IsTextRTL to
use base::i18n::GetFirstStrongCharacterDirection to ensure consistency with
RenderText::GetTextDirection.
This also necessitates modifying SendHomeEvent and SendEvent in
textfield_unittest.cc, to prevent the tests TextfieldTest.HitOutsideTextAreaTest
and TextfieldTest.HitOutsideTextAreaInRTLTest from regressing on MacViews. This
is because currently, SendHomeEvent and SendEndEvent correspond to logical
direction on other platforms but visual direction on Mac.
The reason that the tests TextfieldTest.HitOutsideTextAreaTest and
TextfieldTest.HitOutsideTextAreaInRTLTest passed up-till now is that the
incorrect IsTextRTL and Send{Home/End}Event implementations cancelled each other
out. IsTextRTL depends on GetStringDirection currently, and hence it always
returned false for text of mixed directionality. A SendHomeEvent would do a
Cmd+Left which would generate a moveToLeftEndOfLine. Since IsTextRTL returned
false, a moveToLeftEndOfLine mapped to a moveToBeginningOfLine in
BridgedContentView. Hence a Send{Home/End}Event mapped to
moveTo{Beginning/End}OfLine, the two incorrect implementations cancelling each
other out.
BUG=618184
Committed: https://crrev.com/e86f927228e135784931a9eb9825cb9ec4509267
Cr-Commit-Position: refs/heads/master@{#398736}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address review comments. #
Messages
Total messages: 22 (11 generated)
Description was changed from ========== IsTextRTL fix. ========== to ========== BridgedContentView: Modify IsTextRTL to use GetFirstStrongCharacterDirection. Currently, IsTextRTL method in BridgedContentView uses base::i18n::GetStringDirection to get the string text direction. This is inconsistent with RenderText::GetTextDirection. This CL modifies IsTextRTL to use base::i18n::GetFirstStrongCharacterDirection to ensure consistency with RenderText::GetTextDirection. This also necessitates modifying SendHomeEvent and SendEvent, which are currently not correct for RTL text on Mac. BUG=None. ==========
Description was changed from ========== BridgedContentView: Modify IsTextRTL to use GetFirstStrongCharacterDirection. Currently, IsTextRTL method in BridgedContentView uses base::i18n::GetStringDirection to get the string text direction. This is inconsistent with RenderText::GetTextDirection. This CL modifies IsTextRTL to use base::i18n::GetFirstStrongCharacterDirection to ensure consistency with RenderText::GetTextDirection. This also necessitates modifying SendHomeEvent and SendEvent, which are currently not correct for RTL text on Mac. BUG=None. ========== to ========== BridgedContentView: Modify IsTextRTL to use GetFirstStrongCharacterDirection. Currently, IsTextRTL method in BridgedContentView uses base::i18n::GetStringDirection to get the string text direction. This is inconsistent with RenderText::GetTextDirection. This CL modifies IsTextRTL to use base::i18n::GetFirstStrongCharacterDirection to ensure consistency with RenderText::GetTextDirection. This also necessitates modifying SendHomeEvent and SendEvent in textfield_unittest.cc, which are currently not correct for RTL text on Mac. BUG=None. ==========
karandeepb@chromium.org changed reviewers: + tapted@chromium.org
PTAL Trent. Thanks.
Can you add a bug to track? It can just be "RTL support for views::Textfield on Mac." Also - I'm curious how you discovered this? Was there a bug? - Does this become obsolete if we get the text direction from TextInputClient? https://codereview.chromium.org/2050583002/diff/1/ui/views/controls/textfield... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2050583002/diff/1/ui/views/controls/textfield... ui/views/controls/textfield/textfield_unittest.cc:521: // Move to beginning of line doesn't have a default key binding on Mac. This is now different to "SendHomeEvent". If the purpose of "SendHomeEvent" in the tests is *actually* just to reset the cursor position, then that should be reflected in the name, like MoveCursorToStart, or MoveCursorToEnd -- there might be a test which should _actually_ be testing a home event (e.g. are there any tests with rtl text? do we need to add one for this to spot a regression?
Description was changed from ========== BridgedContentView: Modify IsTextRTL to use GetFirstStrongCharacterDirection. Currently, IsTextRTL method in BridgedContentView uses base::i18n::GetStringDirection to get the string text direction. This is inconsistent with RenderText::GetTextDirection. This CL modifies IsTextRTL to use base::i18n::GetFirstStrongCharacterDirection to ensure consistency with RenderText::GetTextDirection. This also necessitates modifying SendHomeEvent and SendEvent in textfield_unittest.cc, which are currently not correct for RTL text on Mac. BUG=None. ========== to ========== BridgedContentView: Modify IsTextRTL to use GetFirstStrongCharacterDirection. Currently, IsTextRTL method in BridgedContentView uses base::i18n::GetStringDirection to get the string text direction. This is inconsistent with RenderText::GetTextDirection. This CL modifies IsTextRTL to use base::i18n::GetFirstStrongCharacterDirection to ensure consistency with RenderText::GetTextDirection. This also necessitates modifying SendHomeEvent and SendEvent in textfield_unittest.cc, which are currently not correct for RTL text on Mac. BUG=618184 ==========
>>Can you add a bug to track? It can just be "RTL support for views::Textfield on >>Mac." >>Also >> - I'm curious how you discovered this? Was there a bug? >> - Does this become obsolete if we get the text direction from TextInputClient? Added crbug.com/618184. -I discovered this on removing IsTextRTL from BridgedContentView to use TextInputClient::GetTextDirection. Two tests TextfieldTest.OverflowTest and TextfieldTest.OverflowInRTLTest were failing since SendHomeEvent/SendEndEvent are not RTL agnostic on Mac. We didn't detect this uptill now since our implementation of IsTextRTL is inconsistent with RenderText::GetTextDirection. - The changes in bridged_content_view.mm do, but those in textfield_unittest.cc do not. https://codereview.chromium.org/2050583002/diff/1/ui/views/controls/textfield... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2050583002/diff/1/ui/views/controls/textfield... ui/views/controls/textfield/textfield_unittest.cc:521: // Move to beginning of line doesn't have a default key binding on Mac. On 2016/06/08 04:16:45, tapted wrote: > This is now different to "SendHomeEvent". If the purpose of "SendHomeEvent" in > the tests is *actually* just to reset the cursor position, then that should be > reflected in the name, like MoveCursorToStart, or MoveCursorToEnd -- there might > be a test which should _actually_ be testing a home event (e.g. are there any > tests with rtl text? do we need to add one for this to spot a regression? How is it different to SendHomeEvent? Based on the |shift| parameter it can either just be a move to the beginning of line or a move and select to the beginning of line. There are already tests testing RTL. Currently, TextfieldTest.OverflowTest and TextfieldTest.OverflowInRTLTest both should have failed. But the incorrect implementations of IsTextRTL and SendHomeEvent/SendEndEvent cancelled each other out.
On 2016/06/08 04:56:05, karandeepb wrote: > >>Can you add a bug to track? It can just be "RTL support for views::Textfield > on > >>Mac." > >>Also > >> - I'm curious how you discovered this? Was there a bug? > >> - Does this become obsolete if we get the text direction from > TextInputClient? > > Added crbug.com/618184. > -I discovered this on removing IsTextRTL from BridgedContentView to use > TextInputClient::GetTextDirection. Two tests TextfieldTest.OverflowTest and > TextfieldTest.OverflowInRTLTest were failing since SendHomeEvent/SendEndEvent > are not RTL agnostic on Mac. We didn't detect this uptill now since our > implementation of IsTextRTL is inconsistent with RenderText::GetTextDirection. Can you capture this in the CL description? There's a laundry-list of things a CL description should have -- see https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list... What you have captures "description of previous behavior" and "newly introduced differences", but is missing "why the change is made" and "context if it is part of many changes". I'd suggest something like: Currently, IsTextRTL method in BridgedContentView uses base::i18n::GetStringDirection to get the string text direction. This is inconsistent with RenderText::GetTextDirection. Rather than implementing our own IsTextRTL, we want to switch over to TextInputClient::somethingorother which uses blah, so first we need them to be consistent. Just using GetFirstStrongCharacterDirection causes tests Foo and Bar to fail because <reason>. Fix that by doing X. [optional: This works because <reason>] https://codereview.chromium.org/2050583002/diff/1/ui/views/controls/textfield... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2050583002/diff/1/ui/views/controls/textfield... ui/views/controls/textfield/textfield_unittest.cc:521: // Move to beginning of line doesn't have a default key binding on Mac. On 2016/06/08 04:56:05, karandeepb wrote: > On 2016/06/08 04:16:45, tapted wrote: > > This is now different to "SendHomeEvent". If the purpose of "SendHomeEvent" in > > the tests is *actually* just to reset the cursor position, then that should be > > reflected in the name, like MoveCursorToStart, or MoveCursorToEnd -- there > might > > be a test which should _actually_ be testing a home event (e.g. are there any > > tests with rtl text? do we need to add one for this to spot a regression? > > How is it different to SendHomeEvent? It sends Up. It's sorta coincidence that that goes to the beginning of the line. Whereas Cmd+Left still sounds like it matches the comment (even if it didn't actually). If you don't think it's necessary to change the name of `SendHomeEvent` we should at least describe in the comment whether the "start" means the display-direction start or the reading-direction start. Then, I think it would be clearer to say something like // -[NSResponder moveToBeginningOfLine:] is the correct way to do this on Mac, but that doesn't have a default key binding on Mac. Since views::Textfield doesn't currently support multiple lines, the same effect can be achieved by -[NSResponder moveToBeginningOfDocument:], which is mapped to Cmd+Up. But! Also, what is "home" mapped to? (can we now *actually* use Home now that it [maybe] maps to beginning of line (also)?). (but we should still note that this only works on mac because single-line) > Based on the |shift| parameter it can > either just be a move to the beginning of line or a move and select to the > beginning of line. There are already tests testing RTL. Currently, > TextfieldTest.OverflowTest and TextfieldTest.OverflowInRTLTest both should have > failed. This needs to be mentioned in the CL description. > But the incorrect implementations of IsTextRTL and > SendHomeEvent/SendEndEvent cancelled each other out. OverflowTest/OverflowInRTLTest don't call Send{Home,End}Event (right?) So it's still not clear to me whether changing SendHomeEvent in this way is the right fix, or whether the failing tests are doing something wrong around rtl characters, and we're just hiding another bug by overriding a line-navigation (which should be RTL-aware) with a vertical-navigation (which shouldn't be).
Description was changed from ========== BridgedContentView: Modify IsTextRTL to use GetFirstStrongCharacterDirection. Currently, IsTextRTL method in BridgedContentView uses base::i18n::GetStringDirection to get the string text direction. This is inconsistent with RenderText::GetTextDirection. This CL modifies IsTextRTL to use base::i18n::GetFirstStrongCharacterDirection to ensure consistency with RenderText::GetTextDirection. This also necessitates modifying SendHomeEvent and SendEvent in textfield_unittest.cc, which are currently not correct for RTL text on Mac. BUG=618184 ========== to ========== BridgedContentView: Modify IsTextRTL to use GetFirstStrongCharacterDirection. Currently, IsTextRTL method in BridgedContentView uses base::i18n::GetStringDirection to get the string text direction. This is inconsistent with RenderText::GetTextDirection. This CL modifies IsTextRTL to use base::i18n::GetFirstStrongCharacterDirection to ensure consistency with RenderText::GetTextDirection. This also necessitates modifying SendHomeEvent and SendEvent in textfield_unittest.cc, to prevent the tests TextfieldTest.HitOutsideTextAreaTest and TextfieldTest.HitOutsideTextAreaInRTLTest from regressing on MacViews. This is because currently, SendHomeEvent and SendEndEvent are visual direction agnostic on other platforms but not on Mac. BUG=618184 ==========
Description was changed from
==========
BridgedContentView: Modify IsTextRTL to use GetFirstStrongCharacterDirection.
Currently, IsTextRTL method in BridgedContentView uses
base::i18n::GetStringDirection to get the string text direction. This is
inconsistent with RenderText::GetTextDirection. This CL modifies IsTextRTL to
use base::i18n::GetFirstStrongCharacterDirection to ensure consistency with
RenderText::GetTextDirection.
This also necessitates modifying SendHomeEvent and SendEvent in
textfield_unittest.cc, to prevent the tests TextfieldTest.HitOutsideTextAreaTest
and TextfieldTest.HitOutsideTextAreaInRTLTest from regressing on MacViews. This
is because currently, SendHomeEvent and SendEndEvent are visual direction
agnostic on other platforms but not on Mac.
BUG=618184
==========
to
==========
BridgedContentView: Modify IsTextRTL to use GetFirstStrongCharacterDirection.
Currently, IsTextRTL method in BridgedContentView uses
base::i18n::GetStringDirection to get the string text direction. This is
inconsistent with RenderText::GetTextDirection. This CL modifies IsTextRTL to
use base::i18n::GetFirstStrongCharacterDirection to ensure consistency with
RenderText::GetTextDirection.
This also necessitates modifying SendHomeEvent and SendEvent in
textfield_unittest.cc, to prevent the tests TextfieldTest.HitOutsideTextAreaTest
and TextfieldTest.HitOutsideTextAreaInRTLTest from regressing on MacViews. This
is because currently, SendHomeEvent and SendEndEvent correspond to logical
direction on other platforms but visual direction on Mac.
The reason that the tests TextfieldTest.HitOutsideTextAreaTest and
TextfieldTest.HitOutsideTextAreaInRTLTest passed uptill now is that the
incorrect IsTextRTL and Send{Home/End}Event implementations cancelled each other
out. IsTextRTL depends on GetStringDirection currently, and hence it always
returned false for text of mixed directionality. A SendHomeEvent would do a
Cmd+Left which would generate a moveToLeftEndOfLine. Since IsTextRTL returned
false, a moveToLeftEndOfLine mapped to a moveToBeginningOfLine in
BridgedContentView. Hence a Send{Home/End}Event mapped to
moveTo{Beginning/End}OfLine, the two incorrect implementations cancelling each
other out.
BUG=618184
==========
Description was changed from
==========
BridgedContentView: Modify IsTextRTL to use GetFirstStrongCharacterDirection.
Currently, IsTextRTL method in BridgedContentView uses
base::i18n::GetStringDirection to get the string text direction. This is
inconsistent with RenderText::GetTextDirection. This CL modifies IsTextRTL to
use base::i18n::GetFirstStrongCharacterDirection to ensure consistency with
RenderText::GetTextDirection.
This also necessitates modifying SendHomeEvent and SendEvent in
textfield_unittest.cc, to prevent the tests TextfieldTest.HitOutsideTextAreaTest
and TextfieldTest.HitOutsideTextAreaInRTLTest from regressing on MacViews. This
is because currently, SendHomeEvent and SendEndEvent correspond to logical
direction on other platforms but visual direction on Mac.
The reason that the tests TextfieldTest.HitOutsideTextAreaTest and
TextfieldTest.HitOutsideTextAreaInRTLTest passed uptill now is that the
incorrect IsTextRTL and Send{Home/End}Event implementations cancelled each other
out. IsTextRTL depends on GetStringDirection currently, and hence it always
returned false for text of mixed directionality. A SendHomeEvent would do a
Cmd+Left which would generate a moveToLeftEndOfLine. Since IsTextRTL returned
false, a moveToLeftEndOfLine mapped to a moveToBeginningOfLine in
BridgedContentView. Hence a Send{Home/End}Event mapped to
moveTo{Beginning/End}OfLine, the two incorrect implementations cancelling each
other out.
BUG=618184
==========
to
==========
BridgedContentView: Modify IsTextRTL to use GetFirstStrongCharacterDirection.
Currently, IsTextRTL method in BridgedContentView uses
base::i18n::GetStringDirection to get the string text direction. This is
inconsistent with RenderText::GetTextDirection. This CL modifies IsTextRTL to
use base::i18n::GetFirstStrongCharacterDirection to ensure consistency with
RenderText::GetTextDirection.
This also necessitates modifying SendHomeEvent and SendEvent in
textfield_unittest.cc, to prevent the tests TextfieldTest.HitOutsideTextAreaTest
and TextfieldTest.HitOutsideTextAreaInRTLTest from regressing on MacViews. This
is because currently, SendHomeEvent and SendEndEvent correspond to logical
direction on other platforms but visual direction on Mac.
The reason that the tests TextfieldTest.HitOutsideTextAreaTest and
TextfieldTest.HitOutsideTextAreaInRTLTest passed up-till now is that the
incorrect IsTextRTL and Send{Home/End}Event implementations cancelled each other
out. IsTextRTL depends on GetStringDirection currently, and hence it always
returned false for text of mixed directionality. A SendHomeEvent would do a
Cmd+Left which would generate a moveToLeftEndOfLine. Since IsTextRTL returned
false, a moveToLeftEndOfLine mapped to a moveToBeginningOfLine in
BridgedContentView. Hence a Send{Home/End}Event mapped to
moveTo{Beginning/End}OfLine, the two incorrect implementations cancelling each
other out.
BUG=618184
==========
PTAL Trent. Thanks. https://codereview.chromium.org/2050583002/diff/1/ui/views/controls/textfield... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2050583002/diff/1/ui/views/controls/textfield... ui/views/controls/textfield/textfield_unittest.cc:521: // Move to beginning of line doesn't have a default key binding on Mac. On 2016/06/08 05:59:03, tapted wrote: > On 2016/06/08 04:56:05, karandeepb wrote: > > On 2016/06/08 04:16:45, tapted wrote: > > > This is now different to "SendHomeEvent". If the purpose of "SendHomeEvent" > in > > > the tests is *actually* just to reset the cursor position, then that should > be > > > reflected in the name, like MoveCursorToStart, or MoveCursorToEnd -- there > > might > > > be a test which should _actually_ be testing a home event (e.g. are there > any > > > tests with rtl text? do we need to add one for this to spot a regression? > > > > How is it different to SendHomeEvent? > > It sends Up. It's sorta coincidence that that goes to the beginning of the line. > Whereas Cmd+Left still sounds like it matches the comment (even if it didn't > actually). If you don't think it's necessary to change the name of > `SendHomeEvent` we should at least describe in the comment whether the "start" > means the display-direction start or the reading-direction start. Added a comment to SendHomeEvent/SendEndEvent saying that they map to the logical beginning/end of line. > Then, I think it would be clearer to say something like > > // -[NSResponder moveToBeginningOfLine:] is the correct way to do this on Mac, > but that doesn't have a default key binding on Mac. Since views::Textfield > doesn't currently support multiple lines, the same effect can be achieved by > -[NSResponder moveToBeginningOfDocument:], which is mapped to Cmd+Up. Done > But! Also, what is "home" mapped to? (can we now *actually* use Home now that it > [maybe] maps to beginning of line (also)?). (but we should still note that this > only works on mac because single-line) It doesn't map to the moveToBeginningOfLine. You can verify that by pressing Home in a NSTextField. Also, the tests currently depend on key bindings of the machine on which they are run. I had an idea a while back to use a private API called NSKeyBindingManager(iirc) which should enable us to map key strokes to action messages for use in tests. Let me know if you think it's worth exploring. > > Based on the |shift| parameter it can > > either just be a move to the beginning of line or a move and select to the > > beginning of line. There are already tests testing RTL. Currently, > > TextfieldTest.OverflowTest and TextfieldTest.OverflowInRTLTest both should > have > > failed. > > This needs to be mentioned in the CL description. Done. > > But the incorrect implementations of IsTextRTL and > > SendHomeEvent/SendEndEvent cancelled each other out. > > OverflowTest/OverflowInRTLTest don't call Send{Home,End}Event (right?) > > So it's still not clear to me whether changing SendHomeEvent in this way is the > right fix, or whether the failing tests are doing something wrong around rtl > characters, and we're just hiding another bug by overriding a line-navigation > (which should be RTL-aware) with a vertical-navigation (which shouldn't be). Sorry, I recollected the test names from memory and gave the wrong names. The tests are actually TextfieldTest.HitOutsideTextAreaTest and TextfieldTest.HitOutsideTextAreaInRTLTest. You can read the test code to verify that SendHomeEvent/SendEndEvent are meant to correspond to the logical beginning/end of line. Also modified the CL description to show why the tests passed up-till now.
lgtm - thanks!
karandeepb@chromium.org changed reviewers: + msw@chromium.org
PTAL msw@ for ui/views/controls/textfield/textfield_unittest.cc review.
lgtm
The CQ bit was checked by karandeepb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2050583002/20001
Message was sent while issue was closed.
Description was changed from
==========
BridgedContentView: Modify IsTextRTL to use GetFirstStrongCharacterDirection.
Currently, IsTextRTL method in BridgedContentView uses
base::i18n::GetStringDirection to get the string text direction. This is
inconsistent with RenderText::GetTextDirection. This CL modifies IsTextRTL to
use base::i18n::GetFirstStrongCharacterDirection to ensure consistency with
RenderText::GetTextDirection.
This also necessitates modifying SendHomeEvent and SendEvent in
textfield_unittest.cc, to prevent the tests TextfieldTest.HitOutsideTextAreaTest
and TextfieldTest.HitOutsideTextAreaInRTLTest from regressing on MacViews. This
is because currently, SendHomeEvent and SendEndEvent correspond to logical
direction on other platforms but visual direction on Mac.
The reason that the tests TextfieldTest.HitOutsideTextAreaTest and
TextfieldTest.HitOutsideTextAreaInRTLTest passed up-till now is that the
incorrect IsTextRTL and Send{Home/End}Event implementations cancelled each other
out. IsTextRTL depends on GetStringDirection currently, and hence it always
returned false for text of mixed directionality. A SendHomeEvent would do a
Cmd+Left which would generate a moveToLeftEndOfLine. Since IsTextRTL returned
false, a moveToLeftEndOfLine mapped to a moveToBeginningOfLine in
BridgedContentView. Hence a Send{Home/End}Event mapped to
moveTo{Beginning/End}OfLine, the two incorrect implementations cancelling each
other out.
BUG=618184
==========
to
==========
BridgedContentView: Modify IsTextRTL to use GetFirstStrongCharacterDirection.
Currently, IsTextRTL method in BridgedContentView uses
base::i18n::GetStringDirection to get the string text direction. This is
inconsistent with RenderText::GetTextDirection. This CL modifies IsTextRTL to
use base::i18n::GetFirstStrongCharacterDirection to ensure consistency with
RenderText::GetTextDirection.
This also necessitates modifying SendHomeEvent and SendEvent in
textfield_unittest.cc, to prevent the tests TextfieldTest.HitOutsideTextAreaTest
and TextfieldTest.HitOutsideTextAreaInRTLTest from regressing on MacViews. This
is because currently, SendHomeEvent and SendEndEvent correspond to logical
direction on other platforms but visual direction on Mac.
The reason that the tests TextfieldTest.HitOutsideTextAreaTest and
TextfieldTest.HitOutsideTextAreaInRTLTest passed up-till now is that the
incorrect IsTextRTL and Send{Home/End}Event implementations cancelled each other
out. IsTextRTL depends on GetStringDirection currently, and hence it always
returned false for text of mixed directionality. A SendHomeEvent would do a
Cmd+Left which would generate a moveToLeftEndOfLine. Since IsTextRTL returned
false, a moveToLeftEndOfLine mapped to a moveToBeginningOfLine in
BridgedContentView. Hence a Send{Home/End}Event mapped to
moveTo{Beginning/End}OfLine, the two incorrect implementations cancelling each
other out.
BUG=618184
==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from
==========
BridgedContentView: Modify IsTextRTL to use GetFirstStrongCharacterDirection.
Currently, IsTextRTL method in BridgedContentView uses
base::i18n::GetStringDirection to get the string text direction. This is
inconsistent with RenderText::GetTextDirection. This CL modifies IsTextRTL to
use base::i18n::GetFirstStrongCharacterDirection to ensure consistency with
RenderText::GetTextDirection.
This also necessitates modifying SendHomeEvent and SendEvent in
textfield_unittest.cc, to prevent the tests TextfieldTest.HitOutsideTextAreaTest
and TextfieldTest.HitOutsideTextAreaInRTLTest from regressing on MacViews. This
is because currently, SendHomeEvent and SendEndEvent correspond to logical
direction on other platforms but visual direction on Mac.
The reason that the tests TextfieldTest.HitOutsideTextAreaTest and
TextfieldTest.HitOutsideTextAreaInRTLTest passed up-till now is that the
incorrect IsTextRTL and Send{Home/End}Event implementations cancelled each other
out. IsTextRTL depends on GetStringDirection currently, and hence it always
returned false for text of mixed directionality. A SendHomeEvent would do a
Cmd+Left which would generate a moveToLeftEndOfLine. Since IsTextRTL returned
false, a moveToLeftEndOfLine mapped to a moveToBeginningOfLine in
BridgedContentView. Hence a Send{Home/End}Event mapped to
moveTo{Beginning/End}OfLine, the two incorrect implementations cancelling each
other out.
BUG=618184
==========
to
==========
BridgedContentView: Modify IsTextRTL to use GetFirstStrongCharacterDirection.
Currently, IsTextRTL method in BridgedContentView uses
base::i18n::GetStringDirection to get the string text direction. This is
inconsistent with RenderText::GetTextDirection. This CL modifies IsTextRTL to
use base::i18n::GetFirstStrongCharacterDirection to ensure consistency with
RenderText::GetTextDirection.
This also necessitates modifying SendHomeEvent and SendEvent in
textfield_unittest.cc, to prevent the tests TextfieldTest.HitOutsideTextAreaTest
and TextfieldTest.HitOutsideTextAreaInRTLTest from regressing on MacViews. This
is because currently, SendHomeEvent and SendEndEvent correspond to logical
direction on other platforms but visual direction on Mac.
The reason that the tests TextfieldTest.HitOutsideTextAreaTest and
TextfieldTest.HitOutsideTextAreaInRTLTest passed up-till now is that the
incorrect IsTextRTL and Send{Home/End}Event implementations cancelled each other
out. IsTextRTL depends on GetStringDirection currently, and hence it always
returned false for text of mixed directionality. A SendHomeEvent would do a
Cmd+Left which would generate a moveToLeftEndOfLine. Since IsTextRTL returned
false, a moveToLeftEndOfLine mapped to a moveToBeginningOfLine in
BridgedContentView. Hence a Send{Home/End}Event mapped to
moveTo{Beginning/End}OfLine, the two incorrect implementations cancelling each
other out.
BUG=618184
Committed: https://crrev.com/e86f927228e135784931a9eb9825cb9ec4509267
Cr-Commit-Position: refs/heads/master@{#398736}
==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/e86f927228e135784931a9eb9825cb9ec4509267 Cr-Commit-Position: refs/heads/master@{#398736} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
