Bug 105489 – Elements must be reattached when inserted/removed from top layer

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Matt Falkenhagen]

Julien mentioned this in bug 84796 comment 43.

We originally thought that since dialog.showModal() and dialog.close() toggle display: none, the renderer would be attached and detached automatically whenever inserted/removed from the top layer, so it would be inserted in the desired position in the render tree.
However, attach/reattach doesn't necessarily occur between showModal and close calls. For example, we get in trouble in the following case:

top.showModal();
bottom.showModal();
top.offsetTop;  // force a layout
top.close();
top.showModal();

Here, close() will set display: none, but the next showModal() removes that. When recalcStyle occurs, no style change is detected since the forced layout, so no reattach occurs. So the top layer renderers are not in the correct top layer order.

[Matt Falkenhagen]

Created attachment 180422 [details]
Patch

[Elliott Sprehn]

Comment on attachment 180422 [details]
Patch

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

> 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?

> 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.

> LayoutTests/fast/dom/HTMLDialogElement/top-layer-stacking-dynamic-2.html:36
> +document.getElementById('topDialog').close();

These tests would be easier to read if you put the things in variables.

var topDialog = document.getElementById(...);
var bottomDialog = ...;
topDialog.showModal();
...

[WebKit Review Bot]

Comment on attachment 180422 [details]
Patch

Attachment 180422 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15457183

New failing tests:
fast/dom/HTMLDialogElement/top-layer-display-none.html

[Matt Falkenhagen]

Comment on attachment 180422 [details]
Patch

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.

[Matt Falkenhagen]

Created attachment 181858 [details]
Patch

[Matt Falkenhagen]

This patch works now that bug 103477 is fixed. Elliott, Julien: could you take a look?

[Julien Chaffraix]

Comment on attachment 181858 [details]
Patch

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

> LayoutTests/ChangeLog:14
> +        * fast/dom/HTMLDialogElement/top-layer-stacking-dynamic-2-expected.html: Added.
> +        * fast/dom/HTMLDialogElement/top-layer-stacking-dynamic-2.html: Added.
> +        This tests that top layer ordering is correct after removing and readding an element to the top layer.

I prefer more explicit naming instead of "dynamic 2". Here: top-layer-stacking-correct-order-remove-readd.html

> LayoutTests/fast/dom/HTMLDialogElement/top-layer-stacking-dynamic-2-expected.html:13
> +    left: 0; right: 0;

For consistency, this should be split on 2 lines.

[Elliott Sprehn]

Comment on attachment 181858 [details]
Patch

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

> Source/WebCore/dom/Element.cpp:2380
> +    // top layer information is not encoded in style.

The last sentence seems to be missing a word, I'd just leave that sentence off though since you're not providing enough detail about what's missing from the style.

[Matt Falkenhagen]

Created attachment 182240 [details]
patch for landing

[Matt Falkenhagen]

Comment on attachment 181858 [details]
Patch

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

Thanks for the comments.

>> Source/WebCore/dom/Element.cpp:2380
>> +    // top layer information is not encoded in style.
> 
> The last sentence seems to be missing a word, I'd just leave that sentence off though since you're not providing enough detail about what's missing from the style.

Done.

>> LayoutTests/ChangeLog:14
>> +        This tests that top layer ordering is correct after removing and readding an element to the top layer.
> 
> I prefer more explicit naming instead of "dynamic 2". Here: top-layer-stacking-correct-order-remove-readd.html

Done.

>> LayoutTests/fast/dom/HTMLDialogElement/top-layer-stacking-dynamic-2-expected.html:13
>> +    left: 0; right: 0;
> 
> For consistency, this should be split on 2 lines.

This is copy/paste from html.css, which itself is copy/paste from the spec http://www.whatwg.org/specs/web-apps/current-work/multipage/rendering.html#rendering

I guess I'll leave it as is for now. Maybe I should change html.css and the other test files using it.

[WebKit Review Bot]

Comment on attachment 182240 [details]
patch for landing

Rejecting attachment 182240 [details] from commit-queue.

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', u'--status-host=queues.webkit.org', ..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue

/mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/15795470

[Matt Falkenhagen]

Created attachment 182249 [details]
add reviewer to ChangeLog

[WebKit Review Bot]

Comment on attachment 182249 [details]
add reviewer to ChangeLog

Clearing flags on attachment: 182249

Committed r139402: <http://trac.webkit.org/changeset/139402>

[WebKit Review Bot]

Re-opened since this is blocked by bug 107212

[Alexey Proskuryakov]

Bug 106726 comment 8 appears to say that this rollout was in error.

[Matt Falkenhagen]

Created attachment 183699 [details]
patch for relanding

[WebKit Review Bot]

Comment on attachment 183699 [details]
patch for relanding

Clearing flags on attachment: 183699

Committed r140307: <http://trac.webkit.org/changeset/140307>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 107689

[Matt Falkenhagen]

Created attachment 184652 [details]
patch for re-relanding

[Matt Falkenhagen]

Comment on attachment 184652 [details]
patch for re-relanding

Re-relanding. The suspected perf regression that motivated the previous rollouts is mostly likely not a real one. See the discussion in bug 106726.

[WebKit Review Bot]

Comment on attachment 184652 [details]
patch for re-relanding

Rejecting attachment 184652 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 184652, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
eate file /tmp/ppMGHHCP : No space left on device
fatal: pathspec 'LayoutTests/fast/dom/HTMLDialogElement/removed-element-is-removed-from-top-layer-expected.html' did not match any files
Failed to git add LayoutTests/fast/dom/HTMLDialogElement/removed-element-is-removed-from-top-layer-expected.html. at /mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply line 448.

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/16111367

[Build Bot]

Comment on attachment 184652 [details]
patch for re-relanding

Attachment 184652 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16097665

New failing tests:
http/tests/security/cross-origin-plugin-private-browsing-toggled.html

[WebKit Review Bot]

Comment on attachment 184652 [details]
patch for re-relanding

Clearing flags on attachment: 184652

Committed r140931: <http://trac.webkit.org/changeset/140931>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.