Handle table cell backgrounds during display list generation for <table> #20034

Manishearth · 2018-02-13T01:22:14Z

Fixes #19788

This change is

highfive · 2018-02-13T01:22:19Z

Heads up! This PR modifies the following files:

@emilio: components/layout/table_colgroup.rs, components/layout/table.rs, components/layout/fragment.rs, components/layout/flow.rs

highfive · 2018-02-13T01:22:20Z

Warning

These commits modify layout code, but no tests are modified. Please consider adding a test!

Manishearth · 2018-02-14T22:21:34Z

Okay, I've gotten something working. Tables with styled columns/colgroups/rowgroups/rows/tables, that use colspan work.

There are a couple of missing pieces:

I don't handle currentColor correctly
I don't support rowspan (yet)
I no longer call build_display_list in TableRowFlow -- this may cause borders to be skipped? I'm not sure about this
Backgrounds are positioned relative to the cell, not the region being styled

For the last one, it's a bit tricky. I can handle it for row/rowgroup backgrounds by using the stackingcontext-relative position of the element for drawing the background, and then clipping it with the regular element's position.

But for columns/colgroups, we don't store the positions anywhere. We kinda calculate these during border-collapse layout; so it might be worth adding a layout pass to compute these and store them on ColGroupFlow.

Thoughts?

Manishearth · 2018-02-14T22:30:15Z

Oh, also, a lot of this is made complicated by the fact that build_display_list needs a mutable reference, but looking through the code that may not actually be necessary.

mbrubeck · 2018-02-14T23:09:06Z

But for columns/colgroups, we don't store the positions anywhere. We kinda calculate these during border-collapse layout; so it might be worth adding a layout pass to compute these and store them on ColGroupFlow.

Yes, seems like this will be necessary (though this can be a follow-up if you want).

Oh, also, a lot of this is made complicated by the fact that build_display_list needs a mutable reference, but looking through the code that may not actually be necessary.

Yeah... many of the individual builder methods only take&self. Some like build_fragment_type_specific_display_items take &mut self but don't actually need it. It should definitely be possible to do most of this without &mut access.

Does Fragment::build_display_list only use mut to unset self.restyle_damage? If so, maybe we could separate the rest of its body into an &self helper function.

I no longer call build_display_list in TableRowFlow -- this may cause borders to be skipped?

Yes, this will need to be addressed. I haven't looked at your code in detail yet, but it seems there's not a clear place to fit this in, since I think row borders should be drawn after the backgrounds but before the cell contents? Or does this even matter – is it possible for the borders and backgrounds to overlap? If not, then we could draw the row borders in separate loop iterating over the rows...

Manishearth · 2018-02-14T23:39:26Z

Does Fragment::build_display_list only use mut to unset self.restyle_damage?

Yes.

I can handle this by explicitly doing the damage (and nothing else) in TableCellFlow's build_display_list

Manishearth · 2018-02-14T23:45:06Z

Or does this even matter – is it possible for the borders and backgrounds to overlap?

I think not -- however, it is possible for the border and content to overlap since content can spill out. Hmm.

Manishearth · 2018-02-15T00:09:38Z

Made stuff immutable which lets me handle currentcolor easily

Manishearth · 2018-02-15T00:20:24Z

@bors-servo try

let's see what we get

bors-servo · 2018-02-15T00:20:29Z

⌛ Trying commit dc7d706 with merge 2390e18...


        Auto merge of #20034 - Manishearth:table-backgrounds, r=<try>

[WIP] Handle table cell backgrounds during display list generation for <table> Very WIP, currently just contains half the iterator work necessary to do this. Fixes #19788  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20034)

bors-servo · 2018-02-15T00:40:58Z

💔 Test failed - linux-dev

Manishearth · 2018-02-15T01:26:28Z

Rowspan "done".

Given that we don't actually render rowspans where we should, it's a bit lackluster. But if we did, they would have working backgrounds.

Manishearth · 2018-02-16T01:51:25Z

Turns out borders are not skipped; table rows don't draw collapsed borders anyway, they delegate to cells!

I'm pushing bg positioning to a different bug.

There's one missing bit -- restyle damage doesn't get correctly applied here. I guess I should propagate it up in layout/incremental.rs?

Manishearth · 2018-02-16T03:18:28Z

So we fail css/cssom-view/elementFromPoint-dynamic-anon-box.html . I thought this was due to restyle damage, but I tried fixing that in incremental.rs and this still occurs.

mbrubeck · 2018-02-16T21:23:34Z

components/layout/table_row.rs

