Skip to content

[css-overflow-3] 'overflow' 2-value syntax is in wrong order #2988

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

Open
tabatkins opened this issue Aug 3, 2018 · 37 comments
Open

[css-overflow-3] 'overflow' 2-value syntax is in wrong order #2988

tabatkins opened this issue Aug 3, 2018 · 37 comments
Assignees
Labels
css-overflow-3 Current Work

Comments

@tabatkins
Copy link
Member

Currently the 'overflow' 2-value syntax is parsed to be horizontal-vertical (to be precise, the first value maps to overflow-x, the second to overflow-y).

When possible, for any new 2-axis properties we're trying to map them to the logical longhands instead, so this is currently wrong. As well, for horizontal writing modes, the x/y ordering is opposite the logical ordering we've settled on, which is block/inline (to match what 4-value syntaxes do when you specify only two values).

As the two-value syntax is new, this should still be fixable - two values should map to overflow in the block and inline axises, in that order.

@tabatkins tabatkins added the css-overflow-3 Current Work label Aug 3, 2018
@fantasai
Copy link
Collaborator

fantasai commented Aug 3, 2018

It's especially important to keep scroll-snap-align and overflow consistent since they will often be used together.

@css-meeting-bot
Copy link
Member

The Working Group just discussed 'overflow' 2-value syntax is in wrong order, and agreed to the following:

  • RESOLVED: Match the block followed by inline ordering of 2 value pairs for the overflow-x and overflow-y shorthand to be consistent
The full IRC log of that discussion <dael> Topic: 'overflow' 2-value syntax is in wrong order
<dael> github: https://github.com//issues/2988
<dael> Rossen_: This is value ordering and we try to do x then y, but all logical properties are block and then inline. What is the proposal here?
<dael> fantasai: Proposal is to use the logical order, block then inline. Overflow prop do have logicial shorthands. Also pretty important to make sure ordering matches with scroll-snap-align. Having them opposite is fairly confusing
<dael> Rossen_: This would apply to...a handful or properties
<dael> fantasai: Just overflow shorthand property. I don't know of any others, though they might exist
<dael> Rossen_: Do we keep consistency for position properties?
<dbaron> how many engines implement 2-value overflow? Gecko does.
<dael> fantasai: Once in logical coord are block first inline second. All the 4 values are block first inline second
<dael> fantasai: Logicial 2 value coord is mainly background-position and that's a rough WD with logical positioning values
<dael> fantasai: Physical are x then y
<dael> dbaron: How many engines impl?
<dael> fantasai: Just mozilla b/c we jsut added in April
<dael> Rossen_: We've had overflow-x and -y since IE 6
<dael> fantasai: We won't change that.
<bradk> When did we resolve on doing block first? That seems wrong to me.
<rego> Chromium has it in M68 I believe
<dael> Rossen_: Sorry, though you were talking about long hand
<rego> https://www.chromestatus.com/feature/5090725653905408
<dael> Rossen_: So that means compat risk is fairly small
<dael> fantasai: Yeah
<fantasai> bradk, awhile back ... when we were working on css-grid
<dael> Rossen_: dbaron would you guys be okay with this?
<dael> dbaron: Yeah
<dael> Rossen_: Prop: Match the block followed by inline ordering of 2 value pairs for the overflow-x and overflow-y shorthand to be consistent
<dael> Rossen_: Objections?
<fantasai> bradk, I'm not entirely convinced it was a good idea, there were good arguments in both directions... but we should be consistent here
<dael> RESOLVED: Match the block followed by inline ordering of 2 value pairs for the overflow-x and overflow-y shorthand to be consistent

@dbaron
Copy link
Member

dbaron commented Aug 8, 2018

I filed Mozilla bug 1481866 to change this.

@emilio
Copy link
Collaborator

emilio commented Aug 8, 2018

@bradkemper
Copy link
Contributor

