Change dialog to position:fixed, and remove dialog alignment modes. #5936

bfgeek · 2020-09-22T20:58:42Z

At least two implementers are interested (and none opposed):
- [dialog element] <dialog> positioning should be describable in CSS w3c/csswg-drafts#4645
Tests are written and can be reviewed and commented upon at:
- Tests will be updated in https://chromium-review.googlesource.com/c/chromium/src/+/2258897
Implementation bugs are filed:
- Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1675857
- Safari: https://bugs.webkit.org/show_bug.cgi?id=84635 (general dialog bug).

(See WHATWG Working Mode: Changes for more details.)

/infrastructure.html ( diff )
/interactive-elements.html ( diff )
/rendering.html ( diff )

domenic

Looks pretty solid. @sefeng211 could you take a look too?

Also please make sure there are tests for the computedStyle of an unstyled dialog matching the spec's UA stylesheet. web-platform-tests/wpt#5625 discusses the general problem and solution space (/cc @zcorpan) but for now some one-off tests would be good.

domenic · 2020-09-22T21:02:23Z

source

@@ -57253,8 +57251,6 @@ interface <dfn>HTMLDialogElement</dfn> : <span>HTMLElement</span> {

   <li><p>Add an <code data-x="attr-dialog-open">open</code> attribute to <var>subject</var>, whose value is the empty string.</p></li>

-   <li><p>Set the <code>dialog</code> to the <span>centered alignment</span> mode.</p></li>


So is the intention really that show() and showModal() now have the same visual behavior? That's a bit surprising, but if it's what everyone wants...

Yeah, this is a bit surprising to me too. I was under the impression that all discussions that happened around it were targeting centered alignment mode.

I agree this behaviour seems surprising.
It makes sense for showModal() to use position: fixed, but I wouldn't expect that behaviour when calling show().

At risk of this ended up back in the CSSWG, it feels like what would make sense is something like this:

dialog { position: absolute; } dialog:modal { /* Hypothetical pseudoclass that does not currently exist */ position: fixed; }

@sefeng211 - Thoughts on above? I'd be happy with the above proposal where the applied CSS would be:

dialog:modal { position: fixed; overflow: auto; inset-block-start: 0; inset-block-end: 0; /* 6px + 2em = border + padding */ max-width: calc(100% - 6px - 2em); max-height: calc(100% - 6px - 2em); }

I agree. Make dialog to be absolute and modal dialog to be fixed looks much safer!

domenic · 2020-09-22T21:04:33Z

source

  width: fit-content;
  height: fit-content;
+  /* 6px + 2em = border + padding */
+  max-width: calc(100% - 6px - 2em);
+  max-height: calc(100% - 6px - 2em);


Today I learned the default border width is 3px; fun!

Yup! In an ideal world this would have box-sizing: border-box applied, but this would likely change existing users of <dialog> a little too much.

So:

~~I agree that the fixed-pos thing should only be done for modal dialogs.~~ Actually I take it back, need to think a bit more about this.

In Gecko, we're using max-width: 100vw; max-height: 100vh; that seems better than this, as it preserves the right max size when authors change borders / padding, or box-sizing, which I expect to be common

Is block-size: fit-content well defined? In Gecko, it just behaves like auto, so an slightly more expressive way to do this would be inline-size: fit-content; block-size: auto; (or omitting block-size altogether). I guess that can be a separate discussion, but I think it is relevant because if it is specified to behave like auto, or it is not well defined on the block axis, then the top: 0; bottom: 0 will stretch the dialog, I think (late, so double-check me here Ian). In Gecko we do top: 50%; transform: translateY(-50%);, which conveniently is how this is done in the wild in all the examples we found, so it doesn't break positioning by sites, see https://bugzilla.mozilla.org/show_bug.cgi?id=1650820.

For reference, our stylesheet is this as of today. Modulo that bug I linked above (which is what caused us to use transform rather than translate, and which is fixed except for a UA-sniffing thing they're doing), we haven't found any other compat issues since we enabled it by default on Nightly months ago.

In Gecko, we're using max-width: 100vw; max-height: 100vh; that seems better than this, as it preserves the right max size when authors change borders / padding, or box-sizing, which I expect to be common

The max-height is incorrect on FF today by default however, e.g. the border/padding/scrollbar is clipped when the size is exceeded.

Is block-size: fit-content well defined?

block-size: fit-content - This might be something that is generally underspecified.

This is used to trigger centering based on margins, as you've specified a height. This is generally useful for web-developers maybe we should file a bug on css-sizing / css-position to actually specify that this is supported? (To be clear I'm against us changing behaviour here to un-support this :) )

