Closed Bug 1384602 Opened 7 years ago Closed 7 years ago

stylo: Squashed boxes in YouTrack Agile Boards view

Categories

(Core :: CSS Parsing and Computation, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: inejge, Assigned: bzbarsky)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: nightly-community)

Attachments

(8 files, 2 obsolete files)

Attached image YouTrack Agile Board squashed boxes (deleted) —
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0 Build ID: 20170726100241 Steps to reproduce: Enabled Stylo (layout.css.servo.enabled = true) on FF 56.0a1 (2017-07-26) (64-bit) and went to YouTrack Agile Boards view (https://...myjetbrains.com/youtrack/agiles/...) -- can't share the URL, sorry. Actual results: Issue boxes are squashed horizontally, as shown in the attached screen grab. Expected results: The content of the boxes should be visible.
Summary: Squashed boxes in YouTrack Agile Boards view with Stylo → stylo: Squashed boxes in YouTrack Agile Boards view
Component: Untriaged → CSS Parsing and Computation
Product: Firefox → Core
Disabling Stylo and reloading the YouTrack page displays the boxes correctly. Re-enabling and reloading results in the same incorrect display.
Version: 56 Branch → Trunk
I don't have access to the site, so reproducing is likely hard, but that's the kind of thing I suspect could be caused by bug 1384065.
Activating the Inspector while the boxes are incorrectly displayed and hovering over the main display area will usually redisplay the page correctly. Also, collapsing the bottom row of cards will display the rest correctly. When one of other rows is collapsed, the boxes appear, but are narrower than they should be. Some kind of JS interaction?
(In reply to inejge from comment #3) > Activating the Inspector while the boxes are incorrectly displayed and > hovering over the main display area will usually redisplay the page > correctly. Also, collapsing the bottom row of cards will display the rest > correctly. When one of other rows is collapsed, the boxes appear, but are > narrower than they should be. > > Some kind of JS interaction? Seems like we're missing some restyle. It's not necessary but possible that it's due to JS interactions. In any case, it's impossible to diagnose without some kind of testcase...
It seems not an easily reproducible issue to me. I just tested on my mint Linux with same Nightly build but failed to reproduce it. I'm wondering whether we have variant configs in use. Could you reproduce again with latest build or be able to attach about:support for reference?
Flags: needinfo?(inejge)
Attached image YT demo Agile Boards, without stylo (deleted) —
Flags: needinfo?(inejge)
Attached image YT demo Agile Boards, with stylo (deleted) —
Reproduced with latest nightly on a demo YT project. I've attached the screenshots. Since there are fewer columns than on the real YT instance where I first noticed the glitch, box squashing is less pronounced but still easily visible. I'll attach about:support separately.
Attached file about:support (deleted) —
I've enabled guest access on YT, if it's OK to send the link privately please contact me by email.
So I requested the link (thanks!). It seems that we miss a restyle / reflow of something inside a colgroup. The layout of the page looks like: <table> <colgroup> <col>...</col> </colgroup> </table> The table gets resized dinamically but we fail to resize the <col> elements. I'm trying to reduce the page to a simple test-case... Reducing angular apps is fun :/
Flushing styles of the whole page doesn't fix it, but reframing the colgroup does... Presumably we're missing an anon-box restyle somewhere?
Another option is of course that we just missed a reframe change hint or something... I'll try to figure out what the change they're making to the table is exactly.
(maybe we're supposed to post the change hints to the table wrapper? If so, we don't do that right now...)
Btw, this is pretty much confirmed.
Status: UNCONFIRMED → NEW
Ever confirmed: true
If there's any way to attach a testcase here (doesn't have to be minimal, just has to show the problem), that would be pretty useful...
Assignee: nobody → emilio+bugs
Priority: P2 → --
Based on Emilio's investigation in comment 12 ~ comment 15, this is pretty much the same issue in Bug 1385831. Set dependency and leave the bug open for now, so we won't forget to get back here and verify if this is a dup for Bug 1385831.
Depends on: 1385831
P3 since we think this is likely the same as bug 1385831, so that we only track the high priority in one place. We should re-test once a fix for that lands.
Priority: -- → P3
> Based on Emilio's investigation in comment 12 ~ comment 15, this is pretty much the same issue in Bug 1385831. That's entirely unclear to me, given Emilio's comments on IRC. Investigating this now.
Attached file Suspected minimal-ish testcase (deleted) —
This testcase shows the problem both with and without stylo. I suspect that what happens with stylo is that we convert a lazy frame construction that would take the append path for Gecko into "create a frame" style changes that end up taking the insert path for all but the first one for the row (isAppend false in ContentRangeInserted, so calling InsertFrames instead of AppendFrames). The problem is that the InsertFrames codepath for "insertion" at the end into a row in a border-collapse table is buggy. Specifically, nsCellMap::AppendCell, which looks for the first dead celldata and if found mutates it in-place to point to the new cell instead of adding more celldata. But nsCellMap::InsertCell just always inserts a new celldata. In our testcase, after the first cell has been appended to the second row it ends up with three dead cells following it (to store border-collapse border state). When we do another InsertCell(), we just insert a fifth one, and then we think the table has 5 columns and things get all confused.
This makes stylo match gecko on my minimal testcases, but NOT on the original site (though it's closer than it was)
Assignee: emilio+bugs → bzbarsky
Status: NEW → ASSIGNED
OK, I filed bug 1388898 on the underlying table layout issue. I'll post patches that fix the minimalish testcases above (for both stylo and gecko) and the original site, by working around the cellmap breakage for now. The cellmap bits will need more thought.
Depends on: 1388898
No longer depends on: 1385831
MozReview-Commit-ID: 7MyO1ZyS9zu
Attachment #8895570 - Flags: review?(emilio+bugs)
Attachment #8895551 - Attachment is obsolete: true
Attachment #8895517 - Attachment is obsolete: true
Attachment #8895570 - Flags: review?(emilio+bugs) → review+
Attachment #8895571 - Flags: review?(cam) → review+
That second changeset causes an assertion failure on layout/tables/crashtests/460637-2.xhtml because our damage area has height 9 (corresponding the the rowspan), but it's a dead rowspan, so the actual number of rows is 1. This fails the VerifyDamageRect() assertion in AddBCDamageArea called from nsTableFrame::AppendCell. I filed bug 1389295 on this.
Depends on: 1389295
Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/dc3333e3ff2b part 1. When coalescing lazy frame construction reframes for stylo, do it even across comments. r=emilio https://hg.mozilla.org/integration/autoland/rev/924d16d996e2 part 2. Convert inserts to appends on table rows if possible, because the insert codepath is rather buggy. r=heycam
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: