1669734 - dialog.showModal() has wrong position but dialog.show() works

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Steffen Weber]

Attachment: firefox-83-dialog-showmodal-regression.html

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/86.0.4240.75 Safari/537.36

Steps to reproduce:

1. Open https://www.computerbase.de/firefox-83-dialog-showmodal-regression.html

Alternatively, open the attached file.

Interesting observation: The issue disappears when the dialog is opened using "dialog.show()" instead of "dialog.showModal()".

Actual results:

The dialog is positioned too far up. Parts of it are rendered outside the viewport. See the rendering in Firefox 82 or Chrome for a comparison.

Expected results:

The dialog should not be positioned too far up.

Comment 1

[Steffen Weber]

This issue was originally reported by two of our users who were unable to use our consent dialog that we launched today in Firefox 83 Alpha 1: https://www.computerbase.de/forum/threads/in-eigener-sache-auch-wir-kommen-an-einem-consent-dialog-nicht-vorbei.1973328/post-24712583

Comment 2

[Tom S [:evilpie]]

Dialog show and showModal are only enabled on Nightly. That said I can reproduce this issue in Firefox 81 after enabling dom.dialog_element.enabled in about:config.

Comment 3

[Steffen Weber]

Dialog show and showModal are only enabled on Nightly.
Oops, you are right. I must have had the dialog polyfill (https://github.com/GoogleChrome/dialog-polyfill) still active when I tested in older Firefox versions.

Comment 4

[Steffen Weber]

I think this issue can be closed. See:

https://bugzilla.mozilla.org/show_bug.cgi?id=1645046#c18
https://bugzilla.mozilla.org/show_bug.cgi?id=1645046#c19
https://bugzilla.mozilla.org/show_bug.cgi?id=1645046#c20

Comment 5

[inopinatus]

Attachment: firefox-85-86-dialog-showModal-position.html

showModal() is still producing invalid position on 85 and 86. Test document attached.

Curiously, also occurs with the polyfill demos.

Comment 6

[Darryl Pogue]

I believe this might be due to https://bugzilla.mozilla.org/show_bug.cgi?id=567039

Comment 7

[Sean Feng [:sefeng][PTO, back July 1st]]

Ah, so the block size is not calculated correctly with max-height?

Er, I don't know if there's a trick to make it work, maybe we need to bring back top? Thoughts on this emilio?

Comment 8

[Sean Feng [:sefeng][PTO, back July 1st]]

(web platform tests didn't catch this because they set the height and width of the dialog explicitly)

Comment 9

[Emilio Cobos Álvarez (:emilio)]

So <dialog> has position: fixed; top: 0; bottom: 0;, which basically means "take all the available space". It seems blink special-cases height: fit-content here to behave differently from auto, but if you take the test-case in comment 5 and add dialog { height: auto }, then Chrome behaves like us.

The fix for this should probably be something like not entering this } else { branch when the block-size is -moz-fit-content, and resolve the offsets during layout.

But it's not clear to me what the interactions with margins should be though...

Comment 10

[Sean Feng [:sefeng][PTO, back July 1st]]

Keep it on my radar

Comment 11

[Sean Feng [:sefeng][PTO, back July 1st]]

Attachment: Bug 1669734 - For abspos frames, resolve intrinsic BSize keywords to the actual intrinsic BSize, instead of stretching to fill the available space

In this patch, instead of making the block size as much as possible
for margin:auto, height: -moz-fit-content; width: -moz-fit-content, we
use the default size.

Comment 12

[Sean Feng [:sefeng][PTO, back July 1st]]

Attachment: Bug 1669734 - Add tests to test the special layout handling for fit-content, min-content and max-content

Depends on D106497

Comment 13

[Sean Feng [:sefeng][PTO, back July 1st]]

Attachment: Bug 1669734 - Factor out the logic of computing auto margins for abspos frames to its own method

Comment 14

[Sean Feng [:sefeng][PTO, back July 1st]]

Attachment: Bug 1669734: For abspos frames, resolve intrinsic BSize keywords to the actual intrinsic BSize, instead of stretching to fill the available space

This patch creates a new behavior for absolute positioned frames such
that if they have intrinsic BSize keywords (-moz-fit-content, min-content,
max-content) and the margins are auto, instead of taking as much spaces
as possible, use the actual intrinsic BSize as the BSize.
Users can still use auto keyword to make it to fill the available
space.

Depends on D107872

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e52d44308040
Factor out the logic of computing auto margins for abspos frames to its own method r=dholbert,emilio
https://hg.mozilla.org/integration/autoland/rev/8083eaa5d432
For abspos frames, resolve intrinsic BSize keywords to the actual intrinsic BSize, instead of stretching to fill the available space r=emilio,dholbert
https://hg.mozilla.org/integration/autoland/rev/7436beab77b1
Add tests to test the special layout handling for fit-content, min-content and max-content r=emilio

Comment 16

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/28079 for changes under testing/web-platform/tests

Comment 17

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/e52d44308040
https://hg.mozilla.org/mozilla-central/rev/8083eaa5d432
https://hg.mozilla.org/mozilla-central/rev/7436beab77b1

Comment 18

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot