Skip to content

Commit

Permalink
[GridNG] Flip rows and columns for orthogonal sugrids
Browse files Browse the repository at this point in the history
Orthogonal grids flip rows and columns, and subgrids need to support
this when combining lines from the parent grid. This gets us to pass
repeat-auto-fill-003.html, however a slight adjustment was made to the
the test because baselines didn't match the reference. We match WebKit,
but Firefox appears to have a bug with orthogonal baselines. I came
across this WebKit discussion that implies that *they* are incorrect
(and thus we would be too), but I believe Firefox is incorrect here.
See [this discussion](https://bugs.webkit.org/show_bug.cgi?id=236958).

Note that as of the most recent rebase, it picked up
https://chromium-review.googlesource.com/c/chromium/src/+/4355553,
and repeat-auto-fill-003.html no longer passes, so it was added back
to TestExpectations. This should start passing once those regressions
are fixed.

Either way, this behavior is unrelated to the actual test content, so
I made some adjustments to get the baselines to match in all
aforementioned platforms. In particular, I set `line-height`to zero and
set `vertical-align` to `top`.

repeat-auto-fill-003.html is an interesting case, and I believe it's
the only test with nested orthogonal subgrids, so I added a new test
that covers more than auto repeat behaviors.

These changes exposed a bug from my prior auto repeat checkin - the
adjustment for non-repeat lines that come after an auto repeater
shouldn't happen on nested subgrids, as it will shift lines multiple
times for each level of nesting. Ideally, this would happen when we
add the non-repeat named lines. For now, I added a TODO, as this will
require some additional work and testing.

Bug: 618969
Change-Id: Ic8c42189d0563e6b2ed4f322ae89b0583c1fb519
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4355586
Reviewed-by: Alison Maher <almaher@microsoft.com>
Reviewed-by: Ethan Jimenez <ethavar@microsoft.com>
Commit-Queue: Kurt Catti-Schmidt <kschmi@microsoft.com>
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1122607}
  • Loading branch information
