Bug 29697

Does not repro in Mac nightly at r46969. Does repro in Mac nightly at r47011. If I had to guess, I'd guess http://trac.webkit.org/changeset/47008 is the cause.

<rdar://problem/7248529>

Created attachment 40041 [details] Adds a dumpAsText test to reproduce the bug This patch adds a dumpAsText test that reproduces the bug. It seems like it's a bug with createMarkup (not verified yet). It seems like we're overriding the style inherited by the ancestor elements by that of the document.

Did some investigation. The background color is rgba(0,0,0,0) at http://trac.webkit.org/browser/trunk/WebCore/editing/markup.cpp#L1007 We should change the editingStyleAtPosition so that it returns the actual background color instead of CSS value. Adele, is there a function in renderer that returns the effective background color? +adele

No, there isn't. Enrica had been looking into a similar issue with applying the right style when inserting paragraphs. She hit the same problem where the computed style doesn't include the real backgound color. Maybe her fix for that will shed light on how to fix this too.

(In reply to comment #5) > No, there isn't. Enrica had been looking into a similar issue with applying > the right style when inserting paragraphs. She hit the same problem where the > computed style doesn't include the real backgound color. Maybe her fix for > that will shed light on how to fix this too. I see. I considered several options for this over summer, one of which was to traverse the tree backwards to compute the color. But this is problematic when an element is positioned with absolute positioning (i.e. element overflows the parent), or when the background has an alpha value that's neither 0 nor 255. Maybe we should see what Firefox / MSIE do and mimic their behavior.

Created attachment 40966 [details] Patch

(In reply to comment #7) > Created an attachment (id=40966) [details] > Patch This patch looks great but is background color preserved with indentation / outdentation with your approach? Also doesn't your approach imply that we clone b, i, etc... elements even when styleWithCSS is set to true?

Comment on attachment 40966 [details] Patch Rejecting patch 40966 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1 Running build-dumprendertree Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 11411 test cases. editing/execCommand/indent-with-style.html -> failed Exiting early after 1 failures. 3719 tests run. 62.46s total testing time 3718 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 1 test case (<1%) had stderr output

Comment on attachment 40966 [details] Patch Either this is a real failure, or the bots were simply behind? bug 30098.

If you believe this was incorrectly rejected from the queue, please feel free to add it again. :)

Created attachment 40975 [details] Patch2 Adding to the queue one more time

Does WebKit copy the background color when copying text to clipboard with this patch?

Comment on attachment 40966 [details] Patch It looks like there is no difference between this and the new "patch2", so I'm setting cq+ on this one. cq+ is ignored unless there is also a review+ on the patch.

Comment on attachment 40966 [details] Patch Rejecting patch 40966 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1 Running build-dumprendertree Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 11418 test cases. editing/execCommand/indent-with-style.html -> failed Exiting early after 1 failures. 3719 tests run. 56.67s total testing time 3718 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 1 test case (<1%) had stderr output

This patch is no longer valid. I'm working on a new patch that solves the problem once and for all.

Comment on attachment 40966 [details] Patch Clearing adele's r+ and marking this patch as obsolete since Enrica has said it is no longer valid.

Created attachment 41516 [details] Patch3 This patch changes significantly the way Indent is performed, by using an improved version of moveParagraph called moveParagraphWithClones. The idea behind this is to preserve as much as possible the original markup of the paragraph being indented. I plan on changing the outdent part of the command in a future patch as well as the implementation of the InsertList command.

(In reply to comment #18) > This patch changes significantly the way Indent is performed, by using an > improved version of moveParagraph called moveParagraphWithClones. Very nice.

Comment on attachment 41516 [details] Patch3 I remember us talking about moving originals, not clones. Is there any advantage to eventually doing that? We'd lose the ability to rely on DeleteSelectionCommand for removal. + if (item->isTextNode()) + child = static_cast<Element*>(((Node *)item)->cloneNode(true).get()); + else if (isTableElement(item)) + child = item->cloneElementWithChildren(); + else + child = item->cloneElementWithoutChildren(); Hmm. What if the DOM is: <div><textnode 1><textnode 2></div> startNode is the textnode 1. Will textnode 2 be cloned and put in the hierarchy? > +// the hierarchy with all the children. > +// If case startNode and outerBlock differ, we clone each none in between individually Typo: "If case startNode and outerBlock differ". > + for (size_t i = ancestors.size(); i != 0; --i) { > + Element* item = ancestors[i - 1]; Little odd that i is not the array index. Why not loop from i = ancestors.size - 1 while i > 0? > This tests indenting an empty paragraph with bold already applied to it. The word below should be bold. >-<blockquote class="webkit-indent-blockquote" style="margin: 0 0 0 40px; border: none; padding: 0px;"><b>Bold</b></blockquote> >+<blockquote class="webkit-indent-blockquote" style="margin: 0 0 0 40px; border: none; padding: 0px;"><div><b>Bold</b></div></blockquote> It seems OK that we're preserving the block that originally contained the paragraph in this case, but is it always? You may need to parameterize this behavior...I know that at least for deletion we wouldn't want it. + void moveParagraphWithClones(const VisiblePosition&, const VisiblePosition&, Node*, Node*); I know that elsewhere in this file we're bad about this, but...I think you should name the parameters here, since they aren't obvious.

(In reply to comment #20) > (From update of attachment 41516 [details]) > I remember us talking about moving originals, not clones. Is there any > advantage to eventually doing that? We'd lose the ability to rely on > DeleteSelectionCommand for removal. I'm not sure I understand your point here. The new implementation uses DeleteSelectionCommand (otherwise undo won't work) but preserves the existing markup, instead of trying to create a new markup to reproduce the style at the position. The computed style does not represent with 100% accuracy the appearance of the element at the position (i.e. when you have a span wth bg color and a font tag with fontcolor). > > + if (item->isTextNode()) > + child = static_cast<Element*>(((Node > *)item)->cloneNode(true).get()); > + else if (isTableElement(item)) > + child = item->cloneElementWithChildren(); > + else > + child = item->cloneElementWithoutChildren(); > > Hmm. What if the DOM is: > > <div><textnode 1><textnode 2></div> > > startNode is the textnode 1. Will textnode 2 be cloned and put in the > hierarchy? > No, in this case it clones without children. If you have something like that, we produce <blockquote class... style="margin...."><div><text node1></div><div><textnode 2></div><blockquote>. This is the only case I could find where we create an additional div tag. The ultimate goal I have is to change the way Indent is implemented, and do something similar to what the align command does. It will try to find the nearest enclosing block for the selection and simply change the style of the block. (See moveParagraphContentsToNewBlockIfNecessary in CompositeEditCommand) In this case something like <div> Hello <br> World <br> Today </div> where the selection is the first two paragraphs, will be changed by the indent command into: <div style="margin 0 0 0 40px;"> Hello <br> World </div> <div> Today </div> > > > +// the hierarchy with all the children. > > +// If case startNode and outerBlock differ, we clone each none in between individually > > Typo: "If case startNode and outerBlock differ". > Ok. > > > + for (size_t i = ancestors.size(); i != 0; --i) { > > + Element* item = ancestors[i - 1]; > > Little odd that i is not the array index. Why not loop from i = ancestors.size > - 1 while i > 0? > I agree. I'll change it. > > > This tests indenting an empty paragraph with bold already applied to it. The word below should be bold. > >-<blockquote class="webkit-indent-blockquote" style="margin: 0 0 0 40px; border: none; padding: 0px;"><b>Bold</b></blockquote> > >+<blockquote class="webkit-indent-blockquote" style="margin: 0 0 0 40px; border: none; padding: 0px;"><div><b>Bold</b></div></blockquote> > > It seems OK that we're preserving the block that originally contained the > paragraph in this case, but is it always? You may need to parameterize this > behavior...I know that at least for deletion we wouldn't want it. > We are not using this method for deletion. The idea is to use it for commands like Indent, InsertList, Align, etc. moveParagraphWithClones uses internally the deleteSelection command that uses the original moveParagraph. > > + void moveParagraphWithClones(const VisiblePosition&, const > VisiblePosition&, Node*, Node*); > > I know that elsewhere in this file we're bad about this, but...I think you > should name the parameters here, since they aren't obvious. I did it on purpose because I thought this is was the coding style. I don't like it myself and if it is ok with everyone, I'll add the parameters name back.

(In reply to comment #21) > (In reply to comment #20) > > (From update of attachment 41516 [details] [details]) > > I remember us talking about moving originals, not clones. Is there any > > advantage to eventually doing that? We'd lose the ability to rely on > > DeleteSelectionCommand for removal. > > I'm not sure I understand your point here. The new implementation uses > DeleteSelectionCommand (otherwise undo won't work) > but preserves the existing markup, instead of trying to create a new markup to > reproduce the style at the position. The computed style does not represent with > 100% accuracy the appearance of the element at the position (i.e. when you have > a span wth bg color and a font tag with fontcolor). > > > > > + if (item->isTextNode()) > > + child = static_cast<Element*>(((Node > > *)item)->cloneNode(true).get()); > > + else if (isTableElement(item)) > > + child = item->cloneElementWithChildren(); > > + else > > + child = item->cloneElementWithoutChildren(); > > > > Hmm. What if the DOM is: > > > > <div><textnode 1><textnode 2></div> > > > > startNode is the textnode 1. Will textnode 2 be cloned and put in the > > hierarchy? > > > > No, in this case it clones without children. If you have something like that, > we produce > <blockquote class... style="margin...."><div><text node1></div><div><textnode > 2></div><blockquote>. > This is the only case I could find where we create an additional div tag. > The ultimate goal I have is to change the way Indent is implemented, and do > something similar to what the align command > does. It will try to find the nearest enclosing block for the selection and > simply change the style of the block. > (See moveParagraphContentsToNewBlockIfNecessary in CompositeEditCommand) > In this case something like > > <div> > Hello > <br> > World > <br> > Today > </div> > > where the selection is the first two paragraphs, will be changed by the indent > command into: > > <div style="margin 0 0 0 40px;"> > Hello > <br> > World > </div> > <div> > Today > </div> > > > > > > > +// the hierarchy with all the children. > > > +// If case startNode and outerBlock differ, we clone each none in between individually > > > > Typo: "If case startNode and outerBlock differ". > > > > Ok. > > > > > > + for (size_t i = ancestors.size(); i != 0; --i) { > > > + Element* item = ancestors[i - 1]; > > > > Little odd that i is not the array index. Why not loop from i = ancestors.size > > - 1 while i > 0? > > > > I agree. I'll change it. I take it back. This is the correct logic. If ancestors.size() is 1, I still want to execute this loop, with the code you are suggesting I wouldn't. > > > > > > This tests indenting an empty paragraph with bold already applied to it. The word below should be bold. > > >-<blockquote class="webkit-indent-blockquote" style="margin: 0 0 0 40px; border: none; padding: 0px;"><b>Bold</b></blockquote> > > >+<blockquote class="webkit-indent-blockquote" style="margin: 0 0 0 40px; border: none; padding: 0px;"><div><b>Bold</b></div></blockquote> > > > > It seems OK that we're preserving the block that originally contained the > > paragraph in this case, but is it always? You may need to parameterize this > > behavior...I know that at least for deletion we wouldn't want it. > > > > We are not using this method for deletion. The idea is to use it for commands > like Indent, InsertList, Align, etc. > moveParagraphWithClones uses internally the deleteSelection command that uses > the original moveParagraph. > > > > > + void moveParagraphWithClones(const VisiblePosition&, const > > VisiblePosition&, Node*, Node*); > > > > I know that elsewhere in this file we're bad about this, but...I think you > > should name the parameters here, since they aren't obvious. > > I did it on purpose because I thought this is was the coding style. I don't > like it myself and if it is ok with everyone, I'll add the parameters name > back.

Created attachment 41572 [details] Patch4 Fixed some typos, removed commented line in ApplyStyleCommand.cpp and added parameter names to moveParagraphWithClones.

> > > > + for (size_t i = ancestors.size(); i != 0; --i) { > > > > + Element* item = ancestors[i - 1]; > > > > > > Little odd that i is not the array index. Why not loop from i = ancestors.size > > > - 1 while i > 0? > > > > > > > I agree. I'll change it. > > I take it back. This is the correct logic. If ancestors.size() is 1, I still > want to execute this loop, with the code you are suggesting I wouldn't. Oops I meant to say while i >= 0. I guess you need to leave what you've done because size_t can't be negative.

Comment on attachment 41572 [details] Patch4 This looks good. > + Vector<Element*> ancestors; I think you should move this declaration to just before the "for" statement instead of having it at the top of the function. This needs to be a vector of RefPtr<Element> because there's no guarantee that the ancestors won't be deallocated or something like that in JavaScript mutation handlers. > + // first we handle the case of the hierarchy with one node only In WebKit we normally use sentence-style comments, with capital letters and periods. > + if (outerBlock->isTextNode()) > + return static_cast<Element*>(startNode->cloneNode(true).get()); How can an Element* be a text node? > + // insert each element from startNode to outerBlock (excluded) in a list Comment format. > + for (Element* n = static_cast<Element*>(startNode); n && n != outerBlock; n = n->parentElement()) If startNode is guaranteed to be an Element, then it should be an Element*. If it's not guaranteed to be an Element, then this cast to Element* is not guaranteed to work and this should be coded another way that dose not depend on that. > + Element* item = ancestors[i - 1]; > + RefPtr<Element> child; > + if (item->isTextNode()) > + child = static_cast<Element*>(((Node *)item)->cloneNode(true).get()); You should not be casting to Node* here. Instead you should be using cloneElementWithChildren. But also, if item is an Element, then how can it be a text node? We should override some of the Node functions with private non-implemented functions in derived classes so these kinds of mistakes can be caught at compile time. > +// This is a new version of moveParagraph that preserves style by keeping the original markup > +// It is currently used only by IndentOutdentCommand but it is meant to be used in the near > +// future by several other commands such as InsertList and the align commands. Comments should not say things like "new" and "the near future" since they are likely to last a long time. > + RefPtr<Element> newParagraph = cloneHierarchyBetween(start.node(), static_cast<Element*>(startNode)); What guarantees that startNode is an Element*? If it's known to be an Element*, then it should already have that type. If not, then there needs to be some sort of check. This kind of type cast can easily lead to crashes later. > + // Deleting a paragraph will leave a placeholder. Remove it (and prune > + // empty or unrendered parents). WebKit code uses one space after a period, not two. > + // There is a preserved '\n' at caretAfterDelete. > + Text* textNode = static_cast<Text*>(node); What guarantees this is a text node? We can't do this cast unless we are certain it is one. > + if (beforeParagraph.isNotNull() && !isTableElement(beforeParagraph.deepEquivalent().node()) && (!isEndOfParagraph(beforeParagraph) || beforeParagraph == afterParagraph)) { > + // FIXME: Trim text between beforeParagraph and afterParagraph if they aren't equal. > + insertNodeAt(createBreakElement(document()), beforeParagraph.deepEquivalent()); > + // Need an updateLayout here in case inserting the br has split a text node. > + updateLayout(); > + } I don't understand that comment about needing an updateLayout. Normally code calls updateLayout before doing work if it's going to depend on layout. It's not normal to call updateLayout after doing something. There are a lot of stray trailing spaces in the source file. It would be best to not have those. > + void moveParagraphWithClones(const VisiblePosition& startOfParagraphToMove, const VisiblePosition& endOfParagraphToMove, Node* blockNode, Node* startNode); I don't understand what blockNode and startNode are. > + PassRefPtr<Element> cloneHierarchyBetween(Node*, Element*); I'm not sure what it means to be between a Node and an Element. Probably need names for these arguments. Do test cases cover all the branches in the code? I think you need to add more test cases. There are a lot of if statements and such in the code and we need to make sure there is a test that covers each one. I am particularly interested in test cases that involve collapsed whitespace as well. review- because at least some of the comments above will lead to code changes, I think

> I'm not sure I understand your point here. I was wondering if there was any advantage to moving the actual nodes, instead of cloning them, but I can't think of any. Ignore my comment. > > Hmm. What if the DOM is: > > > > <div><textnode 1><textnode 2></div> > > > > startNode is the textnode 1. Will textnode 2 be cloned and put in the > > hierarchy? > > > > No, in this case it clones without children. Then in this case indent must be broken, yes? It will only indent the first text node (and its parents). Here's an better test case, indent: <div>hello<img>world</div> > We are not using this method for deletion. No, but I assumed your new function would eventually be deployed everywhere, in place of the old moveParagraphs. If that's not the plan you can ignore what I said.

Created attachment 41699 [details] Patch5 This patch addresses all the comments made by Darin and covers the case pointed out by Justin in his comments. I've also added a new test case to cover several indent scenarios not covered by the existing tests.

Comment on attachment 41699 [details] Patch5 Looks good! In some cases where you moved code or reused existing code (like for cleanupAfterDeletion), it would be good to make a note of that in the ChangeLog. It makes it a little easier to review. Just something to keep in mind for the future. I'll commit this for you...

Committed revision 49985.