Non-anchored <dialog> elements should be vertically centered in the viewport by default. It seems there are two general approaches to implement this: 1) Change layout to have special handling for positioning dialog elements. 2) Change the dialog's CSS directly to position it. E.g., set the 'position' property to 'fixed' and set 'top' accordingly. I'm not sure if doing so is OK according to the HTML or CSS spec, however; for example, perhaps elements are required to have a certain 'position' property by default. The recommended style sheet for dialog uses position 'absolute':<http://www.whatwg.org/specs/web-apps/current-work/multipage/rendering.html#flow-content-1>
(In reply to comment #0) > Non-anchored <dialog> elements should be vertically centered in the viewport by default. > > It seems there are two general approaches to implement this: > > 1) Change layout to have special handling for positioning dialog elements. > 2) Change the dialog's CSS directly to position it. E.g., set the 'position' property to 'fixed' and set 'top' accordingly. I'm not sure if doing so is OK according to the HTML or CSS spec, however; for example, perhaps elements are required to have a certain 'position' property by default. The recommended style sheet for dialog uses position 'absolute':<http://www.whatwg.org/specs/web-apps/current-work/multipage/rendering.html#flow-content-1> Ideally we'd use the CSS from the spec in html.css. Do you know why it uses position:absolute instead of fixed? It's probably worth tracking down the history of discussion there to see if maybe it's just an oversight.
(In reply to comment #1) > Ideally we'd use the CSS from the spec in html.css. Do you know why it uses position:absolute instead of fixed? Ian Hickson explains: "If it was fixed, and the dialog was taller than the window, there would be no way to view the whole dialog." <http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2012-July/036667.html> Implementation-wise, I guess it may still be possible to do option 2) but special case for dialogs taller than the window, and we'd have to handle size changing dynamically.
Why wouldn't you just do what the spec says? If there's something wrong with what the spec says, I'd like to change the spec.
(In reply to comment #3) > Why wouldn't you just do what the spec says? If there's something wrong with what the spec says, I'd like to change the spec. I think what's in the spec WRT CSS and positioning is fine. Your argument for why absolute is better convinces me. I'm having trouble digesting all the anchor-point stuff. At first glance, I'd prefer a general anchoring solution. In either case, we can get an initial implementation of this that doesn't support anchors to start with.
I'm trying now to implement what the spec says. This means adding special handling to layout for positioning dialog elements. My thinking is it should probably follow much the same code path as for fixed elements, since we want to position the dialog in terms of the viewport. So my approach is to try to get the fixed positioning code to also handle dialog. For example, change RenderLayer::updateLayerPositionsAfterScroll to cause repaint rects to be computed for dialog as well as fixed elements. This involves changing some conditionals to be like (position() == FixedPosition || isDialog()), which may be a little ugly. Now I've gotten RenderView::computeRectForRepaint and RenderView::mapLocalToContainer to be called for dialog as if it were a fixed element, but this is not sufficient for a working implementation. Any thoughts on this approach? Perhaps dialog should be laid out independently of fixed elements instead?
It's looking tricky to get a non-fixed position element to be positioned like a fixed element. Now I'm trying to alter code that gets called during layout like RenderBox::computePositionedLogicalHeight to use an effective top value for getting the dialog to be centered. This will mean triggering layout on a scroll, for dialog elements.
(In reply to comment #6) > It's looking tricky to get a non-fixed position element to be positioned like a fixed element. Why is it not a fixed position element? Don't we set position:fixed in html.css? > Now I'm trying to alter code that gets called during layout like RenderBox::computePositionedLogicalHeight to use an effective top value for getting the dialog to be centered. This will mean triggering layout on a scroll, for dialog elements. I think you probably need a RenderDialog class that subclasses RenderBlock and overrides layout to set the static inline/block positions (e.g. see adjustPositionedBlock) and the continue with RenderBlock's regular layout. You might need to do some more to get it to reposition on resizing the window. Actually, it is supposed to reposition when you resize the window?
(In reply to comment #7) > (In reply to comment #6) > > It's looking tricky to get a non-fixed position element to be positioned like a fixed element. > > Why is it not a fixed position element? Don't we set position:fixed in html.css? As mentioned in comments #2-4, it's position:absolute in the spec and html.css. > > Now I'm trying to alter code that gets called during layout like RenderBox::computePositionedLogicalHeight to use an effective top value for getting the dialog to be centered. This will mean triggering layout on a scroll, for dialog elements. > > I think you probably need a RenderDialog class that subclasses RenderBlock and overrides layout to set the static inline/block positions (e.g. see adjustPositionedBlock) and the continue with RenderBlock's regular layout. Thanks for the idea! I'll give this a try. > You might need to do some more to get it to reposition on resizing the window. Actually, it is supposed to reposition when you resize the window? It seems it should reposition on resize. The spec says the default static position should be computed such that the dialog is centered, and "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." http://www.whatwg.org/specs/web-apps/current-work/multipage/commands.html#the-dialog-element
> It seems it should reposition on resize. The spec says [...] How do you go from what the spec says (as you quoted), to the conclusion that resizing should reposition as well?
(That's not a rhetorical question, by the way; I'm just trying to understand your reading of the spec.)
(In reply to comment #9) > > It seems it should reposition on resize. The spec says [...] > > How do you go from what the spec says (as you quoted), to the conclusion that resizing should reposition as well? I interpreted "the above algorithm" as the calculation described in the previous paragraph ("set up the default static position") to center the dialog in the viewport. Since a resize or scroll would cause the dialog to no longer be centered in the viewport, it sounds like the above algorithm should run again to keep it centered, that is, the top static position would be "next changed by the above algorithm". I guess I'm reading "next changed by" incorrectly. Should the dialog not be repositioned on a scroll or resize?
I notice now the spec specifically says when to "set up the default static position" in the steps for show() and showModal(). So it seems the element should not be repositioned on scroll or resize, after all. The default static position can change only when show() or showModal() is invoked. As suggested in comment #7, I looked into setting the static position before proceeding with RenderBlock::layout, but we need to know the height to center the dialog, and that is computed during layout (in RenderBox::computeLogicalHeight). It looks possible to get it to center by hooking into the computeLogicalHeight code path. I should have a preliminary patch ready soon.
Created attachment 156649 [details] preliminary patch
Comment on attachment 156649 [details] preliminary patch View in context: https://bugs.webkit.org/attachment.cgi?id=156649&action=review I'm hoping to get feedback on this WIP patch. It seems to work in some cases, but also fails in some cases. In the layout test, the final check is failing (when the dialog is in a relative positioned div in an absolute positioned div). The positioning logic is in RenderDialog::adjustDialogStaticPosition, called after the normal RenderBlock::layout() call. The goal is to center the dialog vertically in the viewport when it fits, and to position it at the top of the viewport when it's taller than the viewport. > Source/WebCore/rendering/RenderDialog.cpp:76 > + if (!style()->hasStaticBlockPosition(isHorizontalWritingMode()) || style()->position() != AbsolutePosition) Not sure if we also want to setup the default static position for "position: relative". The spec says specifically we don't do it for "position: static". > Source/WebCore/rendering/RenderDialog.cpp:90 > + setLogicalTop(flooredLayoutSize(localPoint).height()); I'm not sure if this logic is correct, particularly how the absolute coordinate is computed and then translated to local coordinates of our container. This also all probably needs to be updated for correct handling in horizontal vs. vertical writing mode.
Comment on attachment 156649 [details] preliminary patch View in context: https://bugs.webkit.org/attachment.cgi?id=156649&action=review >> Source/WebCore/rendering/RenderDialog.cpp:76 >> + if (!style()->hasStaticBlockPosition(isHorizontalWritingMode()) || style()->position() != AbsolutePosition) > > Not sure if we also want to setup the default static position for "position: relative". The spec says specifically we don't do it for "position: static". Take a look at RenderBlock::adjustPositionedBlock. You want to use the setStatic* methods here. Then I don't think you need either of these checks. Not 100% sure though.
(In reply to comment #15) > Take a look at RenderBlock::adjustPositionedBlock. You want to use the setStatic* methods here. Then I don't think you need either of these checks. Not 100% sure though. Hm, I'm having trouble figuring out how to use the setStatic* methods. The static positions are used to calculate top during RenderBlock::layout, so it's too late to call them after that has run. If I call them before or during RenderBlock::layout, it becomes slightly harder to compute what the static position should be. The code that uses the static position to compute top, computeBlockStaticDistance in RenderBox.cpp, also adds the top value of the parent and its ancestor containers before assigning to top. So I have to offset static position by that amount, so top will ultimately be such that the dialog is centered. However, even if I do that, the final test case still fails. But, this may provide a clue why. The test has been failing by 16 pixels, which happens to be the offset mentioned above (for some reason, RenderBody has a top of 16). So perhaps somehow RenderBody's top comes into play somewhere else. I'm not sure though. In some manual testing, dialog is off by far more than 16 pixels.
Created attachment 157655 [details] WIP patch using setStaticBlockPosition
Comment on attachment 157655 [details] WIP patch using setStaticBlockPosition View in context: https://bugs.webkit.org/attachment.cgi?id=157655&action=review I think the patch is working better than I thought. It seems to be failing only when a horizontal scrollbar is present. I've updated the patch to use setStaticBlockPosition, though I'm not sure where is a good place to call it. > Source/WebCore/rendering/RenderBox.cpp:2903 > + // Set dialog's static position for top. There is likely a better place to do this, but I'm not sure where. It can't be done too early or logicalHeight() just returns 0. For example, before RenderBlock::layout() is called is too early. > Source/WebCore/rendering/RenderBox.cpp:2919 > + } This does the reverse of the computation in computeBlockStaticDistance (which is about to be called), so that the logical top will ultimately be what we want. > LayoutTests/fast/dom/HTMLDialogElement/non-anchored-dialog-positioning.html:81 > +container.style.width = '90%'; It's currently failing when the horizontal scrollbar is present.
Aha! The dialog is positioned correctly even when a horizontal scrollbar is present, but for some reason it's not being painted. It repaints after it is scrolled outside of the viewport and back inside again. It just felt like it was positioned incorrectly since I'd scroll up and down until I found it. It looks like this can be fixed by adding a call to repaint() after the actual top is set. I'm not sure if that's the best solution, though. The layout test's final test case still fails, but that is only by about 16 pixels. In manual testing, the dialog seems to be in the correct position. I'm still unsure what to do for "position: relative". I'm thinking the dialog shouldn't be centered, based on the position descriptions in the CSS spec[1] and the dialog spec's note that for "position: static" the special top value is not used. If someone could sanity check my interpretation, I'd appreciate it. [1] http://www.w3.org/TR/CSS21/visuren.html#propdef-position
Created attachment 158279 [details] Patch
Ojan, could you please review this patch? I returned to the original approach of adjusting position after normal RenderBlock::layout. This is because: - It eliminates the need for "if (isDialog)" cases in RenderBox.cpp - There doesn't seem to be a very good place to use the setStatic* functions. Before calling layout, the dialog height is unknown, so it can't be centered yet. It is still unknown in one place that already calls setStatic* (setStaticPositions in RenderBlockLineLayout.cpp). Once we get to RenderBox::computePositionedLogicalHeight, the static position is used just in the calculation to set top, so it seems cleaner to set top directly. What do you think?
Comment on attachment 158279 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158279&action=review Julien, Tony, do you mind taking a look at the LayoutStateDisabler/repaint stuff? I'm not too familiar with LayoutState. On second thought, I think the setLogicalTop approach is fine. Sorry for leading you astray. Unfortunately, WebKit has a ton of build systems, so you'll need to modify a ton more files (gtk, qt, efl, etc). I recommend looking at other recent patches that have added/removed new files as a reference. > Source/WebCore/rendering/RenderDialog.cpp:36 > +using namespace HTMLNames; What is this for? > Source/WebCore/rendering/RenderDialog.cpp:45 > +RenderDialog::RenderDialog(Node* node) > + : RenderBlock(node) > +{ > +} > + > +RenderDialog::~RenderDialog() > +{ > +} You can just put these in the header since they're empty. > Source/WebCore/rendering/RenderDialog.cpp:64 > + if (isFloating()) > + return "RenderDialog (floating)"; > + if (isOutOfFlowPositioned()) > + return "RenderDialog (positioned)"; > + if (isAnonymousColumnsBlock()) > + return "RenderDialog (anonymous multi-column)"; > + if (isAnonymousColumnSpanBlock()) > + return "RenderDialog (anonymous multi-column span)"; > + if (isAnonymousBlock()) > + return "RenderDialog (anonymous)"; > + if (isAnonymous()) > + return "RenderDialog (generated)"; > + if (isRelPositioned()) > + return "RenderDialog (relative positioned)"; > + if (isRunIn()) > + return "RenderDialog (run-in)"; I don't think you need all this. It's sufficient for now to just return RenderDialog. IIRC, this is only used for dumping the render tree for testing. In fact, it'd be fine for you to leave out this method altogether since we don't even have tests that dump a render tree with dialogs in them. > Source/WebCore/rendering/RenderDialog.cpp:76 > + LayoutStateDisabler layoutStateDisabler(view()); I think this might need a LayoutRepainter and a LayoutStateMaintainer instead (which would also wrap the RenderBlock::layout() method (see, e.g. RenderFlexibleBox::layoutBlock for an example). Alternately, maybe you just need to call repaintDuringLayoutIfMoved at the end instead of repaint. > Source/WebCore/rendering/RenderDialog.cpp:82 > + if (logicalHeight() < visibleHeight) > + absolutePoint.move(0, (visibleHeight - logicalHeight()) / 2); You're mixing logical and physical heights here. The question is what should happen with a dialog that has -webkit-writing-mode:vertical-rl. It should probably center horizontally, right? In that case, you should just use logicalHeights. Would be nice to add a testcase for this as well. > Source/WebCore/rendering/RenderDialog.cpp:84 > + LayoutUnit localTop = LayoutSize(localPoint.x(), localPoint.y()).height(); I'm not sure, but I think you're dealing in physical heights here as well. A test with vertical writing mode should make it clear whether this is right. > Source/WebCore/rendering/RenderDialog.h:55 > +inline RenderDialog* toRenderDialog(RenderObject* object) > +{ > + ASSERT(!object || object->isDialog()); > + return static_cast<RenderDialog*>(object); > +} > + > +// This will catch anyone doing an unnecessary cast. > +void toRenderDialog(const RenderDialog*); I don't think we need these. If these become common enough usage, that we need helper functions,we can add them then. > LayoutTests/fast/dom/HTMLDialogElement/non-anchored-dialog-positioning.html:22 > + shouldBeCloseTo('realTop', expectedTop, 1, true); We should be able to get exact numbers here. shouldBeCloseTo is just for things like transitions where the actual number is a bit racy. Might be simpler if you don't use a font-size based sizes below. Just use pixels.
Comment on attachment 158279 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158279&action=review >> Source/WebCore/rendering/RenderDialog.cpp:76 >> + LayoutStateDisabler layoutStateDisabler(view()); > > I think this might need a LayoutRepainter and a LayoutStateMaintainer instead (which would also wrap the RenderBlock::layout() method (see, e.g. RenderFlexibleBox::layoutBlock for an example). > > Alternately, maybe you just need to call repaintDuringLayoutIfMoved at the end instead of repaint. Yes, I think you want to use LayoutRepainter and LayoutStateMaintainer here. > Source/WebCore/rendering/RenderDialog.cpp:89 > + // FIXME: This is needed because otherwise the dialog is not painted if it causes a horizontal scrollbar to appear. > + // There is probably a better solution. > + repaint(); This shouldn't be necessary. > Source/WebCore/rendering/RenderDialog.h:39 > + RenderDialog(Node*); Nit: explicit > Source/WebCore/rendering/RenderDialog.h:45 > + virtual const char* renderName() const; > + virtual bool isDialog() const { return true; } OVERRIDE
Comment on attachment 158279 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158279&action=review >> Source/WebCore/rendering/RenderDialog.cpp:64 >> + return "RenderDialog (run-in)"; > > I don't think you need all this. It's sufficient for now to just return RenderDialog. IIRC, this is only used for dumping the render tree for testing. > > In fact, it'd be fine for you to leave out this method altogether since we don't even have tests that dump a render tree with dialogs in them. Let's keep the "RenderDialog" case for now. I still see some value in having it instead of postponing until people have submitted tests involving a tree dump and it's more painful to change. > Source/WebCore/rendering/RenderDialog.cpp:68 > +void RenderDialog::layout() Don't you need to update your position if the user scrolls? If that's the case, layout will not handle this case properly. >>> Source/WebCore/rendering/RenderDialog.cpp:76 >>> + LayoutStateDisabler layoutStateDisabler(view()); >> >> I think this might need a LayoutRepainter and a LayoutStateMaintainer instead (which would also wrap the RenderBlock::layout() method (see, e.g. RenderFlexibleBox::layoutBlock for an example). >> >> Alternately, maybe you just need to call repaintDuringLayoutIfMoved at the end instead of repaint. > > Yes, I think you want to use LayoutRepainter and LayoutStateMaintainer here. Mhh, there is an issue here. RenderBlock already handle part of this book keeping. The issue arises from your forcibly moving the renderer *after* RenderBlock triggered the invalidation. This path will cause WebKit to over-paint as you will dirty the original position, the intermediate one (after RenderBlock layout) and the final position. If you had a RenderLayer, it would handle the invalidation properly and *not* cause an over-invalidation. I haven't followed closely your attempt at that so I don't know how much of an option it is.
Comment on attachment 158279 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158279&action=review >>> Source/WebCore/rendering/RenderDialog.cpp:64 >>> + return "RenderDialog (run-in)"; >> >> I don't think you need all this. It's sufficient for now to just return RenderDialog. IIRC, this is only used for dumping the render tree for testing. >> >> In fact, it'd be fine for you to leave out this method altogether since we don't even have tests that dump a render tree with dialogs in them. > > Let's keep the "RenderDialog" case for now. I still see some value in having it instead of postponing until people have submitted tests involving a tree dump and it's more painful to change. Meh. In pratice, we're not really going to have many tests with dialogs in them. But sure, there's no harm, aside from a bit of code bloat. >> Source/WebCore/rendering/RenderDialog.cpp:68 >> +void RenderDialog::layout() > > Don't you need to update your position if the user scrolls? If that's the case, layout will not handle this case properly. No. It's explicitly position:absolute so that scrolling will scroll it. This works around the case where the dialog is larger than the window. >>>> Source/WebCore/rendering/RenderDialog.cpp:76 >>>> + LayoutStateDisabler layoutStateDisabler(view()); >>> >>> I think this might need a LayoutRepainter and a LayoutStateMaintainer instead (which would also wrap the RenderBlock::layout() method (see, e.g. RenderFlexibleBox::layoutBlock for an example). >>> >>> Alternately, maybe you just need to call repaintDuringLayoutIfMoved at the end instead of repaint. >> >> Yes, I think you want to use LayoutRepainter and LayoutStateMaintainer here. > > Mhh, there is an issue here. RenderBlock already handle part of this book keeping. The issue arises from your forcibly moving the renderer *after* RenderBlock triggered the invalidation. This path will cause WebKit to over-paint as you will dirty the original position, the intermediate one (after RenderBlock layout) and the final position. > > If you had a RenderLayer, it would handle the invalidation properly and *not* cause an over-invalidation. I haven't followed closely your attempt at that so I don't know how much of an option it is. Not sure I understand what you're saying here. You're saying that it should require a layer? If so, it already does because we only hit this code path if it's absolutely positioned (see line 73 above).
Comment on attachment 158279 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158279&action=review >>> Source/WebCore/rendering/RenderDialog.cpp:68 >>> +void RenderDialog::layout() >> >> Don't you need to update your position if the user scrolls? If that's the case, layout will not handle this case properly. > > No. It's explicitly position:absolute so that scrolling will scroll it. This works around the case where the dialog is larger than the window. If I am reading the comments correctly, we want the <dialog> to follow the viewport when scrolling but again, I didn't really follow the bug nor the spec. >>>>> Source/WebCore/rendering/RenderDialog.cpp:76 >>>>> + LayoutStateDisabler layoutStateDisabler(view()); >>>> >>>> I think this might need a LayoutRepainter and a LayoutStateMaintainer instead (which would also wrap the RenderBlock::layout() method (see, e.g. RenderFlexibleBox::layoutBlock for an example). >>>> >>>> Alternately, maybe you just need to call repaintDuringLayoutIfMoved at the end instead of repaint. >>> >>> Yes, I think you want to use LayoutRepainter and LayoutStateMaintainer here. >> >> Mhh, there is an issue here. RenderBlock already handle part of this book keeping. The issue arises from your forcibly moving the renderer *after* RenderBlock triggered the invalidation. This path will cause WebKit to over-paint as you will dirty the original position, the intermediate one (after RenderBlock layout) and the final position. >> >> If you had a RenderLayer, it would handle the invalidation properly and *not* cause an over-invalidation. I haven't followed closely your attempt at that so I don't know how much of an option it is. > > Not sure I understand what you're saying here. You're saying that it should require a layer? If so, it already does because we only hit this code path if it's absolutely positioned (see line 73 above). If there is a layer (and there should be - as you pointed out - for absolutely positioned renderer), then you shouldn't need any ad-hoc repainting logic. The layer should invalidate properly in a separate post-layout step.
(In reply to comment #26) > (From update of attachment 158279 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=158279&action=review > > >>> Source/WebCore/rendering/RenderDialog.cpp:68 > >>> +void RenderDialog::layout() > >> > >> Don't you need to update your position if the user scrolls? If that's the case, layout will not handle this case properly. > > > > No. It's explicitly position:absolute so that scrolling will scroll it. This works around the case where the dialog is larger than the window. > > If I am reading the comments correctly, we want the <dialog> to follow the viewport when scrolling but again, I didn't really follow the bug nor the spec. I don't see that in the comments or the spec.
Thanks all for the comments. Sorry for the late response, I was away the past two weeks. (In reply to comment #26) > If I am reading the comments correctly, we want the <dialog> to follow the viewport when scrolling but again, I didn't really follow the bug nor the spec. I originally thought the spec wanted this but I've since realized it says the dialog should not follow the viewport (comment #12). > If there is a layer (and there should be - as you pointed out - for absolutely positioned renderer), then you shouldn't need any ad-hoc repainting logic. The layer should invalidate properly in a separate post-layout step. Yes, there is a layer (I used it in the setStaticBlockPosition approach in the second patch). But it seems like the invalidation isn't happening. Do you know why this may be? Can you point me to where it occurs? (In reply to comment #22) > You're mixing logical and physical heights here. The question is what should happen with a dialog that has -webkit-writing-mode:vertical-rl. It should probably center horizontally, right? In that case, you should just use logicalHeights. Would be nice to add a testcase for this as well. That sounds right. I'll look into this.
(In reply to comment #28) > Thanks all for the comments. Sorry for the late response, I was away the past two weeks. > > (In reply to comment #26) > > If I am reading the comments correctly, we want the <dialog> to follow the viewport when scrolling but again, I didn't really follow the bug nor the spec. > > I originally thought the spec wanted this but I've since realized it says the dialog should not follow the viewport (comment #12). Thanks, I stand corrected. > > If there is a layer (and there should be - as you pointed out - for absolutely positioned renderer), then you shouldn't need any ad-hoc repainting logic. The layer should invalidate properly in a separate post-layout step. > > Yes, there is a layer (I used it in the setStaticBlockPosition approach in the second patch). But it seems like the invalidation isn't happening. Do you know why this may be? There is a possibility for the invalidation not to happen but <dialog> would need to be composited and the compositor should properly handle this case already. My hunch would be that it happens but with the wrong - possibly empty - bounds. > Can you point me to where it occurs? It occurs in RenderLayer::updateLayerPositions. Look for the flags & CheckForRepaint check. This is called in FrameView::layout which passes the CheckForRepaint bit.
(In reply to comment #29) > > Can you point me to where it occurs? > > It occurs in RenderLayer::updateLayerPositions. Look for the flags & CheckForRepaint check. This is called in FrameView::layout which passes the CheckForRepaint bit. It turns out the CheckForRepaint bit is off since FrameView::layout tries to do a full repaint and then switches it off. I think the problem may actually be explained in the FIXME next to the call to view()->repaint(): // FIXME: This isn't really right, since the RenderView doesn't fully encompass the visibleContentRect(). It just happens // to work out most of the time, since first layouts and printing don't have you scrolled anywhere. The repaint call leads to FrameView::repaintContentRectangle, where the rectangle passed in is intersected with visibleContentRect(), resulting in an empty rectangle. It turns out the rectangle passed in is something like: {m_location = {m_x = 0, m_y = 0}, m_size = {m_width = 2118, m_height = 837}} Whereas visibleContentRect is: {m_location = {m_x = 0, m_y = 873}, m_size = {m_width = 2118, m_height = 837}} Based on that and the FIXME comment, it looks like the repaint doesn't work if we are scrolled, which I've been doing during testing. Sure enough, if I don't scroll vertically before the dialog is shown, the repaint occurs. If that's true, then I guess the question is whether we can make the full repaint work even if scrolled, or maybe we can keep the CheckForRepaint bit on.
Created attachment 161896 [details] WIP patch
I've updated the patch to reflect review comments. Is there some adjustment that must be made for relative positioned containers? When the dialog is inside a relatively positioned container the centering is bit off. It looks like the problem may be the call to the absoluteToLocal(). For an absolutely positioned container in the same location, the absolute coordinates computed are the same but the local coordinates are different. I removed some assertions to be able to call absoluteToLocal but I think it's safe (this patch, although not committed, did the same: https://bug-90046-attachments.webkit.org/attachment.cgi?id=154208) I looked into writing mode, but I'm thinking it may be best to just always vertically center for now. This is because it's not yet spec'd what to do, and there are some subtleties such as how a too big dialog should probably be positioned on the right for vertical-rl and left for vertical-lr (and perhaps bottom for horizontal-bt). Also the default CSS doesn't make sense for centering in vertical mode with left: 0 and right: 0; the author would have to override that or we'd have to change to deviate from the spec.
Comment on attachment 161896 [details] WIP patch This patch looks good to me. Can you do the following: 1. Add a FIXME to figure out what RenderDialogs should do in vertical writing mode. 2. Find the author/reviewer of the asserts your removing and CC them on this bug to make sure removing them is correct.
Created attachment 162155 [details] added FIXME for vertical mode
Simon, is it okay to remove the absoluteToLocal assertions in RenderBox.cpp and RenderBoxModelObject.cpp? Also, do you know why the local coordinates may come out differently when the container is relatively vs. absolutely positioned (comment #32)?
Created attachment 162196 [details] fix xcode project
Yes, its OK to remove those assertions.
Comment on attachment 162196 [details] fix xcode project Thanks for the review and comments.
Comment on attachment 162196 [details] fix xcode project Clearing flags on attachment: 162196 Committed r127681: <http://trac.webkit.org/changeset/127681>
All reviewed patches have been landed. Closing bug.