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

Remove invalid references to margin-box #28157

Merged
merged 5 commits into from
Aug 3, 2023
Merged

Remove invalid references to margin-box #28157

merged 5 commits into from
Aug 3, 2023

Conversation

FormularSumo
Copy link
Contributor

@FormularSumo FormularSumo commented Jul 24, 2023

Description

Removed incorrect references and descriptions of the margin-box property

Motivation

margin-box was removed from mask-origin and mask-clip in the 5 August 2021 CSS Masking Module Level 1 Candidate Recommendation

Additional details

https://drafts.fxtf.org/css-masking-1/#changes
https://bugzilla.mozilla.org/show_bug.cgi?id=1823257
web-platform-tests/wpt#41166

margin-box was removed in the 2021 5 August 2021 Candidate Reccomendation, see https://drafts.fxtf.org/css-masking-1/#changes
@FormularSumo FormularSumo requested a review from a team as a code owner July 24, 2023 23:27
@FormularSumo FormularSumo requested review from chrisdavidmills and removed request for a team July 24, 2023 23:27
@github-actions github-actions bot added the Content:CSS Cascading Style Sheets docs label Jul 24, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jul 25, 2023

Copy link
Contributor

@chrisdavidmills chrisdavidmills left a comment

Choose a reason for hiding this comment

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

Thanks for the update, @FormularSumo . Your changes look correct, so I'm approving the PR.

The one comment I have is that I looked around the site for other mentions of margin-box, and I think there might be other places where it is no longer a valid value too — this looked to be the case with clip-path (in Chromium at least), box-sizing (this page implies it still is), and offset-path (in Firefox and Chromium).

I wondered if you wanted to investigate these and correct them as well, in the same PR?

@FormularSumo
Copy link
Contributor Author

Ah I hadn't realised there might be others, but I'm happy to take a look! Will add anything else I find to this pull request and let you know when that's done.

@FormularSumo
Copy link
Contributor Author

FormularSumo commented Jul 27, 2023

I searched the GitHub repo for mentions of margin-box and found 12, including mask-origin and mask-clip and the 3 you mentioned. My full research is here https://docs.google.com/document/d/14gyDzYAsjyP7--8PGQHFawJiI-BREJ2bMXxiQnNt7NM/edit?usp=drivesdk but the conclusion is I think all of them except for mark-origin and mask-clip are correct, however there are 2 which could be improved.

offset-path is the first one. The relevent W3 spec is this one https://www.w3.org/TR/motion-1/#propdef-offset-path. Its value can be none | ray() | path() | <url> | [ <basic-shape> || <geometry-box> ]. Clicking on <geometry-box> takes you to css masking where the possible values are <shape-box> | fill-box | stroke-box | view-box. Clicking on <shape-box> shows the possible values as being margin-box | border-box | padding-box | content-box, same as is found in most of the other pages referencing margin-box and using <geometry-box>.
So I think that means offset-path can be any of those 7 values. The W3 spec I linked to has examples of it being set to margin-box and padding-box which would support that. The MDN page has a Syntax section and Formal Syntax section. The Syntax one only has margin-box and stroke-box as options for <geometry-box>. Meanwhile the Formal Syntax section has 6 of the 7 values listed under <coord-box>, missing margin-box. My understanding is that both of these sections should contain all 7 of the possible values underneath the heading/comment <geometry-box>. I'm happy to make these changes but I don't know too much about this so would like someone else to review/confirm this first.

The other one is very minor, on introduction-to-the-css-box-model, the term margin-box is used to mean the area of the margin. On the W3 spec https://www.w3.org/TR/css-box-4/#margin-area it describes the area contained in the margin edges as margin box, without a dash, which I think makes more sense given it's describing the area of a margin rather than the property margin-box used in <geometry-box>, but again I'm not too sure about this so would appreciated a second opinion.

Also I was wondering, what do you mean by not valid only in Chromium or Firefox? Doesn't the content of mdn pages apply to all browsers (where supported)?

@chrisdavidmills
Copy link
Contributor

Also I was wondering, what do you mean by not valid only in Chromium or Firefox? Doesn't the content of mdn pages apply to all browsers (where supported)?

Sorry yeah, by this I meant that I tested it in the latest browser versions, and in those cases it came up in the devtools as being an unrecognised value.

The spec I'm looking at for offset-path is the latest editor's draft, at https://drafts.fxtf.org/motion-1/#offset-path-property, which is way more up to date than the one you are linking.

@FormularSumo
Copy link
Contributor Author

I've looked through all of them again using the editors drafts this time (updated links in the document I linked). The only one that seems to have changed its use of margin-box is offset-path. <geometry-box> looks like it's been replaced with <coord-box>, which doesn't have margin-box as a possible value (but still has the other 6). So going back to the MDN page, the Formal Syntax section looks correct as it has those 6 values and no margin-box. However the first Syntax section still has

