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

[cssom] [css-fonts] [css-page] Sort out properties and descriptors. #9686

Merged
merged 1 commit into from
Mar 20, 2024

Conversation

emilio
Copy link
Collaborator

@emilio emilio commented Dec 8, 2023

This is intended to address #5649.

@emilio
Copy link
Collaborator Author

emilio commented Dec 8, 2023

@tabatkins: Bikeshed doesn't seem to be liking the dashed attribute syntax, even though I think per the WebIDL grammar is correct. E.g., this diff on top of this PR:

diff --git a/cssom-1/Overview.bs b/cssom-1/Overview.bs
index a2ac5e91e..fdefb5521 100644
--- a/cssom-1/Overview.bs
+++ b/cssom-1/Overview.bs
@@ -2120,7 +2120,7 @@ Issue: Need to define the rules for
 [Exposed=Window]
 interface CSSPageDescriptors : CSSStyleDeclaration {
   attribute [LegacyNullToEmptyString] CSSOMString margin;
   attribute [LegacyNullToEmptyString] CSSOMString marginTop;
+  attribute [LegacyNullToEmptyString] CSSOMString margin-top;
   attribute [LegacyNullToEmptyString] CSSOMString marginRight;
   attribute [LegacyNullToEmptyString] CSSOMString marginBottom;
   attribute [LegacyNullToEmptyString] CSSOMString marginLeft;

Causes:

FATAL ERROR: IDL SYNTAX ERROR LINE: 4 - expected ";" skipped: "-top"
 ✘  Did not generate, due to errors exceeding the allowed error level.

Am I missing something?

@emilio emilio requested a review from svgeesus December 8, 2023 12:52
Copy link
Contributor

@svgeesus svgeesus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall much cleaner than the current mess.

See comment about the new descriptors in CSS Fonts 5.

css-fonts-4/Overview.bs Outdated Show resolved Hide resolved
css-fonts-4/Overview.bs Outdated Show resolved Hide resolved
attribute [LegacyNullToEmptyString] CSSOMString ascentOverride;
attribute [LegacyNullToEmptyString] CSSOMString descentOverride;
attribute [LegacyNullToEmptyString] CSSOMString lineGapOverride;
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should fonts 5 have the same interface too except adding the size-adjust descriptor and the subscript-* descriptors as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

@@ -1052,7 +1052,7 @@ IDL Definition</h4>
[Exposed=Window]
interface CSSKeyframeRule : CSSRule {
attribute CSSOMString keyText;
[SameObject, PutForwards=cssText] readonly attribute CSSStyleDeclaration style;
[SameObject, PutForwards=cssText] readonly attribute CSSStyleProperties style;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the style of keyframe rules should have its own interface to prevent declaring a property defined in CSS Animations?

The <declaration-list> inside of <keyframe-block> accepts any CSS property except those defined in this specification

https://drafts.csswg.org/css-animations-1/#at-ruledef-keyframes

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly...

@tabatkins
Copy link
Member

Bikeshed doesn't seem to be liking the dashed attribute syntax,

Huh, weird that it's valid IDL, but you're right, technically it is, per https://webidl.spec.whatwg.org/#prod-identifier

This would be a bug on plinss/widlparser, then.

emilio added a commit to emilio/widlparser that referenced this pull request Dec 11, 2023
@emilio
Copy link
Collaborator Author

emilio commented Dec 11, 2023

This would be a bug on plinss/widlparser, then.

Thanks, sent plinss/widlparser#83 for that.

plinss pushed a commit to plinss/widlparser that referenced this pull request Dec 19, 2023
* Ignore virtualenv directory.

* tokenizer: Fix identifier regular expression.

This blocks w3c/csswg-drafts#9686

* test: Add tests for dashed identifiers.
attribute [LegacyNullToEmptyString] CSSOMString fontFamily;
attribute [LegacyNullToEmptyString] CSSOMString fontStyle;
attribute [LegacyNullToEmptyString] CSSOMString fontWeight;
attribute [LegacyNullToEmptyString] CSSOMString fontStretch;
Copy link
Collaborator

@cdoublev cdoublev Feb 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

font-stretch (descriptor) is now a legacy alias of font-width (b0660ee). I guess fontWidth should also be defined.

@svgeesus
Copy link
Contributor

@emilio @tabatkins Now that plinss/widlparser#83 has been merged is this ready to go, or does it still need changes?

Copy link
Contributor

@svgeesus svgeesus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be easier to merge this and then iterate on any refinements

cssom-1/Overview.bs Outdated Show resolved Hide resolved
Comment on lines 2119 to 2125
[Exposed=Window]
interface CSSPageDescriptors : CSSStyleDeclaration {
attribute [LegacyNullToEmptyString] CSSOMString margin;
attribute [LegacyNullToEmptyString] CSSOMString marginTop;
attribute [LegacyNullToEmptyString] CSSOMString marginRight;
attribute [LegacyNullToEmptyString] CSSOMString marginBottom;
attribute [LegacyNullToEmptyString] CSSOMString marginLeft;
attribute [LegacyNullToEmptyString] CSSOMString size;
attribute [LegacyNullToEmptyString] CSSOMString marks;
attribute [LegacyNullToEmptyString] CSSOMString bleed;
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a part of me that wonders if we actually what to define this, rather than "all descriptors supported for the @page at-rule", similar to what we do with properties? (Also CSSFontFaceDescriptors above.)

Like, if an implementation supported legacyDescriptor, do we not want it exposed on the interface? Or even a -webkit-legacy-descriptor, do we not want that exposed similarly?

@emilio
Copy link
Collaborator Author

emilio commented Mar 20, 2024

@tabatkins @svgeesus I updated now that the webidl parser is fixed, and it looks good from here. I should look into @gsnedders' suggestion about making descriptors more generically exposed in page and font-face descriptors, but I agree we can probably merge this and iterate on the specific text, because I don't know if legacy descriptors like -webkit-foo (is there any at all?) would be exposed using the WebKitFoo syntax or something else. Does that sound good?

@svgeesus svgeesus merged commit 2e8c502 into main Mar 20, 2024
1 check passed
@svgeesus svgeesus deleted the css-style-decl-split branch March 20, 2024 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants