Bug 234984 – REGRESSION(r287683): <dialog> elements inside clipped/overflowed elements are no longer shown

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Tim Nguyen (:ntim)]

We used to show them before r287683, just at the wrong place, and with the wrong size. Now, we no longer show them.

Example testcases are:

imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/top-layer-containing-block.html [ ImageOnlyFailure ]
imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/top-layer-parent-overflow-clip.html [ ImageOnlyFailure ]
imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/top-layer-parent-overflow-scroll.html [ ImageOnlyFailure ]

[Radar WebKit Bug Importer]

<rdar://problem/87269825>

[Tim Nguyen (:ntim)]

Created attachment 448626 [details]
Patch

[zalan]

Comment on attachment 448626 [details]
Patch

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

It'd be interesting to know if Simon agrees with this direction.

> Source/WebCore/rendering/RenderLayer.cpp:1780
> +static inline bool isContainerForPositioned(RenderLayer& layer, PositionType position, bool establishesTopLayer)

It's hard to tell what establishesTopLayer means in this context without looking at the callsites (like what establishes the top layer).

> LayoutTests/TestExpectations:2361
>  

I am curious about this change (going from Skip -> Crash -while I understand that it is a more precise definition of the expected outcome, I am wondering if there are other reasons)

[Tim Nguyen (:ntim)]

(In reply to zalan from comment #3)
> > LayoutTests/TestExpectations:2361
> >  
> 
> I am curious about this change (going from Skip -> Crash -while I understand
> that it is a more precise definition of the expected outcome, I am wondering
> if there are other reasons)

I changed it to Crash so if there is an unexpected PASS (either by this change, or by other changes I'm working on), it would be caught, while Skip would just leave it disabled, with less testing coverage.

[Simon Fraser (smfr)]

Comment on attachment 448626 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:4549
> +    auto parentLayer = establishesTopLayer() ? renderer().view().layer() : parent();

Calling this 'parentLayer' is confusing because it's not always the parent layer. Maybe containerLayer?

[Tim Nguyen (:ntim)]

(In reply to zalan from comment #3)
> Comment on attachment 448626 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=448626&action=review
> 
> It'd be interesting to know if Simon agrees with this direction.
> 
> > Source/WebCore/rendering/RenderLayer.cpp:1780
> > +static inline bool isContainerForPositioned(RenderLayer& layer, PositionType position, bool establishesTopLayer)
> 
> It's hard to tell what establishesTopLayer means in this context without
> looking at the callsites (like what establishes the top layer).

I followed the naming of the `position` argument, though maybe we should look into merging both argument (taking a renderer?), but let's not do this here.

[Tim Nguyen (:ntim)]

Created attachment 448773 [details]
[fast-cq] Patch

[EWS]

Committed r287845 (245894@main): <https://commits.webkit.org/245894@main>

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