Bug 38874 - Adding block elements to a block and removing them changes the height of the element
Summary: Adding block elements to a block and removing them changes the height of the ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Enrica Casucci
URL:
Keywords: InRadar
Depends on: 39044
Blocks:
  Show dependency treegraph
 
Reported: 2010-05-10 15:44 PDT by Enrica Casucci
Modified: 2010-05-17 13:51 PDT (History)
9 users (show)

See Also:


Attachments
Test case (868 bytes, text/html)
2010-05-10 15:44 PDT, Enrica Casucci
no flags Details
Patch (8.84 KB, patch)
2010-05-10 16:24 PDT, Enrica Casucci
no flags Details | Formatted Diff | Diff
Patch2 (8.89 KB, patch)
2010-05-11 09:35 PDT, Enrica Casucci
hyatt: review-
Details | Formatted Diff | Diff
Patch3 (9.17 KB, text/plain)
2010-05-11 14:04 PDT, Enrica Casucci
no flags Details
A real patch (9.17 KB, patch)
2010-05-11 14:08 PDT, Enrica Casucci
hyatt: review+
Details | Formatted Diff | Diff
Patch4 (10.64 KB, patch)
2010-05-12 14:14 PDT, Enrica Casucci
hyatt: review+
Details | Formatted Diff | Diff
Patch5 (8.91 KB, patch)
2010-05-13 13:39 PDT, Enrica Casucci
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Enrica Casucci 2010-05-10 15:44:50 PDT
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.
Comment 1 Enrica Casucci 2010-05-10 16:24:38 PDT
Created attachment 55618 [details]
Patch
Comment 2 Alexey Proskuryakov 2010-05-10 19:57:25 PDT
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.
Comment 3 Enrica Casucci 2010-05-11 09:35:11 PDT
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.
Comment 4 Alexey Proskuryakov 2010-05-11 10:11:05 PDT
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.
Comment 5 Enrica Casucci 2010-05-11 10:19:24 PDT
(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 6 Dave Hyatt 2010-05-11 13:25:08 PDT
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.
Comment 7 Enrica Casucci 2010-05-11 13:34:51 PDT
(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!
Comment 8 Enrica Casucci 2010-05-11 14:04:38 PDT
Created attachment 55751 [details]
Patch3
Comment 9 Enrica Casucci 2010-05-11 14:08:18 PDT
Created attachment 55753 [details]
A real patch

one more time, with the right flags.
Comment 10 Dave Hyatt 2010-05-12 10:27:45 PDT
Comment on attachment 55753 [details]
A real patch

r=me
Comment 11 Enrica Casucci 2010-05-12 11:30:28 PDT
Committed revision 59245.
Comment 12 Eric Seidel (no email) 2010-05-12 12:11:03 PDT
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
Comment 13 WebKit Review Bot 2010-05-12 12:30:51 PDT
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
Comment 14 Eric Seidel (no email) 2010-05-12 12:32:13 PDT
This may have broken Qt too:
http://build.webkit.org/results/Qt%20Linux%20Release/r59246%20(11650)/results.html
Comment 15 Eric Seidel (no email) 2010-05-12 13:11:23 PDT
Reverted r59245 for reason:

Broke at least one test on multiple platforms

Committed r59256: <http://trac.webkit.org/changeset/59256>
Comment 16 mitz 2010-05-12 13:32:44 PDT
(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.
Comment 17 Enrica Casucci 2010-05-12 14:14:30 PDT
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 18 Dave Hyatt 2010-05-12 14:16:36 PDT
Comment on attachment 55897 [details]
Patch4

r=me
Comment 19 Enrica Casucci 2010-05-12 14:25:08 PDT
Committed revision 59270.
Comment 20 WebKit Review Bot 2010-05-12 14:45:07 PDT
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
Comment 21 Eric Seidel (no email) 2010-05-12 15:05:16 PDT
It appears gtk needs result updates.
Comment 22 Enrica Casucci 2010-05-12 16:11:53 PDT
(In reply to comment #21)
> It appears gtk needs result updates.

Ossy fixed it in r59272 at 2:47pm.
Comment 23 Simon Fraser (smfr) 2010-05-12 21:37:14 PDT
This caused a new crash: bug 39042
Comment 24 Simon Fraser (smfr) 2010-05-12 21:42:44 PDT
We had to roll this out again: bug 39042.
Comment 25 Mark Rowe (bdash) 2010-05-12 21:54:26 PDT
<rdar://problem/7978128>
Comment 26 Enrica Casucci 2010-05-13 13:39:47 PDT
Created attachment 56016 [details]
Patch5

New patch that fixes the crash.
Comment 27 Dave Hyatt 2010-05-13 13:43:41 PDT
Comment on attachment 56016 [details]
Patch5

r=me
Comment 28 Enrica Casucci 2010-05-13 13:49:46 PDT
Committed revision 59385.
Comment 29 WebKit Review Bot 2010-05-13 15:29:35 PDT
http://trac.webkit.org/changeset/59385 might have broken Tiger Intel Release
Comment 30 James Robinson 2010-05-17 13:51:06 PDT
This is still causing crashes: https://bugs.webkit.org/show_bug.cgi?id=39143