Summary: | ASSERT in editing code, ASSERTION FAILED: isStartOfParagraph(startOfParagraphToMove) | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||||
Component: | HTML Editing | Assignee: | Eric Seidel (no email) <eric> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | emacemac7, justin.garcia, sky | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Mac | ||||||||
OS: | OS X 10.5 | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 18858, 22634 | ||||||||
Attachments: |
|
Description
Eric Seidel (no email)
2008-05-14 18:11:55 PDT
doc.execCommand('insertimage'); doc.execCommand('insertorderedlist'); Also crashes. Created attachment 21147 [details]
test case (ASSERT in debug builds)
This is the same ASSERT as is hit by bug 19066 which is a p1 crasher with google docs. Investigating. This might be caused by: // FIXME: Broken for positions before/after images that aren't inline (5027702) VisiblePosition startOfParagraph(const VisiblePosition &c) since we're inserting an image here (although it should be inline)... It seems the problem here is that somehow the "start" of the paragraph being moved it wrong (and includes the list!) (gdb) call startOfParagraphToMove.showTreeForThis() *BODY 0xd90680 OL 0xdd9f90 LI 0xdd9e40 BR 0xdd9e80 IMG 0xdd8570 #text 0xd8ef90 "\n" SCRIPT 0xd928f0 #text 0xd9b220 "\ndocument.designMode = "on";\ndocument.execCommand('selectall');\ndocument.execCommand('insertimage');\ndocument.execCommand('insertorderedlist');\n" the start/end should be just around the <img>. I think the problem may be startOfParagraph() In the list insertion code: if (!listChildNode || switchListType || m_forceCreateList) { // Create list. VisiblePosition start = startOfParagraph(endingSelection().visibleStart()); endingSelection().visibleStart() is correctly right before the <img>, however, when that's turned into "startOfParagraph" then it moves to <body>, 0 I'm not really sure what startOfParagraph() is supposed to do. > endingSelection().visibleStart() is correctly right before the <img>, however,
> when that's turned into "startOfParagraph" then it moves to <body>, 0
>
> I'm not really sure what startOfParagraph() is supposed to do.
This code wants to get the start of the paragraph so that it moves the whole paragraph into the new list item. I'm not sure why it's jumping from [img, 0] to [body, 0], though.
(In reply to comment #7) > This code wants to get the start of the paragraph so that it moves the whole > paragraph into the new list item. I'm not sure why it's jumping from [img, 0] > to [body, 0], though. This jump kinda makes sense. It's jumping to the containing block (which is the body), and setting start to the first offset in the containing block (which makes sense). I'm not sure that it makes sense though to insert the list right before the image, and then move the image inside it. Or at least, by doing so, we change what the "start of the paragraph" is, since now the paragraph should start right after the list. Maybe the right behavior here would be to first wrap the content which we indend to move into the list, into a new block. Then insert the list before the block. And then move the contents of the block into the list, and remove the fake block. Alternatively, we could just learn to update the start position of the paragraph after we insert the list. :) I'd be interested to hear your thoughts Justin. The naive fix of just always updating the "start" position after inserting the list, causes editing/execCommand/19653-1.html to fail, as now the text is inserted before the list instead of inside it. Created attachment 28421 [details]
First attempt at fix, with test case
LayoutTests/ChangeLog | 11 +++++++++++
.../list-wrapping-image-crash-expected.txt | 2 ++
.../execCommand/list-wrapping-image-crash.html | 10 ++++++++++
WebCore/ChangeLog | 18 ++++++++++++++++++
WebCore/editing/InsertListCommand.cpp | 12 +++++++++---
5 files changed, 50 insertions(+), 3 deletions(-)
(In reply to comment #8) > (In reply to comment #7) > > This code wants to get the start of the paragraph so that it moves the whole > > paragraph into the new list item. I'm not sure why it's jumping from [img, 0] > > to [body, 0], though. > > This jump kinda makes sense. It's jumping to the containing block (which is > the body), and setting start to the first offset in the containing block (which > makes sense). Doesn't make sense for startOfParagraph to jump from [img, 0] to [body, 0], [img, 0] is the start of the paragraph. (In reply to comment #11) > Doesn't make sense for startOfParagraph to jump from [img, 0] to [body, 0], > [img, 0] is the start of the paragraph. Why is [img,0] the start of the paragraph? [img,0] is the first position in the content of the paragraph, [body,0] is the first offset in the block of the paragraph. Would [p,0] or [img,0] be the "start of the paragraph" if there was a <p> between the <img> and the body? (In reply to comment #12) > (In reply to comment #11) > > Doesn't make sense for startOfParagraph to jump from [img, 0] to [body, 0], > > [img, 0] is the start of the paragraph. > > Why is [img,0] the start of the paragraph? [img,0] is the first position in > the content of the paragraph, [body,0] is the first offset in the block of the > paragraph. there are two paragraphs, one is in the list ([br, 0] or [li, 0]) and the other starts at [img, 0]. (In reply to comment #13) > > Why is [img,0] the start of the paragraph? [img,0] is the first position in > > the content of the paragraph, [body,0] is the first offset in the block of the > > paragraph. > > there are two paragraphs, one is in the list ([br, 0] or [li, 0]) and the other > starts at [img, 0]. Sure, but the "start of paragraph" is computed from the endSelection().visibleStart() before the list is inserted. At which time [body,0] makes sense it's just the DOM-complaint equivalent of [img, 0]. Ok, so the problem is that [body, 0] is being preferred over [img, 0], because the img doesn't have a height. Normally [img, 0] would be preferred, which is what the list code depends on happening because it inserts a list right before the <img> and doesn't bother to update the start position. This is the check which is allowing [body, 0] to be preferred: if (!node()->hasTagName(htmlTag) && renderer->isBlockFlow() && !hasRenderedNonAnonymousDescendantsWithHeight(renderer) && (toRenderBox(renderer)->height() || node()->hasTagName(bodyTag))) return offset() == 0 && !nodeIsUserSelectNone(node()); because hasRenderedNonAnonymousDescendantsWithHeight is returning false, because the RenderImage has no height. Comment on attachment 28421 [details]
First attempt at fix, with test case
I think Justin Garcia or Darin Adler are the only two people who would ever review this. Marking this as r=justin?
Comment on attachment 28421 [details]
First attempt at fix, with test case
r=me
Committing to http://svn.webkit.org/repository/webkit/trunk ... M LayoutTests/ChangeLog A LayoutTests/editing/execCommand/list-wrapping-image-crash-expected.txt A LayoutTests/editing/execCommand/list-wrapping-image-crash.html M WebCore/ChangeLog M WebCore/editing/InsertListCommand.cpp Committed r44375 |