[webkit-reviews] review canceled: [Bug 105489] Elements must be reattached when inserted/removed from top layer : [Attachment 180422] Patch

[bugzilla-daemon at webkit.org]

Matt Falkenhagen <falken at chromium.org> has canceled Matt Falkenhagen
<falken at chromium.org>'s request for review:
Bug 105489: Elements must be reattached when inserted/removed from top layer
https://bugs.webkit.org/show_bug.cgi?id=105489

Attachment 180422: Patch
https://bugs.webkit.org/attachment.cgi?id=180422&action=review

------- Additional Comments from Matt Falkenhagen <falken at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=180422&action=review


Thanks for the comments!

Hm, this patch also breaks the display-none test, like the one on bug 95946
does. I'll have to look more into that... clearing r? now.

>> Source/WebCore/dom/Element.cpp:1128
>> +	    document()->removeFromTopLayer(this);
> 
> My removal of this didn't break any tests, will this new test break if you
change this back? Can you write a test for this?

I don't think a test can be written for this now, because of how showModal()
and close() work. The breakage is that Document's top layer array retains
elements that are no longer in the top layer, but since NodeRenderingContext
uses Element::isInTopLayer() directly, it won't treat those elements as the top
layer. And since the real top layer elements are still ordered correctly
relative to each other in the array, the stacking is still correct.

We could cause a failure if we add something back into the top layer that is
still in Document's array, but the only way to do that is showModal() which
must come after a close(), which would remove it from the array.

I could write a test that a top layer element removed from the Document and
then readded is no longer in the top layer, but it would pass with or without
this patch.

>> Source/WebCore/dom/Element.cpp:2346
>> +	// top layer information is not encoded in style.
> 
> Eventually we should see if we can fix this, both for our own sanity and
because top layer is generally useful on the web. The idea of being in the top
layer should be a CSS property like -webkit-display-stack: top-layer; and we
just fix the diff code so that any change in webkit-display-stack causes a
reattach.

Yes I think the long term plan is for top layer to be usable via CSS.