Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2175163004:
Add a plain space instead of between text nodes (Closed)
|
Created:
4 years, 5 months ago by joone Modified:
4 years, 4 months ago Reviewers:
yosin_UTC9 CC:
blink-reviews, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a plain space instead of between text nodes
When we rebalance white spaces, can be added as space
under some conditions. This CL adds a condition that the next
sibling text node should not exist.
BUG=310149
TEST=editing/inserting/insert-space.html
Committed: https://crrev.com/8134eeb454ef76c72df8cd7c26f6141072c314cb
Cr-Commit-Position: refs/heads/master@{#407981}
Patch Set 1 #
Total comments: 9
Patch Set 2 : use of assert_selection() and introduce |shouldEmitNBSPbeforeEnd| #
Total comments: 1
Patch Set 3 : don't need to use |Range| #Patch Set 4 : fix a test fail in Win #
Messages
Total messages: 36 (24 generated)
The CQ bit was checked by joone.hur@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
joone.hur@intel.com changed reviewers: + yosin@chromium.org
joone.hur@intel.com changed reviewers: + yosin@chromium.org
Hi yosin@ please review this CL.
Hi yosin@ please review this CL.
Description was changed from ========== Add a plain space instead of between text nodes When we rebalance white spaces, can be added as space in some conditions. This CL adds a confition that the sibling text node should not exist. BUG=310149 TEST=editing/inserting/insert-space.html ========== to ========== Add a plain space instead of between text nodes When we rebalance white spaces, can be added as space under some conditions. This CL adds a confition that the sibling text node should not exist. BUG=310149 TEST=editing/inserting/insert-space.html ==========
Description was changed from ========== Add a plain space instead of between text nodes When we rebalance white spaces, can be added as space under some conditions. This CL adds a confition that the sibling text node should not exist. BUG=310149 TEST=editing/inserting/insert-space.html ========== to ========== Add a plain space instead of between text nodes When we rebalance white spaces, can be added as space under some conditions. This CL adds a condition that the sibling text node should not exist. BUG=310149 TEST=editing/inserting/insert-space.html ==========
https://codereview.chromium.org/2175163004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/editing/inserting/insert-space.html (right): https://codereview.chromium.org/2175163004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/editing/inserting/insert-space.html:18: assert_equals(html_para, '<p id="p1">A B</p>'); Can we use assert_selection()? https://codereview.chromium.org/2175163004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/editing/inserting/insert-space.html:34: assert_equals(html_para, '<p id="p2">A B</p>'); Can we use assert_selection()? https://codereview.chromium.org/2175163004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right): https://codereview.chromium.org/2175163004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:871: if (previousCharacterWasSpace || (!i && startIsStartOfParagraph) || (i + 1 == length && endIsEndOfParagraph && !nextTextSibling)) { Should we insert U+0020 instead of U+00A0 when previous sibling is Text node? https://codereview.chromium.org/2175163004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/EditingUtilities.h (right): https://codereview.chromium.org/2175163004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/EditingUtilities.h:350: String stringWithRebalancedWhitespace(const String&, bool startIsStartOfParagraph, bool endIsEndOfParagraph, bool nextTextSibling = false); nit: to follow other parameter names, |nextSiblingIsTextNode| is better. https://codereview.chromium.org/2175163004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp (right): https://codereview.chromium.org/2175163004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp:738: isEndOfParagraph(visibleDownstreamPos) || (unsigned)downstream == text.length(), How about renaming |endIsEndOfParagraph| to |shouldEmitNBSPbeforeEnd|? If so, we don't need to introduce |nextTextSibling| parameter.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by joone.hur@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by joone.hur@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for review! https://codereview.chromium.org/2175163004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/editing/inserting/insert-space.html (right): https://codereview.chromium.org/2175163004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/editing/inserting/insert-space.html:18: assert_equals(html_para, '<p id="p1">A B</p>'); On 2016/07/25 07:42:22, Yosi_UTC9 wrote: > Can we use assert_selection()? Done. https://codereview.chromium.org/2175163004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/editing/inserting/insert-space.html:34: assert_equals(html_para, '<p id="p2">A B</p>'); On 2016/07/25 07:42:22, Yosi_UTC9 wrote: > Can we use assert_selection()? Done. https://codereview.chromium.org/2175163004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right): https://codereview.chromium.org/2175163004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:871: if (previousCharacterWasSpace || (!i && startIsStartOfParagraph) || (i + 1 == length && endIsEndOfParagraph && !nextTextSibling)) { On 2016/07/25 07:42:23, Yosi_UTC9 wrote: > Should we insert U+0020 instead of U+00A0 when previous sibling is Text node? I'm not sure. In this case, the space is inserted at the end of the first text node. Anyway, we need to find such a case. https://codereview.chromium.org/2175163004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp (right): https://codereview.chromium.org/2175163004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp:738: isEndOfParagraph(visibleDownstreamPos) || (unsigned)downstream == text.length(), On 2016/07/25 07:42:23, Yosi_UTC9 wrote: > How about renaming |endIsEndOfParagraph| to |shouldEmitNBSPbeforeEnd|? > If so, we don't need to introduce |nextTextSibling| parameter. Done.
lgtm w/ small nit https://codereview.chromium.org/2175163004/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/inserting/insert-space.html (right): https://codereview.chromium.org/2175163004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/inserting/insert-space.html:23: selection.addRange(range); We don't need to use |Range|. We could write: selection.collapse(para.firstChild, 1);
lgtm w/ small nit https://codereview.chromium.org/2175163004/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/inserting/insert-space.html (right): https://codereview.chromium.org/2175163004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/inserting/insert-space.html:23: selection.addRange(range); We don't need to use |Range|. We could write: selection.collapse(para.firstChild, 1);
Description was changed from ========== Add a plain space instead of between text nodes When we rebalance white spaces, can be added as space under some conditions. This CL adds a condition that the sibling text node should not exist. BUG=310149 TEST=editing/inserting/insert-space.html ========== to ========== Add a plain space instead of between text nodes When we rebalance white spaces, can be added as space under some conditions. This CL adds a condition that the next sibling text node should not exist. BUG=310149 TEST=editing/inserting/insert-space.html ==========
The CQ bit was checked by joone.hur@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org Link to the patchset: https://codereview.chromium.org/2175163004/#ps60001 (title: "don't need to use |Range|")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by joone.hur@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by joone.hur@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org Link to the patchset: https://codereview.chromium.org/2175163004/#ps80001 (title: "fix a test fail in Win")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add a plain space instead of between text nodes When we rebalance white spaces, can be added as space under some conditions. This CL adds a condition that the next sibling text node should not exist. BUG=310149 TEST=editing/inserting/insert-space.html ========== to ========== Add a plain space instead of between text nodes When we rebalance white spaces, can be added as space under some conditions. This CL adds a condition that the next sibling text node should not exist. BUG=310149 TEST=editing/inserting/insert-space.html ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Add a plain space instead of between text nodes When we rebalance white spaces, can be added as space under some conditions. This CL adds a condition that the next sibling text node should not exist. BUG=310149 TEST=editing/inserting/insert-space.html ========== to ========== Add a plain space instead of between text nodes When we rebalance white spaces, can be added as space under some conditions. This CL adds a condition that the next sibling text node should not exist. BUG=310149 TEST=editing/inserting/insert-space.html Committed: https://crrev.com/8134eeb454ef76c72df8cd7c26f6141072c314cb Cr-Commit-Position: refs/heads/master@{#407981} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/8134eeb454ef76c72df8cd7c26f6141072c314cb Cr-Commit-Position: refs/heads/master@{#407981} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
