Bug 84796 – Implement top layer rendering bits

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Theresa O'Connor]

In https://dvcs.w3.org/hg/fullscreen/raw-file/tip/Overview.html#new-stacking-layer the Fullscreen API spec adds a new stacking layer, the top layer. The Fullscreen API and the HTML <dialog> element both depend on this stacking layer.

[Radar WebKit Bug Importer]

<rdar://problem/11313359>

[Matt Falkenhagen]

I've been looking at the specs to try to figure out the requirements for the top layer.

My understanding is that the top layer can have multiple fullscreen elements and multiple dialogs. Due to requestFullscreen() restrictions, the fullscreen elements on the top layer must be an ancestor chain (the topmost element is the leaf descendant). Typically, you won't see other fullscreened ancestors since the default ::backdrop is a black box covering the whole viewport. But with a custom backdrop style, the ancestors may be visible. However, there is no double rendering of an element: the containing block of each element in the top layer is the initial containing block.

I'm not sure if the current Fullscreen implementation supports rendering multiple fullscreen elements. It might just render the topmost fullscreen element and black out the rest of the screen. So that would have to change to render all elements in the top layer including ::backdrops.

Maybe a good plan would be to first try to get top layer working for the Fullscreen API, then add support for dialog. This is because if we implement it for dialog first, we might end up with something difficult to get to work with the current Fullscreen implementation.

[Matt Falkenhagen]

It seems that RenderFullScreen works now by wrapping a renderer when its element becomes full screen and then unwrapping when it's no longer full screen. So there is one element (and its subtree) that is full screen at a time.

I wonder if we can do something like this instead:

- RenderFullScreen (perhaps renamed "RenderTopLayer") is a child of RenderView
- When an element is added to the top layer, its renderer becomes a child of RenderFullScreen
- When an element is removed from the top layer, its renderer is returned to its original place
- RenderFullScreen handles the special layout and stacking of the elements as described in the spec (z-index has no effect, position:"static" computes to "absolute", and the static position for top, left, right are zero).

Does that sound like an okay approach? Maybe dynamic changes to the DOM tree would be problematic for returning no longer fullscreened elements to their original place in the render tree.

[Jer Noble]

Seems like a good approach to me.(In reply to comment #3)
> It seems that RenderFullScreen works now by wrapping a renderer when its element becomes full screen and then unwrapping when it's no longer full screen. So there is one element (and its subtree) that is full screen at a time.
> 
> I wonder if we can do something like this instead:
> 
> - RenderFullScreen (perhaps renamed "RenderTopLayer") is a child of RenderView
> - When an element is added to the top layer, its renderer becomes a child of RenderFullScreen
> - When an element is removed from the top layer, its renderer is returned to its original place
> - RenderFullScreen handles the special layout and stacking of the elements as described in the spec (z-index has no effect, position:"static" computes to "absolute", and the static position for top, left, right are zero).
> 
> Does that sound like an okay approach? Maybe dynamic changes to the DOM tree would be problematic for returning no longer fullscreened elements to their original place in the render tree.

I would hope that returning un-fullscreened element to their original place would fall out of recalculating the render tree after a style change, in which case changes to the DOM tree during full screen wouldn't be problematic.

I like the idea of replacing RenderFullScreen with a RenderTopLayer-like object.  I'm pretty optimistic about this approach.

[Matt Falkenhagen]

Jer, thanks for the comments.

After investigating some more and talking with morrita@, I'm wondering if we should use RenderLayer to implement this. It sounds like the RenderObject tree should basically correspond to the DOM tree, so moving the renderers around in the tree may cause problems.

I wonder if instead we can extend RenderLayer to do top layer stacking in addition to the existing z-index stacking. Each element in the top layer would have a RenderLayer. Then we can keep the RenderObjects in their usual place in the render tree. It might still be necessary to have their containing block (but not parent) be RenderFullScreen/RenderTopLayer, so it can do any special positioning required.

[Hajime Morrita]

(In reply to comment #5)
> Jer, thanks for the comments.
> 
> After investigating some more and talking with morrita@, I'm wondering if we should use RenderLayer to implement this. It sounds like the RenderObject tree should basically correspond to the DOM tree, so moving the renderers around in the tree may cause problems.
> 
> I wonder if instead we can extend RenderLayer to do top layer stacking in addition to the existing z-index stacking. Each element in the top layer would have a RenderLayer. Then we can keep the RenderObjects in their usual place in the render tree. It might still be necessary to have their containing block (but not parent) be RenderFullScreen/RenderTopLayer, so it can do any special positioning required.

Some followup:

- Specifying z-index gives a layer to the renderer. What special about <dialog>
  is that it might need a layer in some unusual position.
- Render tree represents a CSS Box tree, which roughly matches DOM tree but
  it has its own construction algorithm (specified in CSS standard).
  In my understanding, moving RenderDialog to some special position
  means changing that algorithm.
  In contrast, RenderLayer roughly corresponds the stacking context, 
  so it seems to have natural fit to achieve the modality.

[Ojan Vafai]

CC some people who know things about RenderLayers and stacking contexts.

[Simon Fraser (smfr)]

Doing this via RenderLayer seems reasonable.

[Matt Falkenhagen]

Created attachment 165969 [details]
WIP patch

[Matt Falkenhagen]

I've uploaded a WIP patch for the stacking behavior of the top layer. I'd be happy for any comments.

So far this patch only does stacking. Other behavior for an element in the top layer is not yet implemented, like:
* Its containing block is the initial containing block.

Also, so far only modal dialogs can be added to the top layer, so I've guarded everything behind the DIALOG_ELEMENT flag. The idea is to make fullscreen also use the top layer, so later this can be DIALOG_ELEMENT || FULLSCREEN_API.

[Ojan Vafai]

Comment on attachment 165969 [details]
WIP patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165969&action=review

Looks reasonable to me. I'll give one of the more seasoned RenderLayer folks time to take a look before r+'ing though.

> Source/WebCore/dom/Document.cpp:5500
> +    size_t position = m_topLayer.find(element);
> +    if (position != notFound)

Should you ASSERT(m_topLayer.contains(element)) or ASSERT(position != notFound)? I don't see how you'd get to this code without the element being in m_topLayer.

[Simon Fraser (smfr)]

Comment on attachment 165969 [details]
WIP patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165969&action=review

> Source/WebCore/dom/Document.h:1167
> +    Vector<RefPtr<Element> >& topLayer() { return m_topLayer; }

This function is confusingly named. It doesn't return a layer; it returns a vector of elements. Maybe elementsInTopLayer(), or topLayerElements()?

> Source/WebCore/dom/Document.h:1471
> +    Vector<RefPtr<Element> > m_topLayer;

What's the mechanism for removing elements from this array when they go away? Do we assume that HTMLDialogElement is the only element for now that can get into this list?

> Source/WebCore/dom/Element.h:420
> +    virtual void setIsInTopLayer(bool);

I think this should be setInTopLayer()

> Source/WebCore/dom/ElementRareData.h:125
>  #if ENABLE(FULLSCREEN_API)
>      bool m_containsFullScreenElement;
>  #endif
> +
> +#if ENABLE(DIALOG_ELEMENT)
> +    bool m_isInTopLayer;
> +#endif

Should probably group this bools with the previous two bools, and make them part of the bitfield.

> Source/WebCore/rendering/RenderBox.h:50
> +#if ENABLE(DIALOG_ELEMENT)
> +        || (node() && node()->isElementNode() && toElement(node())->isInTopLayer())
> +#endif

I worry slightly that this will affect performance. Don't top layer elements have non-auto z-index by virtue of being stacking contexts anyway?

> Source/WebCore/rendering/RenderLayer.cpp:4606
> +    if (isRootLayer()) {
> +        Vector<RefPtr<Element> >& topLayer = renderer()->document()->topLayer();
> +        for (Vector<RefPtr<Element> >::iterator i = topLayer.begin(); i != topLayer.end(); ++i) {
> +            RenderLayer* layer = toRenderBoxModelObject((*i)->renderer())->layer();
> +            m_posZOrderList->append(layer);
> +        }
> +    }

This doesn't seem right. You should never go directly from elements to RenderLayers; I think we need this top layer stuff represented in the render tree too. You have to handle stuff like display:none on a <dialog>. What happens if <dialog> elements have z-index? Can that z-index be negative?

> Source/WebCore/rendering/RenderLayer.h:701
> +        return !style->hasAutoZIndex()
> +#if ENABLE(DIALOG_ELEMENT)
> +            || isInTopLayer()
> +#endif
> +            || isRootLayer();
> +    }

I think top element layers would have non-auto z-index?

[Matt Falkenhagen]

Comment on attachment 165969 [details]
WIP patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165969&action=review

Thanks for the review comments!

>> Source/WebCore/dom/Document.cpp:5500
>> +    if (position != notFound)
> 
> Should you ASSERT(m_topLayer.contains(element)) or ASSERT(position != notFound)? I don't see how you'd get to this code without the element being in m_topLayer.

I should probably change setIsInTopLayer(flag) to do nothing if the flag hasn't changed. In the current patch, we get to this code when a non-modal dialog is closed.

>> Source/WebCore/dom/Document.h:1471
>> +    Vector<RefPtr<Element> > m_topLayer;
> 
> What's the mechanism for removing elements from this array when they go away? Do we assume that HTMLDialogElement is the only element for now that can get into this list?

Perhaps Element::removedFrom can call removeFromTopLayer.

>> Source/WebCore/rendering/RenderBox.h:50
>> +#endif
> 
> I worry slightly that this will affect performance. Don't top layer elements have non-auto z-index by virtue of being stacking contexts anyway?

Top layer elements don't necessarily have a non-auto z-index. Actually, z-index has no effect in the top layer. Maybe StyleResolver can give elements in the top layer a dummy non-auto z-index, so requiresLayer doesn't need to be changed?

>> Source/WebCore/rendering/RenderLayer.cpp:4606
>> +    }
> 
> This doesn't seem right. You should never go directly from elements to RenderLayers; I think we need this top layer stuff represented in the render tree too. You have to handle stuff like display:none on a <dialog>. What happens if <dialog> elements have z-index? Can that z-index be negative?

Hmm, is there a good mechanism for representing the top layer order information in the render tree? Something like adding the position in the top layer vector to the renderer, then sorting on that seems possible but a bit clumsy.

The z-index should be ignored for an element in the top layer.

[Simon Fraser (smfr)]

(In reply to comment #13)
> (From update of attachment 165969 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165969&action=review
> 
> Thanks for the review comments!
> 
> >> Source/WebCore/dom/Document.cpp:5500
> >> +    if (position != notFound)
> > 
> > Should you ASSERT(m_topLayer.contains(element)) or ASSERT(position != notFound)? I don't see how you'd get to this code without the element being in m_topLayer.
> 
> I should probably change setIsInTopLayer(flag) to do nothing if the flag hasn't changed. In the current patch, we get to this code when a non-modal dialog is closed.
> 
> >> Source/WebCore/dom/Document.h:1471
> >> +    Vector<RefPtr<Element> > m_topLayer;
> > 
> > What's the mechanism for removing elements from this array when they go away? Do we assume that HTMLDialogElement is the only element for now that can get into this list?
> 
> Perhaps Element::removedFrom can call removeFromTopLayer.
> 
> >> Source/WebCore/rendering/RenderBox.h:50
> >> +#endif
> > 
> > I worry slightly that this will affect performance. Don't top layer elements have non-auto z-index by virtue of being stacking contexts anyway?
> 
> Top layer elements don't necessarily have a non-auto z-index. Actually, z-index has no effect in the top layer.

<http://dvcs.w3.org/hg/fullscreen/raw-file/tip/Overview.html#rendering> says:

"The z-index property has no effect in the top layer."

and
An element in this stack has the following attributes:
* It generates a new stacking context.

So each element can have z-index: 0

Maybe StyleResolver can give elements in the top layer a dummy non-auto z-index, so requiresLayer doesn't need to be changed?

Yes, that's the right way to do it.

> >> Source/WebCore/rendering/RenderLayer.cpp:4606
> >> +    }
> > 
> > This doesn't seem right. You should never go directly from elements to RenderLayers; I think we need this top layer stuff represented in the render tree too. You have to handle stuff like display:none on a <dialog>. What happens if <dialog> elements have z-index? Can that z-index be negative?
> 
> Hmm, is there a good mechanism for representing the top layer order information in the render tree? Something like adding the position in the top layer vector to the renderer, then sorting on that seems possible but a bit clumsy.

Top layer order is just the sibling order of the renderers in the top layer.

There's another missing piece here, which is the backdrop pseudo-element. I think you'll need to make Renderer changes for that. So I think what you need is to make renderers for the top layer stack, including the backdrop. These would get added to the render tree by RenderView, in a way that guarantees they are higher in z-index than everything else in the document.

[Matt Falkenhagen]

(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 165969 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=165969&action=review
> > > This doesn't seem right. You should never go directly from elements to RenderLayers; I think we need this top layer stuff represented in the render tree too. You have to handle stuff like display:none on a <dialog>. What happens if <dialog> elements have z-index? Can that z-index be negative?
> > 
> > Hmm, is there a good mechanism for representing the top layer order information in the render tree? Something like adding the position in the top layer vector to the renderer, then sorting on that seems possible but a bit clumsy.
> 
> Top layer order is just the sibling order of the renderers in the top layer.

I'm a little lost here. Do you mean to arrange the render tree so top layer renderers are siblings in top layer order? I thought we don't want to change the render tree structure (comment #5, #6).

Perhaps an alternative is to keep the renderers in their original place and build a sorted list by having each top layer renderer point to the next one in the stack.

> There's another missing piece here, which is the backdrop pseudo-element. I think you'll need to make Renderer changes for that. So I think what you need is to make renderers for the top layer stack, including the backdrop. These would get added to the render tree by RenderView, in a way that guarantees they are higher in z-index than everything else in the document.

I see what you mean about needing renderer changes for backdrop. It seems we can implement backdrop with a wrapper renderer that is a parent of both the original renderer and the backdrop renderer (like how RenderFullscreen wraps).

I'm not sure I see how RenderView would add these renderers to the render tree. I guess it depends on whether we are changing the render tree structure.

[Matt Falkenhagen]

Created attachment 168181 [details]
represent top layer in render tree as well

[Matt Falkenhagen]

Simon, I've modified the patch so now top layer renderers are added as children of RenderView in top layer stacking order. This way RenderLayer no longer goes directly from the elements in the top layer to their layers.

I still have not tackled :backdrop yet. I hope to get to that in a subsequent patch.

What do you think?

[Simon Fraser (smfr)]

Comment on attachment 168181 [details]
represent top layer in render tree as well

View in context: https://bugs.webkit.org/attachment.cgi?id=168181&action=review

I'd like to see some tests to actually ensure that the rendering is correct, e.g. tests with high z-index elements and dialogs, as well as nested dialogs. I think you also need to test removing showing dialog elements (esp those with dialog children) via JS.

> Source/WebCore/dom/Document.h:1467
> +    Vector<RefPtr<Element> > m_topLayer;

Should be m_topLayerElements

> Source/WebCore/dom/NodeRareData.h:327
> +    bool isInTopLayer() { return m_isInTopLayer; }

Should be const

> Source/WebCore/html/HTMLDialogElement.cpp:56
> +    setIsInTopLayer(false);

Is close() always called when a <dialog> gets destroyed? What if it's removed from the DOM while showing?

> Source/WebCore/rendering/RenderLayer.cpp:4635
> +#if ENABLE(DIALOG_ELEMENT)
> +    if (isRootLayer()) {
> +        RenderObject* view = renderer()->view();
> +        for (RenderObject* child = view->firstChild(); child; child = child->nextSibling()) {
> +            Element* childElement = child->node()->isElementNode() ? toElement(child->node()) : 0;
> +            if (childElement && childElement->isInTopLayer()) {
> +                RenderLayer* layer = toRenderBoxModelObject(child)->layer();
> +                m_posZOrderList->append(layer);
> +            }
> +        }
> +    }
> +#endif

I'm not sure why you need this code here, since your createRendererIfNeeded() changes ensure that the dialog renderers are parented in the view. Or does this ensure that they get appended after z-index sorting? Some comments would help.

[Matt Falkenhagen]

Comment on attachment 168181 [details]
represent top layer in render tree as well

View in context: https://bugs.webkit.org/attachment.cgi?id=168181&action=review

Thanks for the review!

>> Source/WebCore/html/HTMLDialogElement.cpp:56
>> +    setIsInTopLayer(false);
> 
> Is close() always called when a <dialog> gets destroyed? What if it's removed from the DOM while showing?

I think in that case we get to Element::removedFrom which also removes it from the top layer.

>> Source/WebCore/rendering/RenderLayer.cpp:4635
>> +#endif
> 
> I'm not sure why you need this code here, since your createRendererIfNeeded() changes ensure that the dialog renderers are parented in the view. Or does this ensure that they get appended after z-index sorting? Some comments would help.

Without this code, I think the stacking may be wrong if:
- RenderView has a child with a positive z-index, or
- RenderView has a child appended after the top layer renderers, with a child with a positive z-index

I'm not sure whether those are possible. I'll investigate more.

[Matt Falkenhagen]

Created attachment 168661 [details]
Patch

[Matt Falkenhagen]

Simon, I've modified the patch to address your review comments.

(In reply to comment #18)
> I'd like to see some tests to actually ensure that the rendering is correct, e.g. tests with high z-index elements and dialogs, as well as nested dialogs. I think you also need to test removing showing dialog elements (esp those with dialog children) via JS.

I think the tests now cover all of these cases. I've added some comments to the top of the tests to explain what they do.

> > Source/WebCore/html/HTMLDialogElement.cpp:56
> > +    setIsInTopLayer(false);
> 
> Is close() always called when a <dialog> gets destroyed? What if it's removed 
from the DOM while showing?

I've verified Element::removedFrom is called and removes the element from the top layer. This also works for dialogs in the subtree of a removed element.

> 
> > Source/WebCore/rendering/RenderLayer.cpp:4635
> > +#if ENABLE(DIALOG_ELEMENT)
> > +    if (isRootLayer()) {
> > +        RenderObject* view = renderer()->view();
> > +        for (RenderObject* child = view->firstChild(); child; child = child->nextSibling()) {
> > +            Element* childElement = child->node()->isElementNode() ? toElement(child->node()) : 0;
> > +            if (childElement && childElement->isInTopLayer()) {
> > +                RenderLayer* layer = toRenderBoxModelObject(child)->layer();
> > +                m_posZOrderList->append(layer);
> > +            }
> > +        }
> > +    }
> > +#endif
> 
> I'm not sure why you need this code here, since your createRendererIfNeeded() changes ensure that the dialog renderers are parented in the view. Or does this ensure that they get appended after z-index sorting? Some comments would help.

I've verified that this is needed or else a positive z-indexed element is rendered on top of the top layer, and added a comment.

[Matt Falkenhagen]

Created attachment 169862 [details]
sync

[Matt Falkenhagen]

Simon, could you please take another look? Thanks.

[Simon Fraser (smfr)]

Comment on attachment 169862 [details]
sync

View in context: https://bugs.webkit.org/attachment.cgi?id=169862&action=review

> Source/WebCore/dom/Element.cpp:975
> +    setIsInTopLayer(false);

I wonder if you should check isInTopLayer() to avoid a perf hit.

> Source/WebCore/dom/NodeRenderingContext.cpp:272
> +#if ENABLE(DIALOG_ELEMENT)
> +    if (element && element->isInTopLayer()) {
> +        parentRenderer = parentRenderer->view();
> +        nextRenderer = 0;
> +        Vector<RefPtr<Element> >& topLayerElements = document->topLayerElements();
> +        size_t topLayerPosition = topLayerElements.find(element);
> +        ASSERT(topLayerPosition != notFound);
> +        for (RenderObject* child = parentRenderer->firstChild(); child; child = child->nextSibling()) {
> +            Element* childElement = child->node()->isElementNode() ? toElement(child->node()) : 0;
> +            if (childElement && childElement->isInTopLayer()) {
> +                size_t childTopLayerPosition = topLayerElements.find(childElement);
> +                ASSERT(childTopLayerPosition != notFound);
> +                if (topLayerPosition < childTopLayerPosition) {
> +                    nextRenderer = child;
> +                    break;
> +                }
> +            }
> +        }
> +    }
> +#endif

I think it's worth moving this into its own function.

[Hajime Morrita]

Comment on attachment 169862 [details]
sync

View in context: https://bugs.webkit.org/attachment.cgi?id=169862&action=review

> Source/WebCore/dom/Element.cpp:2016
> +    if (isInTopLayer() == flag)

This is regarding of Simon's comment above.

nit: looking up element rare data table twice (through isInTopLayer()) will cost. 
You can retrieve elementRareData() here and see ElementRareData::isInTopLayer() to avoid the extra lookup.

[Matt Falkenhagen]

Created attachment 170637 [details]
review comments & fix assert failure

[Matt Falkenhagen]

Comment on attachment 170637 [details]
review comments & fix assert failure

View in context: https://bugs.webkit.org/attachment.cgi?id=170637&action=review

Thanks for the review!

> Source/WebCore/dom/Element.cpp:2014
> +    ElementRareData* rareData = hasRareData() ? elementRareData() : 0;

morrita: Is this what you mean by avoiding looking up element rare data table twice?

> Source/WebCore/dom/Node.cpp:1328
> +                || (renderer->enclosingLayer()->isInTopLayerSubtree())

jchaffraix: Is this right? This patch makes elements in the top layer move from their regular tree location to be children of RenderView. This assertion was failing for elements in the top layer and their subtree. I'm not sure if these are eventually destroyed. It looks like detach() is called on dialog elements in the top layer, but I don't see it being called on their descendants.

[Build Bot]

Comment on attachment 170637 [details]
review comments & fix assert failure

Attachment 170637 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14553685

[Matt Falkenhagen]

Created attachment 171395 [details]
fix win compile failure

[Matt Falkenhagen]

Hi jchaffraix, could you take a look at my change to the assert in Node::detach (see comment 27)? Thanks.

[Julien Chaffraix]

Comment on attachment 171395 [details]
fix win compile failure

View in context: https://bugs.webkit.org/attachment.cgi?id=171395&action=review

> Source/WebCore/ChangeLog:12
> +        contexts and in the desired order.

I have an issue with the high-level design (nobody seems to have mentioned it so maybe it's just me being dumb here): most of what you are talking about are *rendering* concepts (layer, stacking context, ...). Yet you handle this in the DOM, which doesn't seem right.

Your work is close to what RenderFlowThread is doing and RenderFlowThread doesn't pollute the DOM in the same manner. Could you explain the rationale behind going the DOM way?

> Source/WebCore/dom/Element.cpp:1990
> +    return hasRareData() ? elementRareData()->isInTopLayer() : false;

This will be better written:

return hasRareData() && elementRareData()->isInTopLayer();

> Source/WebCore/dom/Element.cpp:1993
> +void Element::setIsInTopLayer(bool flag)

|flag| is not a good name.

> Source/WebCore/dom/Node.cpp:1328
> +            ASSERT(!renderer || renderer->inRenderFlowThread() || (renderer->enclosingLayer()->isInTopLayerSubtree()));

I think this is the same problem as RenderFlowThread - that you shuffle the renderers in the tree - so it makes sense to extend the ASSERT to cover top-layer.

> Source/WebCore/dom/NodeRareData.h:345
> +    bool m_isInTopLayer : 1;

Nobody mentioned it but if this is going to be changed for each element removal (ie in the hot path), we may consider adding this flag to NodeFlags. This would also avoid the horrible code in setIsInTopLayer.

[Matt Falkenhagen]

Comment on attachment 171395 [details]
fix win compile failure

View in context: https://bugs.webkit.org/attachment.cgi?id=171395&action=review

Hi Julien, thanks for the review. I'll just try to address your design comment for now.

>> Source/WebCore/ChangeLog:12
>> +        contexts and in the desired order.
> 
> I have an issue with the high-level design (nobody seems to have mentioned it so maybe it's just me being dumb here): most of what you are talking about are *rendering* concepts (layer, stacking context, ...). Yet you handle this in the DOM, which doesn't seem right.
> 
> Your work is close to what RenderFlowThread is doing and RenderFlowThread doesn't pollute the DOM in the same manner. Could you explain the rationale behind going the DOM way?

I'm pretty inexperienced at WebKit so the design might not be the best, so thanks for asking.

My thinking is as follows. The spec defines DOM functions showModal() and close() for dialog elements. These push and pop from the top layer stack, which defines the rendered stacking order. So it seems to make sense to store the top layer stack in the DOM and pass the needed information to the rendering parts. I'm not sure if the rendering parts can hold the top layer itself. For example it's possible to call showModal on a dialog that doesn't have a renderer. Or if somehow the render tree must be entirely recreated, if the DOM has the top layer stack the needed information still exists.

It looks like the ordering of renderers in RenderFlowThread is based on the position of the nodes in the DOM tree, which makes it possible to not store additional information in the DOM.

Does that make sense?

[Matt Falkenhagen]

Created attachment 171781 [details]
review comments

[Matt Falkenhagen]

Comment on attachment 171395 [details]
fix win compile failure

View in context: https://bugs.webkit.org/attachment.cgi?id=171395&action=review

Hi Julien, I've updated the patch.

I'd like to commit this soon if possible. Do you still have an issue with the design?

>>> Source/WebCore/ChangeLog:12
>>> +        contexts and in the desired order.
>> 
>> I have an issue with the high-level design (nobody seems to have mentioned it so maybe it's just me being dumb here): most of what you are talking about are *rendering* concepts (layer, stacking context, ...). Yet you handle this in the DOM, which doesn't seem right.
>> 
>> Your work is close to what RenderFlowThread is doing and RenderFlowThread doesn't pollute the DOM in the same manner. Could you explain the rationale behind going the DOM way?
> 
> I'm pretty inexperienced at WebKit so the design might not be the best, so thanks for asking.
> 
> My thinking is as follows. The spec defines DOM functions showModal() and close() for dialog elements. These push and pop from the top layer stack, which defines the rendered stacking order. So it seems to make sense to store the top layer stack in the DOM and pass the needed information to the rendering parts. I'm not sure if the rendering parts can hold the top layer itself. For example it's possible to call showModal on a dialog that doesn't have a renderer. Or if somehow the render tree must be entirely recreated, if the DOM has the top layer stack the needed information still exists.
> 
> It looks like the ordering of renderers in RenderFlowThread is based on the position of the nodes in the DOM tree, which makes it possible to not store additional information in the DOM.
> 
> Does that make sense?

I've added this explanation to the ChangeLog.

>> Source/WebCore/dom/Element.cpp:1990
>> +    return hasRareData() ? elementRareData()->isInTopLayer() : false;
> 
> This will be better written:
> 
> return hasRareData() && elementRareData()->isInTopLayer();

Fixed.

>> Source/WebCore/dom/Element.cpp:1993
>> +void Element::setIsInTopLayer(bool flag)
> 
> |flag| is not a good name.

Fixed.

>> Source/WebCore/dom/NodeRareData.h:345
>> +    bool m_isInTopLayer : 1;
> 
> Nobody mentioned it but if this is going to be changed for each element removal (ie in the hot path), we may consider adding this flag to NodeFlags. This would also avoid the horrible code in setIsInTopLayer.

I think morrita also mentioned this to me but said the remaining bits for NodeFlags are quite valuable. Since this code is still guarded by DIALOG_ELEMENT, I think I'd like to keep it as RareData for now. Maybe if this moves out of the feature flag and merges with fullscreen, we can reconsider putting it in NodeFlags.

[Julien Chaffraix]

Comment on attachment 171781 [details]
review comments

View in context: https://bugs.webkit.org/attachment.cgi?id=171781&action=review

High-level comments: because we insert top layer renderers' into the RenderView's children, they should interact with them during layout. I don't know how much of a pain this is.

> Source/WebCore/ChangeLog:13
> +        This adds the top layer element stack to Document. The stack is
> +        stored in the DOM because pushing and popping from the top layer
> +        occurs via the DOM functions showModal() and close() for dialog
> +        elements. It seems difficult to store the stack as part of rendering
> +        because, for example, an element without a renderer can be pushed on
> +        the top layer.

I think more to the point: the FullScreen specification mandates that we track the ordering of the DOM nodes in the top layer, not the renderers. That makes it hard to implement on the rendering side only.

> Source/WebCore/dom/Document.cpp:5553
> +    ASSERT(m_topLayerElements.contains(element));
> +    size_t position = m_topLayerElements.find(element);

You could replace that with:

size_t position = m_topLayerElements.find(element);
ASSERT(position != notFound);

which would remove one iteration over the Vector in Debug.

> Source/WebCore/dom/Document.h:1165
> +    Vector<RefPtr<Element> >& topLayerElements() { return m_topLayerElements; }

This should be a |const| function, probably returning a const Vector as we don't expect callers to modify it.

> Source/WebCore/dom/Node.cpp:1328
> +            ASSERT(!renderer || renderer->inRenderFlowThread() || (renderer->enclosingLayer()->isInTopLayerSubtree()));

Note that this will involve walking up the render tree in Debug for every detach.

> Source/WebCore/dom/NodeRenderingContext.cpp:230
> +    size_t topLayerPosition = topLayerElements.find(element);
> +    ASSERT(topLayerPosition != notFound);

Couldn't we just find the next renderer from the topLayerElements instead of iterating on all the RenderView's children?

Note that because you compute the positions in the Vector in the loop, the code is O(n^2).

> Source/WebCore/rendering/RenderLayer.cpp:4669
> +        for (RenderObject* child = view->firstChild(); child; child = child->nextSibling()) {
> +            Element* childElement = child->node()->isElementNode() ? toElement(child->node()) : 0;
> +            if (childElement && childElement->isInTopLayer()) {

Same comment here: couldn't we use the Document's list?

> LayoutTests/ChangeLog:16
> +

Per discussion, there should be a test for percent length resolution. This would prove that we correctly layout top-layer elements with their containing block as the initial containing block.

> LayoutTests/fast/dom/HTMLDialogElement/top-layer-stacking.html:21
> +    -webkit-transform: rotateY(45deg);

As discussed I don't know if that covers the hardware accelerated case but it needs to be covered by one test as we disable AC on named flow-thread.

> LayoutTests/fast/dom/HTMLDialogElement/top-layer-stacking.html:27
> +<dialog id="z" style="display: none">This should not be displayed.</dialog>

You should add some red for good measure.

> LayoutTests/fast/dom/HTMLDialogElement/top-layer-stacking.html:49
> +document.getElementById('x').showModal();
> +document.getElementById('y').showModal();
> +document.getElementById('z').showModal();

x, y, z don't really speak to me. Better names: topHiddenDialog, middleFixedDialog, bottomDialog.

[Matt Falkenhagen]

Comment on attachment 171781 [details]
review comments

View in context: https://bugs.webkit.org/attachment.cgi?id=171781&action=review

Hi Julien, thanks for the review and discussion on IRC. I've prepared a new patch.

As for having the renderers parented by RenderView, I'm not sure what to do now. I'm inclined to try this approach now and fix it if we start seeing real pain and have a better understanding of the problem. I took the RenderView approach based on Simon's reviews in comments #12 and #14.

>> Source/WebCore/ChangeLog:13
>> +        the top layer.
> 
> I think more to the point: the FullScreen specification mandates that we track the ordering of the DOM nodes in the top layer, not the renderers. That makes it hard to implement on the rendering side only.

Thanks, that's a better rationale.

>> Source/WebCore/dom/Document.cpp:5553
>> +    size_t position = m_topLayerElements.find(element);
> 
> You could replace that with:
> 
> size_t position = m_topLayerElements.find(element);
> ASSERT(position != notFound);
> 
> which would remove one iteration over the Vector in Debug.

Done.

>> Source/WebCore/dom/Document.h:1165
>> +    Vector<RefPtr<Element> >& topLayerElements() { return m_topLayerElements; }
> 
> This should be a |const| function, probably returning a const Vector as we don't expect callers to modify it.

Done.

>> Source/WebCore/dom/Node.cpp:1328
>> +            ASSERT(!renderer || renderer->inRenderFlowThread() || (renderer->enclosingLayer()->isInTopLayerSubtree()));
> 
> Note that this will involve walking up the render tree in Debug for every detach.

Hm, I'm not sure there's an easy way to prevent this. One idea could be to maintain a isInTopLayerSubtree bit on RenderObject or RenderLayer... but it could be hard to update it when an ancestor renderer changes. Also, there are no bits left in RenderObjectBitfields. Maybe this can be tackled in a subsequent patch.

>> Source/WebCore/dom/NodeRenderingContext.cpp:230
>> +    ASSERT(topLayerPosition != notFound);
> 
> Couldn't we just find the next renderer from the topLayerElements instead of iterating on all the RenderView's children?
> 
> Note that because you compute the positions in the Vector in the loop, the code is O(n^2).

I've revised this to take an O(n) approach using topLayerElements.

>> Source/WebCore/rendering/RenderLayer.cpp:4669
>> +            if (childElement && childElement->isInTopLayer()) {
> 
> Same comment here: couldn't we use the Document's list?

I originally did it that way, but changed it after Simon's review in comment #12 and #14, namely his comment that we shouldn't go directly from elements to layers. So the new approach is to order the top layer renderers as siblings under RenderView. I think this will also help for :backdrop-- a backdrop renderer would be after each top layer renderer, though I haven't attempted it yet.

It may be possible to use the Document's list here as well. I'm not sure if the only concern with going from element to layer is the case where the renderer is NULL, or that it's an abstraction violation. In any case, at least this code is still O(n).

>> LayoutTests/ChangeLog:16
>> +
> 
> Per discussion, there should be a test for percent length resolution. This would prove that we correctly layout top-layer elements with their containing block as the initial containing block.

I've added a percent length value to top-layer-stacking.

>> LayoutTests/fast/dom/HTMLDialogElement/top-layer-stacking.html:21
>> +    -webkit-transform: rotateY(45deg);
> 
> As discussed I don't know if that covers the hardware accelerated case but it needs to be covered by one test as we disable AC on named flow-thread.

It turns out this uses accelerated composition (actually it requires AC), so it does at least work for this case.

I found that if dialog itself has translateZ(0), its fixed child becomes positioned relative to the dialog instead of the browser window. But it turns out this also happens for a non-dialog, normal <div> as well. I think it's an AC bug... I'll file a bug for it.

>> LayoutTests/fast/dom/HTMLDialogElement/top-layer-stacking.html:27
>> +<dialog id="z" style="display: none">This should not be displayed.</dialog>
> 
> You should add some red for good measure.

Done.

>> LayoutTests/fast/dom/HTMLDialogElement/top-layer-stacking.html:49
>> +document.getElementById('z').showModal();
> 
> x, y, z don't really speak to me. Better names: topHiddenDialog, middleFixedDialog, bottomDialog.

Revised the test cases with better ids, except for top-layer-stacking-dynamic.html. I don't think changing those ids would improve readability.

[Matt Falkenhagen]

Created attachment 172521 [details]
more review comments

[Simon Fraser (smfr)]

(In reply to comment #36)
> I found that if dialog itself has translateZ(0), its fixed child becomes positioned relative to the dialog instead of the browser window. But it turns out this also happens for a non-dialog, normal <div> as well. I think it's an AC bug... I'll file a bug for it.

This is how CSS transforms are specified; a transformed element is a positioning container.

[Julien Chaffraix]

Comment on attachment 172521 [details]
more review comments

View in context: https://bugs.webkit.org/attachment.cgi?id=172521&action=review

> Source/WebCore/dom/Element.cpp:2124
> +    if (inTopLayer)
> +        document()->addToTopLayer(this);
> +    else
> +        document()->removeFromTopLayer(this);

As you never check that inTopLayer != wasInTopLayer, shouldn't calling showModalDialog() on an element several times trigger the ASSERT?

> Source/WebCore/dom/NodeRenderingContext.cpp:234
> +        if ((nextRenderer = topLayerElements[i]->renderer())) {

I would split this in 2 as it's difficult to see the side effect:
nextRenderer = topLayerElements[i]->renderer();
if (nextRenderer) {

> Source/WebCore/rendering/RenderLayer.cpp:4673
> +                RenderLayer* layer = toRenderBoxModelObject(child)->layer();

toRenderLayerModelObject(child)->layer()

> LayoutTests/fast/dom/HTMLDialogElement/top-layer-containing-block.html:19
> +        This modal dialog should be unaffected by its ancestor with overflow. It should not be clipped.

A global explanation on showModal vs show would help.

> LayoutTests/fast/dom/HTMLDialogElement/top-layer-containing-block.html:25
> +<div style="position: absolute; top: 1000px; left: 1000px; width: 20px;">

This is not tested by your test: the viewport is 800 x 600 so anything outside is not rendered nor checked by the ref-test.

> LayoutTests/fast/dom/HTMLDialogElement/top-layer-stacking-dynamic-expected.html:1
> +<html>

Tests that don't check for quirksmode behavior should include a doctype declaration: <!DOCTYPE html>

> LayoutTests/fast/dom/HTMLDialogElement/top-layer-stacking-dynamic-expected.html:22
> +The stacking order should be: yellow, magenta, green, pink, cyan (closest to user)

The passing condition should be readable by people non-familiar with the rendering code: "This test passes if you see 5 rectangles stacked in the following order (from bottom to top): yellow, magenta, green, pink, cyan".

This really is a nasty condition for passing btw as it's very broad. Better would be more focused tests that use red / green for passing / failing.

[Matt Falkenhagen]

Comment on attachment 172521 [details]
more review comments

View in context: https://bugs.webkit.org/attachment.cgi?id=172521&action=review

Julien, thanks for the review. Uploading a revised patch...

>> Source/WebCore/dom/Element.cpp:2124
>> +        document()->removeFromTopLayer(this);
> 
> As you never check that inTopLayer != wasInTopLayer, shouldn't calling showModalDialog() on an element several times trigger the ASSERT?

It's checked in the early return. I've rearranged the code to hopefully be more readable.

Also, an existing test, dialog-show-modal.html, covers that multiple showModalDialog() calls result in the correct DOM exception error.

>> Source/WebCore/dom/NodeRenderingContext.cpp:234
>> +        if ((nextRenderer = topLayerElements[i]->renderer())) {
> 
> I would split this in 2 as it's difficult to see the side effect:
> nextRenderer = topLayerElements[i]->renderer();
> if (nextRenderer) {

Done.

>> Source/WebCore/rendering/RenderLayer.cpp:4673
>> +                RenderLayer* layer = toRenderBoxModelObject(child)->layer();
> 
> toRenderLayerModelObject(child)->layer()

Done.

>> LayoutTests/fast/dom/HTMLDialogElement/top-layer-containing-block.html:19
>> +        This modal dialog should be unaffected by its ancestor with overflow. It should not be clipped.
> 
> A global explanation on showModal vs show would help.

Added.

>> LayoutTests/fast/dom/HTMLDialogElement/top-layer-containing-block.html:25
>> +<div style="position: absolute; top: 1000px; left: 1000px; width: 20px;">
> 
> This is not tested by your test: the viewport is 800 x 600 so anything outside is not rendered nor checked by the ref-test.

Actually this div isn't supposed to be visible. The point is that its child dialog should be positioned relative to the initial containing block, and therefore is in the viewport.

However some parts of this test were beyond the 800x600 viewport. I've compressed things so they are visible now.

>> LayoutTests/fast/dom/HTMLDialogElement/top-layer-stacking-dynamic-expected.html:1
>> +<html>
> 
> Tests that don't check for quirksmode behavior should include a doctype declaration: <!DOCTYPE html>

Done.

>> LayoutTests/fast/dom/HTMLDialogElement/top-layer-stacking-dynamic-expected.html:22
>> +The stacking order should be: yellow, magenta, green, pink, cyan (closest to user)
> 
> The passing condition should be readable by people non-familiar with the rendering code: "This test passes if you see 5 rectangles stacked in the following order (from bottom to top): yellow, magenta, green, pink, cyan".
> 
> This really is a nasty condition for passing btw as it's very broad. Better would be more focused tests that use red / green for passing / failing.

I've broken this test into 3 more focused tests: top-layer-nesting.html, top-layer-display-none.html, and top-layer-stacking-dynamic.html.

[Matt Falkenhagen]

Created attachment 172999 [details]
more review comments

[Matt Falkenhagen]

Created attachment 174612 [details]
sync

[Julien Chaffraix]

Comment on attachment 174612 [details]
sync

View in context: https://bugs.webkit.org/attachment.cgi?id=174612&action=review

As mentioned on IRC, there is no logic for detaching / reattaching the top layer elements' when we insert or remove them. When the spec says we should change the top layer bit of an element, it already has to be detached (e.g. showModalDialog) so it should be fine but better to keep that in mind.

> LayoutTests/fast/dom/HTMLDialogElement/top-layer-display-none.html:49
> +document.getElementById('container').style.display = 'none';
> +document.getElementById('displayNoneChild2').showModal();

Not sure this line helps as 'displayNoneChild2' already matches .hidden.

It is probably better to not have 'displayNoneChild2' with .hidden but then add hidden to container's classList.

[Matt Falkenhagen]

Comment on attachment 174612 [details]
sync

View in context: https://bugs.webkit.org/attachment.cgi?id=174612&action=review

Thanks Julien.

>> LayoutTests/fast/dom/HTMLDialogElement/top-layer-display-none.html:49
>> +document.getElementById('displayNoneChild2').showModal();
> 
> Not sure this line helps as 'displayNoneChild2' already matches .hidden.
> 
> It is probably better to not have 'displayNoneChild2' with .hidden but then add hidden to container's classList.

I'm not sure I understand. The .hidden just colors the dialog red to indicate failure. I'll change "hidden" to "red for clarity.

This tests that the dialog is not rendered whether it's added to the top layer before or after its ancestor is set to display: none.

[Matt Falkenhagen]

Created attachment 175122 [details]
patch for landing

[WebKit Review Bot]

Comment on attachment 175122 [details]
patch for landing

Clearing flags on attachment: 175122

Committed r135242: <http://trac.webkit.org/changeset/135242>

[Simon Fraser (smfr)]

This was all rolled out in bug 120467.

[Antoine Quint]

*** Bug 221471 has been marked as a duplicate of this bug. ***

[Tim Nguyen (:ntim)]

Created attachment 435898 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 435920 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 435925 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 435929 [details]
Patch

[EWS]

Committed r281296 (240720@main): <https://commits.webkit.org/240720@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 435929 [details].