Currently, the 2-axis starts-with-horizontal rule, and the up-to-four-compass-directions starts-with-top rule is understandable and mostly internally consistent. Having some values that follow those rules and others that don’t is going to be horribly confusing and hard to remember. I should have said so in the call.

@tabatkins
Copy link
Member Author

Right, but it'll be confusing no matter what, as we have several properties already doing 2-axis in the logical order. The physical order is more or less just a legacy mistake at this point.

@frivoal
Copy link
Collaborator

frivoal commented Aug 15, 2018

I was about to patch the spec, but now I'm confused: is the shorthand supposed to be overflow-block then overflow-inline (logical longhands), or overflow-y then overflow-x (physical longhands)? The resolution sounds like the former (especially since @fantasai invokes consistency with scroll-snap-align as a rationale), but as far as I can tell the code change by Mozilla and the request in Chrome's bug tracker to match the resolution are the later.

It seems to me that they're wrong, but I wasn't there during the call, so I may be missing something. Which is it?

@Loirooriol
Copy link
Contributor

Loirooriol commented Aug 15, 2018

All shorthands that don't contain block or inline in their name seem to correspond to physical longhands, even if they are new like inset. But probably the same can be said about the order (scroll-snap-align is not a shorthand).

@frivoal
Copy link
Collaborator

frivoal commented Aug 15, 2018

that seems to contradict what @tabatkins and @fantasai were saying thought.

for any new 2-axis properties we're trying to map them to the logical longhands

important to keep scroll-snap-align and overflow consistent

@tabatkins
Copy link
Member Author

