1917028 - JIRA Issues "List View" ends up with first row being extra tall, which pushes final row out of view

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Daniel Holbert [:dholbert]]

[Spinning off from bug 1521088 comment 13, for an apparent regression from that bug]

STR:

1. Load some JIRA issues "list view" query that has multiple results, e.g. this one (filtered by "created IN January")
   https://mozilla-hub.atlassian.net/jira/software/c/projects/VPN/issues/?jql=project%20%3D%20%22VPN%22%20AND%20text%20~%20%22created%20IN%20January%22%20ORDER%20BY%20created%20DESC
   (note, you need to have access to and be-signed-in-to Mozilla's jira instance in order to load this page)
2. Look at the results table.

ACTUAL RESULTS:
The first row is extra tall (it's got some unexpected padding on the bottom and top for some reason), and that pushes the last row out of view, forcing you to scroll to see the final result.

EXPECTED RESULTS:
First row shouldn't be extra tall.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: screenshot (expected results on left, actual results on right)

Comment 2

[Daniel Holbert [:dholbert]]

Observations from poking in devtools:

• If I add display:none to the thead element (or its tr child), then the bug goes away (the first row gets the correct size)
• On the other hand, if I add height:80px to the thead element (or any other large height), then the first row grows as well.

So it seems like the first row is adopting some height from the thead for some reason.

Comment 3

[Daniel Holbert [:dholbert]]

I have a reduced testcase from the testcase-reducer add-on; I'll edit it to redact/reduce it further and then will post.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

:TYLin, since you are the author of the regressor, bug 1521088, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 5

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

I can reproduce this via the link in comment 0. Note you need to switch to "DETAIL VIEW" icon to see the tall first table row.

Since Daniel is reducing a testcase, I can look into this bug after a reduced testcase is posted.

Bug 1521088 propagates the IsBResize flag from table wrapper frame into table frame. As a result, it makes the table frame reflow a bit more, and the following code in nsTableFrame::Reflow() now looks fishy ... https://searchfox.org/mozilla-central/rev/a85b25946f7f8eebf466bd7ad821b82b68a9231f/layout/tables/nsTableFrame.cpp#1661-1675

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: partially reduced testcase

in meetings, here's what I've got for my testcase so far. :)

Comment 7

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: reduced testcase

I've got a reduced testcase based on comment 6.

Comment 8

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Re comment 5:

Bug 1521088 propagates the IsBResize flag from table wrapper frame into table frame. As a result, it makes the table frame reflow a bit more,

I think propagating IsBResize flag into the table frame might not be a good idea ... it somehow breaks how we handle percentage block-sizes in table cells. Specifically, both flex & grid container can reflow an item (the table) twice: once during the measuring reflow, and again during the final reflow. In the final reflow, we already known the table's block-size, which was determined in the measuring reflow. However, setting IsBResize flag on the table frame makes a table cell with percentage block-size re-resolve its block-size based on that table's block-size from the measuring reflow.

For example, in the testcase in comment 7, there are multiple table cells that have 100% block-sizes. After wrongly re-resolving the table cells block-size, the table's block-size becomes way larger.

I believe this bug might be related to bug 359481 or bug 1883699 that requires a large rewrite. We shouldn't ship this regression, so I propose we back bug 1521088 out. I'm going to write some WPTs to catch this bug.

Comment 9

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1917028 - Add WPTs for table as flex item or grid item where table cells have a percentage height.

These WPTs catch the bug with the patch landed in bug 1521088 comment 12. All of
them should pass with Firefox Release 130 and Nightly with the backout commit in
bug 1521088 comment 17.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ab9176fc625b
Add WPTs for table as flex item or grid item where table cells have a percentage height. r=dholbert

Comment 11

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/48070 for changes under testing/web-platform/tests

Comment 12

[Dianna Smith [:diannaS]]

Marking 131 as disabled since the regressor (bug 1521088) was backed out of central/beta (132/131).
Leaving 132 as affected (although backed out) so that the web-platform-tests can land.

Comment 13

[Daniel Holbert [:dholbert]]

Leaving 132 as affected (although backed out) so that the web-platform-tests can land.

They already landed, hooray!

So I think we can consider this fixed-by-backout (and assume that we won't regress it when the regressor relands, since we've now got regression tests in-tree that would catch this).

(I'm marking 132 as fixed and adjusting the 131 status from disabled to fixed, too, since I think disabled implies that the bug is still present in that version and/or later versions and that we've simply switched some feature off to paper over it. And that's not the situation here; we've backed out the regressor entirely from all branches, so the bug is indeed fixed and doesn't require any further shepherding to keep it fixed or keep flags in a properly configured state.)

Comment 14

[Daniel Holbert [:dholbert]]

(I guess the WPTs didn't get merged to central yet; they'll be at https://hg.mozilla.org/mozilla-central/rev/ab9176fc625b when comment 10 is merged to central. But the bug-fix (the backout in bug 1521088 comment 17) did indeed hit central, so fixed status is still accurate here.)

Comment 15

[Dianna Smith [:diannaS]]

My thought process was more because the wpt hadn't hit central yet and wanted to make sure it did...and when they do they wont be in 131. But honestly this works better! Thanks for following up!

Comment 16

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/ab9176fc625b

Comment 17

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot