[Webkit-unassigned] [Bug 232185] ASSERT(parent->element()) triggered in Styleable::fromRenderer

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=232185

--- Comment #3 from Tim Nguyen (:ntim) <ntim at apple.com> ---
Comment on attachment 442214
  --> https://bugs.webkit.org/attachment.cgi?id=442214
Patch

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

Here are some suggestions on how to address my previous comments.

>> Source/WebCore/style/Styleable.cpp:57
>> +        return std::nullopt;
> 
> This looks wrong, ::backdrop and ::marker renderers do not have any associated element, so the code below for ::backdrop & ::marker will just stop working altogether.
> 
> I'm surprised no test has caught this so far.
> 
> From a quick look at the render tree dump, overflow-y: -webkit-paged-y on <li> seems to put the marker renderer as a child of RenderMultiColumnFlowThread instead of RenderListItem.
> 
> I'm glad the assert caught this, because it shows a real bug in the code. We should either:
> 
> * In the `case PseudoId::Marker:` branch of this function, loop through renderer ancestors until we find a `RenderListItem`, and return that.
> 
> * Simply disallow multi column flow for list items? See: https://webkit-search.igalia.com/webkit/rev/522cdac023da69b36fa895cbedea14e96f44d678/Source/WebCore/rendering/RenderBlockFlow.cpp#425-426
> 
> I wasn't able to make multicol work properly inside a list item, so maybe this is a good solution? Though not sure if there's any other feature which may wrap renderers and trigger this assert.

Let's loop through parent renderers for now, since column flow does work if you use `columns`.

```
case PseudoId::Marker:
  auto* ancestor = renderer.parent();
  while (ancestor && !ancestor->element()) {
    ancestor = ancestor.parent();
  }
  ASSERT(is<RenderListItem>(ancestor));
  ASSERT(downcast<RenderListItem>(ancestor)->markerRenderer() == &renderer);
  return Styleable(*ancestor->element(), PseudoId::Marker);
```

Please file a followup bug to add a test for the correctness of `Styleable::fromRenderer` for `::backdrop` and `::marker`.

>> LayoutTests/fast/animation/css-animation-marker-crash.html:4
>> +      -webkit-mask-image: url();
> 
> maybe use a more common property like `background: green;`

oh, only composited properties trigger this crash, maybe `opacity: 0`?

> LayoutTests/fast/animation/css-animation-marker-crash.html:12
> +    overflow-y: -webkit-paged-y;

Can you use `columns: 3;` instead? It also triggers this crash, as it triggers column flow. It's more standard/common than overflow: -webkit-paged-y.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20211023/29fa0fb4/attachment.htm>