Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[css-sizing] percentage [max-]width|height and intrinsic sizes #765

Closed
MatsPalmgren opened this issue Nov 25, 2016 · 35 comments
Closed

[css-sizing] percentage [max-]width|height and intrinsic sizes #765

MatsPalmgren opened this issue Nov 25, 2016 · 35 comments
Assignees
Labels
Closed Accepted by CSSWG Resolution Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. css-sizing-3 Current Work Needs Testcase (WPT)

Comments

@MatsPalmgren
Copy link

Background: https://lists.w3.org/Archives/Public/www-style/2015Dec/0117.html
I'm quoting @dbaron's post here verbatim for convenience:

From: L. David Baron [email protected]
Date: Tue, 8 Dec 2015 21:51:01 -0500
To: Karl Dubost [email protected]
Cc: [email protected]
Message-ID: [email protected]

On Tuesday 2015-10-27 09:44 +0900, Karl Dubost wrote:

Gecko (Firefox) has a Webcompat issue related to the use of max-width inside table. I put an example on Codepen 1.

For Webcompat reasons with Blink and WebKit, we will probably need to modify Gecko code (see Bugzilla 2). In the seeAlso section of this bug, you will find some of the reported Web Compat issues.

What I would like to know is if the CSS specification needs to be changed to reflect the reality.
[...]
Currently Blink and Safari are reducing the image so it fits the viewport.
You can see this in recent Web Compat bugs
https://webcompat.com/issues/1838
https://webcompat.com/issues/1837

I summarized the issue in the webcompat space 4.

So the underlying behavior here that needs to be specified is (using
css-sizing terminology) that when either 'width' or 'max-width' on a
replaced element is a percentage, that element's min-content
contribution is zero. Note that given that this occurs for
'max-width', this needs to override the rule that the min-content
contribution is determined by the specified size, since a replaced
element with "width: 100px; max-width: 50%" has a min-content
contribution of 0.

The relevant section of the specification is:
http://dev.w3.org/csswg/css-sizing/#replaced-intrinsic

At least, that's the relevant specification assuming a relatively
strict definition of replaced element. This rule should not apply
to form controls and similar things that aren't explained by CSS.
It should only apply to things that do replaced element sizing such
as images, plugins, videos, and iframes.

I'm not sure if we have a clear enough definition of replaced
element, or whether we perhaps need two definitions.

-David

@MatsPalmgren
Copy link
Author

@dbaron changed our layout in Gecko in this bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=823483
by reverse-engineering the Chrome/Edge behavior (IIUC).
However, there are still some case where UAs are not compatible.
When using vertical writing-mode for example:
https://bugzilla.mozilla.org/show_bug.cgi?id=1308929

It would be great to actually have a specification for this so
that everyone knows what to implement, and have this documented
for authors.

@MatsPalmgren
Copy link
Author

MatsPalmgren commented Nov 25, 2016

I think the upcoming introduction of Grid layout in some browsers makes
this more urgent to address; given the symmetric nature of track sizing
in Grid, it's likely this will lead to web-compat problems.
Here are a few simple examples where Firefox / Chrome differs currently:
https://people-mozilla.org/~mpalmgren/tests/grid/grid-item-image-size-percentages.html

Fwiw, I tend to think that max-size/size should work symmetricly in both axes.

@wolfbeast
Copy link

wolfbeast commented Nov 26, 2016

I'd definitely prefer a definitive spec for this as well, since the undefined behavior makes it possible to implement it in any number of ways that are all logical, but aren't always compatible or giving the expected results to web designers.

@dauwhe dauwhe added the css-sizing-3 Current Work label Jan 9, 2017
@fantasai
Copy link
Collaborator

So, from what Tab and I understand, the proposal is to have image-like replaced elements contribute zero as their min-content and max-content size contributions if they have a percentage as either their specified or max size.

There is an open question on whether this should apply only to the inline axis or to both axes (and in the former case which element is determining the axis mapping).

We're happy to make the necessary changes, but we need

  1. confirmation that this is in fact what you're proposing :)
  2. clarity on which axes are affected (we prefer being symmetric if possible, especially in light of Grid)
  3. WG resolution
  4. a decent definition of "image-like" that separates it from form controls (see [css-images][css-sizing][css-content] elements with intrinsic size but no aspect ratio #1044)

Waiting on your response to the first two, and then we'll Agenda+.

@dbaron
Copy link
Member

dbaron commented Feb 18, 2017

I was proposing that this change affect only min-content sizing and not max-content sizing. I believe changing max-content sizing in this way would break Web-compatibility.

I believe this needs a definition slightly broader than image-like; for us it was essentially img, canvas, plugins (object/embed), video, and iframe. I believe that list was interoperable, although it's perhaps worth retesting.

(The rule also applies to <textarea> and <input type="text">, although only for percentage width and not for percentage max-width. I believe this was also pretty interoperable.)

It's not clear to me what doing the same for both axes means; I don't think there is such a thing as a min-content size in the block axis.

Or is there a question as to whether the height property should also (in English) affect the min-content size contribution?

@fantasai
Copy link
Collaborator

Testcase: http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4905
Renderings: https://developer.microsoft.com/en-us/microsoft-edge/tools/screenshots/?url=http%3A%2F%2Fsoftware.hixie.ch%2Futilities%2Fjs%2Flive-dom-viewer%2F%3Fsaved%3D4905

This is various kinds of insane. I understand wanting to have percentage width and/or max-width make the min-content size zero. I could also understand wanting to do this for images but not form controls or vice versa. But I don't understand wanting to do it for both width and max-width for images, but only width (and not max-width) for form controls.

Do we have to be randomly inconsistent here, or can we spec something somewhat sane like "all replaced elements are affected by both max-width and width" or suchlike?

Generally-speaking, the min-content size in the block axis is (like the max-content size) the content-based size--what results from height: auto. We define the concept so that things like grid layout and orthogonal flows can be defined with symmetry. So, for example, if I ask a grid to size a row filled with images or form controls as “min-content”, it's defined. Similarly if I wrap a float around a vertical-rl form control, its min-content width (logical height) needs to be defined.

Also, one thing to point out here: for form controls the min-content contribution isn't exactly zero, it's UA-defined based on how narrow the form control can actually get. This limitation is probably worth preserving in either case.

@fantasai
Copy link
Collaborator

fantasai commented Feb 23, 2017

Proposal atm is:

  • All replaced elements get a maximally-squashed min-content size if their width or max-width is percentage-based
  • Maximally-squashed more-or-less means the width resulting from a percentage resolved to zero: form controls preserve some amount of their space as needed to render, and calc() expressions retain their fixed-length components.
  • Same constraint is also applied in the height dimension.

The key question is, is this Web-compatible, given form controls don't currently respond to max-width?

@dbaron
Copy link
Member

dbaron commented Mar 1, 2017

I think the phrasing around "maximally-squashed" might just be "add the padding and border and margin outside of the zero width".

@astearns
Copy link
Member

astearns commented Mar 1, 2017

We discussed this on the call and asked some implementors to look at the web compatibility of fantasai's proposal above

@gregwhitworth
Copy link
Contributor

Ok, so based on the @fantasai statement above, to determine the "rough" web compatibility of this we need to essentially look for max-width being used on form controls, correct?

@gregwhitworth
Copy link
Contributor

Anything beyond that - because I'll kick off a crawl looking for this and provide the data here - if you want something more let me know by EOD as I'll kick it off tomorrow. cc: @dbaron @fantasai @MatsPalmgren @astearns

@gregwhitworth
Copy link
Contributor

@fantasai @dbaron Ok here is my recipe I plan to run with: https://github.com/MicrosoftEdge/css-usage/blob/master/src/recipes/max-height-replaced-elems.js

This does what I was saying in IRC that this will determine if a % is used on max-height and is on a replaced element with a height of auto. This does not go the further step of ensure that a parent somewhere up the tree has its height constrained to show this issue. This is more computationally intensive and will slow the crawl down, I think it would be good to cast our net wide and then scope it in. I can actually take the resulting URLs of the wide net and then scope the crawl to only the sites that have the potential problem and look for the actual issue. That said, even upon determining this we'll need to determine how many nodes we need to look up in the tree (if we limit it at all) and then also how we deem if the result would overflow causing the compat issue seen with max-width.

We can figure this out after the crawl is complete. Please let me know ASAP if there is any concern with the recipe I linked to above.

@dbaron
Copy link
Member

dbaron commented Mar 2, 2017

Not sure why you're looking at height rather than width. I think the idea that there are intrinsic sizes in the block dimension is a spec fiction that I'm working on an alternative proposal for. But any issues in the wild would almost certainly be with width, not height.

Also, I think the proposed change to the widely-implemented behavior affects only textarea and input type="text", not img, since the proposal is to change them to match the img, video, etc., behavior.

@gregwhitworth
Copy link
Contributor

I must have mis-read what @fantasai meant by

Same constraint is also applied in the height dimension.

I assumed she was wanting to know about web compat concerns to make the block direction match the inline. My bad, this is easy enough to modify.

Here is the updated file: https://github.com/MicrosoftEdge/css-usage/blob/master/src/recipes/max-width-replaced-elems.js

@dbaron
Copy link
Member

dbaron commented Mar 2, 2017

So, the results of a slightly-more-careful review this time:

So it still needs to test somewhere that max-width is a percentage. And it should probably also allow width to be explicitly auto rather than requiring it to be unspecified (which I assume is what the undefined test is checking).

@gregwhitworth
Copy link
Contributor

Wow, I'm surprised I missed the not checking % - gosh. Added that in.
https://github.com/MicrosoftEdge/css-usage/blob/master/src/recipes/max-width-replaced-elems.js

@gregwhitworth
Copy link
Contributor

Ok, following up on my action - here is what the crawl found:

  • 13,144 unique URLs contained a % on max-width with an auto width
  • 14,155 total hits of input or textarea (this is not the count of inputs/textareas on the page)

Crawl info:

  • URLs 1 million = .01% of sites
  • Top 1K Worldwide + Random Long Tail & Geographic spread

To follow this up for manual inspection, I checked of these URLs if they had a parent with a potential shrink-to-fit parent (abspos, float, table cell, flex).

12,265 of them remain with potential. I did NOT check to see if the parent was setting its own width or if the found element's siblings would have caused the problem to not be noticeable by the end user.

We will follow up on how best we want to determine if this problem will be visible by the end user. @fantasai had some ideas.

@gregwhitworth
Copy link
Contributor

Oh, it's worth noting since @fantasai and I had discussed the possibility of manually looking at these pages to extrapolate on I went ahead and grabbed the ID/classes of the parent, as well as the outerHTML of the element that was using max-width with a %. This should aid in finding the parent quickly and then flooring the s2f to see if the input would overflow with adverse affects on the overall layout to the end user.

@fantasai
Copy link
Collaborator

@gregwhitworth Can you attach a textfile with the list of potentially-affected URLs? We can manually check a selection of them.

@gregwhitworth
Copy link
Contributor

gregwhitworth commented Mar 30, 2017

max-width.xlsx

Ok, @fantasai I have attached it. Let me explain how the columns relate.

Take the following:

TEXTAREA parent MAIN classes main,col-md-8

This should be read that we found the <textarea> matched what we were looking for. To help out in the manual investigation we then are providing the classes for the parent tag which is a <main> element. The classes are main and col-md-8. So the count row then provides the count of textarea that were found that matched on the page. Then the id row provides the id of the parent if it existed (or it's just empty).

I did reach out to our central data science team regarding how to extrapolate on this and they said, without deeper insights that a good starting place is to select 2% of random URLs and review those. If I and this team, have the time I'll try to get a deeper more specific answer to this question so that we can make this calls with an even higher confidence.

Disclaimer - this data is gathered from the web with as limited bias as possible so there may be some offensive sites included in the results.

@fantasai
Copy link
Collaborator

OK, just to clarify, this is a list of pages that have 'width: auto; max-width: ' on either an INPUT or TEXTAREA, correct?

Probably the easiest way to test would be to use a user stylesheet with

input,textarea { width: 0 !important; }

and see if any of the pages appear broken on a random sample of 260 pages from the list.

@fantasai
Copy link
Collaborator

(Aside from having unusably narrow input fields, of course.)

@fantasai
Copy link
Collaborator

fantasai commented Aug 9, 2017

@dbaron / @MatsPalmgren Would you mind reviewing the edits to ensure they are correct? The section is https://drafts.csswg.org/css-sizing-3/#intrinsic-contribution and the proposed edits are:

However, in the case of a replaced box with a percentage-based width/max-width/height/max-height, the percentage is resolved to zero when calculating the min-content contribution in the corresponding axis.
For example, an input assigned width: calc(50% + 50px) has a min-content contribution of 50px plus any horizontal margin/border/padding.

However, I think it wasn't clearly discussed whether the calc() case should resolve to 50px vs. zero. The alternate set of edits would be

However, in the case of a replaced box with a percentage-based width/max-width/height/max-height, the percentage-based size is treated as zero when calculating the min-content contribution in the corresponding axis.
For example, an input assigned width: calc(50% + 50px) has a min-content contribution of zero plus any horizontal margin/border/padding.

@tabatkins
Copy link
Member

It would be clearly, fundamentally, obviously wrong if calc(50% + 50px) ever resolved to zero just because percentages resolve against a zero size. I don't think we seriously need to discuss that.

@fantasai
Copy link
Collaborator

fantasai commented Aug 9, 2017

Testcase: http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=5297
Results: FF and Edge zero out the percentage only. Chrome zeroes out the entire size but only on images?

@tabatkins Sounds good to me. :) I'll close the issue, then, esp. since the WG resolution explicitly includes that wording and you represent Chrome, which is the browser that (afaict) gets it wrong. 😁

@dbaron
Copy link
Member

dbaron commented Aug 9, 2017

I'm certainly OK with trying the first set of edits.

The biggest issue with them is that they need a clearer definition of replaced element. This is the definition of replaced elements that includes not only the ones that honor CSS 2.1's rules on sizing of replaced elements, but also includes most form controls, but not buttons. So I think it's <input> with type other than button/submit/reset, and also <select>, <textarea>, <progress>, and <meter>. I'm not sure about <output>, and I think <fieldset> doesn't count (but it's probably worth testing). The fact that buttons are not included means there isn't an appropriate canned definition floating around.

Another note is that if your testcase determined that Firefox zeroes out only the percentage, then your testcase is wrong, since the last time we talked about this I wrote a patch (still unlanded) to change that.

Also, I filed a Mozilla bug 1388840 on updating the behavior.

@fantasai
Copy link
Collaborator

fantasai commented Aug 9, 2017

Another note is that if your testcase determined that Firefox zeroes out only the percentage, then your testcase is wrong, since the last time we talked about this I wrote a patch (still unlanded) to change that.

That's what I remembered... was very confused when I didn't get that result. Figured it out, here's the updated testcase: http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=5298

@fantasai fantasai added Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. and removed Commenter Response Pending labels Aug 9, 2017
@dbaron
Copy link
Member

dbaron commented Aug 9, 2017

So what about the bit about pointing to an appropriate definition of replaced elements?

@fantasai
Copy link
Collaborator

fantasai commented Oct 18, 2017

Filed that one as #1889 since this issue is plenty long enough and the issue as initially described has been fixed. Please only reopen if you have an objection to the resolution or the edits implementing it; defining “replaced element” is a whole other realm of problem.

@fantasai fantasai added Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. and removed Commenter Not Satisfied labels Oct 18, 2017
@fantasai
Copy link
Collaborator

fantasai commented Oct 27, 2017

Fwiw, we checked in edits for #1889 based on @dbaron's comment and would appreciate a review. Thanks~

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Closed Accepted by CSSWG Resolution Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. css-sizing-3 Current Work Needs Testcase (WPT)
Projects
None yet
Development

No branches or pull requests

10 participants