New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 704415 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue 660265

Blocking:
issue 704421



Sign in to add a comment

canParticipateInFlatTree concept does not appear to make sense and LayoutTreeTraversal is confused about slots

Project Member Reported by esprehn@chromium.org, Mar 23 2017

Issue description

<slot> and active insertion points returns false from canParticipateInFlatTree(), but those elements can have a style, especially in this new display: contents world.

Usage of canParticipateInFlatTree() has created a bunch of bugs:

- Document::styleForElementIgnoringPendingStylesheets does:

  if (!element->canParticipateInFlatTree())
    return ensureStyleResolver().styleForElement(element, nullptr);

This means anyone doing styleForElementIgnoringPendingStylesheets on a <slot> is getting a totally wrong answer for the slot's style, while a div with display: contents does correctly get a style.

- Document::needsLayoutTreeUpdateForNode does:

  if (!node.canParticipateInFlatTree())
    return false;

This means anyone using Document::updateStyleAndLayoutTreeForNode gets the wrong answer for slots.

In fact the only reason some of the code appears to work today is that getComputedStyle() always unconditionally forces a layout when an element is in a shadow tree (which is wrong).

<slot> and all other display: contents things need to be treated the same by the style system and we need to remove all usage of canParticipateInFlatTree() inside it if we want these nodes to have styles.

ex. of a test that only passes because the system forces a recalc today:
fast/alignment/parse-alignment-of-root-elements.html

This styles a <slot> in a shadow tree and uses getComputedStyle() on it, which only works because of the bug where we force a layout.

This also seems wrong because I'm pretty sure <slot> with display: block should generate a box, and the only reason it doesn't is because of display: contents. 

Today the LayoutTreeTraversal skips all slots, but does return display: contents things for parent(), but not for layoutParent(). We inherit the style from parent, but not from layoutParent(), which we only use for checks in StyleAdjuster. This doesn't work though, because <slot>'s are just display: contents things in the UA sheet.

LayoutTreeTraversal::parent() must return insertion points to get the inheritance chain correct.
 
Blocking: 657748

Comment 2 by hayato@chromium.org, Mar 23 2017

Status: Available (was: Untriaged)
Yeah, once "display:contents" is supported, we should update FlatTreeTraversal so that it does include slots. As of now, FlatTreeTraversal is skipping slots intentionally because "display:contents" is not supported.

We should align both things so that it does not have inconsistency.


This is broken already for getComputedStyle() on any insertion point, it just works because of a bad hack. That needs fixing anyway, ignoring anything about display: contents.

Comment 4 by hayato@chromium.org, Mar 23 2017

Do you have a bug link for that?
getComputedStyle() should use FlatTreeTraversal::parent() or something, instead of Node#parent().
No, that's an unrelated bug, the bug I'm talking about is this one.

Document::needsLayoutTreeUpdateForNode returns the wrong value for <slot> because it checks canParticipateInFlatTree and uses FlatTreeTraversal::parent, and we use that inside getComputedStyle by way of updateStyleAndLayoutTreeForNode.

Comment 7 by hayato@chromium.org, Mar 23 2017

Thank you. It looks I misunderstood the point. Let me take a look at code around.

Comment 8 by hayato@chromium.org, Mar 23 2017

Owner: hayato@chromium.org
Status: Assigned (was: Available)
Blocking: 704421
Blockedon: 660265
Status: Available (was: Assigned)
It would be better to work on this after  bug 660265  is resolved.
Blocking: -657748
Cc: -r...@opera.com
Owner: futhark@chromium.org
Status: Fixed (was: Available)

Sign in to add a comment