Issue 1280123004: Don't allow whitespace between elements with display:table-cell

Issue 1280123004: Don't allow whitespace between elements with display:table-cell (Closed)

Created:
5 years, 4 months ago by rhogan

Modified:
5 years, 3 months ago

Reviewers:
mstensho (USE GERRIT), esprehn

CC:
blink-reviews, blink-reviews-rendering, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1

Base URL:
svn://svn.chromium.org/blink/trunk

Target Ref:
refs/heads/master

Project:
blink

Visibility:
Public.

More Reviews

Description

Don't allow whitespace between elements with display:table-cell In https://codereview.chromium.org/1065563005 we started allowing whitespace inside anonymous table cells. We're only interested in anonymous table cells that were generated to contain non-table-part content. If the anonymous cell was generated to contain a form element with display:table-cell, for example, then that is not a situation where we want to treat any whitespace before and after the form element as renderable. BUG=515771 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201329

Patch Set 1 #

Patch Set 2 : Updated #

Patch Set 3 : Updated #

Patch Set 4 : Updated #

Total comments: 3

Patch Set 5 : Updated #

Total comments: 1

Patch Set 6 : Updated #

Patch Set 7 : Updated #

Total comments: 4

Patch Set 8 : Updated #

Created: 5 years, 3 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+152 lines, -28 lines)			Patch
M	LayoutTests/TestExpectations	View	1 2 3 4 5 6 7	1 chunk	+3 lines, -0 lines	0 comments	Download
M	LayoutTests/fast/table/table-row-before-after-content-around-table-cell-expected.txt	View	1 2 3 4 5	1 chunk	+0 lines, -2 lines	0 comments	Download
A	LayoutTests/fast/table/whitespace-between-elements-with-table-cell-display.html	View	1 2 3 4	1 chunk	+28 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/table/whitespace-between-elements-with-table-cell-display-2.html	View	1 2 3 4	1 chunk	+16 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/table/whitespace-between-elements-with-table-cell-display-2-expected.html	View	1 2 3 4	1 chunk	+9 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/table/whitespace-between-elements-with-table-cell-display-3.html	View	1 2 3 4 5	1 chunk	+19 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/table/whitespace-between-elements-with-table-cell-display-3-expected.html	View	1 2 3 4 5	1 chunk	+17 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/table/whitespace-between-elements-with-table-cell-display-4.html	View	1 2 3 4 5	1 chunk	+12 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/table/whitespace-between-elements-with-table-cell-display-4-expected.txt	View	1 2 3 4 5	1 chunk	+5 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/table/whitespace-between-elements-with-table-cell-display-expected.html	View	1 2 3 4	1 chunk	+25 lines, -0 lines	0 comments	Download
M	LayoutTests/platform/linux/tables/mozilla/bugs/bug2479-1-expected.txt	View	1 2 3 4 5	2 chunks	+0 lines, -13 lines	0 comments	Download
M	Source/core/dom/Text.cpp	View	1 2 3 4 5 6 7	3 chunks	+18 lines, -13 lines	0 comments	Download

Messages

Total messages: 17 (3 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

esprehn

https://codereview.chromium.org/1280123004/diff/60001/Source/core/layout/LayoutTableRow.cpp File Source/core/layout/LayoutTableRow.cpp (right): https://codereview.chromium.org/1280123004/diff/60001/Source/core/layout/LayoutTableRow.cpp#newcode119 Source/core/layout/LayoutTableRow.cpp:119: createAndAddAnonymousCell(child, beforeChild); This doesn't feel like a good idea, ...

5 years, 4 months ago (2015-08-17 17:58:54 UTC) #3

rhogan

5 years, 4 months ago (2015-08-17 18:16:57 UTC) #4

esprehn

I'll have to think more about this, letting inputs be display: table-cell like this is ...

5 years, 4 months ago (2015-08-17 18:27:09 UTC) #5

mstensho (USE GERRIT)

If no other engine supports display:table-cell on replaced content, why is that the correct fix? ...

5 years, 4 months ago (2015-08-17 19:38:23 UTC) #7

rhogan

Sorry for taking so long to respond. On 2015/08/17 at 19:38:23, mstensho wrote: > Isn't ...

5 years, 4 months ago (2015-08-24 21:08:33 UTC) #8

mstensho (USE GERRIT)

https://codereview.chromium.org/1280123004/diff/80001/Source/core/dom/Text.cpp File Source/core/dom/Text.cpp (right): https://codereview.chromium.org/1280123004/diff/80001/Source/core/dom/Text.cpp#newcode251 Source/core/dom/Text.cpp:251: return !firstChild || !firstChild->node() || (firstChild->style()->originalDisplay() != TABLE_CELL && ...

5 years, 4 months ago (2015-08-25 11:24:06 UTC) #9

https://codereview.chromium.org/1280123004/diff/80001/Source/core/dom/Text.cpp
File Source/core/dom/Text.cpp (right):

https://codereview.chromium.org/1280123004/diff/80001/Source/core/dom/Text.cp...
Source/core/dom/Text.cpp:251: return !firstChild || !firstChild->node() ||
(firstChild->style()->originalDisplay() != TABLE_CELL &&
!isHTMLTableCellElement(firstChild->node()));
I don't think this is quite right, because:

<!DOCTYPE html>
<style>
input, button {
    display: table-cell;
    width: 50px;
    height: 100px;
    background: blue;
    border: none;
    padding: 0;
    vertical-align: bottom;
}
</style>
<p>There should be a blue square below.</p>
<div style="display:table;">
    <div></div>
    <input>
    <button></button>
</div>

The first child here is a regular DIV, which causes us to return true. It fails
to detect the table-cell displayed siblings.

This is browser interoperability hell. They all prevent whitespace between the
input and the button, but for different reasons. MSIE and Presto actually lay
out the button and input elements as table-cells, it seems, while Gecko just
somehow manages to prevent whitespace between them, without treating them as
table cells. I think MSIE and Presto are right (why should they not become table
cells, when they have display:table-cell?), but that's hard to achieve in Blink,
since there's no display-outer / display-inner separation in the implementation
(Presto has had this since day one, more or less). A table cell cannot be both a
LayoutTableCell and LayoutTextControl at the same time, for instance.

So, yeah, our best bet would be to do exactly like Gecko, which apparently is to
not lay them out as table cells, but somehow prevent space between them.

This change is introducing a regression:

<!DOCTYPE html>
<p>There should be two words (with space between them) below.</p>
<div style="display:table;">
    <input style="display:table-cell; background:transparent; border:none;">
    <span>two</span>
    <span>words</span>
</div>

Finally, I have a general remark about this code (which should be dealt with in
a separate CL):
Why are we returning true if the table cell is childless? Why would we create an
anonymous table cell around ... NOTHING? Also, the name
hasGeneratedAnonymousTableCells() is a bit inaccurate, since it only returns
true in certain cases for anonymous table cells.

rhogan

On 2015/08/25 at 11:24:06, mstensho wrote: > https://codereview.chromium.org/1280123004/diff/80001/Source/core/dom/Text.cpp > Finally, I have a general remark ...

5 years, 4 months ago (2015-08-25 19:36:46 UTC) #10

mstensho (USE GERRIT)

This looks great! https://codereview.chromium.org/1280123004/diff/120001/LayoutTests/fast/table/whitespace-between-elements-with-table-cell-display-3 File LayoutTests/fast/table/whitespace-between-elements-with-table-cell-display-3 (right): https://codereview.chromium.org/1280123004/diff/120001/LayoutTests/fast/table/whitespace-between-elements-with-table-cell-display-3#newcode1 LayoutTests/fast/table/whitespace-between-elements-with-table-cell-display-3:1: <!DOCTYPE html> Poor file name has ...

5 years, 3 months ago (2015-08-26 18:45:56 UTC) #11

rhogan

On 2015/08/26 at 18:45:56, mstensho wrote: > https://codereview.chromium.org/1280123004/diff/120001/Source/core/dom/Text.cpp#newcode263 > Source/core/dom/Text.cpp:263: static inline bool canHaveWhitespaceChildren(const LayoutObject& ...

5 years, 3 months ago (2015-08-27 19:08:26 UTC) #12

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1280123004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1280123004/140001

5 years, 3 months ago (2015-08-27 19:21:45 UTC) #15

commit-bot: I haz the power

Committed patchset #8 (id:140001) as https://src.chromium.org/viewvc/blink?view=rev&revision=201329

5 years, 3 months ago (2015-08-27 19:26:43 UTC) #16

nednguyen

5 years, 3 months ago (2015-09-11 18:24:50 UTC) #17

Message was sent while issue was closed.

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/1338803003/ by nednguyen@google.com.

The reason for reverting is: Compile failure on 8011 stable (mac , precise 32,
precise 64)

BUG=530685.

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages