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

VERIFIED FIXED in Firefox 31

Status

()

defect
P4
normal
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: dbaron, Assigned: mattwoodrow)

Tracking

({css3})

Trunk
mozilla31
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox31 verified)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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).

Updated

5 years ago
Priority: -- → P4
Assignee

Comment 1

5 years ago
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.
Assignee

Comment 2

5 years ago
Attachment #8392095 - Flags: review?(dbaron)
Assignee

Comment 3

5 years ago
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.
Assignee

Comment 4

5 years ago
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

Updated

5 years ago
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.)
Assignee

Comment 6

5 years ago
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)
Assignee

Comment 8

5 years ago
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)
Assignee

Comment 9

5 years ago
Attachment #8392095 - Attachment is obsolete: true
Attachment #8392095 - Flags: review?(dbaron)
Attachment #8394612 - Flags: review?(dbaron)
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+
Assignee

Comment 11

5 years ago
(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: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Posted 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)
Assignee

Comment 16

5 years ago
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

Updated

4 years ago
Depends on: 1203089
You need to log in before you can comment on or make changes to this bug.