Bug 38874 – Adding block elements to a block and removing them changes the height of the element

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Enrica Casucci]

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.

[Enrica Casucci]

Created attachment 55618 [details]
Patch

[Alexey Proskuryakov]

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.

[Enrica Casucci]

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.

[Alexey Proskuryakov]

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.

[Enrica Casucci]

(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.

[Dave Hyatt]

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.

[Enrica Casucci]

(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!

[Enrica Casucci]

Created attachment 55751 [details]
Patch3

[Enrica Casucci]

Created attachment 55753 [details]
A real patch

one more time, with the right flags.

[Dave Hyatt]

Comment on attachment 55753 [details]
A real patch

r=me

[Enrica Casucci]

Committed revision 59245.

[Eric Seidel (no email)]

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

[WebKit Review Bot]

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

[Eric Seidel (no email)]

This may have broken Qt too:
http://build.webkit.org/results/Qt%20Linux%20Release/r59246%20(11650)/results.html

[Eric Seidel (no email)]

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.

[Enrica Casucci]

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.

[Dave Hyatt]

Comment on attachment 55897 [details]
Patch4

r=me

[Enrica Casucci]

Committed revision 59270.

[WebKit Review Bot]

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

[Eric Seidel (no email)]

It appears gtk needs result updates.

[Enrica Casucci]

(In reply to comment #21)
> It appears gtk needs result updates.

Ossy fixed it in r59272 at 2:47pm.

[Simon Fraser (smfr)]

This caused a new crash: bug 39042

[Simon Fraser (smfr)]

We had to roll this out again: bug 39042.

[Mark Rowe (bdash)]

<rdar://problem/7978128>

[Enrica Casucci]

Created attachment 56016 [details]
Patch5

New patch that fixes the crash.

[Dave Hyatt]

Comment on attachment 56016 [details]
Patch5

r=me

[Enrica Casucci]

Committed revision 59385.

[WebKit Review Bot]

http://trac.webkit.org/changeset/59385 might have broken Tiger Intel Release

[James Robinson]

This is still causing crashes: https://bugs.webkit.org/show_bug.cgi?id=39143