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.
Created attachment 180422 [details] Patch
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(); ...
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
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.
Created attachment 181858 [details] Patch
This patch works now that bug 103477 is fixed. Elliott, Julien: could you take a look?
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.
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.
Created attachment 182240 [details] patch for landing
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.
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
Created attachment 182249 [details] add reviewer to ChangeLog
Comment on attachment 182249 [details] add reviewer to ChangeLog Clearing flags on attachment: 182249 Committed r139402: <http://trac.webkit.org/changeset/139402>
Re-opened since this is blocked by bug 107212
Bug 106726 comment 8 appears to say that this rollout was in error.
Created attachment 183699 [details] patch for relanding
Comment on attachment 183699 [details] patch for relanding Clearing flags on attachment: 183699 Committed r140307: <http://trac.webkit.org/changeset/140307>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by bug 107689
Created attachment 184652 [details] patch for re-relanding
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.
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
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
Comment on attachment 184652 [details] patch for re-relanding Clearing flags on attachment: 184652 Committed r140931: <http://trac.webkit.org/changeset/140931>