New issue
Advanced search Search tips

Issue 835607 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 8
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

[css-grid] percentage values in relative offset properties for grid items are resolved incorrectly

Project Member Reported by mpalmg...@mozilla.com, Apr 22 2018

Issue description

Chrome Version: 67.0.3393.4 (Official Build) dev (64-bit)
OS: Linux

What steps will reproduce the problem?
(1)load the testcase at https://bug1443386.bmoattachments.org/attachment.cgi?id=8969956
(2)
(3)

What is the expected result?
The second row of items should be indented by 50px (50% of the 100px column size).

What happens instead?
It's indented by much more.  I'm guessing it's resolved against the grid container width.

The testcase works correctly in Firefox Nightly.
 

Comment 1 by r...@igalia.com, Apr 23 2018

Cc: svil...@igalia.com jfernan...@igalia.com
Status: Available (was: Untriaged)
Test
Components: Blink>Layout
Owner: r...@igalia.com
Status: Started (was: Available)
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 8

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6298f794c7a71b2bd3f3efe137add7ceac5ae38d

commit 6298f794c7a71b2bd3f3efe137add7ceac5ae38d
Author: Manuel Rego Casasnovas <rego@igalia.com>
Date: Mon Oct 08 13:17:08 2018

[css-grid] Fix percentages in relative offsets for grid items

The method LayoutBoxModelObject::RelativePositionOffset()
was not considering the case of grid items,
where the containing block is the grid area.
The patch modifies the method so the new code uses
OverrideContainingBlockContentLogicalWidth|Height when required.

Two new tests are added, the first one does not use percentages
and is already passing. The second one has the very same output
but using percentages which was not working before this patch.

BUG= 835607 
TEST=external/wpt/css/css-grid/grid-items/grid-items-relative-offsets-001.html
TEST=external/wpt/css/css-grid/grid-items/grid-items-relative-offsets-002.html

Change-Id: Icb76f4a521566ad36f87924835b21ae450f2cb24
Reviewed-on: https://chromium-review.googlesource.com/c/1238726
Reviewed-by: Javier Fernandez <jfernandez@igalia.com>
Commit-Queue: Manuel Rego <rego@igalia.com>
Cr-Commit-Position: refs/heads/master@{#597543}
[add] https://crrev.com/6298f794c7a71b2bd3f3efe137add7ceac5ae38d/third_party/WebKit/LayoutTests/external/wpt/css/css-grid/grid-items/grid-items-relative-offsets-001.html
[add] https://crrev.com/6298f794c7a71b2bd3f3efe137add7ceac5ae38d/third_party/WebKit/LayoutTests/external/wpt/css/css-grid/grid-items/grid-items-relative-offsets-002.html
[modify] https://crrev.com/6298f794c7a71b2bd3f3efe137add7ceac5ae38d/third_party/WebKit/LayoutTests/external/wpt/css/css-grid/grid-items/support/grid.css
[modify] https://crrev.com/6298f794c7a71b2bd3f3efe137add7ceac5ae38d/third_party/blink/renderer/core/layout/layout_box.cc
[modify] https://crrev.com/6298f794c7a71b2bd3f3efe137add7ceac5ae38d/third_party/blink/renderer/core/layout/layout_box.h
[modify] https://crrev.com/6298f794c7a71b2bd3f3efe137add7ceac5ae38d/third_party/blink/renderer/core/layout/layout_box_model_object.cc
[modify] https://crrev.com/6298f794c7a71b2bd3f3efe137add7ceac5ae38d/third_party/blink/renderer/core/layout/layout_box_model_object.h
[modify] https://crrev.com/6298f794c7a71b2bd3f3efe137add7ceac5ae38d/third_party/blink/renderer/core/layout/layout_grid.cc

Status: Fixed (was: Started)
JFTR, the patch fixing this introduced a performance regression ( issue #893884 ).

Project Member

Comment 8 by bugdroid1@chromium.org, Oct 12

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4226ddf99103e493d7afb23a4c7902ee496108b6

commit 4226ddf99103e493d7afb23a4c7902ee496108b6
Author: Manuel Rego Casasnovas <rego@igalia.com>
Date: Fri Oct 12 00:23:12 2018

Fix perf regression in LayoutBoxModelObject::RelativePositionOffset()

In r597543 we introduced a performance regression
due to the changes in LayoutBoxModelObject::RelativePositionOffset().
One of the main differences is that AvailableHeight|Width()
was called always as part of the method,
while that was not the case before.

This patches moves the calls to AvailableHeight|Width()
to the moment where they are needed, trying to minimize
the impact in performance and come back to previous numbers.

No new tests as it's already covered by existent ones.

BUG= 893884 , 835607 

Change-Id: Id8aaba4736a821af9f401492206840c12a2be0e6
Reviewed-on: https://chromium-review.googlesource.com/c/1273117
Commit-Queue: Manuel Rego <rego@igalia.com>
Reviewed-by: Javier Fernandez <jfernandez@igalia.com>
Cr-Commit-Position: refs/heads/master@{#599034}
[modify] https://crrev.com/4226ddf99103e493d7afb23a4c7902ee496108b6/third_party/blink/renderer/core/layout/layout_box_model_object.cc

Sign in to add a comment