Closed Bug 976365 Opened 9 years ago Closed 9 years ago

'perspective' != 'none' should create a containing block for absolute and fixed positioned descendants (like transform

Categories

(Core :: Layout, defect, P4)

defect

Tracking

()

VERIFIED FIXED
mozilla31
Tracking Status
firefox31 --- verified

People

(Reporter: dbaron, Assigned: mattwoodrow)

References

Details

(Keywords: css3)

Attachments

(2 files, 1 obsolete file)

Splitting this out from bug 968555.

The 'perspective' CSS property (when not 'none', its initial value) should cause the element to be the containing block for absolute and fixed positioned descendants (as transform does).
Keywords: css3
Priority: -- → P4
http://acko.net/files/slacko/ appears to be affected by this bug. The viewport is hidden, and the scrollable div isn't getting the perspective elements overflow added to its scroll range.
Attachment #8392095 - Flags: review?(dbaron)
This fixes scrolling on the above link. Overflow calculation is still really wrong (watch the scrollbar size jump as you scroll the 3d content off the top), and performance isn't great.
Ah, damn, this hits a lot of the assertions in IsPositioned()/IsTransformed() that assert aFrame->StyleDisplay() == this. I guess that isn't always true for these callers because we're in the process of setting that up?
Assignee: nobody → matt.woodrow
Comment on attachment 8392095 [details] [diff] [review]
Make perspective create a containing block

I suspect the assertions are from the change to ConstructTable, which is wrong because you're passing the wrong frame to IsPositioned.  (And the old code was using the wrong frame too, but didn't call the code that asserted.)  You want newFrame rather than aParentFrame.  (You should check that you agree that makes sense.)
I agree about that case, but the assertion is coming from ConstructFrameFromItemInternal when constructing a scroll frame/scrolled frame.
Do you still want me to review the above patch, or does it need more work?
Flags: needinfo?(matt.woodrow)
It still needs a solution to the assertions, but I'm a bit stuck on what to do. The scroll frame/scrolled frame having different style contexts seems to be the problem, but I'm not sure what to do about it.
Flags: needinfo?(matt.woodrow)
Attachment #8392095 - Attachment is obsolete: true
Attachment #8392095 - Flags: review?(dbaron)
Attachment #8394612 - Flags: review?(dbaron)
Depends on: 975741
Comment on attachment 8394612 [details] [diff] [review]
Make perspective create a containing block v2

>       if ((maybeAbsoluteContainingBlockDisplay->IsAbsolutelyPositionedStyle() ||
>            maybeAbsoluteContainingBlockDisplay->IsRelativelyPositionedStyle() ||
>            (maybeAbsoluteContainingBlockDisplay->HasTransformStyle() &&
>-            cb->IsFrameOfType(nsIFrame::eSupportsCSSTransforms))) &&
>+            cb->IsFrameOfType(nsIFrame::eSupportsCSSTransforms) ||
>+           maybeAbsoluteContainingBlockDisplay->HasPerspectiveStyle())) &&
>           !cb->IsSVGText()) {

Could you:

 (1) put these in the same order as nsStyleDisplay::IsPositioned (i.e., move the HasPerspectiveStyle check up one line so it's right after HasTransformStyle)

 (2) add a comment right above the if saying that this check is equivalent to doing nsStyleDisplay::IsPositioned using the style from the outer frame and the IsFrameOfType check from the inner frame


r=dbaron with that
Attachment #8394612 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] (needinfo? me) (UTC+8, but slow response this week) from comment #10)
> Comment on attachment 8394612 [details] [diff] [review]
> Make perspective create a containing block v2
> 
> >       if ((maybeAbsoluteContainingBlockDisplay->IsAbsolutelyPositionedStyle() ||
> >            maybeAbsoluteContainingBlockDisplay->IsRelativelyPositionedStyle() ||
> >            (maybeAbsoluteContainingBlockDisplay->HasTransformStyle() &&
> >-            cb->IsFrameOfType(nsIFrame::eSupportsCSSTransforms))) &&
> >+            cb->IsFrameOfType(nsIFrame::eSupportsCSSTransforms) ||
> >+           maybeAbsoluteContainingBlockDisplay->HasPerspectiveStyle())) &&
> >           !cb->IsSVGText()) {
> 
> Could you:
> 
>  (1) put these in the same order as nsStyleDisplay::IsPositioned (i.e., move
> the HasPerspectiveStyle check up one line so it's right after
> HasTransformStyle)
> 

nsStyleDisplay::IsPositioned is using HasTransform(nsIFrame*) rather than HasTransformStyle() && IsFrameOfType() (for the same reasons as the rest of this expansion). The ordering is the same, we just have the one line expanded into two.

I'll add the comment you requested.
(In reply to Matt Woodrow (:mattwoodrow) from comment #11)
> nsStyleDisplay::IsPositioned is using HasTransform(nsIFrame*) rather than
> HasTransformStyle() && IsFrameOfType() (for the same reasons as the rest of
> this expansion). The ordering is the same, we just have the one line
> expanded into two.

ah, right.  Sorry.
https://hg.mozilla.org/mozilla-central/rev/8dafd4b9969a
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Attached image rectangle.png
Scrolling works (though is a little choppy), but I see a gray rectangle after clicking on the left side of the page. What do you think?
Flags: needinfo?(matt.woodrow)
I don't think that's related to this bug.
Flags: needinfo?(matt.woodrow)
Verified as fixed using the following environment:
FF 31.0b6
Build Id: 20140630185627
OS: Win 7 x64, Ubuntu 13.04 x64, Mac Os X 10.9.2
Status: RESOLVED → VERIFIED
Keywords: verifyme
Depends on: 1203089
You need to log in before you can comment on or make changes to this bug.