Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2757563004:
Adjust insertion point when listifying unordered list items (Closed)
|
Created:
3 years, 9 months ago by jfernandez Modified:
3 years, 9 months ago CC:
blink-reviews, chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdjust insertion point when listifying unordered list items
During the process of listifying a selection of unordered lists items
the insertion point of the first element is incorretcly selected. The
issue appears when the selection start at an intermediate list item and
ending in a different list item.
Since the process is defined to act on single paragraphs, the firts item
splits the list first and then it removes the intermediate item. This
logic, implemented in 'unlistifyParagraph', causes that 2 independent
lists are created with a <span> element in the middle.
Then, an algorithm implemented in 'listifyParagraph' looks for an
insertion point for the new ordered list item. Since there is a previous
sibling which represents a 'visually distinct position', the <span>
temporary created during the 'unlistifyParagraph' logic is selected.
The incorrectly selected insertion point leads to the subsequent
doApplyForSingleParagraph executions to consider the rest of the list
items in the selection to be part of a different ordered list.
BUG=702792
Review-Url: https://codereview.chromium.org/2757563004
Cr-Commit-Position: refs/heads/master@{#459442}
Committed: https://chromium.googlesource.com/chromium/src/+/ce9fb22d90743b6b08b1a1baa00202d781673269
Patch Set 1 #Patch Set 2 : Rebaseline test expectations. #Patch Set 3 : Added regression test. #
Total comments: 4
Patch Set 4 : Applied suggested changes. #
Total comments: 2
Patch Set 5 : Patch for landing, after applying last suggestions. #Patch Set 6 : Patch rebased #
Messages
Total messages: 18 (10 generated)
Description was changed from ========== Adjust insertion point when listifying unordered list items During the process of listifying a selection of unordered lists items the insertion point of the first element is incorretcly selected. The issue appears when the selection start at an intermediate list item and ending in a different list item. Since the process is defined to act on single paragraphs, the firts item splits the list first and then it removes the intermediate item. This logic, implemented in 'unlistifyParagraph', causes that 2 independent lists are created with a <span> element in the middle. Then, an algorithm implemented in 'listifyParagraph' looks for an insertion point for the new ordered list item. Since there is a previous sibling which represents a 'visually distinct position', the <span> temporary created during the 'unlistifyParagraph' logic is selected. The incorrectly selected insertion point leads to the subsequent doApplyForSingleParagraph executions to consider the rest of the list items in the selection to be part of a different ordered list. BUG= ========== to ========== Adjust insertion point when listifying unordered list items During the process of listifying a selection of unordered lists items the insertion point of the first element is incorretcly selected. The issue appears when the selection start at an intermediate list item and ending in a different list item. Since the process is defined to act on single paragraphs, the firts item splits the list first and then it removes the intermediate item. This logic, implemented in 'unlistifyParagraph', causes that 2 independent lists are created with a <span> element in the middle. Then, an algorithm implemented in 'listifyParagraph' looks for an insertion point for the new ordered list item. Since there is a previous sibling which represents a 'visually distinct position', the <span> temporary created during the 'unlistifyParagraph' logic is selected. The incorrectly selected insertion point leads to the subsequent doApplyForSingleParagraph executions to consider the rest of the list items in the selection to be part of a different ordered list. BUG=702792 ==========
jfernandez@igalia.com changed reviewers: + ojan@chromium.org, yosin@chromium.org, yutak@chromium.org
Patch sent out for review.
https://codereview.chromium.org/2757563004/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/execCommand/script-tests/insert-list-items-inside-another-list.js (right): https://codereview.chromium.org/2757563004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/execCommand/script-tests/insert-list-items-inside-another-list.js:1: description("Test to make sure inserting an ordered/unordered list inside another list works") Please use assert_selection() which is implemented on top of w3c test harness. https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/editing/s... https://codereview.chromium.org/2757563004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/InsertListCommand.cpp (right): https://codereview.chromium.org/2757563004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/InsertListCommand.cpp:618: if (isHTMLSpanElement(insertionPos.anchorNode())) Could you use Position::computeContainerNode() instead of Position::anchorNode()? For PositonType::{Before,After}Anchor, Pos::anchorNode() isn't what we want to use.
PTAL https://codereview.chromium.org/2757563004/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/execCommand/script-tests/insert-list-items-inside-another-list.js (right): https://codereview.chromium.org/2757563004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/execCommand/script-tests/insert-list-items-inside-another-list.js:1: description("Test to make sure inserting an ordered/unordered list inside another list works") On 2017/03/21 05:37:20, yosin_UTC9 wrote: > Please use assert_selection() which is implemented on top of w3c test harness. > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/editing/s... Done. https://codereview.chromium.org/2757563004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/InsertListCommand.cpp (right): https://codereview.chromium.org/2757563004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/InsertListCommand.cpp:618: if (isHTMLSpanElement(insertionPos.anchorNode())) On 2017/03/21 05:37:20, yosin_UTC9 wrote: > Could you use Position::computeContainerNode() instead of > Position::anchorNode()? > For PositonType::{Before,After}Anchor, Pos::anchorNode() isn't what we want to > use. Done.
lgtm w/ optional nit https://codereview.chromium.org/2757563004/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/execCommand/insert-list-items-inside-another-list.html (right): https://codereview.chromium.org/2757563004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/execCommand/insert-list-items-inside-another-list.html:5: <div id="log"></div> nit: We don't need to have this DIV. https://codereview.chromium.org/2757563004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/execCommand/insert-list-items-inside-another-list.html:8: '<div contenteditable><ul><li>|hello</li><li>world</li><li>WebKit</li></ul></div>', nit(optional): It is nice if we use term "Blink". ;-) Or just use meaningless words, e.g. "abc", "def", "ghi"
The CQ bit was checked by jfernandez@igalia.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/2757563004/#ps80001 (title: "Patch for landing, after applying last suggestions.")
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
Failed to apply patch for
third_party/WebKit/LayoutTests/external/wpt/editing/run/insertorderedlist-expected.txt:
While running git apply --index -p1;
error: patch failed:
third_party/WebKit/LayoutTests/external/wpt/editing/run/insertorderedlist-expected.txt:1406
error:
third_party/WebKit/LayoutTests/external/wpt/editing/run/insertorderedlist-expected.txt:
patch does not apply
Patch:
third_party/WebKit/LayoutTests/external/wpt/editing/run/insertorderedlist-expected.txt
Index:
third_party/WebKit/LayoutTests/external/wpt/editing/run/insertorderedlist-expected.txt
diff --git
a/third_party/WebKit/LayoutTests/external/wpt/editing/run/insertorderedlist-expected.txt
b/third_party/WebKit/LayoutTests/external/wpt/editing/run/insertorderedlist-expected.txt
index
e1225c4c841755d2219a0d08ee44fe83a5743cc9..73952eca78d0b58e360a104604e236e20493ca06
100644
---
a/third_party/WebKit/LayoutTests/external/wpt/editing/run/insertorderedlist-expected.txt
+++
b/third_party/WebKit/LayoutTests/external/wpt/editing/run/insertorderedlist-expected.txt
@@ -1406,7 +1406,7 @@ FAIL [["insertorderedlist",""]] "<ul
id=abc><li>foo<li>[bar]<li>baz</ul>" queryC
PASS [["stylewithcss","true"],["insertorderedlist",""]] "<ul
style=color:blue><li>foo<li>[bar]<li>baz</ul>": execCommand("stylewithcss",
false, "true") return value
PASS [["stylewithcss","true"],["insertorderedlist",""]] "<ul
style=color:blue><li>foo<li>[bar]<li>baz</ul>": execCommand("insertorderedlist",
false, "") return value
PASS [["stylewithcss","true"],["insertorderedlist",""]] "<ul
style=color:blue><li>foo<li>[bar]<li>baz</ul>" checks for modifications to
non-editable content
-FAIL [["stylewithcss","true"],["insertorderedlist",""]] "<ul
style=color:blue><li>foo<li>[bar]<li>baz</ul>" compare innerHTML assert_equals:
Unexpected innerHTML (after normalizing inline style) expected "<ul
style=\"color:rgb(0, 0, 255)\"><li>foo</li></ul><ol><li><span
style=\"color:rgb(0, 0, 255)\">bar</span></li></ol><ul style=\"color:rgb(0, 0,
255)\"><li>baz</li></ul>" but got "<ul style=\"color:rgb(0, 0,
255)\"><li>foo</li></ul><span style=\"color:rgb(0, 0,
255)\"><ol><li>bar<br></li></ol></span><ul style=\"color:rgb(0, 0,
255)\"><li>baz</li></ul>"
+FAIL [["stylewithcss","true"],["insertorderedlist",""]] "<ul
style=color:blue><li>foo<li>[bar]<li>baz</ul>" compare innerHTML assert_equals:
Unexpected innerHTML (after normalizing inline style) expected "<ul
style=\"color:rgb(0, 0, 255)\"><li>foo</li></ul><ol><li><span
style=\"color:rgb(0, 0, 255)\">bar</span></li></ol><ul style=\"color:rgb(0, 0,
255)\"><li>baz</li></ul>" but got "<ul style=\"color:rgb(0, 0,
255)\"><li>foo</li></ul><ol><li>bar<br></li></ol><ul style=\"color:rgb(0, 0,
255)\"><li>baz</li></ul>"
PASS [["stylewithcss","true"],["insertorderedlist",""]] "<ul
style=color:blue><li>foo<li>[bar]<li>baz</ul>"
queryCommandIndeterm("stylewithcss") before
FAIL [["stylewithcss","true"],["insertorderedlist",""]] "<ul
style=color:blue><li>foo<li>[bar]<li>baz</ul>" queryCommandState("stylewithcss")
before assert_equals: Wrong result returned expected false but got true
FAIL [["stylewithcss","true"],["insertorderedlist",""]] "<ul
style=color:blue><li>foo<li>[bar]<li>baz</ul>" queryCommandValue("stylewithcss")
before assert_equals: Wrong result returned expected "" but got "true"
@@ -1422,7 +1422,7 @@ FAIL [["stylewithcss","true"],["insertorderedlist",""]]
"<ul style=color:blue><l
PASS [["stylewithcss","false"],["insertorderedlist",""]] "<ul
style=color:blue><li>foo<li>[bar]<li>baz</ul>": execCommand("stylewithcss",
false, "false") return value
PASS [["stylewithcss","false"],["insertorderedlist",""]] "<ul
style=color:blue><li>foo<li>[bar]<li>baz</ul>": execCommand("insertorderedlist",
false, "") return value
PASS [["stylewithcss","false"],["insertorderedlist",""]] "<ul
style=color:blue><li>foo<li>[bar]<li>baz</ul>" checks for modifications to
non-editable content
-FAIL [["stylewithcss","false"],["insertorderedlist",""]] "<ul
style=color:blue><li>foo<li>[bar]<li>baz</ul>" compare innerHTML assert_equals:
Unexpected innerHTML (after normalizing inline style) expected "<ul
style=\"color:rgb(0, 0, 255)\"><li>foo</li></ul><ol><li><font
color=\"#0000ff\">bar</font></li></ol><ul style=\"color:rgb(0, 0,
255)\"><li>baz</li></ul>" but got "<ul style=\"color:rgb(0, 0,
255)\"><li>foo</li></ul><span style=\"color:rgb(0, 0,
255)\"><ol><li>bar<br></li></ol></span><ul style=\"color:rgb(0, 0,
255)\"><li>baz</li></ul>"
+FAIL [["stylewithcss","false"],["insertorderedlist",""]] "<ul
style=color:blue><li>foo<li>[bar]<li>baz</ul>" compare innerHTML assert_equals:
Unexpected innerHTML (after normalizing inline style) expected "<ul
style=\"color:rgb(0, 0, 255)\"><li>foo</li></ul><ol><li><font
color=\"#0000ff\">bar</font></li></ol><ul style=\"color:rgb(0, 0,
255)\"><li>baz</li></ul>" but got "<ul style=\"color:rgb(0, 0,
255)\"><li>foo</li></ul><ol><li>bar<br></li></ol><ul style=\"color:rgb(0, 0,
255)\"><li>baz</li></ul>"
PASS [["stylewithcss","false"],["insertorderedlist",""]] "<ul
style=color:blue><li>foo<li>[bar]<li>baz</ul>"
queryCommandIndeterm("stylewithcss") before
PASS [["stylewithcss","false"],["insertorderedlist",""]] "<ul
style=color:blue><li>foo<li>[bar]<li>baz</ul>" queryCommandState("stylewithcss")
before
FAIL [["stylewithcss","false"],["insertorderedlist",""]] "<ul
style=color:blue><li>foo<li>[bar]<li>baz</ul>" queryCommandValue("stylewithcss")
before assert_equals: Wrong result returned expected "" but got "true"
@@ -1438,7 +1438,7 @@ FAIL [["stylewithcss","false"],["insertorderedlist",""]]
"<ul style=color:blue><
PASS [["stylewithcss","true"],["insertorderedlist",""]] "<ul
style=text-indent:1em><li>foo<li>[bar]<li>baz</ul>": execCommand("stylewithcss",
false, "true") return value
PASS [["stylewithcss","true"],["insertorderedlist",""]] "<ul
style=text-indent:1em><li>foo<li>[bar]<li>baz</ul>":
execCommand("insertorderedlist", false, "") return value
PASS [["stylewithcss","true"],["insertorderedlist",""]] "<ul
style=text-indent:1em><li>foo<li>[bar]<li>baz</ul>" checks for modifications to
non-editable content
-FAIL [["stylewithcss","true"],["insertorderedlist",""]] "<ul
style=text-indent:1em><li>foo<li>[bar]<li>baz</ul>" compare innerHTML
assert_equals: Unexpected innerHTML (after normalizing inline style) expected
"<ul style=\"text-indent:1em\"><li>foo</li></ul><ol><li>bar</li></ol><ul
style=\"text-indent:1em\"><li>baz</li></ul>" but got "<ul
style=\"text-indent:1em\"><li>foo</li></ul><span
style=\"text-indent:1em\"><ol><li>bar<br></li></ol></span><ul
style=\"text-indent:1em\"><li>baz</li></ul>"
+FAIL [["stylewithcss","true"],["insertorderedlist",""]] "<ul
style=text-indent:1em><li>foo<li>[bar]<li>baz</ul>" compare innerHTML
assert_equals: Unexpected innerHTML (after normalizing inline style) expected
"<ul style=\"text-indent:1em\"><li>foo</li></ul><ol><li>bar</li></ol><ul
style=\"text-indent:1em\"><li>baz</li></ul>" but got "<ul
style=\"text-indent:1em\"><li>foo</li></ul><ol><li>bar<br></li></ol><ul
style=\"text-indent:1em\"><li>baz</li></ul>"
PASS [["stylewithcss","true"],["insertorderedlist",""]] "<ul
style=text-indent:1em><li>foo<li>[bar]<li>baz</ul>"
queryCommandIndeterm("stylewithcss") before
PASS [["stylewithcss","true"],["insertorderedlist",""]] "<ul
style=text-indent:1em><li>foo<li>[bar]<li>baz</ul>"
queryCommandState("stylewithcss") before
FAIL [["stylewithcss","true"],["insertorderedlist",""]] "<ul
style=text-indent:1em><li>foo<li>[bar]<li>baz</ul>"
queryCommandValue("stylewithcss") before assert_equals: Wrong result returned
expected "" but got "false"
@@ -1454,7 +1454,7 @@ FAIL [["stylewithcss","true"],["insertorderedlist",""]]
"<ul style=text-indent:1
PASS [["stylewithcss","false"],["insertorderedlist",""]] "<ul
style=text-indent:1em><li>foo<li>[bar]<li>baz</ul>": execCommand("stylewithcss",
false, "false") return value
PASS [["stylewithcss","false"],["insertorderedlist",""]] "<ul
style=text-indent:1em><li>foo<li>[bar]<li>baz</ul>":
execCommand("insertorderedlist", false, "") return value
PASS [["stylewithcss","false"],["insertorderedlist",""]] "<ul
style=text-indent:1em><li>foo<li>[bar]<li>baz</ul>" checks for modifications to
non-editable content
-FAIL [["stylewithcss","false"],["insertorderedlist",""]] "<ul
style=text-indent:1em><li>foo<li>[bar]<li>baz</ul>" compare innerHTML
assert_equals: Unexpected innerHTML (after normalizing inline style) expected
"<ul style=\"text-indent:1em\"><li>foo</li></ul><ol><li>bar</li></ol><ul
style=\"text-indent:1em\"><li>baz</li></ul>" but got "<ul
style=\"text-indent:1em\"><li>foo</li></ul><span
style=\"text-indent:1em\"><ol><li>bar<br></li></ol></span><ul
style=\"text-indent:1em\"><li>baz</li></ul>"
+FAIL [["stylewithcss","false"],["insertorderedlist",""]] "<ul
style=text-indent:1em><li>foo<li>[bar]<li>baz</ul>" compare innerHTML
assert_equals: Unexpected innerHTML (after normalizing inline style) expected
"<ul style=\"text-indent:1em\"><li>foo</li></ul><ol><li>bar</li></ol><ul
style=\"text-indent:1em\"><li>baz</li></ul>" but got "<ul
style=\"text-indent:1em\"><li>foo</li></ul><ol><li>bar<br></li></ol><ul
style=\"text-indent:1em\"><li>baz</li></ul>"
PASS [["stylewithcss","false"],["insertorderedlist",""]] "<ul
style=text-indent:1em><li>foo<li>[bar]<li>baz</ul>"
queryCommandIndeterm("stylewithcss") before
PASS [["stylewithcss","false"],["insertorderedlist",""]] "<ul
style=text-indent:1em><li>foo<li>[bar]<li>baz</ul>"
queryCommandState("stylewithcss") before
FAIL [["stylewithcss","false"],["insertorderedlist",""]] "<ul
style=text-indent:1em><li>foo<li>[bar]<li>baz</ul>"
queryCommandValue("stylewithcss") before assert_equals: Wrong result returned
expected "" but got "true"
@@ -1529,7 +1529,7 @@ FAIL [["insertorderedlist",""]] "<ul
id=abc><li>foo<li>bar<li>[baz]</ul>" queryC
PASS [["stylewithcss","true"],["insertorderedlist",""]] "<ul
style=color:blue><li>foo<li>bar<li>[baz]</ul>": execCommand("stylewithcss",
false, "true") return value
PASS [["sty…
(message too large)
Patchset #6 (id:100001) has been deleted
The CQ bit was checked by jfernandez@igalia.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/2757563004/#ps120001 (title: "Patch rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1490357334152050,
"parent_rev": "e6eb96d661fb4e2ddf0ab5baaf6ee389854f0a86", "commit_rev":
"ce9fb22d90743b6b08b1a1baa00202d781673269"}
Message was sent while issue was closed.
Description was changed from ========== Adjust insertion point when listifying unordered list items During the process of listifying a selection of unordered lists items the insertion point of the first element is incorretcly selected. The issue appears when the selection start at an intermediate list item and ending in a different list item. Since the process is defined to act on single paragraphs, the firts item splits the list first and then it removes the intermediate item. This logic, implemented in 'unlistifyParagraph', causes that 2 independent lists are created with a <span> element in the middle. Then, an algorithm implemented in 'listifyParagraph' looks for an insertion point for the new ordered list item. Since there is a previous sibling which represents a 'visually distinct position', the <span> temporary created during the 'unlistifyParagraph' logic is selected. The incorrectly selected insertion point leads to the subsequent doApplyForSingleParagraph executions to consider the rest of the list items in the selection to be part of a different ordered list. BUG=702792 ========== to ========== Adjust insertion point when listifying unordered list items During the process of listifying a selection of unordered lists items the insertion point of the first element is incorretcly selected. The issue appears when the selection start at an intermediate list item and ending in a different list item. Since the process is defined to act on single paragraphs, the firts item splits the list first and then it removes the intermediate item. This logic, implemented in 'unlistifyParagraph', causes that 2 independent lists are created with a <span> element in the middle. Then, an algorithm implemented in 'listifyParagraph' looks for an insertion point for the new ordered list item. Since there is a previous sibling which represents a 'visually distinct position', the <span> temporary created during the 'unlistifyParagraph' logic is selected. The incorrectly selected insertion point leads to the subsequent doApplyForSingleParagraph executions to consider the rest of the list items in the selection to be part of a different ordered list. BUG=702792 Review-Url: https://codereview.chromium.org/2757563004 Cr-Commit-Position: refs/heads/master@{#459442} Committed: https://chromium.googlesource.com/chromium/src/+/ce9fb22d90743b6b08b1a1baa002... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as https://chromium.googlesource.com/chromium/src/+/ce9fb22d90743b6b08b1a1baa002... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