Yes, our entire argument, and the resolution, was to have the order be "block, then inline", not "x, then y". (The resolution is... confusingly written, since it refers to "the overflow-x and overflow-y shorthand" rather than... the "overflow" property, which predates either.

All shorthands that don't contain block or inline in their name seem to correspond to physical shorthands

Incorrect (see any of the Grid properties). And doesn't make a ton of sense, since a property that applies to both axises wouldn't specialize its name with "block" or "inline" - that's for sub-properties that are doing a single axis.

@frivoal
Copy link
Collaborator

frivoal commented Aug 15, 2018

Ok, so I'll fix the spec per the resolution (i.e. in terms of logical directions), and have pinged Chrome and Mozilla in their relevant bugs about the misunderstanding.

@emilio
Copy link
Collaborator

emilio commented Aug 15, 2018

I think that's way riskier than what we interpreted. For example, code like this will stop working:

  document.body.style.overflow = "hidden";
  document.body.style.overflowX // Will start returning "", but returned "hidden" up until now.

I'm not sure that's totally acceptable...

@emilio
Copy link
Collaborator

emilio commented Aug 15, 2018

Also, we haven't implemented overflow-inline / overflow-block in Firefox yet, which means that what was stated as a tweak to an existing shorthand now requires implementing two properties and breaking the behavior of the pre-existing shorthand.

@frivoal frivoal self-assigned this Aug 15, 2018
@frivoal
Copy link
Collaborator

frivoal commented Aug 15, 2018

Discussed this a bit on the call:

fantasai: It's block then inline, not y then x
emilio: We're changing the shorthands the property expands to?
emilio: Like, the longhands. It's way more risky then a tweak to shorthand. If you access via CSSOM it may break.
fantasai: I see problem. For a one value value of overflow shorthand it has to be assigned to phsyical longhand. For 2 value it has to be assigned to logical. That would not break anything other then the last 4 months of work
emilio: Right, but that i s not how any other shorthand works
fantasai: We're intending the syntax change to happen, but haven't decided on it. For margin/border/padding there is intent to have a switch for if assigning to physical or logical. background-position is aligned to do this.
fantasai: There are 2 types of values for background position. One set will assign to physical and the other to logical
florian: Why does it make a difference to physical or logical in case where assigning to long hand?
fantasai: https://drafts.csswg.org/css-backgrounds-4/#the-background-position
emilio: B/c you can read longhand in CSSOM
emilio: If you read element/style you don't resolve the properties there
TabAtkins: You don't resolve any...oh, you do shorthands. nevermind
florian: I suggest we hash that out on github.

@emilio
Copy link
Collaborator

emilio commented Aug 15, 2018

Also, I'm not sure about the shorthand-expands-to-different longhands thing @fantasai mentions. I think that's not a trivial change at all.

For example we need to define how the shorthand serialization argument works in that case. If I did:

overflow-x: hidden;
overflow-y: hidden;
overflow-block: visible;
overflow-inline: hidden;
  • What should element.style.overflow return? Empty string presumably?
  • What should element.style.cssText serialize to?

That all needs more thought... Not saying that it's impossible to spec or implement, but...

@Loirooriol
Copy link
Contributor

Incorrect (see any of the Grid properties)

Sure, grid properties refer to columns or rows which are logical, but there aren't physical properties. In cases with both physical and logical longhands, the shorthand seems to refer to the physical ones.

Changing overflow to refer to logical longhands is not backwards compatible, and I would avoid hacks to make it half physical half logical. The extra complexity for CSSOM doesn't seem worth it.

@tabatkins
Copy link
Member Author

In cases with both physical and logical longhands, the shorthand seems to refer to the physical ones.

Because all of those are legacy shorthands dating back before we started adding logical longhands. In the time since we started doing that, we've defaulted to logical, and in fact have generally done logical-only.

Changing overflow to refer to logical longhands is not backwards compatible, and I would avoid hacks to make it half physical half logical. The extra complexity for CSSOM doesn't seem worth it.

In all current impls overflow only takes a single value; except for the shorthand-expansion-in-specified-style issue that Emilio brought up (thanks!), it's unobservable which pair of longhands it canonically expands to. We should think more about that one exposed spot.

For example we need to define how the shorthand serialization argument works in that case. If I did: [snip]

Note that this isn't a backwards-compat question, as this is assuming use of the new logical longhands, nor it is even overflow-specific; this is just a general question about how shorthands with both physical and logical longhands should serialize.

(I'm not sure if this question is answered in css-logical offhand, as I'm not an editor - @fantasai or @frivoal?)

@Loirooriol
Copy link
Contributor

Because all of those are legacy shorthands dating back before we started adding logical longhands.

inset is added in CSS Logical and corresponds to physical longhands.

except for the shorthand-expansion-in-specified-style

Yes, that's what I don't think it's backwards compatible.

this is just a general question about how shorthands with both physical and logical longhands should serialize

Shorthands only correspond to physical or logical longhands, not both.

For example, element.style.margin will be empty string if you set logical margin longhands but not the physical ones. Or if you first set the physical ones and then you override them with logical ones, then element.style.margin will serialize the physical ones even if they are not being used. And if you set physical ones but not logical ones, element.style.marginBlock will be empty string.

That's how it has been implemented in Firefox and Blink. (Well, right now Blink never serializes logical shorthands, I'm writing a patch right now).

@fantasai
Copy link
Collaborator

inset is added in CSS Logical and corresponds to physical longhands.

Yes, because it needed to be consistent with the rest of the margin/padding/border set.
overflow needs to be consistent with scroll-snap-align, more than anything.

Shorthands only correspond to physical or logical longhands, not both.

Shorthands correspond to both. We're still working out the exact mechanism, but they do correspond to both.

For example, element.style.margin will be empty string if you set logical margin longhands but not the physical ones. Or if you first set the physical ones and then you override them with logical ones, then element.style.margin will serialize the physical ones even if they are not being used. And if you set physical ones but not logical ones, element.style.marginBlock will be empty string.

Once we have a syntax for representing a logical margin declaration, it should return a value. (It's currently an open issue.)

@emilio
Copy link
Collaborator

emilio commented Sep 19, 2018

I'm reverting the swap Firefox made in https://bugzilla.mozilla.org/show_bug.cgi?id=1492567. There's no point in shipping overflow-y, then overflow-x if it's neither what the WG wants (which is way more subtle), nor what other engines are going to ship (https://bugs.chromium.org/p/chromium/issues/detail?id=872356), nor what we've shipped already from Firefox 61 (which is overflow-x, then overflow-y).

@fantasai
Copy link
Collaborator

@emilio I would understand if you reverted to only accepting one value, but reverting to the opposite of the spec says means that we will end up with Web content depending on what you are shipping. I don't think this is a good plan. Either ship something compatible with the spec or object to the spec being incompatible with what you insist on shipping.

@emilio
Copy link
Collaborator

emilio commented Sep 20, 2018

I replied on the bug but:

@emilio I would understand if you reverted to only accepting one value, but reverting to the opposite of the spec says means that we will end up with Web content depending on what you are shipping. I don't think this is a good plan. Either ship something compatible with the spec or object to the spec being incompatible with what you insist on shipping.

I'm reverting to what we (and other engines) are shipping, to avoid shipping something that is both incompatible with the spec and with our previous behavior.

We've been shipping the shorthand in the x/y form since Firefox 61. If we want to unship that (which I could understand) I can do that, but I definitely would like that to go through the regular path and not getting the change in a beta without any nightly cycle. Also, we should ask the Blink folks to unship their shorthand too in that case.

My objection with what the CSSWG seemed to resolve (which both me and David misinterpreted), apart from the ones I've expressed in comments above, is mostly that the WG decided we wanted to change a shorthand to work like no other shorthand currently does (referring to some properties sometimes, and to others other times) after one implementation had already shipped the previously-resolved thing, which is the opposite behavior, and another had already implemented and cleared for shipping the same way.

@BillGoldstein
Copy link

BillGoldstein commented Sep 20, 2018

Suggestion: 2 shorthands
overflow: x y
&
overflow: logical block inline

It would seem to be more consistent with rest of css logical.
Guess it'd imply new overflow-inline & overflow-block longhands.

@BillGoldstein
Copy link

BillGoldstein commented Sep 20, 2018

Guess it'd imply new overflow-inline & overflow-block longhands.

which obviously already exist in overflow 3.

@fantasai
Copy link
Collaborator

@emilio If you have an objection to what's in the spec, you should raise that issue, not leave an offhand comment in a bug that you've implemented the opposite of the spec without any indication of what you expect the WG or other implementers to do about it. If you go back and read my comment, I said there were two conditions either of which are reasonable positions to take: a) You implement something compatible with the spec. b) You implement something else and ask the spec to change. Your comments read off as c) You implement something incompatible with the spec, and don't ask for the spec to change. I don't think c) is reasonable behavior and consider it bad form.

@emilio
Copy link
Collaborator

emilio commented Oct 21, 2018

To be clear, I have not implemented anything related to this spec change. I did implement the original resolution, which we and Blink were shipping, and I have just reverted a patch which also diverged from this resolution, in order to avoid shipping two different things that diverged from the spec.

I don't really think that should be controversial, tbh.

@majido
Copy link
Contributor

majido commented Oct 22, 2018

@tabatkins mentioned this bug today. We should try to change this in Blink before the usage grows large and the compat issue difficult to deal with. Note that we have already switched scroll-snap-align to have the block/inline order so it makes sense to change overflow parsing to match.

At the moment I am trying to understand the current usage and the scope of potential breakage. At the moment I am looking at httparchive data (bug) since Blink usage stats are most likely not helpful since this overloads existing single-value overflow. If you have ideas how to better understand the potential compat issue please chime in that bug.

@ewilligers
Copy link
Contributor

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed 'overflow' 2-value syntax is in wrong order.

The full IRC log of that discussion <dael> Topic: 'overflow' 2-value syntax is in wrong order
<dael> github: https://github.com//issues/2988
<dael> astearns: IRC chat above about it
<dael> fantasai: Looks like waiting on majidvp making change in Blink which requires him doing some research
<dael> florian: What are we blocked on majidvp for?
<astearns> majidvp> fantasai: no new update. I have not had a chance to work on this since last update. My next step was to look at the mobile data from httparchive. Pending that does not raise a red flag we can try to fix this in Blink.
<dael> TabAtkins: He's an interested impl who understand problem and wants to fix.
<dael> florian: He's attempting to do what we spec and if he succeeds that overrules the concerns?
<dael> TabAtkins: I believe so. I think only concern is compat so us being happy avoids that
<dael> emilio: Compat concern, but also shorthands that expand to multiple longhands. Don't know if Blink plans to impl, but need to do more off spec work.
<dael> TabAtkins: Yes, that's discussion from TPAC. That's a larger issue that needs to be resolved in sensible way.
<dael> TabAtkins: We already have a number of proerties with this problem, so we need to solve for all
<dael> emilio: Cannot fix this without figuring out whole thing
<dael> TabAtkins: We've already got margin-start and margin-left
<dael> emilio: margin shorthand is only physical
<dael> TabAtkins: Yes, but need to worry about shorthand intereaction with both. Same thing you've got here.
<dael> emilio: It's another level of interaction
<dael> TabAtkins: I don't understand how different
<dael> florian: Same as having the extra keyword on shorthands to say. We don't have that
<dael> TabAtkins: If you set margin and margin-inline-start and ask for margin, we don't know what it should return
<dael> emilio: We do
<dael> florian: Margin is shorthand of physical only.
<dael> fantasai: It's impl that way. If you replace margin with physical shorthands you get corrent. gCS it's mapped across both
<dael> emilio: gCS is different b/c knows writing mode. Issue is specified style. This would be a case where there is no answer
<dael> TabAtkins: If overflow 2 value is logical and margin is phsyical it's congruint
<dael> emilio: When you spec overflow it maps to 4 properties. Overflow shorthand can take different prop
<dael> florian: Shorthand to longhand is parse time, but need to parse on computed value
<dael> TabAtkins: Way we're talking is it's parse time. Overfloew 2 value expands to logicial long hands.
<dael> emilio: But only when in 2 value
<dael> TabAtkins: When in 1 doesn't matter
<dael> emilio: Does matter because then you need to return something for physical for compat
<dbaron> I guess you can't hear me?
<dael> TabAtkins: Same problem as margin. Set margin-start and margin below it blows away margin-start
<dael> emilio: It's about setting, not getting.
<dael> emilio: I filed a bunch of issues about serialization not round tripping. I'm pretty sure a shorthand that expands to multiple prop is a new problem
<RRSAgent> I have made the request to generate https://www.w3.org/2018/11/14-css-minutes.html tantek
<dael> dbaron: Example of something where behavior now isn't spec: If we assume that we haven't intro logical. If stylesheet says overflow-x:visitible and overflow-y:visible if you call on overflow you get visible
<dael> dbaron: If you have overflow-x:visitible and overflow-y:visible overflow-block:visitible and overflow-start:visible what do you get?
<RRSAgent> I'm logging. I don't understand 'make minutes public', tantek. Try /msg RRSAgent help
<emilio> dbaron++
<dael> dbaron: Point isn't what answers are it's that answers aren't obvious in spec now
<dael> TabAtkins: So it's legacy behavior running into this. I missed that.
<dael> dbaron: I don't think it's legacy. I don't know any impl that has impl both logicila and physical shorthands.
<dael> TabAtkins: Blink has logical of block
<dael> dbaron: Longahnds, but shorthands aren't mapped. I think that's b/c there's so many spec issues in OM
<dbaron> s/call on/call getPropertyValue on/
<dael> emilio: Have logical long hands and shorthands, but they only map to logical long hands
<dael> TabAtkins: In cases right now where single property has both logical and physical underneath it only maps...I think we wanted the logical longhand to be ignored from shorthand after
<dael> TabAtkins: While there are legacy issues with overflow in particular, the issue in general applies to all of our logical and physical properties.
<dael> TabAtkins: I completely agree let's define that
<tantek> regrets+
<RRSAgent> I have made the request to generate https://www.w3.org/2018/11/14-css-minutes.html tantek
<dael> florian: majidvp experiment may provide feedback

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 28, 2019
Per w3c/csswg-drafts#2988 the overflow
shorthand should be parsed in logical order (block then inline) instead
of physical order (x then y).