/* Geometry box */
offset-path: margin-box;
offset-path: stroke-box;

which I now think should be more like

/* Coord box */
offset-path: content-box;
offset-path: padding-box;
offset-path: border-box;
offset-path: fill-box;
offset-path: stroke-box;
offset-path: view-box;

Does that look correct to you?

@chrisdavidmills
Copy link
Contributor

chrisdavidmills commented Aug 2, 2023

Does that look correct to you?

Yes. For fixing the margin-box issue on the page, that looks like the correct thing to do (there are other basic-shape values not covered, see https://drafts.csswg.org/css-shapes-1/#supported-basic-shapes, but that seems out of scope for this PR, so I think that could be handled separately)

Geometry box is from an old spec, and has since been replaced with Coord box. Additionally only some of the possible values were listed, now all of them are
In https://drafts.csswg.org/css-box-4/#margin-edge the area of the margin edges is described as the margin box, without a dash. margin-box is used elsewhere in https://drafts.csswg.org/css-box-4/#keywords as a keyword for some box edges. I think the use of it on this page is much more similar to the first example so probably shouldn't use a dash.
@FormularSumo FormularSumo changed the title Remove references to margin-box Remove invalid references to margin-box Aug 2, 2023
@FormularSumo
Copy link
Contributor Author

FormularSumo commented Aug 2, 2023

I've fixed the margin-box issues on that page, and also made the minor change to introduction-to-the-css-box-model that I mentioned above. I see what you mean about some of the other basic shape values missing. ellipse(), and rect()/xywh() - although those ones seem to be missing from other pages as well. As you say, outside of this PR.

Edit, seems like issues have already been created for adding rect() and xywh() - see #28287 and #28286

@FormularSumo
Copy link
Contributor Author

Do you know how I would remove margin-box from the Formal Syntax section of mask-origin and mask-clip? In the source code they just have {{csssyntax}} and I'm not sure where that data is pulled from.

Copy link
Contributor

@chrisdavidmills chrisdavidmills left a comment

Choose a reason for hiding this comment

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

Perfect, I think this PR is wrapped up for now; thanks for the contribution, @FormularSumo!

@chrisdavidmills chrisdavidmills merged commit 28505c5 into mdn:main Aug 3, 2023
6 checks passed
@chrisdavidmills
Copy link
Contributor

Do you know how I would remove margin-box from the Formal Syntax section of mask-origin and mask-clip? In the source code they just have {{csssyntax}} and I'm not sure where that data is pulled from.

@FormularSumo The formal syntax data is pulled from the definitions available in https://github.com/mdn/data, specifically in the css/properties.json file; see mask-clip for example.

Ah, but margin-box is part of the <shape-box> type definition, which is part of <geometry-box>, so you'd have to remove it from there.

The trouble is, I don't know whether this would then cause other overall property definitions to be wrong, so this would require some investigation. It is a bit of an unstable house of cards.

@wbamberg
Copy link
Collaborator

Do you know how I would remove margin-box from the Formal Syntax section of mask-origin and mask-clip? In the source code they just have {{csssyntax}} and I'm not sure where that data is pulled from.

@FormularSumo The formal syntax data is pulled from the definitions available in https://github.com/mdn/data, specifically in the css/properties.json file; see mask-clip for example.

Not any more: since mdn/yari#4656 formal syntax is pulled from the specs, via the W3C's @webref/css package. The "syntax" bit of mdn/data isn't used in MDN any more (and if we could figure it out, we'd use webref for the other stuff in mdn/data).

Ah, but margin-box is part of the <shape-box> type definition, which is part of <geometry-box>, so you'd have to remove it from there.

The trouble is, I don't know whether this would then cause other overall property definitions to be wrong, so this would require some investigation. It is a bit of an unstable house of cards.

FormularSumo added a commit to FormularSumo/content that referenced this pull request Dec 31, 2023
Since w3c/fxtf-drafts#439 and the related pull request were closed, mask-clip uses <coord-box> instead of <geometry-box>. This was mostly fixed in this mdn#28157 previous pull request but I forgot to update this comment as well.
teoli2003 pushed a commit that referenced this pull request Dec 31, 2023
Since w3c/fxtf-drafts#439 and the related pull request were closed, mask-clip uses <coord-box> instead of <geometry-box>. This was mostly fixed in this #28157 previous pull request but I forgot to update this comment as well.
dipikabh pushed a commit to dipikabh/content that referenced this pull request Jan 17, 2024
Since w3c/fxtf-drafts#439 and the related pull request were closed, mask-clip uses <coord-box> instead of <geometry-box>. This was mostly fixed in this mdn#28157 previous pull request but I forgot to update this comment as well.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:CSS Cascading Style Sheets docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants