Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reland: Correct clip rectangle when block-fragmented. #28395

Merged
merged 1 commit into from Apr 7, 2021

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

This is a re-land of CL:2780489, but this time also remove a bogus
expectation file (added in CL:2071386), to avoid Mac failures.
Original commit message text follows:

We used to apply the whole stitched LayoutBox clip rectangle to every
fragment (we don't store clip rectangles per fragment). To fix this,
post-process the clip rectangle, to clip it against the bounds of the
fragment.

Fixes one test, but test coverage seemed low, so I added 3 more, one for
each writing mode.

This also fixes the following tests when CompositeAfterPaint +
LayoutNGBlockFragmentation are enabled:

external/wpt/css/css-multicol/composited-under-clip-under-multicol.html
fast/multicol/composited-relpos-clipped.html
fast/multicol/composited-relpos-in-clipped.html

.. but note that the expectation files for the latter two are wrong
(pre-CompositeAfterPaint behavior is expected), so they will show up as
failing still.

Long-term we should probably consider storing a clip rectangle per
fragment. There are callers of the original
NGPhysicalBoxFragment::OverflowClipRect() (the one that doesn't take an
incoming break token) in NGBoxFragmentPainter that still don't do the
right thing (no access to the incoming break token there, so no easy way
to fix).

Bug: 1191162, 829028
Change-Id: If0d9e2c636d553186deb8cc3b022d8f729b3f1be
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2809397
Reviewed-by: Koji Ishii <kojii@chromium.org>
Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/master@{#869946}

This is a re-land of CL:2780489, but this time also remove a bogus
expectation file (added in CL:2071386), to avoid Mac failures.
Original commit message text follows:

We used to apply the whole stitched LayoutBox clip rectangle to every
fragment (we don't store clip rectangles per fragment). To fix this,
post-process the clip rectangle, to clip it against the bounds of the
fragment.

Fixes one test, but test coverage seemed low, so I added 3 more, one for
each writing mode.

This also fixes the following tests when CompositeAfterPaint +
LayoutNGBlockFragmentation are enabled:

  external/wpt/css/css-multicol/composited-under-clip-under-multicol.html
  fast/multicol/composited-relpos-clipped.html
  fast/multicol/composited-relpos-in-clipped.html

.. but note that the expectation files for the latter two are wrong
(pre-CompositeAfterPaint behavior is expected), so they will show up as
failing still.

Long-term we should probably consider storing a clip rectangle per
fragment. There are callers of the original
NGPhysicalBoxFragment::OverflowClipRect() (the one that doesn't take an
incoming break token) in NGBoxFragmentPainter that still don't do the
right thing (no access to the incoming break token there, so no easy way
to fix).

Bug: 1191162, 829028
Change-Id: If0d9e2c636d553186deb8cc3b022d8f729b3f1be
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2809397
Reviewed-by: Koji Ishii <kojii@chromium.org>
Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/master@{#869946}
Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The review process for this patch is being conducted in the Chromium project.

@chromium-wpt-export-bot chromium-wpt-export-bot merged commit 50cbbb7 into master Apr 7, 2021
@chromium-wpt-export-bot chromium-wpt-export-bot deleted the chromium-export-c306fd1f16 branch April 7, 2021 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants