Created attachment 55612 [details] Test case 1. Create an empty editable element. (<div contentEditable="true"></div> 2. Add block elements to it. (<div contentEditable="true"><div>aaa</div><div>bbb</div></div>) 3. Remove all the added elements (<div contentEditable="true"></div>) EXPECTED: the height of the element is the same before and after the dom mutation. ACTUAL: the height of the element is different.
Created attachment 55618 [details] Patch
A passing test shouldn't have red rectangles in its rendering. Also, the attached test case doesn't seem to work in Firefox (I didn't try one from the patch, but it looks identical). Keeping r? for code review, but please fix the test.
Created attachment 55710 [details] Patch2 Changed the border color to green and used appendChild instead of innerText to allow you to see the test in Firefox. Firefox behaves differently because it returns 0 height for empty editable blocks, but it is consistent. This bugs is about inconsistency of before and after. I've discussed this with Hyatt on IRC and he agreed on the approach.
Another way to fix this for Firefox would be to use innerHTML or textContent instead of innerText. I prefer innerHTML, because it works in both Firefox and IE. > Changed the border color to green I should have mentioned that the best color for this is blue - red is failure, green is success, and other colors are for content that indicates neither. But it's less important than not making it red.
(In reply to comment #4) > Another way to fix this for Firefox would be to use innerHTML or textContent instead of innerText. I prefer innerHTML, because it works in both Firefox and IE. > > > Changed the border color to green > > I should have mentioned that the best color for this is blue - red is failure, green is success, and other colors are for content that indicates neither. But it's less important than not making it red. I will change it to blue when I check in. Thanks for pointing me to the link with the guidelines.
Comment on attachment 55710 [details] Patch2 You really want to revert back to childrenInline = true when kids get removed, not at layout time. RenderBlock::removeChild seems like a better place to do this than layoutBlock. Something like this: if (!firstChild() && !childrenInline() && isRenderBlock()) setChildrenInline(true); Strictly speaking you could even revert back to childrenInline = true if all you have left are floating/positioned children, but that could get expensive to check, so I wouldn't bother.
(In reply to comment #6) > (From update of attachment 55710 [details]) > You really want to revert back to childrenInline = true when kids get removed, not at layout time. RenderBlock::removeChild seems like a better place to do this than layoutBlock. Something like this: > > if (!firstChild() && !childrenInline() && isRenderBlock()) > setChildrenInline(true); > > Strictly speaking you could even revert back to childrenInline = true if all you have left are floating/positioned children, but that could get expensive to check, so I wouldn't bother. That was my initial thought, but I wasn't sure if was missing some cases. I'll change the code the way you suggest it. Thanks!
Created attachment 55751 [details] Patch3
Created attachment 55753 [details] A real patch one more time, with the right flags.
Comment on attachment 55753 [details] A real patch r=me
Committed revision 59245.
Looks like this changed the results of fast/events/mouseout-dead-node.html: http://build.webkit.org/results/Tiger%20Intel%20Release/r59246%20(11943)/fast/events/mouseout-dead-node-pretty-diff.html
http://trac.webkit.org/changeset/59245 might have broken Leopard Intel Release (Tests), SnowLeopard Intel Release (Tests), and GTK Linux 64-bit Release The following changes are on the blame list: http://trac.webkit.org/changeset/59243 http://trac.webkit.org/changeset/59244 http://trac.webkit.org/changeset/59245 http://trac.webkit.org/changeset/59246
This may have broken Qt too: http://build.webkit.org/results/Qt%20Linux%20Release/r59246%20(11650)/results.html
Reverted r59245 for reason: Broke at least one test on multiple platforms Committed r59256: <http://trac.webkit.org/changeset/59256>
(In reply to comment #9) > one more time, with the right flags. You can change the flags after uploading an attachment by clicking the Details link next to it.
Created attachment 55897 [details] Patch4 Added the new result for the fast/event/mouseout-dead-node.html tests that I missed in the previous patch.
Comment on attachment 55897 [details] Patch4 r=me
Committed revision 59270.
http://trac.webkit.org/changeset/59270 might have broken GTK Linux 32-bit Release The following changes are on the blame list: http://trac.webkit.org/changeset/59269 http://trac.webkit.org/changeset/59270
It appears gtk needs result updates.
(In reply to comment #21) > It appears gtk needs result updates. Ossy fixed it in r59272 at 2:47pm.
This caused a new crash: bug 39042
We had to roll this out again: bug 39042.
<rdar://problem/7978128>
Created attachment 56016 [details] Patch5 New patch that fixes the crash.
Comment on attachment 56016 [details] Patch5 r=me
Committed revision 59385.
http://trac.webkit.org/changeset/59385 might have broken Tiger Intel Release
This is still causing crashes: https://bugs.webkit.org/show_bug.cgi?id=39143