Closed Bug 1692607 Opened 3 years ago Closed 3 years ago

Empty columns doesn't apply CSS border-spacing (cellspacing)

Categories

(Core :: Layout: Tables, defect, P4)

Product:
Core
Component:
Layout: Tables
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
87 Branch
Tracking Flags:
Tracking Status
firefox87 --- fixed

People

(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)

Details

(Keywords: testcase)

Attachments

(2 files)

Testcase #1
583 bytes, text/html
Details
Bug 1692607 - Make table columns without an originating cell have cell spacing if their corresponding <col> has a non-zero definite computed size/min-size. r=dholbert
48 bytes, text/x-phabricator-request
Details | Review
Attached file Testcase #1 —

Kudos to atotic@chromium.org for finding this bug: https://bugs.chromium.org/p/chromium/issues/detail?id=1173766#c5

The two tables in Testcase #1 should have the same border-box width.
I'm guessing we're missing to apply border-spacing between empty columns.

https://drafts.csswg.org/css-tables-3/#total-horizontal-border-spacing

I've developed a comprehensive test case for column-track merging in wpt.
If you are fixing issues with it, it might come in useful:
https://wpt.fyi/results/css/css-tables/column-track-merging.html?label=experimental&label=master&aligned

Nice, thanks Aleks.

So, we actually skip cell-spacing for empty columns intentionally:
https://searchfox.org/mozilla-central/rev/bf03edd45ee0804f382c1f05ec88a05f5d88833c/layout/tables/nsTableFrame.cpp#3873
I wonder if the spec said something different in the past?

Returning true there makes the testcase renders as expected.
I guess we should change it to check for <col> with an explicit size instead.

Assignee: nobody → mats

I see. You explicitly return false if COL is inside auto table, and has no originating cells.

I think explicit size is a better criteria, since that matches what happens in fixed. It would also make sense to developers: if you specify column widths, you do not want table to change size when you add cells later.

I do not think that standard is explicit about this. Every browser did something different: Chrome/Safari/IE just merged COLs without originating cells, whether they had widths or not. That was clearly against the standard. FF was the most standard compliant.

I'd like to ship "COLs have border-spacing" in Chrome if it is web compatible. My code will be in M91 for sure, and maybe M90.

I think FF will be happy with the new Chrome tables implementation, it is closer to FF that Chrome Legacy. I wrote a bunch of tests testing what I believe standard should be, and am slowly landing them in css/css-tables/tentative.

Also thank you for having the most standard-compliant implementation. Without it, my job would have been much harder.

Aleks, I think the check should be non-zero definite size, so width:10%, width:calc(100px + 1%) and width:0 should all be ignored when there's no originating cell. Firefox, Safari and Chrome (Legacy) all agree on that. Chrome TablesNG is incompatible. I filed https://bugs.chromium.org/p/chromium/issues/detail?id=1179497 about that.

Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5085098b5d9a
Make table columns without an originating cell have cell spacing if their corresponding <col> has a non-zero definite computed size/min-size.  r=dholbert
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/27685 for changes under testing/web-platform/tests

I can do this. Just want to make sure this is what we want.
Usually, these questions are resolved by csswg. Tables have been shipping since '95 and standard is still very incomplete.
For example, for mergeable tracks, this is what we get:
track is mergeable if "there is no table-column/table-row box defining the said track explicitly".
0px widths, %ge vs px, none of that is covered by the standard.

Is merging %ge columns the right thing to do?
Browser implementor viewpoint:

  • FF does it, and FF was the most standard compliant, rational, table implementation of all browsers. For me, this is the best supporting argument.
  • Safari/LegacyChrome do it, but they also merge all columns.

Developer viewpoint:
When would a developer care about mergeable columns? The best use case I could come up with is this:
I have a table whose data has not loaded yet. I'd like table columns to keep the same width after data is loaded.
My header columns have colspan>1 cells, defining several mergeable columns. N

<th colspan=2>Foo</th>
<th colspan=2>Bar</th>

I've split the Foo and Bar field into two cells in the data part of the table, so a data line looks something like this:
<td>Phooey</td>
<td>Fooey</td>
<td>Barsky</td>
<td>Zbasky</td>

And I'd like my data columns to have following widths: 20% 30% 20% 30%, so I add these tags:
<col style="width:20%">
<col style="width:30%">
<col style="width:20%">
<col style="width:30%">

Your proposal is to ignore the %ges. I feel that the developer would expect the %ges to behave like pixels.

No browser does this today, but they all do something different.

I am not sure whether not ignoring %ges is even an option, whether it is web compatible.
I am already concerned about not merging pixel columns, because only FF does it..
I'll have more data about any web compatibility problems once TablesNG enter beta.

My feeling is that because empty column widths were so incompatible, developers did not rely on them behaving in any particular way.

Wait, I just took a look at my column merging test:
http://wpt.live/css/css-tables/column-track-merging.html

Here, Firefox does not merge %ge columns in test #12.

I thought that <col span=5> might be the problem, but 5 cols behave the same way.

Upstream PR was closed without merging

The failure is TEST-UNEXPECTED-PASS, fwiw. It's odd that it only failed on Android given that we don't have any platform specific code in table layout. Maybe this change tickles an existing incremental reflow bug on this platform only?
Anyway, the failure doesn't indicate that anything is wrong with the patch per se, so I'll just annotate that test as random on that platform for now and reland it...

Flags: needinfo?(mats)
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c4132e0c4149
Make table columns without an originating cell have cell spacing if their corresponding <col> has a non-zero definite computed size/min-size.  r=dholbert
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/097b4127ec15
set column-track-merging.html table 11 as passing. CLOSED TREE
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/40ce0fdd47c0
set column-track-merging.html table 11 as passing. CLOSED TREE
Upstream PR merged by moz-wptsync-bot

https://hg.mozilla.org/mozilla-central/rev/c4132e0c4149
https://hg.mozilla.org/mozilla-central/rev/40ce0fdd47c0

Status: NEW → RESOLVED
Closed: 3 years ago
status-firefox87: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 87 Branch

(In reply to Aleks Totic from comment #9)

My feeling is that because empty column widths were so incompatible, developers did not rely on them behaving in any particular way.

Yeah, I don't feel strongly either way. In this bug I'm only addressing the issue that we failed to add in the border spacing for the non-zero columns that were there, which is clearly wrong.

FYI, I haven't been involved in the css-tables spec discussions so I don't know how much consensus there is on this particular issue. I just filed the Chromium issue since it seemed Firefox and the spec agreed on that test. Reading the spec again, I'm less sure what the intent of the spec really is regarding merging columns... Perhaps it would be simpler to just merge all columns that don't have an originating cell (in auto mode)? And not merge any columns in fixed mode.

I filed https://github.com/w3c/csswg-drafts/issues/6023 to sort this out.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: