Bug 258907

Pull request: https://github.com/WebKit/WebKit/pull/15583

Pull request: https://github.com/WebKit/WebKit/pull/15689

Created attachment 467077 [details]
Patch

Created attachment 467078 [details]
Patch

Comment on attachment 467078 [details]
Patch

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

> Source/WebCore/dom/FullscreenManager.cpp:478
> +    RenderBlock::removePositionedObject(downcast<RenderBox>(*renderer));

Note that it's possible that the block is just static before the top layer change (if someone sets `position: static !important;` on a dialog/popover).

Not sure if this needs to be guarded conditionally or not, but just mentioning it in case it does.

> Source/WebCore/dom/FullscreenManager.cpp:541
>          ancestor->addToTopLayer();

We probably want to do this `markRendererDirtyAfterTopLayerChange` logic for all ancestor documents, since when going fullscreen, the ancestor iframe(s) are getting pushed to the top layer as well, so this set up could have the same bug:

<div style="width: 500px; height: 500px; will-change: transform">
  <iframe src="document-that-calls-request-fullscreen.html" style="container-type: size">
</div>

Comment on attachment 467078 [details]
Patch

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

>> Source/WebCore/dom/FullscreenManager.cpp:541
>>          ancestor->addToTopLayer();
> 
> We probably want to do this `markRendererDirtyAfterTopLayerChange` logic for all ancestor documents, since when going fullscreen, the ancestor iframe(s) are getting pushed to the top layer as well, so this set up could have the same bug:
> 
> <div style="width: 500px; height: 500px; will-change: transform">
>   <iframe src="document-that-calls-request-fullscreen.html" style="container-type: size">
> </div>

(So I think the `markRendererDirtyAfterTopLayerChange`  logic should either be in `addToTopLayer()` or the for loop)

(In reply to Tim Nguyen (:ntim) from comment #5)
> Comment on attachment 467078 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=467078&action=review
> 
> > Source/WebCore/dom/FullscreenManager.cpp:478
> > +    RenderBlock::removePositionedObject(downcast<RenderBox>(*renderer));
> 
> Note that it's possible that the block is just static before the top layer
> change (if someone sets `position: static !important;` on a dialog/popover).
At this point we are _after_ the style update and from layout perspective, whether or not the author says "static !important", it is still an out-of-flow box and is treated accordingly (see render tree dumps)

(In reply to zalan from comment #7)
> (In reply to Tim Nguyen (:ntim) from comment #5)
> > Comment on attachment 467078 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=467078&action=review
> > 
> > > Source/WebCore/dom/FullscreenManager.cpp:478
> > > +    RenderBlock::removePositionedObject(downcast<RenderBox>(*renderer));
> > 
> > Note that it's possible that the block is just static before the top layer
> > change (if someone sets `position: static !important;` on a dialog/popover).
> At this point we are _after_ the style update and from layout perspective,
> whether or not the author says "static !important", it is still an
> out-of-flow box and is treated accordingly (see render tree dumps)

In the fullscreen case yes, because of the UA stylesheet forces everything with the fullscreen flag to `position: fixed !important`, but not necessarily dialog/popover (but I guess you're not handling them here so it doesn't matter too much).

Comment on attachment 467078 [details]
Patch

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

> Source/WebCore/dom/FullscreenManager.cpp:476
> +    ASSERT(renderer->isFixedPositioned());

Can't a top layer element also be position:absolute?

(In reply to Simon Fraser (smfr) from comment #9)
> Comment on attachment 467078 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=467078&action=review
> 
> > Source/WebCore/dom/FullscreenManager.cpp:476
> > +    ASSERT(renderer->isFixedPositioned());
> 
> Can't a top layer element also be position:absolute?
No, not at this point.

Created attachment 467081 [details]
Patch

> We probably want to do this `markRendererDirtyAfterTopLayerChange` logic for
> all ancestor documents
That's a good point. I didn't know subframe could initiate fullscreen all the way to the top.

(In reply to zalan from comment #12)
> > We probably want to do this `markRendererDirtyAfterTopLayerChange` logic for
> > all ancestor documents
> That's a good point. I didn't know subframe could initiate fullscreen all
> the way to the top.
I haven't managed yet to put such test case together though. iframes work fine when testing manually but for some reason they don't return the correct size when running layout test.

Comment on attachment 467081 [details]
Patch

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

I can try to help with the fullscreen testcase

> Source/WebCore/dom/FullscreenManager.cpp:532
> +        auto containingBlockBeforeStyleUpdate = WeakPtr<RenderBlock> { };

setFullscreenFlag(true) will apply the :fullscreen pseudo  class styles, so "BeforeStyleUpdate" is vague here.

(In reply to Tim Nguyen (:ntim) from comment #14)
> Comment on attachment 467081 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=467081&action=review
> 
> I can try to help with the fullscreen testcase
> 
> > Source/WebCore/dom/FullscreenManager.cpp:532
> > +        auto containingBlockBeforeStyleUpdate = WeakPtr<RenderBlock> { };
> 
> setFullscreenFlag(true) will apply the :fullscreen pseudo  class styles, so
> "BeforeStyleUpdate" is vague here.
please suggest a better name!

(In reply to zalan from comment #15)
> (In reply to Tim Nguyen (:ntim) from comment #14)
> > Comment on attachment 467081 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=467081&action=review
> > 
> > I can try to help with the fullscreen testcase
> > 
> > > Source/WebCore/dom/FullscreenManager.cpp:532
> > > +        auto containingBlockBeforeStyleUpdate = WeakPtr<RenderBlock> { };
> > 
> > setFullscreenFlag(true) will apply the :fullscreen pseudo  class styles, so
> > "BeforeStyleUpdate" is vague here.
> please suggest a better name!

Accurate names could be:
- containingBlockBeforeStyleResolution
- containingBlockAfterFullscreenStyleChanges

oldContainingBlock would be fine too.

BeforeStyleUpdate is ambiguous.

Comment on attachment 467081 [details]
Patch

r=me with the var rename + the extra testcase

r=me with the var rename + the extra testcase

Comment on attachment 467081 [details]
Patch

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

> LayoutTests/fullscreen/fullscreen-containing-block-change.html:14
> +            <div style="position: absolute; left: 0; right: 0; top: 100px; bottom: 0; background-color: greenyellow;"></div>

optional, but we could assert the width of this descendant too.

I am going to discuss this with Simon before landing.

Comment on attachment 467081 [details]
Patch

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

> Source/WebCore/dom/FullscreenManager.cpp:475
> +    // Let's carry out the same set of tasks we would normally do when containg block changes for out-of-flow content.

"containg". Can this comment mention the other location this is called?

> Source/WebCore/dom/FullscreenManager.cpp:530
>          ancestor->setFullscreenFlag(true);

Other code uses a Style::PseudoClassChangeInvalidation. Should we do that here?

> Source/WebCore/dom/FullscreenManager.cpp:534
> +            containingBlockBeforeStyleUpdate = renderer->containingBlock();

Should grab this before setFullscreenFlag(true)

> Source/WebCore/dom/FullscreenManager.cpp:543
> +        markRendererDirtyAfterTopLayerChange(ancestor->renderer(), containingBlockBeforeStyleUpdate.get());

Do we need the same fix in HTMLElement::showPopover() and HTMLDialogElement::showModal()?

Created attachment 467119 [details]
Patch

Created attachment 467121 [details]
Patch

> r=me with the var rename + the extra testcase
Had to remove the iframe test case. It was flaky timeout on both of my machines (and that's inline with what I experienced while I was trying to put a test case together)

> Other code uses a Style::PseudoClassChangeInvalidation. Should we do that
> here?
Maybe? That's a question for Tim.

> Do we need the same fix in HTMLElement::showPopover() and
> HTMLDialogElement::showModal()?
Probably yes, but also we should figure out a better fix to replace this manual handling.

Committed 266309@main (f82dc74981d8): <https://commits.webkit.org/266309@main>

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

(In reply to zalan from comment #25)
> > Other code uses a Style::PseudoClassChangeInvalidation. Should we do that
> > here?
> Maybe? That's a question for Tim.

setFullscreenFlag() already uses `Style::PseudoClassChangeInvalidation`, but it's not sufficient, which is why we use `resolveStyle()` (though ideally, we should find out why exactly that is needed and replace it).

> > Do we need the same fix in HTMLElement::showPopover() and
> > HTMLDialogElement::showModal()?
> Probably yes, but also we should figure out a better fix to replace this
> manual handling.

In showPopover and showModal, we don't resolveStyle() in between adding the styles and adding to the top layer, so this fix isn't needed.

(In reply to Tim Nguyen (:ntim) from comment #27)
> (In reply to zalan from comment #25)
> > > Other code uses a Style::PseudoClassChangeInvalidation. Should we do that
> > > here?
> > Maybe? That's a question for Tim.
> 
> setFullscreenFlag() already uses `Style::PseudoClassChangeInvalidation`, but
> it's not sufficient, which is why we use `resolveStyle()` (though ideally,
> we should find out why exactly that is needed and replace it).
> 
> > > Do we need the same fix in HTMLElement::showPopover() and
> > > HTMLDialogElement::showModal()?
> > Probably yes, but also we should figure out a better fix to replace this
> > manual handling.
> 
> In showPopover and showModal, we don't resolveStyle() in between adding the
> styles and adding to the top layer, so this fix isn't needed.
Sure, on trunk, but this approach is not future proof (i.e. we should not need to do any kind of manual dirty bit setting at this level)

(In reply to zalan from comment #28)
> (In reply to Tim Nguyen (:ntim) from comment #27)
> > (In reply to zalan from comment #25)
> > > > Other code uses a Style::PseudoClassChangeInvalidation. Should we do that
> > > > here?
> > > Maybe? That's a question for Tim.
> > 
> > setFullscreenFlag() already uses `Style::PseudoClassChangeInvalidation`, but
> > it's not sufficient, which is why we use `resolveStyle()` (though ideally,
> > we should find out why exactly that is needed and replace it).
> > 
> > > > Do we need the same fix in HTMLElement::showPopover() and
> > > > HTMLDialogElement::showModal()?
> > > Probably yes, but also we should figure out a better fix to replace this
> > > manual handling.
> > 
> > In showPopover and showModal, we don't resolveStyle() in between adding the
> > styles and adding to the top layer, so this fix isn't needed.
> Sure, on trunk, but this approach is not future proof (i.e. we should not
> need to do any kind of manual dirty bit setting at this level)

At some point, we'll probably want to rework top layer rendering to be a CSS property, so we can support transitions and other things. I suspect that this will replace this fix.

(In reply to Tim Nguyen (:ntim) from comment #27)
> In showPopover and showModal, we don't resolveStyle() in between adding the
> styles and adding to the top layer, so this fix isn't needed.

Right, but why do we need to resolveStyle in the fullscreen case?

(In reply to Simon Fraser (smfr) from comment #30)
> (In reply to Tim Nguyen (:ntim) from comment #27)
> > In showPopover and showModal, we don't resolveStyle() in between adding the
> > styles and adding to the top layer, so this fix isn't needed.
> 
> Right, but why do we need to resolveStyle in the fullscreen case?

Needs more investigation, but this predates the new fullscreen API fwiw.

If I remove it, I recall a bunch of assertions on fullscreen tests.