1173958 - convert FixedTableLayoutStrategy to work with logical coordinates

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Jonathan Kew [:jfkthame]]

The table-layout:fixed equivalent to bug 1173305. This should be basically a bunch of s/width/isize/ and such like.

Comment 1

[Simon Montagu :smontagu]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd0249371ae9 (Since tables are still forced to horizontal writing mode, this is only to make sure that no existing tests regress).

I came across a problem with trying to adapt tests for fixed table layout to vertical writing modes: setting height on column seems to have no effect (where existing tests set width). Is that a known issue?

Comment 2

[Jonathan Kew [:jfkthame]]

(In reply to Simon Montagu :smontagu from comment #1)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd0249371ae9 (Since
> tables are still forced to horizontal writing mode, this is only to make
> sure that no existing tests regress).

In my local builds and in try jobs, I've removed that CSS line; maybe it's almost time to take it out of nightly, too, so that we can begin actually testing.

> 
> I came across a problem with trying to adapt tests for fixed table layout to
> vertical writing modes: setting height on column seems to have no effect
> (where existing tests set width). Is that a known issue?

I don't think so, offhand -- I just tried a minimal test locally and it seemed to have the expected effect. But I do have some additional patches in my local build, in addition to what's currently on inbound, so that might be an issue.

Probably worth filing a separate bug with testcase; we can always resolve/dupe it if it turns out to be part of something else.

Comment 3

[Simon Montagu :smontagu]

(In reply to Jonathan Kew (:jfkthame) from comment #2)
> > I came across a problem with trying to adapt tests for fixed table layout to
> > vertical writing modes: setting height on column seems to have no effect
> > (where existing tests set width). Is that a known issue?
> 
> I don't think so, offhand -- I just tried a minimal test locally and it
> seemed to have the expected effect.

Does your minimal test use the height property or set the height via style? If I'm reading the specs properly, height=... on <col> is not expected to work, because <col> (and <colgroup> and <table>) only have a width attribute and no height attribute, so I am in some doubt how to adapt the fixed table layout algorithm for vertical writing modes. Making width mean inline-size seems like the only practical option, but that would be inconsistent with the same attribute on other elements.

Comment 4

[Jonathan Kew [:jfkthame]]

(In reply to Simon Montagu :smontagu from comment #3)
> (In reply to Jonathan Kew (:jfkthame) from comment #2)
> > > I came across a problem with trying to adapt tests for fixed table layout to
> > > vertical writing modes: setting height on column seems to have no effect
> > > (where existing tests set width). Is that a known issue?
> > 
> > I don't think so, offhand -- I just tried a minimal test locally and it
> > seemed to have the expected effect.
> 
> Does your minimal test use the height property or set the height via style?

Ah... I set height via style.

> If I'm reading the specs properly, height=... on <col> is not expected to
> work, because <col> (and <colgroup> and <table>) only have a width attribute
> and no height attribute, so I am in some doubt how to adapt the fixed table
> layout algorithm for vertical writing modes.

Hmm. That's a good question, which I haven't thought about yet. I suppose the other possible approach would be to say that in vertical mode, these elements support a height attribute instead of the width attribute. But that would be a spec issue to be considered.

What do IE/webkit/blink do?

Comment 5

[Simon Montagu :smontagu]

It looks as if webkit/blink support both height and width attributes, but only use the one that computes to inline size. IE doesn't seem to support height.

All browsers seem to support both height and width on <table>, FWIW. 

Anyway, I'm going to leave this issue on one side for now and concentrate on dimensions set from style rather than HTML attributes.

Comment 6

[Simon Montagu :smontagu]

Attachment: Patch

Patch, tests to follow

Comment 7

[Jonathan Kew [:jfkthame]]

Comment on attachment 8625889 [details] [diff] [review]
Patch

Review of attachment 8625889 [details] [diff] [review]:
-----------------------------------------------------------------

Patch looks great, thanks. Tests would also be good, obviously, though testcases with writing-mode:vertical-rl probably can't land yet as that case is known to be unreliable (I'm working on it!).

Comment 8

[Simon Montagu :smontagu]

Attachment: Reformatting

Whitespace only follow-up patch to format the source like other mozilla source files

Comment 9

[Simon Montagu :smontagu]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=961b993e1a79

Comment 10

[Simon Montagu :smontagu]

Attachment: Reftests

These are a selection from the tests at http://test.csswg.org/harness/test/css21_dev/section/17.5.2.1/ converted to vertical writing-modes. Most of them work both in vertical-lr and vertical-rl, with a little care, except for those affected by bug 1177690.

Comment 11

[Jonathan Kew [:jfkthame]]

Comment on attachment 8626532 [details] [diff] [review]
Reformatting

Review of attachment 8626532 [details] [diff] [review]:
-----------------------------------------------------------------

rs=me.

::: layout/tables/FixedTableLayoutStrategy.cpp
@@ +286,5 @@
> +            // care?)
> +            nscoord spacing = mTableFrame->GetColSpacing(col);
> +            colISize = ((colISize + spacing) / colSpan) - spacing;
> +            if (colISize < 0)
> +              colISize = 0;

While you're here, this could use braces.

@@ +323,5 @@
>          }
> +        nscoord colISize = colFrame->GetFinalISize();
> +        colISize -= NSToCoordFloor(colFrame->GetPrefPercent() * reduceRatio);
> +        if (colISize < 0)
> +          colISize = 0;

braces

@@ +341,5 @@
> +        NS_ERROR("column frames out of sync with cell map");
> +        continue;
> +      }
> +      if (colFrame->GetFinalISize() == unassignedMarker)
> +        colFrame->SetFinalISize(toAssign);

