1202029 - Use the containing block for determining perspective for transformed elements.

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Matt Woodrow (:mattwoodrow)]

Attachment: perspective-containing-block

The latest versions of the transforms spec [1] say that we need to check the containing block element of the transformed element when looking for a perspective property, rather than the parent element.

I don't think this should break web compat, since it seems very rare that the author would have perspective on an ancestor that isn't the parent and really didn't want the perspective applied.


[1] https://drafts.csswg.org/css-transforms/#accumulated-3d-transformation-matrix-computation

Comment 1

[Matt Woodrow (:mattwoodrow)]

This doesn't quite work currently.

For scrollframes we create an outer HTMLScroll frame (which has the perspective style) and an inner BlockFrame (which doesn't).

For a child frame of the inner scrolled one, GetContainingBlock() returns the BlockFrame, which doesn't have the perspective style, and things break.

Doing something like GetContainingBlock()->GetContent()->GetPrimaryFrame() solves this (for this case at least).

David, any idea what the 'right' way to solve this is?

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #1)
> This doesn't quite work currently.
> 
> For scrollframes we create an outer HTMLScroll frame (which has the
> perspective style) and an inner BlockFrame (which doesn't).
> 
> For a child frame of the inner scrolled one, GetContainingBlock() returns
> the BlockFrame, which doesn't have the perspective style, and things break.
> 
> Doing something like GetContainingBlock()->GetContent()->GetPrimaryFrame()
> solves this (for this case at least).
> 
> David, any idea what the 'right' way to solve this is?

I think the right way is to move the workaround for that bug in GetContainingBlock() from GetPercentBSize() in nsLayoutUtils.cpp (skipping the scrolledContent frames) into GetNearestBlockContainer (which is a helper for GetContainingBlock).

I think this probably fixes a correctness bug in many of the callers of GetContainingBlock; it's possible it might break something in some of the other callers, though.  I'm most worried about:

 (a) the callers in nsComputedDOMStyle, since I suspect that GetContentRect() won't do the right thing since we'll have the padding on the scrolledContent frame instead of the outer.

 (b) the changes that result from the change to nsHTMLReflowState::mCBReflowState


If you don't want to deal with this, you could use the same workaround as GetPercentBSize and file a followup on the rest.  (Or, alternatively, add a flags param to GetContainingBlock and convert GetPercentBSize and this to use non-default flags, and file a followup on the rest.)

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Use the containing block for perspective

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8659420 [details] [diff] [review]
Use the containing block for perspective

nsDisplayTransform::GetDeltaToPerspectiv...:

>+  nsIFrame* parent = aFrame->GetContainingBlock(nsIFrame::SKIP_SCROLLED_FRAME);

Maybe call it cbFrame instead of parent?

>+  const nsStyleDisplay* display = parent->StyleContext()->StyleDisplay();

just parent->StyleDisplay() (i.e., drop the "->StyleContext()")



... and then the same two comments on
nsDisplayTransform::FrameTransformProperties::FrameTransformProperties



>+  nsIFrame* f = nullptr;

No need to initialize, given that it's set just below in both branches.

FinishAndStoreOverflow:

>   // Store the passed in overflow area if we are a preserve-3d frame or we have
>   // a transform, and it's not just the frame bounds.
>-  if (Preserves3D() || HasPerspective() || IsTransformed()) {
>+  if (Preserves3D() || IsTransformed()) {

I don't actually follow this change, although it does at least make
the code agree with the comment.


nsIFrame.h:

Could you add comments to GetContainingBlock saying that passing the
SKIP_SCROLLED_FRAME flag is needed to follow the spec correctly.

Could you also add comments to IsContainingBlock saying that it returns
whether this frame can be a containing block for an in-flow block or an
inline frame, but that, like GetContainingBlock, it incorrectly treats
the scrolledContent frame as a containing block.


Though, I actually think it would probably be better to pass the
flags parameter through to GetNearestBlockContainer and IsContainingBlock,
handle it in IsContainingBlock, and then fix the caller of IsContainingBlock
in RecomputePerspectiveChildrenOverflow to pass the flag instead.
(And you could make the flags parameter mandatory rather than optional
for IsContainingBlock.)


And could you file a followup bug on looking into other callers of GetContainingBlock?


r=dbaron with that

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f9c34eb2d61

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4f9c34eb2d61