I'm also opposed to using transform as you've done this would break/prevent any sites using transform to animate a dialog in/out etc.

block-size: fit-content - This might be something that is generally underspecified.

Quickly looking at css-position you could make the argument that Blink's behaviour is expected.

I'm also opposed to using transform as you've done this would break/prevent any sites using transform to animate a dialog in/out etc.

How so? The transform is overriden by the authors. These are not !important styles.

The max-height is incorrect on FF today by default however, e.g. the border/padding/scrollbar is clipped when the size is exceeded.

Ah, that's right... hm, I guess we either need to add box-sizing: border-box, or subtract border/padding. I think doing border-box sizing is cleaner, but if you'd rather leave the calc() that's probably fine too.

Ah, that's right... hm, I guess we either need to add box-sizing: border-box, or subtract border/padding. I think doing border-box sizing is cleaner, but if you'd rather leave the calc() that's probably fine too.

Right - the change to border-box will likely break too many things.

How so? The transform is overriden by the authors. These are not !important styles.

This will break existing animations, e.g. if I've specified an animation from translate(-1000px) -> translate(0) this will look broken in FF as they don't accumulate.

I think you want max-width/height: stretch ?

sefeng211

Looks good!

sefeng211 · 2020-09-22T22:53:34Z

source

@@ -57253,8 +57251,6 @@ interface <dfn>HTMLDialogElement</dfn> : <span>HTMLElement</span> {

   <li><p>Add an <code data-x="attr-dialog-open">open</code> attribute to <var>subject</var>, whose value is the empty string.</p></li>

-   <li><p>Set the <code>dialog</code> to the <span>centered alignment</span> mode.</p></li>


Yeah, this is a bit surprising to me too. I was under the impression that all discussions that happened around it were targeting centered alignment mode.

emilio · 2020-09-23T18:27:58Z

source

-  inset-inline-start: 0; inset-inline-end: 0;
+  position: fixed;
+  overflow: auto;
+  top: 0; right: 0; bottom: 0; left: 0;


this can be written as inset: 0;

emilio · 2020-09-23T18:34:49Z

source

  width: fit-content;
  height: fit-content;
+  /* 6px + 2em = border + padding */
+  max-width: calc(100% - 6px - 2em);
+  max-height: calc(100% - 6px - 2em);


So:

~~I agree that the fixed-pos thing should only be done for modal dialogs.~~ Actually I take it back, need to think a bit more about this.

In Gecko, we're using max-width: 100vw; max-height: 100vh; that seems better than this, as it preserves the right max size when authors change borders / padding, or box-sizing, which I expect to be common

Is block-size: fit-content well defined? In Gecko, it just behaves like auto, so an slightly more expressive way to do this would be inline-size: fit-content; block-size: auto; (or omitting block-size altogether). I guess that can be a separate discussion, but I think it is relevant because if it is specified to behave like auto, or it is not well defined on the block axis, then the top: 0; bottom: 0 will stretch the dialog, I think (late, so double-check me here Ian). In Gecko we do top: 50%; transform: translateY(-50%);, which conveniently is how this is done in the wild in all the examples we found, so it doesn't break positioning by sites, see https://bugzilla.mozilla.org/show_bug.cgi?id=1650820.

For reference, our stylesheet is this as of today. Modulo that bug I linked above (which is what caused us to use transform rather than translate, and which is fixed except for a UA-sniffing thing they're doing), we haven't found any other compat issues since we enabled it by default on Nightly months ago.

emilio · 2020-09-23T18:55:33Z

So regarding changing the styling of non-modal dialogs... I think I don't have a super-strong opinion. I think those right now don't have any specialness (other than the styling defined on the spec), so it may be less risky to just change the modal ones to be fixed.

bfgeek · 2020-09-23T19:25:37Z

So regarding changing the styling of non-modal dialogs... I think I don't have a super-strong opinion. I think those right now don't have any specialness (other than the styling defined on the spec), so it may be less risky to just change the modal ones to be fixed.

We'll need a :modal pseudo class in that case, (unless you are suggesting that we write to the inline-styles or something?)

emilio · 2020-09-24T08:47:22Z

We'll need a :modal pseudo class in that case, (unless you are suggesting that we write to the inline-styles or something?)

Yeah, that's effectively what we have internally in Gecko. Not sure if such a pseudo-class needs to be exposed to the web though.

bfgeek · 2020-09-24T13:05:14Z

Yeah, that's effectively what we have internally in Gecko. Not sure if such a pseudo-class needs to be exposed to the web though.

I'd slightly prefer to expose this to web-developers as 1) allows greater control, and 2) doesn't seem to be a reason not to?

bfgeek · 2020-10-01T19:16:26Z

@emilio - Are you ok w/ the height: fit-content (and I'll file an issue on the CSSWG spec clarifying our behaviour?) - and OK w/ the additional of a :modal pseudo?

emilio · 2020-10-01T20:26:00Z

Yes, I'd be ok with those. And per our other discussion it should be fine to make the max/min-height calc(100% - borderpadding) rather than viewport units / box-size.

bfgeek · 2020-10-02T19:06:12Z

@sefeng211 @emilio @domenic - I've added the :modal pseudo-class which relies on a flag within each dialog to conditionally set the position: fixed etc styles, PTAL :) .

domenic

LGTM, editorially and matches my expectations. I'm happy to merge after @emilio confirms. It seems like @sefeng211 already has chimed in to agree with this version.

sefeng211

LGTM!

emilio

Looks good, thanks!

annevk · 2020-10-05T11:34:40Z

@whatwg/css do new pseudo-classes need to be coordinated with the CSS WG?

(Apologies if I missed something, but I don't see it in https://drafts.csswg.org/selectors/ and it's not discussed in w3c/csswg-drafts#4645 either as far as I can tell.)

bfgeek · 2020-10-06T22:34:00Z

@whatwg/css do new pseudo-classes need to be coordinated with the CSS WG?

Not typically - many examples of pseudo-classes being defined outside of css specifications, e.g.
Many of the pseudo-classes in the html spec, e.g. https://html.spec.whatwg.org/#selector-in-range
https://immersive-web.github.io/dom-overlays/#xr-overlay
etc.

annevk · 2020-10-07T08:34:36Z

I'm not familiar with XR, but https://drafts.csswg.org/selectors/#range-pseudos covers the one you mentioned in HTML. And I'm pretty sure the CSS WG wanted to be involved in any additions going forward as HTML minting a couple came as a surprise. (It also seems there needs to be some involvement as there is no registry other than the Selectors document.)

fantasai · 2020-10-23T01:13:11Z

source

+  inset-block-start: 0; inset-block-end: 0;
+  /* 6px + 2em = border + padding */
+  max-width: calc(100% - 6px - 2em);
+  max-height: calc(100% - 6px - 2em);


I don't think this is going to cascade well. If the author changes the border width at all, it'll be wrong, right? You should use the stretch keyword.

I'm fine w/ this if @emilio is. Change pre-emptively to 'stretch'.

@domenic not sure if you noticed this but @emilio pointed out to me that currently no browser implements stretch. Firefox might eventually, but if someone were to write a test about the computed value for a dialog element that is modal, they would not see stretch...

Hmm. Will browsers block implementing dialog until they implement stretch then? Or should we change this to something implementable?

I don't think Firefox would block on stretch.

We don't have stretch but would use -webkit-fill-available which we've been slowly been moving to the defined stretch behaviour. For this particular case it is a drop in replacement. However happy also if we keep as the calc(100% - 6px - 2em).

Hmm. So I guess we have two concerns here:

Interop. If Blink uses -webkit-fill-available and Gecko uses calc(), it sounds like there will be interop problems if the author changes the border width. Is this OK?

Compat. If both engines use calc(), then we might be prevented from ever upgrading to stretch since it would impact the layout of <dialog>s whose border width was changed.

These trade off against each other, e.g. by sacrificing interop, we can preserve a bit of flexibility for future changes.

If we prioritize interop, then I'd say the spec (and Blink) should probably use calc(), and in the future, when both engines implement stretch, we could consider upgrading the spec, subject to the usual slow change process for any compat-impacting upgrade of this sort.

If we prioritize compat, then the spec should probably use stretch, since it indicates the desired behavior and roughly matches Blink's -webkit-fill-available IIUC.

(There's a third concern, roughly "conformance", which is that it'd sure be nice if everyone agreed with the spec on things like getComputedStyle(dialog).maxWidth. But that's pretty low priority.)

@emilio Thoughts?

For the inline axis we could probably use -moz-available. We don't have anything hooked up for the block axis to do this, I think, but it may not be too hard to either hook that or implement stretch.

Or we can probably just use calc(), honestly I think it's ~fine.

This effectively only changes the total max size of the dialog which is intended to prevent unreachable dialogs. It is mostly a safety mechanism. I don't think we'd have much trouble changing it to stretch in the future (because an overflowing modal dialog is basically always a bug, and stretch will be either equivalent or superior to both solutions).

I think the most concerning case against calc() is full-width dialog in the inline-axis which the author has styled to use less padding/border (or box-sizing: border-box). In that case calc() will incorrectly shrink the dialog too much in the inline axis and it could look a bit ugly (I'm not too concerned for shrinking too much in the block axis I think...).

But generally it seems both Chromium and Gecko have a solution for the inline axis that does the same as stretch, and I don't think using either solution prevents us from moving to stretch in the future.

I agree it likely won't prevent us from moving to stretch in the future, I've changed back to the calc() version.

fantasai · 2020-10-23T01:14:39Z

source

+dialog:modal {
+  position: fixed;
+  overflow: auto;
+  inset-block-start: 0; inset-block-end: 0;


These are already cascading in from the default styles for DIALOG, so they're not necessary.

They are necessary as the default styles only include the inline direction variants.

fantasai · 2020-10-23T01:15:27Z

source

@@ -113100,6 +113068,14 @@ dialog {
  background: white;
  color: black;
 }
+dialog:modal {
+  position: fixed;
+  overflow: auto;


Would it make sense to incorporate this also into the default styles for DIALOG?

I don't think so?

@fserb Can you explain why for dialog:modal was added overflow: auto?

That mean if dialog has html dropdowns or html tooltips which shows out of the dialog are broken because dialog shows scrolls by default. Can we eliminate scrolls in dialogs at all by default?

cc @sefeng211 @domenic

I think scroll by default makes sense, otherwise dialogs that are larger than the viewport are completely unreadable. If you're 100% sure that your content won't overflow the viewport you can set overflow: visible.

It is rare case when dialog has the same size as viewport, any case the change was break us. And question that Do we have strong reason to have overflow: auto (Ideally - to remove it.).

It could happen fairly easily if you have a small viewport and large content in the dialog. Defaulting to not leaving content unreachable makes sense imo.

bfgeek · 2020-10-28T17:46:05Z

@emilio @domenic @annevk I've change the pseudo to :-internal-modal add added prose that should should not be exposed outside the UA stylesheet. We can describe the dialog:-internal-modal rule in prose instead, but if describing these things in prose has taught us anything it'll likely be incorrect :).

domenic · 2020-10-28T17:46:38Z

I don't think we should definite an internal pseudo-class for this, if it's not observable by web content. Instead I'd suggest using prose. E.g. like is done in other sentences in the Rendering section like

When an input element's dir attribute is in the auto state and its type attribute is in the Text state, then the user agent is expected to act as if it had a user-agent-level style sheet rule setting the 'unicode-bidi' property to 'plaintext'.

or

When the document's character encoding is ISO-8859-8, the following rules are additionally expected to apply, following those above:

domenic · 2020-10-28T17:47:24Z

Oh, comment collision. Yeah, I do prefer prose a good amount, and I think it aligns better with precedent.

bfgeek · 2020-10-28T17:51:00Z

I don't think we should definite an internal pseudo-class for this, if it's not observable by web content. Instead I'd suggest using prose. E.g. like is done in other sentences in the Rendering section like

Even if this is how all the UAs do this?

domenic · 2020-10-28T17:53:50Z

Yeah. I think although it is convenient for UAs to mix together internal implementation details with the author-facing pseudo-class concept, it's not a great idea at the spec level, and muddies the meaning of pseudo-classes.

If the CSSWG would like to formally define a notion of internal pseudo-classes, and if we did a wholesale update of all similar cases in HTML (e.g. an :-internal-ISO-8859-8 pseudo-class), then it would be a reasonable route. But until then, I think they should be in prose.

Things are changing

bfgeek · 2020-10-28T18:45:48Z

If the CSSWG would like to formally define a notion of internal pseudo-classes

RESOLVED: Add a hidden pseudo-class for this purpose, in order to solve styling issue while leavine open the possibility of HTML improving its API

?

bfgeek · 2020-10-28T18:46:00Z

(from w3c/csswg-drafts#4645 )

domenic · 2020-10-28T18:48:18Z

Oh OK, so you'll also work on defining the internal pseudo-class concept in CSS? It feels like that would involve changes to the parser and cascade specs, right?

domenic

LGTM with potential nit

source

domenic

Continues to LGTM. Will give this another day or two in case folks want to review further, but I'm looking forward to merging this.

… fixed separately)

annevk · 2020-10-29T09:52:08Z

#5936 (comment) is probably worth looking into once more before merging.

…fgeek/dialog

annevk · 2020-11-04T07:35:03Z

Could someone update OP with the implementation bugs?

bfgeek · 2020-11-06T19:19:54Z

Could someone update OP with the implementation bugs?

Done - @emilio I used mozilla/wg-decisions#412 lmk if you'd like something else.

emilio · 2020-11-06T19:43:26Z

https://bugzilla.mozilla.org/show_bug.cgi?id=1675857, thanks @bfgeek

Change dialog to position:fixed, and remove dialog alignment modes.

ab983f0

domenic approved these changes Sep 22, 2020

View reviewed changes

domenic added topic: dialog The <dialog> element topic: rendering normative change labels Sep 22, 2020

sefeng211 approved these changes Sep 22, 2020

View reviewed changes

emilio reviewed Sep 23, 2020

View reviewed changes

Add :modal pseudo-class

15bf0ab

domenic previously approved these changes Oct 2, 2020

View reviewed changes

sefeng211 approved these changes Oct 2, 2020

View reviewed changes

emilio approved these changes Oct 3, 2020

View reviewed changes

annevk added the do not merge yet Pull request must not be merged per rationale in comment label Oct 5, 2020

bfgeek mentioned this pull request Oct 7, 2020

[dialog element] <dialog> positioning should be describable in CSS w3c/csswg-drafts#4645

Closed

dpogue mentioned this pull request Oct 21, 2020

alignment with whatwg/html spec AyogoHealth/ay-dialog#27

Closed

fantasai reviewed Oct 23, 2020

View reviewed changes

CSSWG resolution making :modal an internal pseudo-class.

bc610f1

Now in prose.

0138915

domenic approved these changes Oct 28, 2020

View reviewed changes

source Show resolved Hide resolved

move to rendering section.

e9afd60

domenic approved these changes Oct 28, 2020

View reviewed changes

indentation nit (also seems wrong for xref CSS but that's prolly best…

f0231fa

… fixed separately)

annevk approved these changes Oct 29, 2020

View reviewed changes

bfgeek added 2 commits October 29, 2020 15:02

back to calc() version.

d2039fb

Merge branch 'bfgeek/dialog' of https://github.com/bfgeek/html into b…

a75a0d8

…fgeek/dialog

domenic removed the do not merge yet Pull request must not be merged per rationale in comment label Nov 3, 2020

domenic merged commit 979af15 into whatwg:master Nov 3, 2020

bfgeek deleted the bfgeek/dialog branch November 6, 2020 19:19

sefeng211 mentioned this pull request Dec 10, 2020

Unset is_modal flag when modal dialog is removed from the document #6216

Closed

		@@ -57253,8 +57251,6 @@ interface <dfn>HTMLDialogElement</dfn> : <span>HTMLElement</span> {

		<li><p>Add an <code data-x="attr-dialog-open">open</code> attribute to <var>subject</var>, whose value is the empty string.</p></li>

		<li><p>Set the <code>dialog</code> to the <span>centered alignment</span> mode.</p></li>

Change dialog to position:fixed, and remove dialog alignment modes. #5936

Change dialog to position:fixed, and remove dialog alignment modes. #5936

Conversation

bfgeek commented Sep 22, 2020 • edited Loading

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpogue Sep 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilio Sep 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sefeng211 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilio Sep 23, 2020 • edited Loading

Choose a reason for hiding this comment

emilio commented Sep 23, 2020

bfgeek commented Sep 23, 2020

emilio commented Sep 24, 2020

bfgeek commented Sep 24, 2020

bfgeek commented Oct 1, 2020

emilio commented Oct 1, 2020

bfgeek commented Oct 2, 2020

domenic left a comment

Choose a reason for hiding this comment

sefeng211 left a comment

Choose a reason for hiding this comment

emilio left a comment

Choose a reason for hiding this comment

annevk commented Oct 5, 2020

bfgeek commented Oct 6, 2020

annevk commented Oct 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bfgeek commented Oct 28, 2020

domenic commented Oct 28, 2020

domenic commented Oct 28, 2020

bfgeek commented Oct 28, 2020

domenic commented Oct 28, 2020

bfgeek commented Oct 28, 2020

bfgeek commented Oct 28, 2020

domenic commented Oct 28, 2020

domenic left a comment

Choose a reason for hiding this comment

domenic left a comment

Choose a reason for hiding this comment

annevk commented Oct 29, 2020

annevk commented Nov 4, 2020

bfgeek commented Nov 6, 2020

emilio commented Nov 6, 2020

bfgeek commented Sep 22, 2020 •

edited

Loading

dpogue Sep 23, 2020 •

edited

Loading

emilio Sep 23, 2020 •

edited

Loading

emilio Sep 23, 2020 •

edited

Loading