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-logical-1] Interaction of shorthands and logical properties #3030

Closed
fantasai opened this issue Aug 17, 2018 · 13 comments
Closed

[css-logical-1] Interaction of shorthands and logical properties #3030

fantasai opened this issue Aug 17, 2018 · 13 comments

Comments

@fantasai
Copy link
Collaborator

Relevant www-style threads:
https://lists.w3.org/Archives/Public/www-style/2014Dec/0319.html
https://lists.w3.org/Archives/Public/www-style/2014Dec/0369.html

@fantasai fantasai added the css-logical-1 Current Work label Aug 17, 2018
@FremyCompany
Copy link
Contributor

FremyCompany commented Aug 18, 2018

Proposal:

During parsing of a css declaration block, shorthands (like margin and all) expands only to the set of longhand (grouped by mapping logic: so either logical or physical) required to describe their value, and by default resort to use physical properties if both sets are possible. Note that they can only expand if they do not contain var() substitutions.

During serialization, each logical property group is considered, and shorthand values can be emitted instead of the longhands only if the all the longhands of the same mapping logic are present and consecutive (ignoring all properties that are not in the group). Otherwise, the shorthand is not used when serializing the properties in the group.

During serialization, a specific rule should be added in case all longhand properties of one mapping logic are defined, and are followed by all longhand properties of the other mapping logic (this would result in the shorthand being emitted twice, in which case the rule would discard its first appearance)

ex:

margin-top: 0px; margin-left: 0px; margin-bottom: 0px; margin-right: 0px;
margin-inline-start: 10px; margin-block-start: 10px; margin-inline-end: 10px; margin-inline-end: 10px

should serialize as margin: logical 10px (if supported) or just margin-inline-start: 10px; margin-block-start: 10px; margin-inline-end: 10px; margin-inline-end: 10px (otherwise), ignoring the set of longhand properties of physical mapping logic whose all values are to be ignored in the end.

When removing a shorthand property, all the longhand associated with that shorthand should be removed, regardless of their mapping kind.

@FremyCompany
Copy link
Contributor

(all not setting the logical properties only works however if margin-inline-start: inherit has the exact same behavior as margin-left: inherit and does not inherit from margin-inline-start from the parent if that parent has a different writing mode)

@FremyCompany
Copy link
Contributor

FremyCompany commented Aug 18, 2018

proposed text for the statement above would be something like:

logical properties cannot inherit.
when a logical property should inherit, the physical property it maps to must inherit instead

@dbaron
Copy link
Member

dbaron commented Aug 18, 2018

I'm reasonably happy with @FremyCompany's proposal above. One thought about another option is that we could say that appending a shorthand property could set all of its constituent shorthands of one mapping logic and also remove all of the constituent shorthands of the other. We could even go further and do that same removal during serialization.

I'd also note that we need to consider shorthands (e.g., border) where some of the longhands form logical property groups but some do not (the longhands of border-image). (border is also special because its longhands include three logical property groups, for border-color, border-style, and border-width.)

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed CSS Logical Properties, and agreed to the following:

  • RESOLVED: proposal from fremy: During serialization, each logical property group is considered, and shorthand values can be emitted instead of the longhands only if the all the longhands of the same mapping logic are present and consecutive (ignoring all properties that are not in the group). Otherwise, the shorthand is not used when serializing the properties in the group.
  • RESOLVED: proposal from fremy:During serialization, each logical property group is considered, and shorthand values can be emitted instead of the longhands only if the all the longhands of the same mapping logic are present and consecutive (ignoring all properties that are not in the group). Otherwise, the shorthand is not used when serializing the properties in the group.
The full IRC log of that discussion <fantasai> Topic: CSS Logical Properties
<fantasai> github: https://github.com//issues/3030
<Rossen_> github: https://github.com//issues/3030
<tantek> fremy: main issue is that in CSS spec it defines how to serialize properties, contains a lot of tricks to merge declarations
<tantek> ... e.g. if you set all four border properties, the rules merge it to a single declaration
<tantek> ... when I looked at the logical properties, this is not as easy
<tantek> ... even if you have all four, you cannot always merge them into a single property
<tantek> ... it means you have to detect this case (e.g. inline-start) and do something smart
<tantek> ... logical and physical properties
<tantek> ... proposing a couple of rules to the serialization steps
<tantek> ... to only merge all four if they are of the same type
<tantek> ... while we were looking at this, a few more updates
<tantek> ... e.g. dbaron proposed if you set the margin property on the style attribute
<tantek> ... there is no reason to keep the margin-inline properties
<tantek> ... the logical ones
<fantasai> (because they have all now been overridden)
<tantek> ... proposal: make sure that the grouping would work
<tantek> ... making sure when you set things you override propertly
<emilio> q+
<tantek> s/propertly/properly
<tantek> emilio: Gecko has the concept of logical property groups
<tantek> ... we added it to the spec
<tantek> ... this concept already exists
<tantek> ... e.g. border-inline-start:10px; border-start:20px;
<tantek> ... Gecko will do the right thing
<tantek> ... the concept is already in the spec. this is an obvious bugfix
<tantek> fremy: the link you sent sounds like exactly what I'm proposing that seems fine
<Rossen_> See the refs for https://drafts.csswg.org/css-logical-1/#logical-property-group
<tantek> s/the link you sent/emilio's link https://drafts.csswg.org/css-logical-1/#logical-property-group
<tantek> fantasai: we should make sure ... interleaved ... then that can be folded into a shorthand
<emilio> https://drafts.csswg.org/cssom/#set-a-css-declaration is already using this concept
<emilio> We should use it from serialization
<tantek> ... the other part of your proposal, if a shorthand is set, then you can drop any previous declarations that set any property in that logical property group
<tantek> fremy: that sounds correct to me
<tantek> Rossen_: do we not have this currently specified? the part you just added fantasai ? in terms of the behavior?
<tantek> fantasai: I think in terms of how everything is supposed to behave like cascade, etc. is defined. this is about CSSOM interaction
<tantek> Rossen_: so this is going into CSSOM then? mostly?
<tantek> fantasai: I guess so. emilio and I will work it out
<tantek> Rossen_: any other opinions?
<tantek> emilio: I need to look a bit more closely at the replace all the physical longhands
<tantek> ... because there are more complex cases
<tantek> ... e.g. shorthands that only override two properties
<tantek> ... like the serialization stuff when stuff is interleaved, that's good
<tantek> ... the replace of the existing physical properties, that may or may not be confusing
<tantek> fantasai: I think they are two separate proposals that happen to be in the same issue
<tantek> Rossen_: can you separate them for resolutions?
<fantasai> First proposal from fremy: During parsing of a css declaration block, shorthands (like margin and all) expands only to the set of longhand (grouped by mapping logic: so either logical or physical) required to describe their value, and by default resort to use physical properties if both sets are possible. Note that they can only expand if they do not contain var() substitutions.
<fantasai> Second proposal from fremy: During serialization, each logical property group is considered, and shorthand values can be emitted instead of the longhands only if the all the longhands of the same mapping logic are present and consecutive (ignoring all properties that are not in the group). Otherwise, the shorthand is not used when serializing the properties in the group.
<tantek> Rossen_: please read the first one, which is about the behavior specifying, and how declaration blocks behave between shorthands and longhands and logical properties
<tantek> fantasai: looks like I'm finding more pieces of it
<tantek> emilio: the first one I assume every browser already does that
<emilio> s/emilio/fremy
<tantek> ... it's not clear from the spec, but I assume that should work regardless
<fantasai> Third piece from fremy: When removing a shorthand property, all the longhand associated with that shorthand should be removed, regardless of their mapping kind.
<TabAtkins> +1, all sound good
<fantasai> Fourth piece from dbaron: appending a shorthand property could set all of its constituent shorthands of one mapping logic and also remove all of the constituent shorthands of the other.
<tantek> Rossen_: any objections to accepting this first proposal
<tantek> emilio: q, we are talking about shorthands that map to both physical and logical properties? but those do not exist in browser today
<tantek> fremy: it's possible, back them we had proposal for margin, I don't know if we still have that
<tantek> fantasai: we never figured out a syntax that was appropriate
<tantek> fremy: that was mostly to make sure that case worked
<tantek> ... I don't think it's wrong to add this even if we don't have the mechanism
<tantek> emilio: not saying it's wrong, just possibly confusing
<tantek> how would you test it?
<tantek> RESOLVED: proposal from fremy: During serialization, each logical property group is considered, and shorthand values can be emitted instead of the longhands only if the all the longhands of the same mapping logic are present and consecutive (ignoring all properties that are not in the group). Otherwise, the shorthand is not used when serializing the properties in the group.
<tantek> fremy: mostly the loop needs to be modified
<tantek> Rossen_: wanting make sure everyone is comfortable with accepting this change
<emilio> yeah, +1 for the serialization change, that's unobjectionable imo
<fremy> https://www.w3.org/TR/cssom-1/#serialize-a-css-declaration-block
<tantek> ... without delaying people too much, any objections or wait til Thu morning?
<tantek> fremy: for me both is fine
<Rossen_> RESOLVED: proposal from fremy:During serialization, each logical property group is considered, and shorthand values can be emitted instead of the longhands only if the all the longhands of the same mapping logic are present and consecutive (ignoring all properties that are not in the group). Otherwise, the shorthand is not used when serializing the properties in the group.

