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

Revert "Make 3D Rendering Context follow DOM tree for absolute/fixed position." #28825

Merged
merged 1 commit into from May 5, 2021

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented May 4, 2021

This reverts commit ed024d718b866ebc95f9eaed49a6c6dc546113d1.

Reason for revert: The original rationale that this would not change
any behavior without TransformInterop enabled was incorrect, because
backface-visibility: hidden (which does not create a containing block
for absolute/fixed) causes NeedsTransform() to be true, which thus
causes UpdateTransform to update rendering_context_id and
should_flatten_inherited_transform.

Original change's description:

Make 3D Rendering Context follow DOM tree for absolute/fixed position.

When TransformInterop is enabled, make the notion of 3D Rendering
Context follow the DOM tree for absolute and fixed-positioned elements
like it does for everything else.

When TransformInterop is not enabled, there are no differences between
following the DOM tree versus the containing block tree, which this
DCHECK()s temporarily while storing the data in two places. This is
because the objects that changes of rendering_context_id and
should_flatten_inherited_transform are associated with are all either
containing blocks for absolute and fixed positioned elements or (for
SVG) are associated closely enough with such containing blocks.

(This is intended to change behavior only when
RuntimeEnabledFeatures::TransformInteropEnabled(), though it should
(once the temporary members are removed) unconditionally reduce the
size of structs used on the stack.)

Bug: 1189985
Change-Id: I5d3bff009e2fbac00d3d011a6a9adc77ad2f9829
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2776711
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: David Baron <dbaron@chromium.org>
Cr-Commit-Position: refs/heads/master@{#878158}

Bug: 1189985
Fixed: 1204790, 1204853
Change-Id: Idb2a635edd9b762454bb23c536cb37bc2ac7330f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2873105
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: David Baron <dbaron@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#879166}

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.

…position."

This reverts commit ed024d718b866ebc95f9eaed49a6c6dc546113d1.

Reason for revert: The original rationale that this would not change
any behavior without TransformInterop enabled was incorrect, because
backface-visibility: hidden (which does not create a containing block
for absolute/fixed) causes NeedsTransform() to be true, which thus
causes UpdateTransform to update rendering_context_id and
should_flatten_inherited_transform.

Original change's description:
> Make 3D Rendering Context follow DOM tree for absolute/fixed position.
>
> When TransformInterop is enabled, make the notion of 3D Rendering
> Context follow the DOM tree for absolute and fixed-positioned elements
> like it does for everything else.
>
> When TransformInterop is not enabled, there are no differences between
> following the DOM tree versus the containing block tree, which this
> DCHECK()s temporarily while storing the data in two places.  This is
> because the objects that changes of rendering_context_id and
> should_flatten_inherited_transform are associated with are all either
> containing blocks for absolute and fixed positioned elements or (for
> SVG) are associated closely enough with such containing blocks.
>
> (This is intended to change behavior only when
> RuntimeEnabledFeatures::TransformInteropEnabled(), though it should
> (once the temporary members are removed) unconditionally reduce the
> size of structs used on the stack.)
>
> Bug: 1189985
> Change-Id: I5d3bff009e2fbac00d3d011a6a9adc77ad2f9829
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2776711
> Reviewed-by: Philip Rogers <pdr@chromium.org>
> Commit-Queue: David Baron <dbaron@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#878158}

Bug: 1189985
Fixed: 1204790, 1204853
Change-Id: Idb2a635edd9b762454bb23c536cb37bc2ac7330f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2873105
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: David Baron <dbaron@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#879166}
@jpchase
Copy link

jpchase commented May 5, 2021

The failing check is "without-changes", and it looks like some infra type error. Going to force merge.

@jpchase jpchase merged commit 503ab1f into master May 5, 2021
@jpchase jpchase deleted the chromium-export-cl-2873105 branch May 5, 2021 16:04
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

4 participants