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-box-4][css-backgrounds-4][css-shapes-1] Cleanup <*-box> definitions #9505

Merged
merged 6 commits into from
Nov 10, 2023

Conversation

cdoublev
Copy link
Collaborator

Follow-up on 7dc439c, fixing speced/bikeshed#2679.

The commit removed the <box> production (which became undefined) and replaced <box> references with <visual-box>, but some <box> references remained in other specs (including CSS Box, which defines <visual-box>).

@cdoublev
Copy link
Collaborator Author

Hmm, <visual-box>/fill-box and other links are incorrect. I am not sure how to fix them, except by re-introducing <box> = <visual-box> | <layout-box> | ....

@cdoublev cdoublev marked this pull request as draft October 20, 2023 10:01
@cdoublev
Copy link
Collaborator Author

Ok, I reverted the namespace change, and introduced an informal <box> definition (without a production rule).

I also simplified the production rules to also fix the namespaces defined for each individual keyword.

CSS Shapes 1 defines <shape-box> with the same keywords than <layout-box>. I leave it up to the spec authors to replace it with <layout-box>, as well as in CSS Masking 1, which includes <shape-box> in <geometry-box>.

As a recap with flattened production rules:

  <visual-box> = content-box | padding-box | border-box
  <layout-box> = content-box | padding-box | border-box | margin-box
   <shape-box> = content-box | padding-box | border-box | margin-box
<geometry-box> = content-box | padding-box | border-box | margin-box | fill-box | stroke-box | view-box
   <paint-box> = content-box | padding-box | border-box | fill-box | stroke-box
   <coord-box> = content-box | padding-box | border-box | fill-box | stroke-box | view-box

Proposed:

  <visual-box> = content-box | padding-box | border-box
  <layout-box> = <visual-box> | margin-box
   <shape-box> = <visual-box> | margin-box
<geometry-box> = <shape-box> | fill-box | stroke-box | view-box
  <paint-box>  = <visual-box> | fill-box | stroke-box
  <coord-box>  = <paint-box> | view-box

Ideally (imo):

  <visual-box> = content-box | padding-box | border-box
  <layout-box> = <visual-box> | margin-box
   <paint-box> = <visual-box> | fill-box | stroke-box
   <coord-box> = <paint-box> | view-box
<geometry-box> = <layout-box> | fill-box | stroke-box | view-box

@cdoublev cdoublev marked this pull request as ready for review October 20, 2023 12:44
@cdoublev cdoublev changed the title [css-box-4][css-backgrounds-4][css-shapes-1] Replace <box> definition with <visual-box> [css-box-4][css-backgrounds-4][css-shapes-1] Cleanup <*-box> definitions Oct 20, 2023
@w3cbot
Copy link

w3cbot commented Oct 20, 2023

svgeesus marked as non substantive for IPR from ash-nazg.

@tabatkins
Copy link
Member

Your for values are broken. Note that Bikeshed doesn't actually process for values in any way, they're just opaque strings; the fact that <coord-box> includes <visual-box> doesn't automatically make the values that are for="<visual-box>" also for <coord-box>, etc.

So you need to accumulate all the <*-box> variants that include each keyword in the for values for that keyword.

@cdoublev
Copy link
Collaborator Author

the fact that <coord-box> includes <visual-box> doesn't automatically make the values that are for="<visual-box>" also for <coord-box>, etc.

I think this is the way w3c/reffy will process it, assuming <visual-box> is defined for <coord-box> (and other productions), which I forgot to define.

Here is the (simplified) representation of the data I expect to find in w3c/webref:
  "values": [
    {
      "name": "box",
      "values": [
        {
          "name": "coord-box",
          "value": "<paint-box> | view-box",
          "values": [
            {
              "name": "paint-box",
              "value": "<visual-box> | fill-box | stroke-box",
              "values": [
                {
                  "name": "visual-box",
                  "value": "content-box | padding-box | border-box",
                  "values": [{ "name": "content-box" }, { "name": "padding-box" }, { "name": "border-box" }]
                },
                { "name": "fill-box" },
                { "name": "view-box" }
              ]
            }
          ]
        },
        {
          "name": "geometry-box",
          "value": "<layout-box> | fill-box | stroke-box | view-box",
          "values": [
            {
              "name": "layout-box",
              "value": "<visual-box> | margin-box",
              "values": [
                {
                  "name": "visual-box",
                  "value": "content-box | padding-box | border-box",
                  "values": [{ "name": "content-box" }, { "name": "padding-box" }, { "name": "border-box" }]
                },
                { "name": "margin-box" }
              ]
            },
            { "name": "fill-box" },
            { "name": "stroke-box" },
            { "name": "view-box" }
          ]
        },
        {
          "name": "layout-box",
          "value": "<visual-box> | margin-box",
          "values": [
            {
              "name": "visual-box",
              "value": "content-box | padding-box | border-box",
              "values": [{ "name": "content-box" }, { "name": "padding-box" }, { "name": "border-box" }],
            },
            { "name": "margin-box" }
          ]
        },
        {
          "name": "paint-box",
          "value": "<visual-box> | fill-box | stroke-box",
          "values": [
            {
              "name": "visual-box",
              "value": "content-box | padding-box | border-box",
              "values": [{ "name": "content-box" }, { "name": "padding-box" }, { "name": "border-box" }],
            },
            { "name": "fill-box" },
            { "name": "view-box" }
          ]
        },
        {
          "name": "visual-box",
          "value": "content-box | padding-box | border-box",
          "values": [{ "name": "content-box" }, { "name": "padding-box" }, { "name": "border-box" }],
        },
      ]
    },
  ]

Would it be ok? Cc @tidoust for a review.

Actually, I do not care about the context (namespace) of a production because I am only interested by its syntax (value), assuming I am right to consider it context-insensitive.

And I am not sure it is right to restrict eg. <visual-box> for all other <-*box> productions. One may think that <visual-box> cannot be used outside of these contexts.

I do not even need <box> references to be removed from CSS Box. I just need the changes in CSS Backgrounds 4 and CSS Shapes. So feel free to make whatever changes you think are appropriate.

@cdoublev
Copy link
Collaborator Author

cdoublev commented Nov 6, 2023

I went ahead and defined each keyword with the appropriate contexts. Bikeshed does not report any errors.

@tabatkins tabatkins merged commit 798ba91 into w3c:main Nov 10, 2023
1 check passed
@cdoublev cdoublev deleted the pr-9505 branch November 11, 2023 10:33
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.

None yet

3 participants