braces

Comment 12

[Jonathan Kew [:jfkthame]]

(In reply to Simon Montagu :smontagu from comment #9)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=961b993e1a79

I don't think this job actually ran your new tests, because the reftests/writing-mode/tables directory wasn't listed in the reftests/writing-mode/reftest.list manifest. (Note that the initial table-reftest patch from bug 1077521 got backed out for Android failures; it should be re-landing later today once bug 1177600 and bug 1177606 are fixed.)

Comment 13

[Jonathan Kew [:jfkthame]]

(In reply to Simon Montagu :smontagu from comment #10)
> Created attachment 8626541 [details] [diff] [review]
> Reftests
> 
> These are a selection from the tests at
> http://test.csswg.org/harness/test/css21_dev/section/17.5.2.1/ converted to
> vertical writing-modes. Most of them work both in vertical-lr and
> vertical-rl, with a little care, except for those affected by bug 1177690.

I'm curious how many of these tests are actually affected by table-layout:fixed? I tried a couple of random examples and it appeared to make no visible difference.... it'd be nice to have at least a few tests where the table-layout setting is having a clear effect. Do you know if that's true for some of these?

Comment 14

[Simon Montagu :smontagu]

(In reply to Jonathan Kew (:jfkthame) from comment #12)
> I don't think this job actually ran your new tests, because the
> reftests/writing-mode/tables directory wasn't listed in the
> reftests/writing-mode/reftest.list manifest.

*sigh* you're right of course. Locally I was launching reftests locally in the tables directory so that didn't matter.

> I'm curious how many of these tests are actually affected by
> table-layout:fixed? I tried a couple of random examples and it appeared to
> make no visible difference....

Is this the same question as "do these tests fail without the patch?" If so, it's about half-and-half. Here are the numbers of the ones that fail:

005
009
010
012
013
016
023
025
026
028
030
031

I assume that even the ones that don't fail before this specific patch will fail before some of the other recent table-layout patches, so they are all testing *something*

Comment 15

[Jonathan Kew [:jfkthame]]

(In reply to Simon Montagu :smontagu from comment #14)
> > I'm curious how many of these tests are actually affected by
> > table-layout:fixed? I tried a couple of random examples and it appeared to
> > make no visible difference....
> 
> Is this the same question as "do these tests fail without the patch?" If so,
> it's about half-and-half. Here are the numbers of the ones that fail:

Yes, that's the question; thanks, that saves me trawling through them all to figure out which are affected.

> I assume that even the ones that don't fail before this specific patch will
> fail before some of the other recent table-layout patches, so they are all
> testing *something*

Indeed; I'm happy to have them all, just wanted to be sure that at least some of them were specific to the code being changed here.

It'd probably be worth another try run with the tests enabled, but be sure to include at least bug 1177600, otherwise I believe you'll get failures on android.

Comment 16

[Simon Montagu :smontagu]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ef32dce5eb7

(In reply to Jonathan Kew (:jfkthame) from comment #15)
> It'd probably be worth another try run with the tests enabled, but be sure
> to include at least bug 1177600, otherwise I believe you'll get failures on
> android.

Unfortunately I only saw this after pushing.

Comment 17

[Jonathan Kew [:jfkthame]]

Never mind; just ignore Android orange on vertical-rl tests, in that case.

Comment 18

[Jonathan Kew [:jfkthame]]

When I ran these tests locally, I get a failure on 006 (both vlr and vrl):

 0:46.07 REFTEST TEST-UNEXPECTED-FAIL | file:///Users/jkew/mozdev/mozilla-central/layout/reftests/writing-mode/tables/fixed-table-layout-006-vlr.html | image comparison (==), max difference: 51, number of differing pixels: 600

though the discrepancy isn't obvious to the eye. Maybe this is retina-only?

The four tests marked as "fail" all passed for me with the additional patches from bug 1157569, so I think they're going to be OK soon.

Comment 19

[Jonathan Kew [:jfkthame]]

Comment on attachment 8626541 [details] [diff] [review]
Reftests

Review of attachment 8626541 [details] [diff] [review]:
-----------------------------------------------------------------

I won't pretend I've studied all these, but the idea seems fine. rs=me.

Comment 20

[Jonathan Kew [:jfkthame]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=89c1f479d6fa

Comment 21

[Jonathan Kew [:jfkthame]]

(In reply to Simon Montagu :smontagu from comment #16)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ef32dce5eb7

Tryserver seems to think that test 010 is going to fail on most platforms. It looks like some kind of discrepancy in baseline-alignment between the test and reference, and is probably somewhat dependent on the platform font metrics.

Modifying the reference so that it also puts the "filler text" into a table cell might help; or I'd be OK with simply disabling this test for now and filing a followup to investigate it further.

Comment 22

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/047c419b040d
https://hg.mozilla.org/integration/mozilla-inbound/rev/958046997215
https://hg.mozilla.org/integration/mozilla-inbound/rev/d94a96e41f50

Comment 23

[Jonathan Kew [:jfkthame]]

I went ahead and pushed the patches here (hoping to catch the upcoming merge), with the 010 tests marked as random for now. Filed bug 1178059 about the problematic test.

Comment 24

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bed9dd9ca9cb

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/047c419b040d
https://hg.mozilla.org/mozilla-central/rev/958046997215
https://hg.mozilla.org/mozilla-central/rev/d94a96e41f50
https://hg.mozilla.org/mozilla-central/rev/bed9dd9ca9cb