Closed
Bug 1237805
Opened 8 years ago
Closed 8 years ago
[css-grid] Make 'auto-fit' to remove all empty (repeat) tracks, not just those at the end.
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)
References
Details
Attachments
(2 files, 1 obsolete file)
13.81 KB,
patch
|
Details | Diff | Splinter Review | |
8.05 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
https://lists.w3.org/Archives/Public/www-style/2016Jan/0031.html " - The potential options for how to drop repeated grid tracks with auto-fit came down to: 1. Drop all empty columns 2. Drop empty columns from either end 3. Drop empty columns from end end. - Several people were against 3 because it removed the symmetry of the property. - The group didn't have a preference between 1 and 2, so decided to stay with 1 since that's what's in the spec now. "
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8705639 -
Flags: review?(dholbert)
Assignee | ||
Comment 2•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9fbf2f0a658a&selectedJob=15194856
Comment 3•8 years ago
|
||
Comment on attachment 8705639 [details] [diff] [review] fix Review of attachment 8705639 [details] [diff] [review]: ----------------------------------------------------------------- holding off on r+ for the moment, due to the n^2 algorithm issue (see below) ::: layout/generic/nsGridContainerFrame.cpp @@ +2140,5 @@ > } > > + // Count empty 'auto-fit' tracks in the repeat() range. > + // |colAdjust| will have a count for each line in the grid of how many > + // tracks were removed before that line. s/"were removed"/"were empty"/. (We're not removing yet -- we're just counting empty tracks.) At first, I interpreted "before" here to mean "directly before". But you mean before-going-all-the-way-back-to-the-grid-start. To avoid that ambiguity, consider a rewording like: s/before that line/between the start of the grid and that line/ @@ +2158,5 @@ > + colAdjust->SetLength(numColLines); > + PodZero(colAdjust->Elements(), colAdjust->Length()); > + } > + for (uint32_t i = col + 1, len = colAdjust->Length(); i < len; ++i) { > + ++(*colAdjust)[i]; This looks like an n^2 algorithm -- for each empty track, we walk up to the end of the grid. That could get expensive, if there are a lot of empty tracks. Can you restructure this to just use a single loop, over all of the columns, which tracks numEmptyCols up to the current point & sets colAdjust[i] to the current value of numEmptyCols? (Same applies to the rows loop below this.) ::: layout/generic/nsGridContainerFrame.h @@ +192,5 @@ > /** > * Translate the lines to account for (empty) removed tracks. This method > * is only for grid items and should only be called after placement. > + * aNumRemovedTracks says how many tracks were removed before each line > + * in the grid. As noted elsewhere, "before" is ambiguous here. Consider rewording, or adding a clarifying parenthetical like "(between the start of the grid and that line)" at the end. @@ +200,5 @@ > MOZ_ASSERT(mStart != kAutoLine, "invalid resolved line for a grid item"); > MOZ_ASSERT(mEnd != kAutoLine, "invalid resolved line for a grid item"); > + uint32_t numRemovedTracks = aNumRemovedTracks[mStart]; > + mStart -= numRemovedTracks; > + mEnd -= numRemovedTracks; Could you assert here that no tracks were removed *between* mStart and mEnd? I think that's equivalent to asserting that aNumRemovedTracks[mStart] == aNumRemovedTracks[mEnd - 1], but I'm not 100% sure...
Assignee | ||
Comment 4•8 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #3) > > + // Count empty 'auto-fit' tracks in the repeat() range. > > + // |colAdjust| will have a count for each line in the grid of how many > > + // tracks were removed before that line. > At first, I interpreted "before" here to mean "directly before". But you > mean before-going-all-the-way-back-to-the-grid-start. That seems like a nonsensical interpretation to me since a "directly before" count could only be 0 or 1 which contradicts "count ... tracks". > To avoid that ambiguity, consider a rewording like: > s/before that line/between the start of the grid and that line/ Sure, that's more precise. > This looks like an n^2 algorithm -- for each empty track, we walk up to the > end of the grid. That could get expensive, if there are a lot of empty > tracks. Ah, good catch. Fixed. > ::: layout/generic/nsGridContainerFrame.h > > + uint32_t numRemovedTracks = aNumRemovedTracks[mStart]; > > + mStart -= numRemovedTracks; > > + mEnd -= numRemovedTracks; > > Could you assert here that no tracks were removed *between* mStart and mEnd? Sure, aNumRemovedTracks[mStart] == aNumRemovedTracks[mEnd] should hold. (in fact all aNumRemovedTracks[mStart..mEnd] should be equal but the above seems like a good enough check)
Attachment #8705639 -
Attachment is obsolete: true
Attachment #8705639 -
Flags: review?(dholbert)
Attachment #8705868 -
Flags: review?(dholbert)
Comment 5•8 years ago
|
||
Comment on attachment 8705868 [details] [diff] [review] fix Review of attachment 8705868 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, looks good! r=me
Attachment #8705868 -
Flags: review?(dholbert) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ba193524a8b https://hg.mozilla.org/integration/mozilla-inbound/rev/ceccb1580caa
Assignee | ||
Updated•8 years ago
|
Flags: in-testsuite+
Comment 7•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2ba193524a8b https://hg.mozilla.org/mozilla-central/rev/ceccb1580caa
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in
before you can comment on or make changes to this bug.
Description
•