Updated web platform tests.

Bug: 872356
Change-Id: I674f8a42b2712e7d8fdfa4ba4a7575ed2fb7bc85
@jonjohnjohnson
Copy link

Any news here on the experiment/data? We still got x/y in the wild and just hoping for a shorthand similar to inset that can take the impending logical keyword modifier.

@andruud
Copy link
Member

andruud commented May 24, 2019

So, wait, this means overflow now has two shorthand expansions associated with it, depending on whether the 1-value syntax or 2-value syntax is used? I agree with @emilio that this is unfortunate.

@ewilligers
Copy link
Contributor

@fantasai
Copy link
Collaborator

It seems that it's too late to change overflow away from "x y" ordering, regardless of whether it's a good idea.

I think the follow-up question is, can we / should we switch the order of scroll-snap-align to match overflow (at least in horizontal writing modes)? Or should it stay consistent with place-* (and inconsistent with overflow)?

Fwiw, the other 2-value properties that are following the "block inline" order are: grid/grid-template, gap, place-content/place-self/place-items.

@frivoal
Copy link
Collaborator

frivoal commented Aug 11, 2021

Unless we want to resolve that we're happy with the status quo, we probably could do with a use counter on scroll-snap-align, to see if it's even possible to change it.

@dbaron
Copy link
Member

dbaron commented Aug 25, 2021

Do you need a use counter beyond https://chromestatus.com/metrics/css/popularity#scroll-snap-align ? 4.6% seems to suggest there's a pretty big chance it's not changeable.

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [css-overflow-3] 'overflow' 2-value syntax is in wrong order, and agreed to the following:

  • RESOLVED: scroll-snap-align and future 2-axis properties use the logical model
The full IRC log of that discussion <dael> Topic: [css-overflow-3] 'overflow' 2-value syntax is in wrong order
<dael> github: https://github.com//issues/2988
<dael> astearns: We talked about this once
<dael> tab
<dael> TabAtkins: Thread conclusion is it was too late to change to block inline ordering. Stick to horz vertical
<dael> TabAtkins: Designed scroll snap align to match. The question, then, is if we can or should switch scroll-snap-align to match or keep with other logical properties
<dael> TabAtkins: fantasai said in issue we have two properties using block already. I suspect right answer is overflow is an unfortunate legacy and leave s-s-a to line up with the logical and suspect other values will be logical in future
<dael> TabAtkins: My preference is s-s-a and future 2 value directional shorthands are logical
<dael> astearns: Comment from David about s-s-a- usage being relatively high
<heycam> q+
<dael> TabAtkins: I was saying we should not change
<dael> TabAtkins: Keep all the current properties logical and align s-s-a with those
<astearns> ack heycam
<dael> heycam: A long time ago there were proposals to have keywords to opt into logical. Would that smooth over the edges of properties we can't change so authors can stay in logical if they stick the keyword in
<dael> TabAtkins: Very possibly. I'm not deep into lore of why we haven't gone with that. Hvae to ask editor on logical properties spec. Could be an encouragement for that model
<dael> TabAtkins: I suggest we resolve s-s-a and future 2-axis properties use the logical model
<dael> astearns: Looking back to see what we resolved originally
<dael> astearns: Logical is what s-s-a uses currently, yes?
<dael> TabAtkins: Yes
<dael> astearns: Any more discussion about the proposal?
<dael> astearns: Prop: s-s-a and future 2-axis properties use the logical model
<dael> astearns: This could be ameliorated by a switch to logical
<dael> astearns: Obj?
<dael> RESOLVED: scroll-snap-align and future 2-axis properties use the logical model

@lilles
Copy link
Member

lilles commented May 2, 2022

Should this issue be closed with no change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
css-overflow-3 Current Work
Projects
None yet
Development

No branches or pull requests