Bug 227801 – Implement ::backdrop pseudo element

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Tim Nguyen (:ntim)]

https://fullscreen.spec.whatwg.org/#new-stacking-layer

https://fullscreen.spec.whatwg.org/#css-pe-backdrop

[Radar WebKit Bug Importer]

<rdar://problem/80330009>

[Tim Nguyen (:ntim)]

Created attachment 435364 [details]
Patch

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Tim Nguyen (:ntim)]

Created attachment 435369 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 435406 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 435408 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 435410 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 435411 [details]
Patch

[Antti Koivisto]

Comment on attachment 435411 [details]
Patch

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

Seems pretty good but lets do another round.

> Source/WebCore/rendering/RenderElement.cpp:2378
> +RenderBlockFlow* RenderElement::backdropRenderer() const

This could return WeakPtr for safety.

> Source/WebCore/rendering/updating/RenderTreeBuilder.cpp:895
> +void RenderTreeBuilder::updateBackdropRenderer(RenderElement& renderer)

This could go to RenderTreeBuilder::GeneratedContent and maybe invoked from GeneratedContent::updatePseudoElement

> Source/WebCore/rendering/updating/RenderTreeBuilder.cpp:905
> +    auto style = renderer.getCachedPseudoStyle(PseudoId::Backdrop, renderer.document().renderStyle());

You can use renderer.view().style(), so you don't bounce into DOM side unnecessarily (it is the same thing).

> Source/WebCore/rendering/updating/RenderTreeBuilder.cpp:907
> +    if (!style || style->display() == DisplayType::None)
> +        return;

Shouldn't we destroy an existing renderer in this case? 

If so we probably should have a test.

> Source/WebCore/rendering/updating/RenderTreeBuilder.cpp:911
> +    auto* backdropRenderer = renderer.backdropRenderer();

This should be a WeakPtr. We have had lots of safety issues in render tree building code.

> Source/WebCore/rendering/updating/RenderTreeBuilder.cpp:923
> +    RenderElement* currentParent = backdropRenderer->parent();
> +    RenderElement* newParent = renderer.parent();

WeakPtrs

> Source/WebCore/style/StyleTreeResolver.cpp:287
> -    
> -    auto& parentStyle = *elementUpdate.style;
> +
> +    auto& parentStyle = pseudoId == PseudoId::Backdrop ? *element.document().renderStyle() : *elementUpdate.style;
>      auto* parentBoxStyle = parentBoxStyleForPseudo(elementUpdate);
> -    
> +

This is unnecessary as the code doesn't currently handle Backdrop. You could assert against it.

[Antti Koivisto]

Comment on attachment 435411 [details]
Patch

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

>> Source/WebCore/rendering/updating/RenderTreeBuilder.cpp:905
>> +    auto style = renderer.getCachedPseudoStyle(PseudoId::Backdrop, renderer.document().renderStyle());
> 
> You can use renderer.view().style(), so you don't bounce into DOM side unnecessarily (it is the same thing).

Also you could add a comment (like a spec reference) about the odd parent style.

[Antti Koivisto]

Comment on attachment 435411 [details]
Patch

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

> Source/WebCore/rendering/updating/RenderTreeBuilder.cpp:904
> +    // Update ::backdrop renderer style or create renderer

This comment is kinda pointless though.

[Tim Nguyen (:ntim)]

Created attachment 435420 [details]
Patch

[Antti Koivisto]

Comment on attachment 435420 [details]
Patch

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

> Source/WebCore/rendering/updating/RenderTreeUpdaterGeneratedContent.cpp:189
> +    WeakPtr backdropRenderer = renderer.backdropRenderer();

could be auto

> Source/WebCore/rendering/updating/RenderTreeUpdaterGeneratedContent.cpp:201
> +    WeakPtr currentParent = makeWeakPtr(backdropRenderer->parent());
> +    WeakPtr newParent = makeWeakPtr(renderer.parent());

these too

[Tim Nguyen (:ntim)]

Created attachment 435432 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 435481 [details]
Patch

[EWS]

Committed r281229 (240666@main): <https://commits.webkit.org/240666@main>

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