Bug 95946

Summary: <dialog> centering breaks when there is a relatively positioned ancestor container
Product: WebKit Reporter: Matt Falkenhagen <falken>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: NEW ---    
Severity: Normal CC: cigitia, dglazkov, eoconnor, eric, esprehn, gyuyoung.kim, ian, jamesr, jchaffraix, kling, koivisto, ojan.autocc, rakuco, simon.fraser, tabatkins, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 84635    
Attachments:
Description Flags
testcase
none
Patch
none
Patch
none
review comments
none
WIP patch none

Description Matt Falkenhagen 2012-09-06 01:02:15 PDT
Created attachment 162443 [details]
testcase

Non-anchored dialogs are vertically centered by default (bug 90670), but this seems broken when the dialog is nested in positioned containers including a relatively positioned one.
Comment 1 Matt Falkenhagen 2012-11-13 02:08:47 PST
CC'ing some people from bug 90670.

It looks like in RenderDialog::layout, absoluteToLocal() returns incorrect results since the containing elements haven't yet completed layout.  In the attached test case of:

    <div style="position: absolute; top: 1000px">
        <div style="position: relative; top: 0px">
            <dialog>...

The absolutely positioned div still has a m_frameRect with a y() = 0 instead of 1000. It becomes 1000 only after layout is completed. Since absoluteToLocal depends on m_frameRect (via RenderBoxModelObject::mapAbsoluteToLocalPoint -> RenderBox::offsetFromContainer -> RenderBox::topLeftLocationOffset), the result is wrong.

I'll have to investigate this more.
Comment 2 Ojan Vafai 2012-11-13 09:42:38 PST
I'm less familiar with when we're allowed to call absoluteToLocal, but you definitely can't depend on ancestors having been layed out in the Dialog's layout method.