KurtCattiSchmidt authored and Chromium LUCI CQ committed Mar 27, 2023
1 parent a4b3ad5 commit 10baa4b
Show file tree
Hide file tree
Showing 4 changed files with 622 additions and 31 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ NGGridLineResolver::NGGridLineResolver(
// merging. These are positive and relative to index 0, so we only need to
// clamp values above `subgrid_span_size`.
for (const auto& pair : subgrid_map) {
WTF::Vector<wtf_size_t> clamped_list;
Vector<wtf_size_t> clamped_list;
for (const auto& position : pair.value) {
if (position > subgrid_span_size)
clamped_list.push_back(subgrid_span_size);
Expand All @@ -136,7 +136,7 @@ NGGridLineResolver::NGGridLineResolver(
// (`parent_map`). The map is a key-value store with keys as the implicit
// line name and the value as an array of ascending indices.
for (const auto& pair : parent_map) {
WTF::Vector<wtf_size_t> merged_list;
Vector<wtf_size_t> merged_list;
for (const auto& position : pair.value) {
auto IsGridAreaInSubgridRange = [&]() -> bool {
// Returns true if a given position is within either the implicit
Expand Down Expand Up @@ -224,8 +224,8 @@ NGGridLineResolver::NGGridLineResolver(
[](NamedGridLinesMap& subgrid_map,
const NamedGridLinesMap& parent_auto_repeat_map,
const blink::ComputedGridTrackList& track_list, GridSpan subgrid_span,
wtf_size_t auto_repetitions,
bool is_opposite_direction_to_parent) -> void {
wtf_size_t auto_repetitions, bool is_opposite_direction_to_parent,
bool is_nested_subgrid) -> void {
const wtf_size_t auto_repeat_track_count =
track_list.TrackList().AutoRepeatTrackCount();
const wtf_size_t auto_repeat_total_tracks =
Expand All @@ -238,24 +238,27 @@ NGGridLineResolver::NGGridLineResolver(
// come after the auto repeater. This is because they were parsed without
// knowledge of the number of repeats. Now that we know how many auto
// repeats there are, we need to shift the existing entries by the total
// number of auto repeat tracks.
// number of auto repeat tracks. For now, skip this on nested subgrids,
// as it will double-shift non-auto repeat lines.
// TODO(kschmi): Properly shift line names after the insertion point for
// nested subgrids. This should happen in `MergeNamedGridLinesWithParent`.
// TODO(kschmi): Do we also need to do this for implicit lines?
const wtf_size_t insertion_point = track_list.auto_repeat_insertion_point;
const wtf_size_t last_auto_repeat_index =
insertion_point + auto_repeat_total_tracks;
for (const auto& pair : subgrid_map) {
WTF::Vector<wtf_size_t> shifted_list;
for (const auto& position : pair.value) {
if (position >= insertion_point) {
wtf_size_t expanded_position = position + last_auto_repeat_index;
// These have already been offset relative to index 0, so explicitly
// do not offset by `subgrid_span` like we do below.
if (subgrid_span.Contains(expanded_position)) {
shifted_list.push_back(expanded_position);
if (!is_nested_subgrid) {
for (const auto& pair : subgrid_map) {
Vector<wtf_size_t> shifted_list;
for (const auto& position : pair.value) {
if (position >= insertion_point) {
wtf_size_t expanded_position = position + auto_repeat_total_tracks;
// These have already been offset relative to index 0, so explicitly
// do not offset by `subgrid_span` like we do below.
if (subgrid_span.Contains(expanded_position)) {
shifted_list.push_back(expanded_position);
}
}
}
subgrid_map.Set(pair.key, shifted_list);
}
subgrid_map.Set(pair.key, shifted_list);
}

// Now expand the auto repeaters into `subgrid_map`.
Expand Down Expand Up @@ -308,12 +311,15 @@ NGGridLineResolver::NGGridLineResolver(
}
}
};

// TODO(kschmi) - Account for orthogonal writing modes and swap rows/columns.
const bool is_opposite_direction_to_parent =
(grid_style.Direction() != parent_line_resolver.style_->Direction());
grid_style.Direction() != parent_line_resolver.style_->Direction();
const bool is_parallel_to_parent =
IsParallelWritingMode(grid_style.GetWritingMode(),
parent_line_resolver.style_->GetWritingMode());

if (subgrid_area.columns.IsTranslatedDefinite()) {
const auto track_direction_in_parent =
is_parallel_to_parent ? kForColumns : kForRows;
MergeNamedGridLinesWithParent(
*subgridded_columns_merged_explicit_grid_line_names_,
parent_line_resolver.ExplicitNamedLinesMap(kForColumns),
Expand All @@ -323,29 +329,39 @@ NGGridLineResolver::NGGridLineResolver(
parent_line_resolver.ImplicitNamedLinesMap(kForColumns),
subgrid_area.columns);
// Expand auto repeaters from the parent into the named line map.
// TODO(kschmi): Also expand the subgrid's repeaters. Otherwise, we could
// have issues with precedence.
ExpandAutoRepeatTracksFromParent(
*subgridded_columns_merged_explicit_grid_line_names_,
parent_line_resolver.AutoRepeatLineNamesMap(kForColumns),
parent_line_resolver.ComputedGridTrackList(kForColumns),
subgrid_area.columns, parent_line_resolver.AutoRepetitions(kForColumns),
is_opposite_direction_to_parent);
parent_line_resolver.AutoRepeatLineNamesMap(track_direction_in_parent),
parent_line_resolver.ComputedGridTrackList(track_direction_in_parent),
subgrid_area.columns,
parent_line_resolver.AutoRepetitions(track_direction_in_parent),
is_opposite_direction_to_parent,
parent_line_resolver.IsSubgridded(track_direction_in_parent));
}
if (subgrid_area.rows.IsTranslatedDefinite()) {
const auto track_direction_in_parent =
is_parallel_to_parent ? kForRows : kForColumns;
MergeNamedGridLinesWithParent(
*subgridded_rows_merged_explicit_grid_line_names_,
parent_line_resolver.ExplicitNamedLinesMap(kForRows), subgrid_area.rows,
is_opposite_direction_to_parent);
parent_line_resolver.ExplicitNamedLinesMap(track_direction_in_parent),
subgrid_area.rows, is_opposite_direction_to_parent);
MergeImplicitLinesWithParent(
*subgridded_rows_merged_implicit_grid_line_names_,
parent_line_resolver.ImplicitNamedLinesMap(kForRows),
subgrid_area.rows);
// Expand auto repeaters from the parent into the named line map.
// TODO(kschmi): Also expand the subgrid's repeaters. Otherwise, we could
// have issues with precedence.
ExpandAutoRepeatTracksFromParent(
*subgridded_rows_merged_explicit_grid_line_names_,
parent_line_resolver.AutoRepeatLineNamesMap(kForRows),
parent_line_resolver.ComputedGridTrackList(kForRows), subgrid_area.rows,
parent_line_resolver.AutoRepetitions(kForRows),
is_opposite_direction_to_parent);
parent_line_resolver.AutoRepeatLineNamesMap(track_direction_in_parent),
parent_line_resolver.ComputedGridTrackList(track_direction_in_parent),
subgrid_area.rows,
parent_line_resolver.AutoRepetitions(track_direction_in_parent),
is_opposite_direction_to_parent,
parent_line_resolver.IsSubgridded(track_direction_in_parent));
}
}

Expand Down
Loading

0 comments on commit 10baa4b

Please sign in to comment.