emilio added a commit that referenced this issue Oct 23, 2020
… property group / mapping logic in between the longhands of that shorthand.

Per #3030.
fantasai pushed a commit that referenced this issue Oct 23, 2020
… property group / mapping logic in between the longhands of that shorthand.

Per #3030.
@emilio
Copy link
Collaborator

emilio commented Nov 5, 2020

Spec edits have been made, https://bugzilla.mozilla.org/show_bug.cgi?id=1675514 adds tests and fixes Gecko to follow the new spec text.

@emilio emilio closed this as completed Nov 5, 2020
@Loirooriol
Copy link
Contributor

@emilio Not sure if I get it the spec text right, taking for example this declaration block:

margin-top: 10px;
margin-right: 10px;
margin-left: 10px;
margin-inline-start: 10px;
margin-block-start: 10px;
margin-block-end: 10px;
margin-inline-end: 10px;
margin-bottom: 10px

So we reach the new step with

property = margin-top
already-serialized = []
shorthands = [margin, all]
shorthand = margin
longhands = [margin-top, margin-right, margin-left, margin-inline-start,
             margin-block-start, margin-block-end, margin-inline-end, margin-bottom]
current-longhands = [...longhands]

Then, we have mixed mapping logic, but all declarations in declaration block are in current longhands, so it seems to me we would serialize using margin?

@emilio
Copy link
Collaborator

emilio commented Nov 5, 2020

@Loirooriol how so? if shorthand is margin, only margin-top, margin-right, margin-left and margin-bottom should be in current longhands, right? What makes the logical properties end up in there? They're not mapped to margin... and yeah, the "mapping" terminology used in that section of the spec is not great, but basically current longhands is meant to be "the longhands shorthand expands to", which for margin doesn't include the logical properties.

@Loirooriol
Copy link
Contributor

@emilio Didn't we decide this:

During parsing of a css declaration block, shorthands (like margin and all) expands only to the set of longhand (grouped by mapping logic: so either logical or physical) required to describe their value

So margin is a shorthand of both the physical and the logical longhands, and the set of properties it expands to depends on the value. (Of course that's not what implementations do, but I think is how it's supposed to be?)

@emilio
Copy link
Collaborator

emilio commented Nov 5, 2020

