Bug 105489 - Elements must be reattached when inserted/removed from top layer
Summary: Elements must be reattached when inserted/removed from top layer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Matt Falkenhagen
URL:
Keywords:
Depends on: 107212 107689
Blocks: 84796
  Show dependency treegraph
 
Reported: 2012-12-19 18:29 PST by Matt Falkenhagen
Modified: 2013-01-27 17:56 PST (History)
10 users (show)

See Also:


Attachments
Patch (5.86 KB, patch)
2012-12-20 15:12 PST, Matt Falkenhagen
no flags Details | Formatted Diff | Diff
Patch (9.16 KB, patch)
2013-01-08 23:39 PST, Matt Falkenhagen
no flags Details | Formatted Diff | Diff
patch for landing (9.20 KB, patch)
2013-01-10 18:44 PST, Matt Falkenhagen
no flags Details | Formatted Diff | Diff
add reviewer to ChangeLog (9.20 KB, patch)
2013-01-10 20:25 PST, Matt Falkenhagen
no flags Details | Formatted Diff | Diff
patch for relanding (9.38 KB, patch)
2013-01-20 20:18 PST, Matt Falkenhagen
no flags Details | Formatted Diff | Diff
patch for re-relanding (9.41 KB, patch)
2013-01-24 21:06 PST, Matt Falkenhagen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Falkenhagen 2012-12-19 18:29:25 PST
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.
Comment 1 Matt Falkenhagen 2012-12-20 15:12:25 PST
Created attachment 180422 [details]
Patch
Comment 2 Elliott Sprehn 2012-12-20 15:34:54 PST
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 3 WebKit Review Bot 2012-12-20 15:56:54 PST
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 4 Matt Falkenhagen 2012-12-20 18:14:38 PST
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.
Comment 5 Matt Falkenhagen 2013-01-08 23:39:15 PST
Created attachment 181858 [details]
Patch
Comment 6 Matt Falkenhagen 2013-01-09 00:30:14 PST
This patch works now that bug 103477 is fixed. Elliott, Julien: could you take a look?
Comment 7 Julien Chaffraix 2013-01-10 15:37:00 PST
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 8 Elliott Sprehn 2013-01-10 15:41:17 PST
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.
Comment 9 Matt Falkenhagen 2013-01-10 18:44:57 PST
Created attachment 182240 [details]
patch for landing
Comment 10 Matt Falkenhagen 2013-01-10 19:28:31 PST
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 11 WebKit Review Bot 2013-01-10 20:15:10 PST
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
Comment 12 Matt Falkenhagen 2013-01-10 20:25:05 PST
Created attachment 182249 [details]
add reviewer to ChangeLog
Comment 13 WebKit Review Bot 2013-01-10 20:47:45 PST
Comment on attachment 182249 [details]
add reviewer to ChangeLog

Clearing flags on attachment: 182249

Committed r139402: <http://trac.webkit.org/changeset/139402>
Comment 14 WebKit Review Bot 2013-01-17 18:02:14 PST
Re-opened since this is blocked by bug 107212
Comment 15 Alexey Proskuryakov 2013-01-18 10:43:45 PST
Bug 106726 comment 8 appears to say that this rollout was in error.
Comment 16 Matt Falkenhagen 2013-01-20 20:18:18 PST
Created attachment 183699 [details]
patch for relanding
Comment 17 WebKit Review Bot 2013-01-21 00:01:07 PST
Comment on attachment 183699 [details]
patch for relanding

Clearing flags on attachment: 183699

Committed r140307: <http://trac.webkit.org/changeset/140307>
Comment 18 WebKit Review Bot 2013-01-21 00:01:12 PST
All reviewed patches have been landed.  Closing bug.
Comment 19 WebKit Review Bot 2013-01-23 07:29:23 PST
Re-opened since this is blocked by bug 107689
Comment 20 Matt Falkenhagen 2013-01-24 21:06:14 PST
Created attachment 184652 [details]
patch for re-relanding
Comment 21 Matt Falkenhagen 2013-01-24 22:32:09 PST
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 22 WebKit Review Bot 2013-01-24 22:35:17 PST
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 23 Build Bot 2013-01-25 01:02:49 PST
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 24 WebKit Review Bot 2013-01-27 17:56:35 PST
Comment on attachment 184652 [details]
patch for re-relanding

Clearing flags on attachment: 184652

Committed r140931: <http://trac.webkit.org/changeset/140931>
Comment 25 WebKit Review Bot 2013-01-27 17:56:41 PST
All reviewed patches have been landed.  Closing bug.