Closed Bug 1669734 Opened 8 months ago Closed 3 months ago

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

Categories

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

Firefox 83
defect

Tracking

()

RESOLVED FIXED
88 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox86 --- wontfix
firefox87 --- wontfix
firefox88 --- fixed

People

(Reporter: steffen.weber, Assigned: sefeng)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(5 files, 1 obsolete file)

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.

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

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.

Component: Untriaged → DOM: Core & HTML
Product: Firefox → Core

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.

Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Firefox 83 regression with test-case: dialog.showModal() has wrong position but dialog.show() works → dialog.showModal() has wrong position but dialog.show() works
Severity: -- → S3
Priority: -- → P3

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

Curiously, also occurs with the polyfill demos.

Attachment #9195979 - Attachment description: firefox-86-86-dialog-showModal-position.html → firefox-85-86-dialog-showModal-position.html
Flags: needinfo?(sefeng)
Regressed by: 1675857

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?

Flags: needinfo?(sefeng) → needinfo?(emilio)

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

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...

Flags: needinfo?(emilio)

Keep it on my radar

Flags: needinfo?(sefeng)
Assignee: nobody → sefeng
Flags: needinfo?(sefeng)

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.

Attachment #9205512 - Attachment description: Special case MozFitContent for resolving its Block size if margin is auto → Bug 1669734 - Special case MozFitContent for resolving its Block size if margin is auto
Attachment #9205512 - Attachment description: Bug 1669734 - Special case MozFitContent for resolving its Block size if margin is auto → Bug 1669734 - Special case MozFitContent for resolving its block size if margin is auto
Attachment #9205512 - Attachment description: Bug 1669734 - Special case MozFitContent for resolving its block size if margin is auto → Bug 1669734 - Use regular auto block sizes to resolve intrinsic block sizes

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

Attachment #9208164 - Attachment is obsolete: true
Attachment #9205512 - Attachment description: Bug 1669734 - Use regular auto block sizes to resolve intrinsic block sizes → Bug 1669734: For abspos frames, resolve intrinsic BSize keywords to the actual intrinsic BSize, instead of stretching to fill the available space
Attachment #9205512 - Attachment description: Bug 1669734: For abspos frames, resolve intrinsic BSize keywords to the actual intrinsic BSize, instead of stretching to fill the available space → Bug 1669734 - For abspos frames, resolve intrinsic BSize keywords to the actual intrinsic BSize, instead of stretching to fill the available space
Pushed by sefeng@mozilla.com:
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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/28079 for changes under testing/web-platform/tests
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → 88 Branch
Upstream PR merged by moz-wptsync-bot
You need to log in before you can comment on or make changes to this bug.