New issue
Advanced search Search tips

Issue 734908 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Task

Blocking:
issue 728151
issue 733257



Sign in to add a comment

Investigate if shadow dom can avoid LazyReattachIfAttached and mark dirty

Project Member Reported by kojii@chromium.org, Jun 20 2017

Issue description

Shadow DOM uses LazyReattachIfAttached to invalidate layout objects.

Talked with Hayato@, it might be better to set flag instead, since LazyReattachIfAttached() attaches lazily but detaches immediately, and detach while shadow DOM is in the mid of mutating DOM tree may not be stable.

This is a meta bug to investigate if this is feasible.
 

Comment 1 by hayato@chromium.org, Jun 20 2017

Components: Blink>DOM>ShadowDOM
Yeah, it is worth investigating. I'll work on this.

Comment 2 by kojii@chromium.org, Jun 23 2017

Blocking: 728151
Cc: yosin@chromium.org
Quick experiment shows 3 tests to fail:
* external/wpt/shadow-dom/MouseEvent-prototype-offsetX-offsetY.html
* fast/alignment/parse-alignment-of-root-elements.html
* shadow-dom/layout-1.html
https://codereview.chromium.org/2945993002

Maybe these read style/layout after UpdateDistribution() without ensuring layout clean? Haven't really looked into them, just wild guess.

Comment 3 by kojii@chromium.org, Jun 23 2017

Blocking: 733257

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

I have not looked deeper, but ensuring layout clean is the engine's responsibility, instead of user's responsibility, when a user accesses some info which needs clean layout, isn't it?

Comment 5 by kojii@chromium.org, Jun 23 2017

Do you mean "engine" is lifecycle and "user" is our code using layout?

I'm not very familiar with lifecycles, so I might be wrong, but it looks to me that LazyReattachIfAttached() is not lifecycle friendly, since it destroys LayoutObject immediately.

Mark dirty and let the cycle to clean it up looks more lifecycle friendly, and hence this bug...did I misunderstand somewhere?

Comment 6 by hayato@chromium.org, Jun 23 2017

Ah, I meant *Blink* by engine, and JavaScript by users.
In other words, I thought that users don't have to call document.body.offsetLeft before calling some other APIs which need clean layout.

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

Basically, I strongly support to stop using LazyReattachIfAttached there.
My concern is a regression.

Comment 8 by kojii@chromium.org, Jun 23 2017

> Ah, I meant *Blink* by engine, and JavaScript by users.

Oh, I see now. In my previous comment:
> Maybe these read style/layout after UpdateDistribution() without ensuring layout clean?

By "these" I meant "our code in Blink used by these tests", not "these javascript". Sorry for to be too terse.

> Basically, I strongly support to stop using LazyReattachIfAttached there.
> My concern is a regression.

Agreed. And I guess you're the only person who can make it right.

Comment 9 by hayato@chromium.org, Jun 26 2017

Labels: -Pri-3 Pri-2
I have taken look at a failing test, shadow-dom/layout-1.html.
This looks a real regression; a shadow host's child would be rendered even if a shadow root is attached.

It looks we need further tweaks for layout tree construction itself, as well as marking flag dirty. Let me take a deeper look.
Blockedon: 776656
Blockedon: -776656
Let 776656 unblock this. This is a more general issue.

Sign in to add a comment