1236828 - Support position: absolute on top layer element

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

The Fullscreen API spec states that top layer elements can have position value 'absolute' or 'fixed', but currently we only support 'fixed' because we only need that for fullscreen.

However, we eventually want to also support 'absolute' for <dialog>.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

It seems a naive change in css frame constructor and rule node doesn't work as expected. Scrolling via mouse wheel doesn't move the absolutely-positioned top layer element. I guess there is some layer-related optimization kicks in, but do not have idea what it is and how can I fix it.

Matt, could you provide some information around this? Currently the display lists of all top layer elements are built in the viewport frame to ensure their painting order, however the containing block of absolutely-positioned elements is the initial containing block. I guess building the display lists in canvas frame could fix this, but I suspect that may violate the rendering order.

Comment 2

[Matt Woodrow (:mattwoodrow)]

If we want elements within the top layer to be scrollable, then we probably need those frames to be children of the root scrollframe.

We compute the on-screen position of elements by walking up the frame ancestors and adding the top/left of each one. We need the scrollable frame to be in this chain in order to get scroll position taken into account.

Can we have the frames there, and then fix the display list sorting code to make sure the top level content ends up in the right place?

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

They are already children of the initial containing block (the canvas frame I suppose), so should be descendent of the root scrollframe. It is just that their display list are built by the viewport frame rather than their containing block.

When I drag the scrollbar, the absolutely-positioned top layer element moves as expected, but when I use the mouse wheel, the element is not moved until a next paint, and a white area is scrolled instead. So it seems there is some compositing optimization for scrolling?

Comment 4

[Matt Woodrow (:mattwoodrow)]

Does it work if you disable layers.async-pan-zoom.enabled (needs a restart), or if you disable e10s?

If so, then it's probably a problem in the way we annotate layers to tell APZ how to scroll them.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Yeah, it works with either layers.async-pan-zoom.enabled or e10s disabled.

So, it seems yes, it is something related to APZ. That means I would need to put them in a layer in the first place? Is there any document or could you point me to some source file about working with APZ?

Comment 6

[Matt Woodrow (:mattwoodrow)]

There isn't really, sorry.

You shouldn't really need explicit layers as such.

Each display item has an 'animated geometry root' (AGR) which is the topmost frame that we expect that display item to stay fixed relative to (for example, an ancestor with animated left, or the root frame in a scroll container).

FrameLayerBuilder creates separate layers for each set of consecutive display items with the same AGR, so we'd expect scrolled top layer content and fixed top layer content to be split into separate layers already.

We then tag these layers with FrameMetrics object that describe the scrollable containers that contain them, so that APZ knows how to handle them.

Can you enable layout.display-list.dump in a debug build, and then upload the dump somewhere.

It'll dump for every paint (and for both processes), so expect a lot of output.

If you enable the pref, load the broken content and then kill the process it should be easy to find the relevant dump at the end.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Attachment: related paint log

This should be the relevant log.

The BackgroundColor at the last line is the absolutely-positioned top layer element (id:top-layer-test）, and its parent wrapper is the wrap list for all items in the top layer.

From this dump, the most obvious difference between that item and other scrollable items is the value inside scrollClip(). Other items have scrollClip(<0,5100,75060,12900> [async-scrollable]) while that of the top layer item is just scrollClip(). This could be the reason.

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Attachment: the whole dump

You can use id:top-layer-test to locate the target display item.

Comment 9

[Matt Woodrow (:mattwoodrow)]

You are correct :)

The scroll clip (chain) attached to a display item is how FrameLayerBuilder finds the scroll frames that might scroll a given item.

We add an entry to the chain when we go through ScrollFrameHelper::BuildDisplayList so that descendant items know they are scrollable.

The custom display list building code that bypasses this isn't getting the scroll clips setup, so we don't have any scrolling information.

CC'ing Markus, since he's the scroll clip expert :)

Comment 10

[Matt Woodrow (:mattwoodrow)]

I think we need to build the top layer display items from within ScrollFrameHelper::BuildDisplayList (at least the ones that are attached to that frame), and then fix display list sorting to get them back to the right place.

The other option is factoring out all the scroll clip setup code so we can make sure the same code runs when doing the custom display list building in ViewportFrame, but that seems more error prone.

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1236828 part 1 - Apply proper clip state to top layer frames. r=mstange

Review commit: https://reviewboard.mozilla.org/r/44543/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44543/

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1236828 part 2 - Make frame constructor support absolutely-positioned top layer frame. r=bz

Review commit: https://reviewboard.mozilla.org/r/44545/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44545/

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1236828 part 3 - Allow setting position: absolute for top layer element and add test. r=heycam

Review commit: https://reviewboard.mozilla.org/r/44547/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44547/

Comment 14

[Markus Stange [:mstange]]

Comment on attachment 8738443 [details]
MozReview Request: Bug 1236828 part 1 - Apply proper clip state to top layer frames. r=mstange

https://reviewboard.mozilla.org/r/44543/#review41337

::: layout/generic/nsViewportFrame.cpp
(Diff revision 1)
>        if (!(frame->GetStateBits() & NS_FRAME_OUT_OF_FLOW)) {
>          MOZ_ASSERT(!elem->GetParent()->IsHTMLElement(), "HTML element "
>                     "should always be out-of-flow if in the top layer");
>          continue;
>        }
> -      MOZ_ASSERT(frame->GetParent() == this);

Is this assert wrong? Why?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8738444 [details]
MozReview Request: Bug 1236828 part 2 - Make frame constructor support absolutely-positioned top layer frame. r=bz

https://reviewboard.mozilla.org/r/44545/#review41385

::: layout/base/nsCSSFrameConstructor.cpp:761
(Diff revision 1)
>  
>    // Containing block information for out-of-flow frames.
>    nsAbsoluteItems           mFixedItems;
>    nsAbsoluteItems           mAbsoluteItems;
>    nsAbsoluteItems           mFloatedItems;
> -  // Items in the top layer are always fixed positioned children of the
> +  // The containing block of frame in the top layer is defined by the

"of a frame"

::: layout/base/nsCSSFrameConstructor.cpp:762
(Diff revision 1)
>    // Containing block information for out-of-flow frames.
>    nsAbsoluteItems           mFixedItems;
>    nsAbsoluteItems           mAbsoluteItems;
>    nsAbsoluteItems           mFloatedItems;
> -  // Items in the top layer are always fixed positioned children of the
> -  // viewport frame. It differs from mFixedItems that the items here
> +  // The containing block of frame in the top layer is defined by the
> +  // spec: fixed-positioned frames are children of viewport frame, and

"of the viewport frame"

::: layout/base/nsCSSFrameConstructor.cpp:1140
(Diff revision 1)
>      MOZ_ASSERT(aStyleDisplay->IsAbsolutelyPositionedStyle(),
>                 "Top layer items should always be absolutely positioned");
> -    return mTopLayerItems.containingBlock;
> +    if (aStyleDisplay->mPosition == NS_STYLE_POSITION_FIXED) {
> +      MOZ_ASSERT(mTopLayerFixedItems.containingBlock, "No root frame?");
> +      return mTopLayerFixedItems.containingBlock;
> +    } else {

Please lose the else-after-return.

r=me

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

https://reviewboard.mozilla.org/r/44543/#review41337

> Is this assert wrong? Why?

Hmmm, this actually should belong to part 2. With part 2, top layer frames would be child of the canvas frame when they have "position: absolute" rather than fixed, and thus this assert no longer valid.

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

It seems dbaron currently has a great backlog on reviewing. As this is a trivial change on style part, I'd try heycam. Hmmm, it seems heycam has some backlog there as well, but might be slightly better than dbaron :)

Comment 18

[Cameron McCormack (:heycam)]

Comment on attachment 8738445 [details]
MozReview Request: Bug 1236828 part 3 - Allow setting position: absolute for top layer element and add test. r=heycam

https://reviewboard.mozilla.org/r/44547/#review41939

Can you add a test for :-moz-full-screen::backdrop { position: absolute; }?

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

Then I found a crash :/

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8738443 [details]
MozReview Request: Bug 1236828 part 1 - Apply proper clip state to top layer frames. r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44543/diff/1-2/

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8738444 [details]
MozReview Request: Bug 1236828 part 2 - Make frame constructor support absolutely-positioned top layer frame. r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44545/diff/1-2/

Comment 22

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8738445 [details]
MozReview Request: Bug 1236828 part 3 - Allow setting position: absolute for top layer element and add test. r=heycam

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44547/diff/1-2/

Comment 23

[Cameron McCormack (:heycam)]

Comment on attachment 8738445 [details]
MozReview Request: Bug 1236828 part 3 - Allow setting position: absolute for top layer element and add test. r=heycam

https://reviewboard.mozilla.org/r/44547/#review41955

While I don't think it's needed for this bug's patches specifically, it would be good to have a test for ::backdrop with top/right/bottom/left values that don't cover the entire viewport, too.

::: dom/html/test/file_fullscreen-backdrop.html:95
(Diff revision 2)
> +  setBackdropStyle("position: absolute");
> +  info("Changing position shouldn't immediately affect the view");
> +  assertWindowPureColor(window, "black");
> +
> +  window.scroll(0, screen.height);
> +  info("Scrolled up the absolutely-positioned absolute");

"Scrolled up the absolutely-positioned element"?

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8738444 [details]
MozReview Request: Bug 1236828 part 2 - Make frame constructor support absolutely-positioned top layer frame. r=bz

https://reviewboard.mozilla.org/r/44545/#review42063

r=me, assuming the only substantive change here is which framelist the backdrop placeholder goes in.

Comment 25

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8738443 [details]
MozReview Request: Bug 1236828 part 1 - Apply proper clip state to top layer frames. r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44543/diff/2-3/

Comment 26

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8738444 [details]
MozReview Request: Bug 1236828 part 2 - Make frame constructor support absolutely-positioned top layer frame. r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44545/diff/2-3/

Comment 27

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8738445 [details]
MozReview Request: Bug 1236828 part 3 - Allow setting position: absolute for top layer element and add test. r=heycam

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44547/diff/2-3/

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b7f833f9fe09
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e846f999e8b
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6e275dfcdee

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b7f833f9fe09
https://hg.mozilla.org/mozilla-central/rev/5e846f999e8b
https://hg.mozilla.org/mozilla-central/rev/f6e275dfcdee