Bug 227801 - Implement ::backdrop pseudo element
Summary: Implement ::backdrop pseudo element
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Nguyen (:ntim)
URL:
Keywords: InRadar
Depends on: 229042
Blocks: dialog-element
  Show dependency treegraph
 
Reported: 2021-07-08 10:05 PDT by Tim Nguyen (:ntim)
Modified: 2021-08-19 15:15 PDT (History)
23 users (show)

See Also:


Attachments
Patch (17.45 KB, patch)
2021-08-11 11:43 PDT, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff
Patch (19.59 KB, patch)
2021-08-11 12:41 PDT, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff
Patch (21.34 KB, patch)
2021-08-12 03:33 PDT, Tim Nguyen (:ntim)
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (21.41 KB, patch)
2021-08-12 03:42 PDT, Tim Nguyen (:ntim)
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (21.61 KB, patch)
2021-08-12 04:02 PDT, Tim Nguyen (:ntim)
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (21.62 KB, patch)
2021-08-12 04:08 PDT, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff
Patch (22.61 KB, patch)
2021-08-12 06:38 PDT, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff
Patch (22.60 KB, patch)
2021-08-12 10:18 PDT, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff
Patch (21.79 KB, patch)
2021-08-13 07:19 PDT, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Radar WebKit Bug Importer 2021-07-08 10:06:43 PDT
<rdar://problem/80330009>
Comment 2 Tim Nguyen (:ntim) 2021-08-11 11:43:07 PDT
Created attachment 435364 [details]
Patch
Comment 3 EWS Watchlist 2021-08-11 11:44:30 PDT
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Comment 4 Tim Nguyen (:ntim) 2021-08-11 12:41:48 PDT
Created attachment 435369 [details]
Patch
Comment 5 Tim Nguyen (:ntim) 2021-08-12 03:33:16 PDT
Created attachment 435406 [details]
Patch
Comment 6 Tim Nguyen (:ntim) 2021-08-12 03:42:55 PDT
Created attachment 435408 [details]
Patch
Comment 7 Tim Nguyen (:ntim) 2021-08-12 04:02:05 PDT
Created attachment 435410 [details]
Patch
Comment 8 Tim Nguyen (:ntim) 2021-08-12 04:08:28 PDT
Created attachment 435411 [details]
Patch
Comment 9 Antti Koivisto 2021-08-12 04:44:01 PDT
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.
Comment 10 Antti Koivisto 2021-08-12 04:52:20 PDT
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.
Comment 11 Antti Koivisto 2021-08-12 04:53:02 PDT
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.
Comment 12 Tim Nguyen (:ntim) 2021-08-12 06:38:44 PDT
Created attachment 435420 [details]
Patch
Comment 13 Antti Koivisto 2021-08-12 09:09:08 PDT
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
Comment 14 Tim Nguyen (:ntim) 2021-08-12 10:18:24 PDT
Created attachment 435432 [details]
Patch
Comment 15 Tim Nguyen (:ntim) 2021-08-13 07:19:40 PDT
Created attachment 435481 [details]
Patch
Comment 16 EWS 2021-08-19 02:02:11 PDT
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].