I don't think we resolved on that, did we? The only shorthand that includes both logical and physical properties is all, and the order of its expansion should be defined in that case, but the algorithm would handle that correctly afaict.

@Loirooriol
Copy link
Contributor

Well that was the "First proposal from fremy" and I think we resolved it, though in the "RESOLVED" the wrong text was copied (so both resolutions are identical)

@emilio
Copy link
Collaborator

emilio commented Nov 5, 2020

So even if we resolved on this:

First proposal from fremy: During parsing of a css declaration block, shorthands (like margin and all) expands only to the set of longhand (grouped by mapping logic: so either logical or physical) required to describe their value, and by default resort to use physical properties if both sets are possible. Note that they can only expand if they do not contain var() substitutions.

still it seems the new spec text isn't wrong as long as margin is defined to be a shorthand for only physical properties... If / when that changes we might want to revisit that.

I still think the above proposal is wrong. all should expand to both logical and physical properties IMO.

@FremyCompany
Copy link
Contributor

So even if we resolved on this:

First proposal from fremy: During parsing of a css declaration block, shorthands (like margin and all) expands only to the set of longhand (grouped by mapping logic: so either logical or physical) required to describe their value, and by default resort to use physical properties if both sets are possible. Note that they can only expand if they do not contain var() substitutions.

still it seems the new spec text isn't wrong as long as margin is defined to be a shorthand for only physical properties... If / when that changes we might want to revisit that.

I still think the above proposal is wrong. all should expand to both logical and physical properties IMO.

On second thoughts, I did not consider all very well in my statement. I thought only of hypothetical properties like margin that would cover both logical and physical longhands part of the same group. In the case of all, it is more difficult to figure that out, because that would potentially depend for each group (could be physical for margin, but logical for padding).

moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 8, 2020
As per w3c/csswg-drafts#3030.

Also remove a system font special-case. It's generally handled in
shorthands/font.mako.rs, and when it's not, it's really the same issue
as w3c/csswg-drafts#2515.

Browsers are all over the place for the system font case, so I think
it's fine.

Differential Revision: https://phabricator.services.mozilla.com/D96045

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1675514
gecko-commit: 6dcbbdb84c453149f460f6868eeb135d6622c57d
gecko-reviewers: boris
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Nov 8, 2020
…ies. r=boris

As per w3c/csswg-drafts#3030.

Also remove a system font special-case. It's generally handled in
shorthands/font.mako.rs, and when it's not, it's really the same issue
as w3c/csswg-drafts#2515.

Browsers are all over the place for the system font case, so I think
it's fine.

Differential Revision: https://phabricator.services.mozilla.com/D96045
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 8, 2020
As per w3c/csswg-drafts#3030.

Also remove a system font special-case. It's generally handled in
shorthands/font.mako.rs, and when it's not, it's really the same issue
as w3c/csswg-drafts#2515.

Browsers are all over the place for the system font case, so I think
it's fine.

Differential Revision: https://phabricator.services.mozilla.com/D96045

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1675514
gecko-commit: 6dcbbdb84c453149f460f6868eeb135d6622c57d
gecko-reviewers: boris
sidvishnoi pushed a commit to sidvishnoi/gecko-webmonetization that referenced this issue Nov 10, 2020
…ies. r=boris

As per w3c/csswg-drafts#3030.

Also remove a system font special-case. It's generally handled in
shorthands/font.mako.rs, and when it's not, it's really the same issue
as w3c/csswg-drafts#2515.

Browsers are all over the place for the system font case, so I think
it's fine.

Differential Revision: https://phabricator.services.mozilla.com/D96045
emilio added a commit to servo/servo that referenced this issue Feb 26, 2021
As per w3c/csswg-drafts#3030.

Also remove a system font special-case. It's generally handled in
shorthands/font.mako.rs, and when it's not, it's really the same issue
as w3c/csswg-drafts#2515.

Browsers are all over the place for the system font case, so I think
it's fine.

Differential Revision: https://phabricator.services.mozilla.com/D96045
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants