Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2135993003:
Add null check to fix crash in blink::ComputedStyle::display (Closed)
|
Created:
4 years, 5 months ago by joone Modified:
4 years, 5 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 null check to fix crash in blink::ComputedStyle::display
These is a case that node->computedStyle() returns null
so it needs to check whether computedStyle() returns
null or not.
BUG=626991
TEST=editing/execCommand/crash-inserting-span.html
Committed: https://crrev.com/e7f2bcf1c9f2c9f7d998328cc47face9b75cca1e
Cr-Commit-Position: refs/heads/master@{#405733}
Patch Set 1 #
Total comments: 10
Patch Set 2 : add a test case #
Total comments: 2
Patch Set 3 : simplify the test case #
Total comments: 6
Patch Set 4 : use of assert_selection #
Total comments: 9
Patch Set 5 : use a local variable to hold |ComputedStyle| #Patch Set 6 : remove CreateLink command #
Total comments: 5
Patch Set 7 : use of the short form for running the insertHTML command #
Total comments: 2
Messages
Total messages: 39 (11 generated)
joone.hur@intel.com changed reviewers: + yosin@chromium.org
Hi yosin@ please take a look at this CL.
Could you add test case for this change? https://codereview.chromium.org/2135993003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.cpp (right): https://codereview.chromium.org/2135993003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.cpp:797: static bool isListItemStyle(const HTMLElement& element) Can make |isListItem()| in "EditingUtilitis.h" to use computed style and use it here? https://codereview.chromium.org/2135993003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.cpp:799: if (element.computedStyle() && element.computedStyle()->display() == LIST_ITEM) nit: We should avoid to use |return true| with |return false| const ComputedStyle* style = element.computedStyle(); return style && style->display() == LIST_ITEM; https://codereview.chromium.org/2135993003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.cpp:804: static bool isTableCellStyle(const HTMLElement& element) Can make |isTableCell()| in "EditingUtilitis.h" to use computed style and use it here? https://codereview.chromium.org/2135993003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.cpp:806: if (element.computedStyle() && element.computedStyle()->display() == TABLE_CELL) nit: We should avoid to use |return true| with |return false| const ComputedStyle* style = element.computedStyle(); return style && style->display() == LIST_ITEM; https://codereview.chromium.org/2135993003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.cpp:815: I think computedStyle() should not be return nullptr, but current implementation returns nullptr. I guess this situation is changed by "Splitting up Style Resolution and Layout Tree Construction" https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/style$20t... It seems null-pointer reference is occurred at L834 of handleStyleSpansBeforeInsertion() instead of here: 834 if (node->computedStyle()->display() == INLINE) { node = enclosingBlock(insertionPos.anchorNode()); if (!node) return false; } if (followBlockElementStyle(node)) { fragment.removeNodePreservingChildren(wrappingStyleSpan); return true; }
Hi yosin@ I tried to use computed style in isListItem and isTableCell, but it causes crashes and test fails so I just updated isInline function to check if computedStyle() returns null. I will prepare for a separate CL to use computed style in isListItem and isTableCell. Thanks, Joone https://codereview.chromium.org/2135993003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.cpp (right): https://codereview.chromium.org/2135993003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.cpp:797: static bool isListItemStyle(const HTMLElement& element) On 2016/07/12 01:30:36, Yosi_UTC9 wrote: > Can make |isListItem()| in "EditingUtilitis.h" to use computed style and use it > here? Done. https://codereview.chromium.org/2135993003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.cpp:799: if (element.computedStyle() && element.computedStyle()->display() == LIST_ITEM) On 2016/07/12 01:30:36, Yosi_UTC9 wrote: > nit: We should avoid to use |return true| with |return false| > > const ComputedStyle* style = element.computedStyle(); > return style && style->display() == LIST_ITEM; Done. https://codereview.chromium.org/2135993003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.cpp:804: static bool isTableCellStyle(const HTMLElement& element) On 2016/07/12 01:30:36, Yosi_UTC9 wrote: > Can make |isTableCell()| in "EditingUtilitis.h" to use computed style and use it > here? Done. https://codereview.chromium.org/2135993003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.cpp:806: if (element.computedStyle() && element.computedStyle()->display() == TABLE_CELL) On 2016/07/12 01:30:36, Yosi_UTC9 wrote: > nit: We should avoid to use |return true| with |return false| > > const ComputedStyle* style = element.computedStyle(); > return style && style->display() == LIST_ITEM; Done. https://codereview.chromium.org/2135993003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.cpp:815: On 2016/07/12 01:30:36, Yosi_UTC9 wrote: > I think computedStyle() should not be return nullptr, but current implementation > returns nullptr. > I guess this situation is changed by "Splitting up Style Resolution and Layout > Tree Construction" > https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/style$20t... > > > It seems null-pointer reference is occurred at L834 of > handleStyleSpansBeforeInsertion() instead of here: > > 834 if (node->computedStyle()->display() == INLINE) { > node = enclosingBlock(insertionPos.anchorNode()); > if (!node) > return false; > } > > if (followBlockElementStyle(node)) { > fragment.removeNodePreservingChildren(wrappingStyleSpan); > return true; > } > > We can use isInline() function defined in EditingUtilities.cpp
Description was changed from ========== Add null check to fix crash in blink::ComputedStyle::display It seems that element.computedStyle() returns null so it needs to check whether computedStyle() returns null or not. BUG=626991 ========== to ========== Add null check to fix crash in blink::ComputedStyle::display It seems that node->computedStyle() returns null so it needs to check whether computedStyle() returns null or not. BUG=626991 ==========
Description was changed from ========== Add null check to fix crash in blink::ComputedStyle::display It seems that node->computedStyle() returns null so it needs to check whether computedStyle() returns null or not. BUG=626991 ========== to ========== Add null check to fix crash in blink::ComputedStyle::display It seems that node->computedStyle() returns null so it needs to check whether computedStyle() returns null or not. BUG=626991 ==========
Please ignore "Done." in my previous comment. :-)
https://codereview.chromium.org/2135993003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html (right): https://codereview.chromium.org/2135993003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html:25: document.execCommand("inserthtml", false, "<span id='green' style='color:green'>green</span>"); Could you try to simplify test case? We need to have a HTML just before "insertHTML" command rather than constructing HTML by script. We know it is time consuming but we would like to have simple test case.
https://codereview.chromium.org/2135993003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html (right): https://codereview.chromium.org/2135993003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html:25: document.execCommand("inserthtml", false, "<span id='green' style='color:green'>green</span>"); On 2016/07/13 02:19:43, Yosi_UTC9 wrote: > Could you try to simplify test case? We need to have a HTML just before > "insertHTML" command rather than constructing HTML by script. > We know it is time consuming but we would like to have simple test case. Okay, I will try to add HTML code instead of running runTest().
yosin@ could you take a look at the updated test case?
https://codereview.chromium.org/2135993003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html (right): https://codereview.chromium.org/2135993003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html:4: <div id="description"> I think we don't need to have "description" DIV. https://codereview.chromium.org/2135993003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html:5: <span><p>test</p><p>On success, you will see a series of "<span class="pass">PASS</span>" messages, followed by "<span class="pass">TEST COMPLETE</span>".</p></span> We might use random text such as "foo", "bar", "abc" etc to reduce length of text. https://codereview.chromium.org/2135993003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html:14: document.execCommand("CreateLink", 0, "foo"); Can we use HTML after executing "createLink"? Also, can we use assert_selection() to verify final state of selection? https://codereview.chromium.org/2135993003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html:15: document.execCommand("inserthtml", false, "<span id='green' style='color:green'>green</span>"); Let's use single-quote for JavaScript and double-quote for HTML including embedded into JS.
https://codereview.chromium.org/2135993003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html (right): https://codereview.chromium.org/2135993003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html:14: document.execCommand("CreateLink", 0, "foo"); On 2016/07/13 06:44:10, Yosi_UTC9 wrote: > Can we use HTML after executing "createLink"? What does it mean?
https://codereview.chromium.org/2135993003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html (right): https://codereview.chromium.org/2135993003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html:14: document.execCommand("CreateLink", 0, "foo"); On 2016/07/13 at 07:31:50, joone wrote: > On 2016/07/13 06:44:10, Yosi_UTC9 wrote: > > Can we use HTML after executing "createLink"? > > What does it mean? Since "createLink" isn't part of nullptr reference. We can have a resulted HTML fragment of "createLink" command. I would like to have assert_selection( '...^...|...', ['insertHTML', '<span id="green" style="color:green">green</span>'], '...^...|...'); Note: Please read "..." as real HTML markup.
Patchset #4 (id:60001) has been deleted
On 2016/07/13 08:02:42, Yosi_UTC9 wrote: > https://codereview.chromium.org/2135993003/diff/40001/third_party/WebKit/Layo... > File > third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html > (right): > > https://codereview.chromium.org/2135993003/diff/40001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html:14: > document.execCommand("CreateLink", 0, "foo"); > On 2016/07/13 at 07:31:50, joone wrote: > > On 2016/07/13 06:44:10, Yosi_UTC9 wrote: > > > Can we use HTML after executing "createLink"? > > > > What does it mean? > > Since "createLink" isn't part of nullptr reference. We can have a resulted HTML > fragment of "createLink" command. > > I would like to have > assert_selection( > '...^...|...', > ['insertHTML', '<span id="green" style="color:green">green</span>'], > '...^...|...'); > > Note: Please read "..." as real HTML markup. Thanks for the answer. I've updated the test case to use assert_selection().
Description was changed from ========== Add null check to fix crash in blink::ComputedStyle::display It seems that node->computedStyle() returns null so it needs to check whether computedStyle() returns null or not. BUG=626991 ========== to ========== Add null check to fix crash in blink::ComputedStyle::display These is a case that node->computedStyle() returns null so it needs to check whether computedStyle() returns null or not. BUG=626991 ==========
Description was changed from ========== Add null check to fix crash in blink::ComputedStyle::display These is a case that node->computedStyle() returns null so it needs to check whether computedStyle() returns null or not. BUG=626991 ========== to ========== Add null check to fix crash in blink::ComputedStyle::display These is a case that node->computedStyle() returns null so it needs to check whether computedStyle() returns null or not. BUG=626991 ==========
https://codereview.chromium.org/2135993003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html (right): https://codereview.chromium.org/2135993003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html:7: // This test case was added to reproduce the crash reported by ClusterFuzz. We don't need to have this comment, we can use git log to know what this test is. https://codereview.chromium.org/2135993003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html:13: selection.document.designMode = 'on'; Please enclose a sample with '<div contenteditable>' to avoid to use "document.designMode='on'" https://codereview.chromium.org/2135993003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html:14: selection.document.execCommand('CreateLink', false, 'foo'); Could you encode result of "createLink" command in sample input and expected result? https://codereview.chromium.org/2135993003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right): https://codereview.chromium.org/2135993003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:768: return node && node->computedStyle() && node->computedStyle()->display() == INLINE; Since |Node::computedStyle()| isn't simple getter, could you use local variable to hold |ComputedStyle| to avoid calling |Node::computedStyle()| twice?
Description was changed from ========== Add null check to fix crash in blink::ComputedStyle::display These is a case that node->computedStyle() returns null so it needs to check whether computedStyle() returns null or not. BUG=626991 ========== to ========== Add null check to fix crash in blink::ComputedStyle::display These is a case that node->computedStyle() returns null so it needs to check whether computedStyle() returns null or not. BUG=626991 TEST=editing/execCommand/crash-inserting-span.html ==========
Could you take a look at the updated CL? https://codereview.chromium.org/2135993003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html (right): https://codereview.chromium.org/2135993003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html:7: // This test case was added to reproduce the crash reported by ClusterFuzz. On 2016/07/14 01:08:52, Yosi_UTC9 wrote: > We don't need to have this comment, we can use git log to know what this test > is. Done. https://codereview.chromium.org/2135993003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html:13: selection.document.designMode = 'on'; On 2016/07/14 01:08:52, Yosi_UTC9 wrote: > Please enclose a sample with '<div contenteditable>' to avoid to use > "document.designMode='on'" Done. https://codereview.chromium.org/2135993003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html:14: selection.document.execCommand('CreateLink', false, 'foo'); On 2016/07/14 01:08:52, Yosi_UTC9 wrote: > Could you encode result of "createLink" command in sample input and expected > result? Done. https://codereview.chromium.org/2135993003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right): https://codereview.chromium.org/2135993003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:768: return node && node->computedStyle() && node->computedStyle()->display() == INLINE; On 2016/07/14 01:08:52, Yosi_UTC9 wrote: > Since |Node::computedStyle()| isn't simple getter, > could you use local variable to hold |ComputedStyle| to avoid calling > |Node::computedStyle()| twice? Done.
https://codereview.chromium.org/2135993003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html (right): https://codereview.chromium.org/2135993003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html:14: selection.document.execCommand('CreateLink', false, 'foo'); On 2016/07/14 at 07:06:54, joone wrote: > On 2016/07/14 01:08:52, Yosi_UTC9 wrote: > > Could you encode result of "createLink" command in sample input and expected > > result? > > Done. PS#5 still has "createLink", did you forget to update this?
On 2016/07/14 07:53:34, Yosi_UTC9 wrote: > https://codereview.chromium.org/2135993003/diff/80001/third_party/WebKit/Layo... > File > third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html > (right): > > https://codereview.chromium.org/2135993003/diff/80001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html:14: > selection.document.execCommand('CreateLink', false, 'foo'); > On 2016/07/14 at 07:06:54, joone wrote: > > On 2016/07/14 01:08:52, Yosi_UTC9 wrote: > > > Could you encode result of "createLink" command in sample input and expected > > > result? > > > > Done. > > PS#5 still has "createLink", did you forget to update this? Do you want me to update the test case as follows? assert_selection( '<div contenteditable><span><p>^test</p><p>foo<span>bar</span>foo<span>bar</span>.|</p></span></div>', selection => { selection.document.execCommand('inserthtml', false, '<a href=\"foo\"><span style=\"color:green\">green</span></a>'); }, '<div contenteditable><span><p><a href=\"foo\"><span style=\"color:green\">green|</span></a></p></span></div>'); If so, the crash cannot be reproduced.
On 2016/07/14 at 08:14:00, joone.hur wrote: > On 2016/07/14 07:53:34, Yosi_UTC9 wrote: > > https://codereview.chromium.org/2135993003/diff/80001/third_party/WebKit/Layo... > > File > > third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html > > (right): > > > > https://codereview.chromium.org/2135993003/diff/80001/third_party/WebKit/Layo... > > third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html:14: > > selection.document.execCommand('CreateLink', false, 'foo'); > > On 2016/07/14 at 07:06:54, joone wrote: > > > On 2016/07/14 01:08:52, Yosi_UTC9 wrote: > > > > Could you encode result of "createLink" command in sample input and expected > > > > result? > > > > > > Done. > > > > PS#5 still has "createLink", did you forget to update this? > > Do you want me to update the test case as follows? > > assert_selection( > '<div contenteditable><span><p>^test</p><p>foo<span>bar</span>foo<span>bar</span>.|</p></span></div>', > selection => { > selection.document.execCommand('inserthtml', false, '<a href=\"foo\"><span style=\"color:green\">green</span></a>'); > }, > '<div contenteditable><span><p><a href=\"foo\"><span style=\"color:green\">green|</span></a></p></span></div>'); > > If so, the crash cannot be reproduced. No. CreateLink command is applied to selection instead of inserted HTML. So, test could be written as: assert_selection( // Not sure actual selection after createLink '<div contenteditable><span><p><a href="foo">|test</p><p>foo<span>bar</span>foo<span>bar</span>.</a></p></span></div>', selection => { selection.document.execCommand('inserthtml', false, '<a href=\"foo\"><span style=\"color:green\">green</span></a>'); }, '<div contenteditable><span><p><a href=\"foo\"><span style=\"color:green\">green|</span></a></p></span></div>');
On 2016/07/14 09:03:06, Yosi_UTC9 wrote: > On 2016/07/14 at 08:14:00, joone.hur wrote: > > On 2016/07/14 07:53:34, Yosi_UTC9 wrote: > > > > https://codereview.chromium.org/2135993003/diff/80001/third_party/WebKit/Layo... > > > File > > > third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html > > > (right): > > > > > > > https://codereview.chromium.org/2135993003/diff/80001/third_party/WebKit/Layo... > > > > third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html:14: > > > selection.document.execCommand('CreateLink', false, 'foo'); > > > On 2016/07/14 at 07:06:54, joone wrote: > > > > On 2016/07/14 01:08:52, Yosi_UTC9 wrote: > > > > > Could you encode result of "createLink" command in sample input and > expected > > > > > result? > > > > > > > > Done. > > > > > > PS#5 still has "createLink", did you forget to update this? > > > > Do you want me to update the test case as follows? > > > > assert_selection( > > '<div > contenteditable><span><p>^test</p><p>foo<span>bar</span>foo<span>bar</span>.|</p></span></div>', > > selection => { > > selection.document.execCommand('inserthtml', false, '<a > href=\"foo\"><span style=\"color:green\">green</span></a>'); > > }, > > '<div contenteditable><span><p><a href=\"foo\"><span > style=\"color:green\">green|</span></a></p></span></div>'); > > > > If so, the crash cannot be reproduced. > > No. CreateLink command is applied to selection instead of inserted HTML. > So, test could be written as: > assert_selection( > // Not sure actual selection after createLink > '<div contenteditable><span><p><a > href="foo">|test</p><p>foo<span>bar</span>foo<span>bar</span>.</a></p></span></div>', > selection => { > selection.document.execCommand('inserthtml', false, '<a > href=\"foo\"><span style=\"color:green\">green</span></a>'); > }, > '<div contenteditable><span><p><a href=\"foo\"><span > style=\"color:green\">green|</span></a></p></span></div>'); Thanks for the example. Please see the updated test case.
lgtm w/ small nits Thanks for having simple test case! https://codereview.chromium.org/2135993003/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html (right): https://codereview.chromium.org/2135993003/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html:9: '<div contenteditable><span><a href=\"foo\"><p>^test</p><p>foo<span>bar</span>foo<span>bar</span>.|</p></a></span></div>', nit: No need to escape double quote. This is the reason why we use single quote in JavaScript. https://codereview.chromium.org/2135993003/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html:10: selection => { nit: You can use short form: ['insertHTML', '<span style="color:green">green</span>'] https://codereview.chromium.org/2135993003/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html:13: '<div contenteditable><span><p><a href=\"foo\"><span style=\"color:green\">green|</span></a></p></span></div>'); nit: No need to escape double quote. This is the reason why we use single quote in JavaScript.
https://codereview.chromium.org/2135993003/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html (right): https://codereview.chromium.org/2135993003/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html:10: selection => { On 2016/07/15 02:19:00, Yosi_UTC9 wrote: > nit: You can use short form: > ['insertHTML', '<span style="color:green">green</span>'] assert_selection( '<div contenteditable><span><a href="foo"><p>^test</p><p>foo<span>bar</span>foo<span>bar</span>.|</p></a></span></div>', ['insertHTML', '<span style="color:green">green</span>'], '<div contenteditable><span><p><a href="foo"><span style="color:green">green|</span></a></p></span></div>'); Is this right? it returns an error as follows: This is a testharness.js-based test. FAIL Replace the selected tags with span tags Invalid tester: insertHTML,<span style="color:green">green</span> Harness: the test ran to completion. So I tried to run it as follows: assert_selection( '<div contenteditable><span><a href="foo"><p>^test</p><p>foo<span>bar</span>foo<span>bar</span>.|</p></a></span></div>', selection => ['insertHTML', '<span style="color:green">green</span>'], '<div contenteditable><span><p><a href="foo"><span style="color:green">green|</span></a></p></span></div>'); There is no error, but the command doesn't work.
https://codereview.chromium.org/2135993003/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html (right): https://codereview.chromium.org/2135993003/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html:10: selection => { On 2016/07/15 at 05:57:52, joone wrote: > On 2016/07/15 02:19:00, Yosi_UTC9 wrote: > > nit: You can use short form: > > ['insertHTML', '<span style="color:green">green</span>'] > > assert_selection( > '<div contenteditable><span><a href="foo"><p>^test</p><p>foo<span>bar</span>foo<span>bar</span>.|</p></a></span></div>', > ['insertHTML', '<span style="color:green">green</span>'], > '<div contenteditable><span><p><a href="foo"><span style="color:green">green|</span></a></p></span></div>'); > > Is this right? it returns an error as follows: > > This is a testharness.js-based test. > FAIL Replace the selected tags with span tags Invalid tester: insertHTML,<span style="color:green">green</span> > Harness: the test ran to completion. > > So I tried to run it as follows: > assert_selection( > '<div contenteditable><span><a href="foo"><p>^test</p><p>foo<span>bar</span>foo<span>bar</span>.|</p></a></span></div>', > selection => ['insertHTML', '<span style="color:green">green</span>'], > '<div contenteditable><span><p><a href="foo"><span style="color:green">green|</span></a></p></span></div>'); > > There is no error, but the command doesn't work. Oops, sorry for confusion. I said wrong thing. We could write: assert_selection( '<div contenteditable><span><a href="foo"><p>^test</p><p>foo<span>bar</span>foo<span>bar</span>.|</p></a></span></div>', 'insertHTML <span style="color:green">green</span>', '<div contenteditable><span><p><a href="foo"><span style="color:green">green|</span></a></p></span></div>'); Use space separated string instead of array. Here is an implementation of assert_selection: https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/editing/a... function assertSelection( inputText, tester, expectedText, opt_description = '') { /** @type {string} */ const description = opt_description === '' ? assembleDescription() : opt_description; checkExpectedText(expectedText); const sample = new Sample(inputText); if (typeof(tester) === 'function') { tester.call(window, sample.selection); } else if (typeof(tester) === 'string') { const strings = tester.split(' '); sample.document.execCommand(strings[0], false, strings[1]); } else { throw new Error(`Invalid tester: ${tester}`); } It used be an array but it was changed to string to be shorter.
On 2016/07/15 07:06:41, Yosi_UTC9 wrote: > https://codereview.chromium.org/2135993003/diff/120001/third_party/WebKit/Lay... > File > third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html > (right): > > https://codereview.chromium.org/2135993003/diff/120001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html:10: > selection => { > On 2016/07/15 at 05:57:52, joone wrote: > > On 2016/07/15 02:19:00, Yosi_UTC9 wrote: > > > nit: You can use short form: > > > ['insertHTML', '<span style="color:green">green</span>'] > > > > assert_selection( > > '<div contenteditable><span><a > href="foo"><p>^test</p><p>foo<span>bar</span>foo<span>bar</span>.|</p></a></span></div>', > > ['insertHTML', '<span style="color:green">green</span>'], > > '<div contenteditable><span><p><a href="foo"><span > style="color:green">green|</span></a></p></span></div>'); > > > > Is this right? it returns an error as follows: > > > > This is a testharness.js-based test. > > FAIL Replace the selected tags with span tags Invalid tester: insertHTML,<span > style="color:green">green</span> > > Harness: the test ran to completion. > > > > So I tried to run it as follows: > > assert_selection( > > '<div contenteditable><span><a > href="foo"><p>^test</p><p>foo<span>bar</span>foo<span>bar</span>.|</p></a></span></div>', > > selection => ['insertHTML', '<span style="color:green">green</span>'], > > '<div contenteditable><span><p><a href="foo"><span > style="color:green">green|</span></a></p></span></div>'); > > > > There is no error, but the command doesn't work. > > Oops, sorry for confusion. I said wrong thing. > > We could write: > assert_selection( > '<div contenteditable><span><a > href="foo"><p>^test</p><p>foo<span>bar</span>foo<span>bar</span>.|</p></a></span></div>', > 'insertHTML <span style="color:green">green</span>', > '<div contenteditable><span><p><a href="foo"><span > style="color:green">green|</span></a></p></span></div>'); > > Use space separated string instead of array. > > Here is an implementation of assert_selection: > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/editing/a... > > function assertSelection( > inputText, tester, expectedText, opt_description = '') { > /** @type {string} */ > const description = > opt_description === '' ? assembleDescription() : opt_description; > checkExpectedText(expectedText); > const sample = new Sample(inputText); > if (typeof(tester) === 'function') { > tester.call(window, sample.selection); > } else if (typeof(tester) === 'string') { > const strings = tester.split(' '); > sample.document.execCommand(strings[0], false, strings[1]); > } else { > throw new Error(`Invalid tester: ${tester}`); > } > > > It used be an array but it was changed to string to be shorter. split function doesn't work with html tag so strings[1] is empty.
On 2016/07/15 07:41:11, joone wrote: > split function doesn't work with html tag so strings[1] is empty. strings[1] seems to have "green".
On 2016/07/15 07:43:38, joone wrote: > On 2016/07/15 07:41:11, joone wrote: > > split function doesn't work with html tag so strings[1] is empty. > > strings[1] seems to have "green". It has "<span" because there is a space among html tag.
https://codereview.chromium.org/2135993003/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html (right): https://codereview.chromium.org/2135993003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html:10: 'insertHTML <span>green</span>', I removed the style attribute then it works. Anyway, we need to improve assert_selection function to handle attribute values.
https://codereview.chromium.org/2135993003/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html (right): https://codereview.chromium.org/2135993003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/editing/execCommand/crash-inserting-span.html:10: 'insertHTML <span>green</span>', On 2016/07/15 at 08:23:33, joone wrote: > I removed the style attribute then it works. > Anyway, we need to improve assert_selection function to handle attribute values. Right. I'll improve assert_selection later. Thanks for finding! Let's commit this patch.
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/2135993003/#ps140001 (title: "use of the short form for running the insertHTML command")
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 null check to fix crash in blink::ComputedStyle::display These is a case that node->computedStyle() returns null so it needs to check whether computedStyle() returns null or not. BUG=626991 TEST=editing/execCommand/crash-inserting-span.html ========== to ========== Add null check to fix crash in blink::ComputedStyle::display These is a case that node->computedStyle() returns null so it needs to check whether computedStyle() returns null or not. BUG=626991 TEST=editing/execCommand/crash-inserting-span.html ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Add null check to fix crash in blink::ComputedStyle::display These is a case that node->computedStyle() returns null so it needs to check whether computedStyle() returns null or not. BUG=626991 TEST=editing/execCommand/crash-inserting-span.html ========== to ========== Add null check to fix crash in blink::ComputedStyle::display These is a case that node->computedStyle() returns null so it needs to check whether computedStyle() returns null or not. BUG=626991 TEST=editing/execCommand/crash-inserting-span.html Committed: https://crrev.com/e7f2bcf1c9f2c9f7d998328cc47face9b75cca1e Cr-Commit-Position: refs/heads/master@{#405733} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/e7f2bcf1c9f2c9f7d998328cc47face9b75cca1e Cr-Commit-Position: refs/heads/master@{#405733} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
