Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
602423002:
Mixing content editable and non-editable in direction RTL (Closed)
|
Created:
6 years, 2 months ago by Habib Virji Modified:
6 years, 2 months ago CC:
blink-reviews Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionMixing content editable and non-editable in direction RTL
Content editable when mixed in direction rtl, it was not moving caret because editing boundary was not calculated correctly. In RTL scenario, nodes need to be calculated from after or before for left and right respectively. Based on the direction of the block calculates appropriate editing boundary.
R=yosin
BUG=409517
TEST=editing/selection/skip-non-editable-rtl.html
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183843
Patch Set 1 #
Total comments: 8
Patch Set 2 : Added isEnclosingBoxHasLeftToRightDirection in place of directionOfEnclosingBlock #
Total comments: 2
Patch Set 3 : Updated as per codereview comment #
Total comments: 2
Patch Set 4 : Updated as per code review comments #
Total comments: 5
Patch Set 5 : Re-changed back to patch 1 changes #
Messages
Total messages: 32 (5 generated)
Please take a look.
@eae: Can you please take a look.
@eae: Can you please take a look.
habib.virji@samsung.com changed reviewers: + behdad@google.com
leviw@chromium.org changed reviewers: + yosin@chromium.org
It seems text direction is only "LTR" and "RTL"[1]. Is it better to have bool isEnclosingBoxLeftRight(const Position&) instead of TextDirection directionOfEnclosingBlock(const Position&)? [1] http://dev.w3.org/csswg/css-writing-modes/#propdef-direction https://codereview.chromium.org/602423002/diff/1/Source/core/editing/VisibleP... File Source/core/editing/VisiblePosition.cpp (right): https://codereview.chromium.org/602423002/diff/1/Source/core/editing/VisibleP... Source/core/editing/VisiblePosition.cpp:283: // FIXME: This may need to do something different from "before". Q: Can we remove this FIXME by this patch? https://codereview.chromium.org/602423002/diff/1/Source/core/editing/VisibleP... Source/core/editing/VisiblePosition.cpp:284: return directionOfEnclosingBlock(left.deepEquivalent()) == LTR ? honorEditingBoundaryAtOrBefore(left) : honorEditingBoundaryAtOrAfter(left); nit: It is better to use |isLeftToRightDirection(TextDirection)|. https://codereview.chromium.org/602423002/diff/1/Source/core/editing/VisibleP... Source/core/editing/VisiblePosition.cpp:451: // FIXME: This may need to do something different from "after". Q: Can we remove this FIXME by this patch? https://codereview.chromium.org/602423002/diff/1/Source/core/editing/VisibleP... Source/core/editing/VisiblePosition.cpp:452: return directionOfEnclosingBlock(right.deepEquivalent()) == LTR ? honorEditingBoundaryAtOrAfter(right) : honorEditingBoundaryAtOrBefore(right); nit: It is better to use |isLeftToRightDirection(TextDirection)|.
Thanks Yosi, few questions though: 1. I agree with your suggestion regarding isEnclosingBoxLeftRight, but can i rename it as isEnclosingBoxHasLeftToRightDirection, it will return true if LTR or else false if RTL. 2. Regarding: Source/core/editing/VisiblePosition.cpp:284: do you mean should use isLeftToRightDirection or isEnclosingBoxHasLeftToRightDirection, here as box is not defined in VisiblePosition::right/left, to get property isLeftToRightDirection will have to implement something similar to directionOfEnclosingBlock in both VisiblePosition::right/left. > directionOfEnclosingBlock(left.deepEquivalent()) == LTR ? > honorEditingBoundaryAtOrBefore(left) : honorEditingBoundaryAtOrAfter(left); > nit: It is better to use |isLeftToRightDirection(TextDirection)|. On 2014/09/29 01:29:03, Yosi_UTC9 wrote: > It seems text direction is only "LTR" and "RTL"[1]. Is it better to have bool > isEnclosingBoxLeftRight(const Position&) instead of TextDirection > directionOfEnclosingBlock(const Position&)? > > [1] http://dev.w3.org/csswg/css-writing-modes/#propdef-direction > > https://codereview.chromium.org/602423002/diff/1/Source/core/editing/VisibleP... > File Source/core/editing/VisiblePosition.cpp (right): > > https://codereview.chromium.org/602423002/diff/1/Source/core/editing/VisibleP... > Source/core/editing/VisiblePosition.cpp:283: // FIXME: This may need to do > something different from "before". > Q: Can we remove this FIXME by this patch? > > https://codereview.chromium.org/602423002/diff/1/Source/core/editing/VisibleP... > Source/core/editing/VisiblePosition.cpp:284: return > directionOfEnclosingBlock(left.deepEquivalent()) == LTR ? > honorEditingBoundaryAtOrBefore(left) : honorEditingBoundaryAtOrAfter(left); > nit: It is better to use |isLeftToRightDirection(TextDirection)|. > > https://codereview.chromium.org/602423002/diff/1/Source/core/editing/VisibleP... > Source/core/editing/VisiblePosition.cpp:451: // FIXME: This may need to do > something different from "after". > Q: Can we remove this FIXME by this patch? > > https://codereview.chromium.org/602423002/diff/1/Source/core/editing/VisibleP... > Source/core/editing/VisiblePosition.cpp:452: return > directionOfEnclosingBlock(right.deepEquivalent()) == LTR ? > honorEditingBoundaryAtOrAfter(right) : honorEditingBoundaryAtOrBefore(right); > nit: It is better to use |isLeftToRightDirection(TextDirection)|.
On 2014/09/29 09:37:25, Habib Virji wrote: > Thanks Yosi, few questions though: > > 1. I agree with your suggestion regarding isEnclosingBoxLeftRight, but can i > rename it as isEnclosingBoxHasLeftToRightDirection, it will return true if LTR > or else false if RTL. Agree, |isEnclosingBoxHasLeftToRightDirection| is better name. > 2. Regarding: Source/core/editing/VisiblePosition.cpp:284: do you mean should > use isLeftToRightDirection or isEnclosingBoxHasLeftToRightDirection, here as box > is not defined in VisiblePosition::right/left, to get property > isLeftToRightDirection will have to implement something similar to > directionOfEnclosingBlock in both VisiblePosition::right/left. > Sorry for confusion, I mean use |isEnclosingBoxHasLeftToRightDirection(right.deepEquivalen())| rather than isLeftToRightDirection(directionOfEnclosingBlock(right.deepEquivalent()))|
Thanks Yosi, updated as suggested. https://codereview.chromium.org/602423002/diff/1/Source/core/editing/VisibleP... File Source/core/editing/VisiblePosition.cpp (right): https://codereview.chromium.org/602423002/diff/1/Source/core/editing/VisibleP... Source/core/editing/VisiblePosition.cpp:283: // FIXME: This may need to do something different from "before". On 2014/09/29 01:29:03, Yosi_UTC9 wrote: > Q: Can we remove this FIXME by this patch? Done. https://codereview.chromium.org/602423002/diff/1/Source/core/editing/VisibleP... Source/core/editing/VisiblePosition.cpp:284: return directionOfEnclosingBlock(left.deepEquivalent()) == LTR ? honorEditingBoundaryAtOrBefore(left) : honorEditingBoundaryAtOrAfter(left); On 2014/09/29 01:29:03, Yosi_UTC9 wrote: > nit: It is better to use |isLeftToRightDirection(TextDirection)|. Done. https://codereview.chromium.org/602423002/diff/1/Source/core/editing/VisibleP... Source/core/editing/VisiblePosition.cpp:451: // FIXME: This may need to do something different from "after". On 2014/09/29 01:29:03, Yosi_UTC9 wrote: > Q: Can we remove this FIXME by this patch? Done. https://codereview.chromium.org/602423002/diff/1/Source/core/editing/VisibleP... Source/core/editing/VisiblePosition.cpp:452: return directionOfEnclosingBlock(right.deepEquivalent()) == LTR ? honorEditingBoundaryAtOrAfter(right) : honorEditingBoundaryAtOrBefore(right); On 2014/09/29 01:29:03, Yosi_UTC9 wrote: > nit: It is better to use |isLeftToRightDirection(TextDirection)|. Done.
On 2014/09/29 11:13:43, Habib Virji wrote: > Thanks Yosi, updated as suggested. > > https://codereview.chromium.org/602423002/diff/1/Source/core/editing/VisibleP... > File Source/core/editing/VisiblePosition.cpp (right): > > https://codereview.chromium.org/602423002/diff/1/Source/core/editing/VisibleP... > Source/core/editing/VisiblePosition.cpp:283: // FIXME: This may need to do > something different from "before". > On 2014/09/29 01:29:03, Yosi_UTC9 wrote: > > Q: Can we remove this FIXME by this patch? > > Done. > > https://codereview.chromium.org/602423002/diff/1/Source/core/editing/VisibleP... > Source/core/editing/VisiblePosition.cpp:284: return > directionOfEnclosingBlock(left.deepEquivalent()) == LTR ? > honorEditingBoundaryAtOrBefore(left) : honorEditingBoundaryAtOrAfter(left); > On 2014/09/29 01:29:03, Yosi_UTC9 wrote: > > nit: It is better to use |isLeftToRightDirection(TextDirection)|. > > Done. > > https://codereview.chromium.org/602423002/diff/1/Source/core/editing/VisibleP... > Source/core/editing/VisiblePosition.cpp:451: // FIXME: This may need to do > something different from "after". > On 2014/09/29 01:29:03, Yosi_UTC9 wrote: > > Q: Can we remove this FIXME by this patch? > > Done. > > https://codereview.chromium.org/602423002/diff/1/Source/core/editing/VisibleP... > Source/core/editing/VisiblePosition.cpp:452: return > directionOfEnclosingBlock(right.deepEquivalent()) == LTR ? > honorEditingBoundaryAtOrAfter(right) : honorEditingBoundaryAtOrBefore(right); > On 2014/09/29 01:29:03, Yosi_UTC9 wrote: > > nit: It is better to use |isLeftToRightDirection(TextDirection)|. > > Done. @yosi are changes okay?
yosin@google.com changed reviewers: + yosin@google.com
https://codereview.chromium.org/602423002/diff/20001/Source/core/editing/html... File Source/core/editing/htmlediting.cpp (right): https://codereview.chromium.org/602423002/diff/20001/Source/core/editing/html... Source/core/editing/htmlediting.cpp:360: return renderer ? (renderer->style()->direction() == LTR) : true; nit: We don't need to use "?" operator here. We could write: return !renderer || render->style()->direction == LTR
Thanks. Updated as suggested. https://codereview.chromium.org/602423002/diff/20001/Source/core/editing/html... File Source/core/editing/htmlediting.cpp (right): https://codereview.chromium.org/602423002/diff/20001/Source/core/editing/html... Source/core/editing/htmlediting.cpp:360: return renderer ? (renderer->style()->direction() == LTR) : true; On 2014/10/01 08:00:11, yosin wrote: > nit: We don't need to use "?" operator here. We could write: > return !renderer || render->style()->direction == LTR Done.
@yosin: can you please take a look if changes are okay?
habib.virji@samsung.com changed reviewers: + yosin@chromium.org - yosin@google.com
On 2014/10/05 19:36:21, Habib Virji wrote: > @yosin: can you please take a look if changes are okay?
https://codereview.chromium.org/602423002/diff/40001/Source/core/editing/Visi... File Source/core/editing/VisibleUnits.cpp (right): https://codereview.chromium.org/602423002/diff/40001/Source/core/editing/Visi... Source/core/editing/VisibleUnits.cpp:354: bool blockDirection = isEnclosingBoxHasLeftToRightDirection(visiblePosition.deepEquivalent()); nit: Please rename |blockDirection| to |isLeftToRight| or something appropriate.
Thanks updated as suggested. https://codereview.chromium.org/602423002/diff/40001/Source/core/editing/Visi... File Source/core/editing/VisibleUnits.cpp (right): https://codereview.chromium.org/602423002/diff/40001/Source/core/editing/Visi... Source/core/editing/VisibleUnits.cpp:354: bool blockDirection = isEnclosingBoxHasLeftToRightDirection(visiblePosition.deepEquivalent()); On 2014/10/08 08:32:51, Yosi_UTC9 wrote: > nit: Please rename |blockDirection| to |isLeftToRight| or something appropriate. Done.
Code changes and tests are fine to me. Please wait RTL experts comment.
On 2014/10/09 00:55:51, Yosi_UTC9 wrote: > Code changes and tests are fine to me. > Please wait RTL experts comment. @leviw/eae: any suggestions?
This is something that levi needs to review.
Your change to the helper method in htmlediting doesn't make sense. https://codereview.chromium.org/602423002/diff/60001/Source/core/editing/Visi... File Source/core/editing/VisiblePosition.cpp (right): https://codereview.chromium.org/602423002/diff/60001/Source/core/editing/Visi... Source/core/editing/VisiblePosition.cpp:283: return isEnclosingBoxHasLeftToRightDirection(left.deepEquivalent()) ? honorEditingBoundaryAtOrBefore(left) : honorEditingBoundaryAtOrAfter(left); Use the old method and do your bool check here... https://codereview.chromium.org/602423002/diff/60001/Source/core/editing/html... File Source/core/editing/htmlediting.cpp (right): https://codereview.chromium.org/602423002/diff/60001/Source/core/editing/html... Source/core/editing/htmlediting.cpp:354: bool isEnclosingBoxHasLeftToRightDirection(const Position& position) Why are you changing this function? isEnclosingBoxHasLeftToRightDirection isn't proper english.
https://codereview.chromium.org/602423002/diff/60001/Source/core/editing/html... File Source/core/editing/htmlediting.cpp (right): https://codereview.chromium.org/602423002/diff/60001/Source/core/editing/html... Source/core/editing/htmlediting.cpp:354: bool isEnclosingBoxHasLeftToRightDirection(const Position& position) On 2014/10/14 23:04:40, leviw wrote: > Why are you changing this function? isEnclosingBoxHasLeftToRightDirection isn't > proper english. Since |TextDirection| has two values, |LTR| and |RTL| only. I'm not sure why we write |direction() == LTR| rather than |isLTR()|. leviw@, Could you explain? I guess it relates RTL handling. Thanks in advance.
On 2014/10/15 at 00:55:22, yosin wrote: > https://codereview.chromium.org/602423002/diff/60001/Source/core/editing/html... > File Source/core/editing/htmlediting.cpp (right): > > https://codereview.chromium.org/602423002/diff/60001/Source/core/editing/html... > Source/core/editing/htmlediting.cpp:354: bool isEnclosingBoxHasLeftToRightDirection(const Position& position) > On 2014/10/14 23:04:40, leviw wrote: > > Why are you changing this function? isEnclosingBoxHasLeftToRightDirection isn't > > proper english. > > Since |TextDirection| has two values, |LTR| and |RTL| only. I'm not sure why we write |direction() == LTR| rather than |isLTR()|. > > leviw@, Could you explain? I guess it relates RTL handling. > Thanks in advance. - The grammar of the new method is wrong. - It's more explicit at the callsites what we're dealing with by using TextDirection. The new FrameSelection::directionOfEnclosingBlock() code is just un-doing the work from htmlediting.
On 2014/10/15 01:05:25, leviw wrote: > On 2014/10/15 at 00:55:22, yosin wrote: > > > https://codereview.chromium.org/602423002/diff/60001/Source/core/editing/html... > > File Source/core/editing/htmlediting.cpp (right): > > > > > https://codereview.chromium.org/602423002/diff/60001/Source/core/editing/html... > > Source/core/editing/htmlediting.cpp:354: bool > isEnclosingBoxHasLeftToRightDirection(const Position& position) > > On 2014/10/14 23:04:40, leviw wrote: > > > Why are you changing this function? isEnclosingBoxHasLeftToRightDirection > isn't > > > proper english. > > > > Since |TextDirection| has two values, |LTR| and |RTL| only. I'm not sure why > we write |direction() == LTR| rather than |isLTR()|. > > > > leviw@, Could you explain? I guess it relates RTL handling. > > Thanks in advance. > > - The grammar of the new method is wrong. > - It's more explicit at the callsites what we're dealing with by using > TextDirection. The new FrameSelection::directionOfEnclosingBlock() code is just > un-doing the work from htmlediting. Thanks for explanation. My brain was dominated by |isRTL()|. Habib Virji, sorry for miss leading. m(_ _)m
Thanks Yosin and Levi. @leviw: Can you please review if the changes are okay. It is basically rolled back to my patch 1. https://codereview.chromium.org/602423002/diff/60001/Source/core/editing/Visi... File Source/core/editing/VisiblePosition.cpp (right): https://codereview.chromium.org/602423002/diff/60001/Source/core/editing/Visi... Source/core/editing/VisiblePosition.cpp:283: return isEnclosingBoxHasLeftToRightDirection(left.deepEquivalent()) ? honorEditingBoundaryAtOrBefore(left) : honorEditingBoundaryAtOrAfter(left); On 2014/10/14 23:04:40, leviw wrote: > Use the old method and do your bool check here... Done. https://codereview.chromium.org/602423002/diff/60001/Source/core/editing/html... File Source/core/editing/htmlediting.cpp (right): https://codereview.chromium.org/602423002/diff/60001/Source/core/editing/html... Source/core/editing/htmlediting.cpp:354: bool isEnclosingBoxHasLeftToRightDirection(const Position& position) On 2014/10/14 23:04:40, leviw wrote: > Why are you changing this function? isEnclosingBoxHasLeftToRightDirection isn't > proper english. Done.
The CQ bit was checked by leviw@chromium.org
LGTM.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/602423002/90001
Message was sent while issue was closed.
Committed patchset #5 (id:90001) as 183843 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