-
-        self.block_flow.build_display_list_for_block(state, border_painting_mode);
+    fn build_display_list(&mut self, _: &mut DisplayListBuildState) { 
+        // handled in TableCellStyleInfo::build_display_list


We'll still need to remove REPAINT damage here, or somewhere.

mbrubeck · 2018-02-16T21:29:51Z

components/layout/table_cell.rs

+        if self.visible {
+            // we skip setting the damage in TableCellStyleInfo::build_display_list()
+            // because we only have immutable access
+            self.block_flow.fragment.restyle_damage.remove(ServoRestyleDamage::REPAINT);


This should probably be unconditional. (I think this was a bug in the old code.)

mbrubeck · 2018-02-16T21:51:47Z

components/layout/table.rs

+        if !self.cell.visible || self.cell.block_flow.fragment.style()
+                                     .get_inheritedbox().visibility != Visibility::Visible {
+            return
+        }


I believe for empty cells (cell.visible == false), we should still draw the column and row backgrounds, just not the cell background and contents, per the spec: https://drafts.csswg.org/css2/tables.html#table-layers

I tested this in Firefox, it doesn't work for columns at least. The cell disappears entirely.

Or rows. It only works for the surrounding table, and we still handle that

mbrubeck · 2018-02-16T22:22:35Z

@bors-servo r+

Thanks especially for factoring these traversals out into iterators. ❤️

bors-servo · 2018-02-16T22:22:35Z

📌 Commit 1a03cd6 has been approved by mbrubeck

ferjm · 2018-02-21T09:54:01Z

@bors-servo retry

{"status": "CRASH", "group": "default", "message": null, "stack": null, "subtest": null, "test": "/WebCryptoAPI/idlharness.https.worker.html", "line": 147751, "action": "test_result", "expected": "OK"}

bors-servo · 2018-02-21T10:34:18Z

⌛ Testing commit 285313f with merge 96f36f7...


        Auto merge of #20034 - Manishearth:table-backgrounds, r=mbrubeck

Handle table cell backgrounds during display list generation for <table> Fixes #19788  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20034)

bors-servo · 2018-02-21T11:09:12Z

💔 Test failed - linux-rel-wpt

CYBAI · 2018-02-21T12:35:30Z

Error in linux-rel-nogate

error[E0425]: cannot find value `handle` in this scope
    --> components/script/dom/xmlhttprequest.rs:1068:47
     |
1068 |             self.global().unschedule_callback(handle);
     |                                               ^^^^^^ not found in this scope

error: aborting due to previous error

error: Could not compile `script`.

Manishearth · 2018-02-21T15:52:49Z

@bors retry

new failures, probably intermittents

{"status": "TIMEOUT", "group": "default", "message": null, "stack": null, "subtest": null, "test": "/css/CSS2/floats-clear/clear-float-003.xht", "line": 5420, "action": "test_result", "expected": "PASS"}
{"status": "CRASH", "group": "default", "message": null, "stack": null, "subtest": null, "test": "/html/semantics/selectors/pseudo-classes/checked.html", "line": 139160, "action": "test_result", "expected": "OK"}

CYBAI · 2018-02-21T16:40:09Z

@bors-servo retry

(I guess @Manishearth called wrong bors XD?)

bors-servo · 2018-02-21T17:00:06Z

⌛ Testing commit 285313f with merge 0b4ea01...


        Auto merge of #20034 - Manishearth:table-backgrounds, r=mbrubeck

Handle table cell backgrounds during display list generation for <table> Fixes #19788  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20034)

bors-servo · 2018-02-21T17:21:33Z

💔 Test failed - linux-rel-css

Manishearth · 2018-02-21T17:27:20Z

@bors-servo retry

no space on device

bors-servo · 2018-02-21T17:27:26Z

⚡ Previous build results for android, linux-dev, mac-dev-unit are reusable. Rebuilding only arm32, arm64, linux-rel-css, linux-rel-wpt, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev...

bors-servo · 2018-02-21T17:34:06Z

💔 Test failed - linux-rel-wpt

Manishearth · 2018-02-21T17:44:47Z

@bors-servo retry

font intermittent crash

bors-servo · 2018-02-21T17:44:51Z

⚡ Previous build results for android, arm32, arm64, linux-dev, linux-rel-css, mac-dev-unit, mac-rel-wpt1, windows-msvc-dev are reusable. Rebuilding only linux-rel-wpt, mac-rel-css1, mac-rel-css2, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4...

bors-servo · 2018-02-21T18:12:41Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: mbrubeck
Pushing 0b4ea01 to master...

highfive assigned glennw Feb 13, 2018

highfive added the S-awaiting-review label Feb 13, 2018

Manishearth force-pushed the Manishearth:table-backgrounds branch from 3b5ea02 to 32ece4d Feb 14, 2018

highfive added S-tests-failed and removed S-tests-failed labels Feb 15, 2018

Manishearth changed the title ~~[WIP] Handle table cell backgrounds during display list generation for <table>~~ Handle table cell backgrounds during display list generation for <table> Feb 16, 2018

mbrubeck assigned mbrubeck and unassigned glennw Feb 16, 2018

mbrubeck reviewed Feb 16, 2018

View changes

Manishearth force-pushed the Manishearth:table-backgrounds branch from f15cead to 3dc3683 Feb 16, 2018

mbrubeck reviewed Feb 16, 2018

View changes

highfive removed the S-awaiting-review label Feb 16, 2018

highfive added the S-tests-failed label Feb 21, 2018

highfive added S-awaiting-merge and removed S-tests-failed labels Feb 21, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels Feb 21, 2018

highfive added S-awaiting-merge and removed S-tests-failed labels Feb 21, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels Feb 21, 2018

highfive added the S-tests-failed label Feb 21, 2018

bors-servo merged commit 285313f into servo:master Feb 21, 2018
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

bors-servo mentioned this pull request Feb 21, 2018

Reorganise display list builder #20031

Merged

Manishearth mentioned this pull request Feb 21, 2018

Intermittent timeout on /css/CSS2/floats-clear/clear-float-003.xht #20100

Closed

Manishearth deleted the Manishearth:table-backgrounds branch Feb 26, 2018

servo / servo

Handle table cell backgrounds during display list generation for <table> #20034

Handle table cell backgrounds during display list generation for <table> #20034

Manishearth commented Feb 13, 2018 •

edited

highfive commented Feb 13, 2018

highfive commented Feb 13, 2018

Manishearth commented Feb 14, 2018 •

edited

Manishearth commented Feb 14, 2018

mbrubeck commented Feb 14, 2018

Manishearth commented Feb 14, 2018

Manishearth commented Feb 14, 2018

Manishearth commented Feb 15, 2018

Manishearth commented Feb 15, 2018

bors-servo commented Feb 15, 2018

bors-servo commented Feb 15, 2018

Manishearth commented Feb 15, 2018

Manishearth commented Feb 16, 2018

Manishearth commented Feb 16, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mbrubeck commented Feb 16, 2018

bors-servo commented Feb 16, 2018

ferjm commented Feb 21, 2018

bors-servo commented Feb 21, 2018

bors-servo commented Feb 21, 2018

CYBAI commented Feb 21, 2018 •

edited

Manishearth commented Feb 21, 2018

CYBAI commented Feb 21, 2018 •

edited

bors-servo commented Feb 21, 2018

bors-servo commented Feb 21, 2018

Manishearth commented Feb 21, 2018

bors-servo commented Feb 21, 2018

bors-servo commented Feb 21, 2018

Manishearth commented Feb 21, 2018

bors-servo commented Feb 21, 2018

bors-servo commented Feb 21, 2018

servo / servo

Join GitHub today

Handle table cell backgrounds during display list generation for <table> #20034

Handle table cell backgrounds during display list generation for <table> #20034

Conversation

Manishearth commented Feb 13, 2018 • edited

highfive commented Feb 13, 2018

highfive commented Feb 13, 2018

Manishearth commented Feb 14, 2018 • edited

Manishearth commented Feb 14, 2018

mbrubeck commented Feb 14, 2018

Manishearth commented Feb 14, 2018

Manishearth commented Feb 14, 2018

Manishearth commented Feb 15, 2018

Manishearth commented Feb 15, 2018

bors-servo commented Feb 15, 2018

bors-servo commented Feb 15, 2018

Manishearth commented Feb 15, 2018

Manishearth commented Feb 16, 2018

Manishearth commented Feb 16, 2018

This comment has been minimized.

mbrubeck Feb 16, 2018

This comment has been minimized.

mbrubeck Feb 16, 2018

This comment has been minimized.

mbrubeck Feb 16, 2018

This comment has been minimized.

Manishearth Feb 16, 2018

This comment has been minimized.

Manishearth Feb 16, 2018

mbrubeck commented Feb 16, 2018

bors-servo commented Feb 16, 2018

ferjm commented Feb 21, 2018

bors-servo commented Feb 21, 2018

bors-servo commented Feb 21, 2018

CYBAI commented Feb 21, 2018 • edited

Manishearth commented Feb 21, 2018

CYBAI commented Feb 21, 2018 • edited

bors-servo commented Feb 21, 2018

bors-servo commented Feb 21, 2018

Manishearth commented Feb 21, 2018

bors-servo commented Feb 21, 2018

bors-servo commented Feb 21, 2018

Manishearth commented Feb 21, 2018

bors-servo commented Feb 21, 2018

bors-servo commented Feb 21, 2018

Manishearth commented Feb 13, 2018 •

edited

Manishearth commented Feb 14, 2018 •

edited

CYBAI commented Feb 21, 2018 •

edited

CYBAI commented Feb 21, 2018 •

edited