Simon or James might have good thoughts on the right way to go about this.
Comment 3 Simon Fraser (smfr) 2012-11-13 10:37:43 PST
Use CSS?
Comment 4 Matt Falkenhagen 2012-11-14 00:23:50 PST
(In reply to comment #3)
> Use CSS?

I don't see how to solve the problem with CSS. The spec demands the dialog has position:absolute and is initially centered in the viewport. For example, position:fixed can't be used since the dialog must scroll along with the document.

I could set 'top' manually but the problem is the same as before; we don't know the desired value in RenderDialog::layout since the ancestors are not layed out yet.

I wonder if it can be solved by adding a dialog repositioning step after the whole render tree has completed layout.
Comment 5 Matt Falkenhagen 2012-11-15 04:16:16 PST
I have a question. The spec demands that the special centering happens only on a dialog.show() or dialog.showModal() DOM call. Once the dialog is centered, it is never recentered until show/showModal() is called again. show/showModal triggers a layout because the dialog gets a renderer attached then. Does this mean during layout we know there is at most one dialog that must be centered?

If so, an extra positioning step at the end of layout may be feasible, by just keeping track of that one dialog (perhaps by keeping a pointer in RenderView or Document).

Otherwise, I think we basically need a second layout pass. Just keeping track of the list of dialogs to center doesn't work in case of nested dialogs.

Relatedly, if my reading of the spec is correct, there is another bug in the current implementation because the dialog is recentered if a relayout occurs (e.g., by zooming in/out). It should only be centered once.
Comment 6 Simon Fraser (smfr) 2012-11-15 08:11:06 PST
Having the dialog not be re-centered on relayout is crazy; that's a recipe for unstable layout. If the user resizes the window, the dialog needs to remain centered I would think.
Comment 7 Matt Falkenhagen 2012-11-15 08:31:18 PST
(In reply to comment #6)
> Having the dialog not be re-centered on relayout is crazy; that's a recipe for unstable layout. If the user resizes the window, the dialog needs to remain centered I would think.

I think it may be strange to scroll the dialog out of view and then have it center again on zoom in/out or window resize. I can try to ask the motivation for the rule in the spec (the wording is "This top static position must remain the element's top static position until it is next changed by the above algorithm or the next one.")

When else can a relayout occur? I'm trying to understand how this would be unstable.
Comment 8 Simon Fraser (smfr) 2012-11-15 11:18:19 PST
(In reply to comment #7)
> (In reply to comment #6)
> > Having the dialog not be re-centered on relayout is crazy; that's a recipe for unstable layout. If the user resizes the window, the dialog needs to remain centered I would think.
> 
> I think it may be strange to scroll the dialog out of view and then have it center again on zoom in/out or window resize. I can try to ask the motivation for the rule in the spec (the wording is "This top static position must remain the element's top static position until it is next changed by the above algorithm or the next one.")
> 
> When else can a relayout occur? I'm trying to understand how this would be unstable.

Any time any content on the page changes.
Comment 9 Matt Falkenhagen 2012-11-19 00:21:52 PST
(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > Having the dialog not be re-centered on relayout is crazy; that's a recipe for unstable layout. If the user resizes the window, the dialog needs to remain centered I would think.
> > 
> > I think it may be strange to scroll the dialog out of view and then have it center again on zoom in/out or window resize. I can try to ask the motivation for the rule in the spec (the wording is "This top static position must remain the element's top static position until it is next changed by the above algorithm or the next one.")
> > 
> > When else can a relayout occur? I'm trying to understand how this would be unstable.
> 
> Any time any content on the page changes.

I'm also not sure we want the dialog to re-center in this case. It may be surprising to scroll a dialog away and have it reappear at seemingly random times when unrelated content changes.

I think if we want to re-center, we should change the spec. I have a feeling for what you mean by crazy and unstable layout, but I don't think I understand enough to justify the position to WHATWG. Can you give concrete concerns we can bring up with WHATWG?
Comment 10 Ian 'Hixie' Hickson 2012-12-10 15:56:22 PST
Recentering vertically when you increase the width kinda makes sense, but recentering vertically when you change the height doesn't (consider a long page where the dialog came up on the screen, then the user scrolled down, and then the user increased the height of the window to see more — moving the dialog then is weird).

Should we also update the position of anchored dialogs if the thing they are anchored to moves?

I've updated the spec regarding width changes (which includes zoom changes) updating the vertical position. I haven't made anchoring update in realtime. If you think that's wrong, please do mail the list.
Comment 11 Ojan Vafai 2012-12-10 16:20:24 PST
I think the confusion is that we are currently centering during *layout* and only the first layout, which is a relatively random point. Instead Dialog.show should force a synchronous layout, and then do the centering. It's a bit gross to add a sync layout, but I don't see an alternative.

As far as recentering when the width changes, it's not clear to me what the best behavior is. I can see it argued either way.

(In reply to comment #10)
> Should we also update the position of anchored dialogs if the thing they are anchored to moves?
> 
> I've updated the spec regarding width changes (which includes zoom changes) updating the vertical position. I haven't made anchoring update in realtime. If you think that's wrong, please do mail the list.

If we're going to have anchoring at all, then the dialog should move if the anchor moves. I don't see any other sane behavior, although that does complicate implementation considerably. Forgetting complicated things like animations for the moment, if you have the dialog anchored to an element and the page is scrolled, the dialog should clearly move with the anchor, no?

Am I missing something? Matt, mind sending an email to whatwg to this effect (assuming you agree)?
Comment 12 Simon Fraser (smfr) 2012-12-10 17:31:34 PST
(In reply to comment #11)
> If we're going to have anchoring at all, then the dialog should move if the anchor moves. I don't see any other sane behavior, although that does complicate implementation considerably. Forgetting complicated things like animations for the moment, if you have the dialog anchored to an element and the page is scrolled, the dialog should clearly move with the anchor, no?
> 
> Am I missing something? Matt, mind sending an email to whatwg to this effect (assuming you agree)?

I agree! Layout has to be "stable". You can't have one behavior when the dialog is first displayed, and them some magic behavior on later layouts.
Comment 13 Matt Falkenhagen 2012-12-10 18:30:43 PST
(In reply to comment #12)
> I agree! Layout has to be "stable". You can't have one behavior when the dialog is first displayed, and them some magic behavior on later layouts.

I agree with Ojan that we should do the centering in dialog.show() (after forcing a sync layout), instead of during layout. This follows the spec, except for the new width changed rule Ian made. And I think it can be considered stable layout.

If we attempt to center during layout and follow the spec, we'd have to do magic behavior on later layouts to not recenter except for the width changed case.

I also agree with dialog following its anchor when it moves. Actually I had thought the current spec already implies this (with the language "*While* an element A is magically aligned to an element B..."). I'll write to whatwg.
Comment 14 Matt Falkenhagen 2012-12-18 13:58:18 PST
Created attachment 180017 [details]
Patch
Comment 15 WebKit Review Bot 2012-12-18 15:04:55 PST
Comment on attachment 180017 [details]
Patch

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

New failing tests:
fast/dom/HTMLDialogElement/top-layer-display-none.html
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html
fast/dom/HTMLDialogElement/top-layer-stacking-dynamic.html
Comment 16 Matt Falkenhagen 2012-12-18 17:04:07 PST
Comment on attachment 180017 [details]
Patch

It seems this unmasks or causes a bug with the reparenting of top layer elements to be children of RenderView. I'll clear r? for now.
Comment 17 Matt Falkenhagen 2012-12-20 18:16:40 PST
(In reply to comment #16)
> (From update of attachment 180017 [details])
> It seems this unmasks or causes a bug with the reparenting of top layer elements to be children of RenderView. I'll clear r? for now.

top-layer-stacking-dynamic.html is failing due to bug 105489. I'm not sure yet why top-layer-display-none.html is failing. It seems like an error with moving the renderers around.
Comment 18 Matt Falkenhagen 2013-01-17 00:38:39 PST
Created attachment 183135 [details]
Patch
Comment 19 Matt Falkenhagen 2013-01-17 00:49:30 PST
Comment on attachment 183135 [details]
Patch

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

Now that some bugs in top layer have been fixed (bug 103477, bug 105489), this patch is working now.

> Source/WebCore/html/HTMLDialogElement.cpp:122
> +    return new (arena) RenderBlock(this);

Is it okay to use RenderBlock although <dialog> is an inline, not block, element? I think for centering it needs to be a RenderBlock for access to RenderBlock::height(). Also, it still works as an inline element despite this, with a test like: <dialog style="position: static">Hi</dialog> <dialog style="position: static">there</dialog> (bug 106538 has such a test).
Comment 20 Ojan Vafai 2013-01-17 18:44:48 PST
Comment on attachment 183135 [details]
Patch

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

>> Source/WebCore/html/HTMLDialogElement.cpp:122
>> +    return new (arena) RenderBlock(this);
> 
> Is it okay to use RenderBlock although <dialog> is an inline, not block, element? I think for centering it needs to be a RenderBlock for access to RenderBlock::height(). Also, it still works as an inline element despite this, with a test like: <dialog style="position: static">Hi</dialog> <dialog style="position: static">there</dialog> (bug 106538 has such a test).

position:absolute should force the display to being block. See StyleResolver::adjustRenderStyle. So, I think you maybe want to look at the RenderStyle passed in to return either a RenderBlock or a RenderInline. Not 100% sure.

Also you might still want a RenderDialog. I think you might want the cssTopIsOverridden member variable there so you can clear it appropriately on styleDidChange. Specifically, I"m thinking of code like the following:
dialog.open();
dialog.style.top = "10px";

We'll want to clear the cssTopIsOverriden bit.

Not sure if there's a better way to do it. Antti or Kling might have good ideas.
Comment 21 Elliott Sprehn 2013-01-17 18:46:39 PST
(In reply to comment #20)
> ...
> 
> position:absolute should force the display to being block. See StyleResolver::adjustRenderStyle. So, I think you maybe want to look at the RenderStyle passed in to return either a RenderBlock or a RenderInline. Not 100% sure.
> 
> Also you might still want a RenderDialog. I think you might want the cssTopIsOverridden member variable there so you can clear it appropriately on styleDidChange. Specifically, I"m thinking of code like the following:
> dialog.open();
> dialog.style.top = "10px";
> 

The thing we have to watch out for is dialog { display: table; } which means we can't just return a RenderBlock or a RenderDialog, unless we plan to put the actual <dialog> renderer inside that thing.
Comment 22 Ojan Vafai 2013-01-17 18:56:12 PST
(In reply to comment #21)
> (In reply to comment #20)
> > ...
> > 
> > position:absolute should force the display to being block. See StyleResolver::adjustRenderStyle. So, I think you maybe want to look at the RenderStyle passed in to return either a RenderBlock or a RenderInline. Not 100% sure.
> > 
> > Also you might still want a RenderDialog. I think you might want the cssTopIsOverridden member variable there so you can clear it appropriately on styleDidChange. Specifically, I"m thinking of code like the following:
> > dialog.open();
> > dialog.style.top = "10px";
> > 
> 
> The thing we have to watch out for is dialog { display: table; } which means we can't just return a RenderBlock or a RenderDialog, unless we plan to put the actual <dialog> renderer inside that thing.

It's a good point. Are there any use cases for inline dialogs? Maybe we should force all dialogs to their equivalentBlockDisplay in StyleResolver::adjustRenderStyle. That will mean that we can't just return a RenderBlock here. Actually, we should just remove the createRenderer override and I think it'll all work fine.
Comment 23 Matt Falkenhagen 2013-01-17 23:59:23 PST
Thanks for all the comments! I have a new patch that doesn't override createRenderer. Also I changed the tests against <div> positioning to be against <span> since <dialog> is an inline element.

(In reply to comment #20)
> Also you might still want a RenderDialog. I think you might want the cssTopIsOverridden member variable there so you can clear it appropriately on styleDidChange. Specifically, I"m thinking of code like the following:
> dialog.open();
> dialog.style.top = "10px";
 
Oh, good point. I wanted cssTopIsOverridden on the DOM side because it has to survive the renderer being deleted (which I think would happen if display: none is set, or for some reason a detach/reattach occurs). But we must handle this case, too.

(In reply to comment #22)
> Actually, we should just remove the createRenderer override and I think it'll all work fine.

Yes, looks like I was mistaken. It works without the createRenderer override.
Comment 24 Matt Falkenhagen 2013-01-18 00:05:14 PST
Created attachment 183389 [details]
review comments
Comment 25 WebKit Review Bot 2013-01-18 01:54:49 PST
Comment on attachment 183389 [details]
review comments

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

New failing tests:
fast/dom/HTMLDialogElement/top-layer-stacking-dynamic.html
Comment 26 WebKit Review Bot 2013-01-18 02:58:25 PST
Comment on attachment 183389 [details]
review comments

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

New failing tests:
fast/dom/HTMLDialogElement/top-layer-stacking-dynamic.html
Comment 27 Matt Falkenhagen 2013-01-29 00:18:11 PST
(In reply to comment #20)
> Specifically, I"m thinking of code like the following:
> dialog.open();
> dialog.style.top = "10px";
> 
> We'll want to clear the cssTopIsOverriden bit.

What should happen with:

  dialog.show();
  dialog.style.top = dialog.style.top;

It's strange if it's not a no-op, but it's also strange if it doesn't clear the bit as dialog.style.top = "10px" would. But I don't think we can detect this change in styleDidChange.

Another problem is the method is not called for each individual change. Specifically, for:

  dialog.show();  // 'top' is set to something like 281px
  dialog.style.top = "10px";

styleDidChange is called only after the "10px". On the other hand, for:

  dialog.show();  // 'top' is set to something like 281px
  dialog.offsetHeight; // force layout
  dialog.style.top = "10px";

styleDidChange is called for "281px" and "10px". So it can't determine when top is set by the user vs. the show() call.

I wonder if we should rethink this to not be setting "top". Can we easily make a new property like -webkit-centered-dialog-top and use that during layout when it's set and top is auto?
Comment 28 Simon Fraser (smfr) 2013-01-29 10:16:58 PST
This is all so gross. Why can't the spec just describe the layout of <dialog> using existing CSS positioning techniques?
Comment 29 Matt Falkenhagen 2013-01-29 17:13:27 PST
(In reply to comment #28)
> This is all so gross. Why can't the spec just describe the layout of <dialog> using existing CSS positioning techniques?

I guess there is no reliable way to vertically center in the viewport using CSS. Or is there? If so it's looking like it would make sense to use that.

When talking to Ojan and Tab a while back, there was an idea to do something like:

     top: scrollTop px
     bottom: calc(scrollTop px + 100vh)

But it turned out we still require a sync layout in dialog.show()   (I don't quite remember the details...)
Comment 30 Matt Falkenhagen 2013-01-29 17:54:04 PST
For more background, the spec describes the layout of <dialog> in terms of setting up the default static position when dialog.show() is called. I think our pain in implementing the spec is when layout happens and when dialog.show() are called are different.

We can't compute the proper position in show() until layout has happened. Once we know the proper position, we can't just move the renderer because that will just get reset on subsequent relayouts. There is no way, I think, to keep the default static position on a DOM object (hmm... maybe add it to HTMLDialogElement, set needs layout, and have RenderBlock have a special case for dialog nodes?)

That's where we got the idea to set 'top' directly.

Do you have a suggestion for how we can change the spec? My impression is that it's really open for feedback.
Comment 31 Simon Fraser (smfr) 2013-01-29 18:18:26 PST
I guess what i'm saying is that dialog layout needs to be possible using existing (or possibly) new CSS concepts, and not some magic positioning. Ideally we should be able to implement <dialog> with just some style in the UA stylesheet. If not, something is wrong.
Comment 32 Tab Atkins 2013-01-29 19:19:45 PST
(In reply to comment #31)
> I guess what i'm saying is that dialog layout needs to be possible using existing (or possibly) new CSS concepts, and not some magic positioning. Ideally we should be able to implement <dialog> with just some style in the UA stylesheet. If not, something is wrong.

Agreed.  This is the purpose of my "top-layer positioning" thread on www-style - to get the ball rolling on speccing the necessary concepts to make this work.
Comment 33 Ojan Vafai 2013-01-30 19:05:16 PST
(In reply to comment #32)
> (In reply to comment #31)
> > I guess what i'm saying is that dialog layout needs to be possible using existing (or possibly) new CSS concepts, and not some magic positioning. Ideally we should be able to implement <dialog> with just some style in the UA stylesheet. If not, something is wrong.
> 
> Agreed.  This is the purpose of my "top-layer positioning" thread on www-style - to get the ball rolling on speccing the necessary concepts to make this work.

I don't see how this helps this case. The issue here is the vertical centering that only happens when you call show, but then allows you to scroll up and down (moving the dialog) so you can get to possibly overflowing content.

(In reply to comment #31)
> I guess what i'm saying is that dialog layout needs to be possible using existing (or possibly) new CSS concepts, and not some magic positioning. Ideally we should be able to implement <dialog> with just some style in the UA stylesheet. If not, something is wrong.

I see what you're saying here, but I don't see how CSS would define this sort of thing. As I say above, we want it to center initialy, but still scroll.

I guess another way we could define this, and maybe this is what Tab is getting at, is that there is a (overflow:auto?) top-layer in which the dialog is centered and if the dialog is larger than the viewport, you can scroll within the top-layer?
Comment 34 Simon Fraser (smfr) 2013-01-30 20:03:07 PST
(In reply to comment #33)

> I see what you're saying here, but I don't see how CSS would define this sort of thing. As I say above, we want it to center initialy, but still scroll.

Can you be more precise? Center vertically (in the page? viewport?) but scroll (scroll the page, or the <dialog> contents)?
Comment 35 Matt Falkenhagen 2013-01-30 20:17:46 PST
(In reply to comment #34)
> (In reply to comment #33)
> 
> > I see what you're saying here, but I don't see how CSS would define this sort of thing. As I say above, we want it to center initialy, but still scroll.
> 
> Can you be more precise? Center vertically (in the page? viewport?) but scroll (scroll the page, or the <dialog> contents)?

It's center vertically in the viewport when the dialog is first shown, but still be able to scroll the page so the dialog moves up and down. So, not position: fixed.
Comment 36 Simon Fraser (smfr) 2013-01-30 20:26:16 PST
(In reply to comment #35)
> (In reply to comment #34)
> > (In reply to comment #33)
> > 
> > > I see what you're saying here, but I don't see how CSS would define this sort of thing. As I say above, we want it to center initialy, but still scroll.
> > 
> > Can you be more precise? Center vertically (in the page? viewport?) but scroll (scroll the page, or the <dialog> contents)?
> 
> It's center vertically in the viewport when the dialog is first shown, but still be able to scroll the page so the dialog moves up and down. So, not position: fixed.

I don't think the spec should prescribe behavior like this that can't be described in CSS terms.
Comment 37 Tab Atkins 2013-01-31 11:08:30 PST
(In reply to comment #36)
> (In reply to comment #35)
> > (In reply to comment #34)
> > > (In reply to comment #33)
> > > 
> > > > I see what you're saying here, but I don't see how CSS would define this sort of thing. As I say above, we want it to center initialy, but still scroll.
> > > 
> > > Can you be more precise? Center vertically (in the page? viewport?) but scroll (scroll the page, or the <dialog> contents)?
> > 
> > It's center vertically in the viewport when the dialog is first shown, but still be able to scroll the page so the dialog moves up and down. So, not position: fixed.
> 
> I don't think the spec should prescribe behavior like this that can't be described in CSS terms.

That's why I'm trying to fix that on the CSS side.  The spec's behavior is the better behavior - using fixpos is *not* very good.  We should just expose this better variety of behavior more directly.
Comment 38 Matt Falkenhagen 2013-02-19 04:50:01 PST
Created attachment 189061 [details]
WIP patch
Comment 39 Matt Falkenhagen 2013-02-19 04:50:51 PST
I've uploaded a new patch. I know spec work is in progress on adding CSS support for this positioning, but I hope this patch can be accepted in the meantime. This patch should accurately implement the current HTML spec, whereas the current implementation fails to implement the spec and doesn't even correctly do what it's trying to do (hence this bug).

This patch stores the 'default static top' position in a new CSS property so it avoids the problem of detecting if top is overridden, discussed earlier.  I don't want to expose this CSS property to the web; it should be used purely for internal implementation. Is there a way to hide the property, or alternatively, hang something off RenderStyle that's not a CSS property?
Comment 40 Matt Falkenhagen 2013-04-08 21:33:08 PDT
Comment on attachment 189061 [details]
WIP patch

Clearing r? I don't have a plan to continue this for WebKit. Thank you for all the comments.