[PEPC] Provide partial padding support to allow for common use case [chromium/src : main]

0 views
Skip to first unread message

Blink W3C Test Autoroller (Gerrit)

unread,
Apr 16, 2024, 10:21:51 AMApr 16
to Andy Paicu, Tricium, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org

Message from Blink W3C Test Autoroller

Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/45739.

When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.

WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process

Open in Gerrit

Related details

Attention set is empty
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I19e6f9ff6fcd11073b1da0efaa68f743ea3886bb
Gerrit-Change-Number: 5458152
Gerrit-PatchSet: 1
Gerrit-Owner: Andy Paicu <andy...@chromium.org>
Gerrit-Reviewer: Andy Paicu <andy...@chromium.org>
Gerrit-CC: Alexis Menard <alexis...@intel.com>
Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
Gerrit-Comment-Date: Tue, 16 Apr 2024 14:21:40 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Andy Paicu (Gerrit)

unread,
Apr 16, 2024, 4:14:34 PMApr 16
to Anders Hartvoll Ruud, David Baron, Blink W3C Test Autoroller, Tricium, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
Attention needed from Anders Hartvoll Ruud and David Baron

Andy Paicu added 1 comment

Patchset-level comments
File-level comment, Patchset 5 (Latest):
Andy Paicu . resolved

Hello Anders and David, PTAL.

David, am I right about the fact that `calc-size` can't be used in padding?
Anders, PTAL at the other CSS stuff.

Open in Gerrit

Related details

Attention is currently required from:
  • Anders Hartvoll Ruud
  • David Baron
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I19e6f9ff6fcd11073b1da0efaa68f743ea3886bb
Gerrit-Change-Number: 5458152
Gerrit-PatchSet: 5
Gerrit-Owner: Andy Paicu <andy...@chromium.org>
Gerrit-Reviewer: Anders Hartvoll Ruud <and...@chromium.org>
Gerrit-Reviewer: Andy Paicu <andy...@chromium.org>
Gerrit-Reviewer: David Baron <dba...@chromium.org>
Gerrit-CC: Alexis Menard <alexis...@intel.com>
Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
Gerrit-Attention: David Baron <dba...@chromium.org>
Gerrit-Attention: Anders Hartvoll Ruud <and...@chromium.org>
Gerrit-Comment-Date: Tue, 16 Apr 2024 20:14:23 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

David Baron (Gerrit)

unread,
Apr 16, 2024, 9:58:41 PMApr 16
to Andy Paicu, Anders Hartvoll Ruud, David Baron, Blink W3C Test Autoroller, Tricium, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
Attention needed from Anders Hartvoll Ruud and Andy Paicu

David Baron added 1 comment

Patchset-level comments
Andy Paicu . unresolved

Hello Anders and David, PTAL.

David, am I right about the fact that `calc-size` can't be used in padding?
Anders, PTAL at the other CSS stuff.

David Baron

You're correct that `calc-size()` can't be used in `padding`. But I'm not entirely sure what it is that you wanted to do with it. `calc()` can be used in `padding`. Would that have worked?

Or, taking an entirely different approach, would `box-sizing: border-box` have worked? (That does expose a slightly different developer-facing API, but that seems like it might be justified in this case if it helps the restrictions here make sense.)

Open in Gerrit

Related details

Attention is currently required from:
  • Anders Hartvoll Ruud
  • Andy Paicu
Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I19e6f9ff6fcd11073b1da0efaa68f743ea3886bb
    Gerrit-Change-Number: 5458152
    Gerrit-PatchSet: 5
    Gerrit-Owner: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Reviewer: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: David Baron <dba...@chromium.org>
    Gerrit-CC: Alexis Menard <alexis...@intel.com>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-Attention: Andy Paicu <andy...@chromium.org>
    Gerrit-Attention: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Comment-Date: Wed, 17 Apr 2024 01:58:31 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Andy Paicu <andy...@chromium.org>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    David Baron (Gerrit)

    unread,
    Apr 16, 2024, 10:06:01 PMApr 16
    to Andy Paicu, Anders Hartvoll Ruud, David Baron, Blink W3C Test Autoroller, Tricium, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
    Attention needed from Anders Hartvoll Ruud and Andy Paicu

    David Baron added 1 comment

    Patchset-level comments
    Andy Paicu . unresolved

    Hello Anders and David, PTAL.

    David, am I right about the fact that `calc-size` can't be used in padding?
    Anders, PTAL at the other CSS stuff.

    David Baron

    You're correct that `calc-size()` can't be used in `padding`. But I'm not entirely sure what it is that you wanted to do with it. `calc()` can be used in `padding`. Would that have worked?

    Or, taking an entirely different approach, would `box-sizing: border-box` have worked? (That does expose a slightly different developer-facing API, but that seems like it might be justified in this case if it helps the restrictions here make sense.)

    David Baron

    (That said, I think it would help me understand the general approach here if I could see a statement of what you're intending to allow vs. forbid with this change.)

    Gerrit-Comment-Date: Wed, 17 Apr 2024 02:05:53 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: David Baron <dba...@chromium.org>
    Comment-In-Reply-To: Andy Paicu <andy...@chromium.org>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Anders Hartvoll Ruud (Gerrit)

    unread,
    Apr 17, 2024, 5:01:08 AMApr 17
    to Andy Paicu, David Baron, Blink W3C Test Autoroller, Tricium, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
    Attention needed from Andy Paicu and David Baron

    Anders Hartvoll Ruud added 1 comment

    Patchset-level comments
    Andy Paicu . unresolved

    Hello Anders and David, PTAL.

    David, am I right about the fact that `calc-size` can't be used in padding?
    Anders, PTAL at the other CSS stuff.

    David Baron

    You're correct that `calc-size()` can't be used in `padding`. But I'm not entirely sure what it is that you wanted to do with it. `calc()` can be used in `padding`. Would that have worked?

    Or, taking an entirely different approach, would `box-sizing: border-box` have worked? (That does expose a slightly different developer-facing API, but that seems like it might be justified in this case if it helps the restrictions here make sense.)

    David Baron

    (That said, I think it would help me understand the general approach here if I could see a statement of what you're intending to allow vs. forbid with this change.)

    Anders Hartvoll Ruud

    I can't really recommend implementing your own interpretation of "padding", it must surely cause some inconsistencies with other parts of CSS.

    I assume we can't just impose some limit on the padding (in `em`) in this case?

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Andy Paicu
    • David Baron
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I19e6f9ff6fcd11073b1da0efaa68f743ea3886bb
    Gerrit-Change-Number: 5458152
    Gerrit-PatchSet: 5
    Gerrit-Owner: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Reviewer: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: David Baron <dba...@chromium.org>
    Gerrit-CC: Alexis Menard <alexis...@intel.com>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-Attention: David Baron <dba...@chromium.org>
    Gerrit-Attention: Andy Paicu <andy...@chromium.org>
    Gerrit-Comment-Date: Wed, 17 Apr 2024 09:00:58 +0000
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Andy Paicu (Gerrit)

    unread,
    Apr 17, 2024, 5:39:15 AMApr 17
    to Anders Hartvoll Ruud, David Baron, Blink W3C Test Autoroller, Tricium, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
    Attention needed from Anders Hartvoll Ruud and David Baron

    Andy Paicu added 1 comment

    Patchset-level comments
    Andy Paicu . unresolved

    Hello Anders and David, PTAL.

    David, am I right about the fact that `calc-size` can't be used in padding?
    Anders, PTAL at the other CSS stuff.

    David Baron

    You're correct that `calc-size()` can't be used in `padding`. But I'm not entirely sure what it is that you wanted to do with it. `calc()` can be used in `padding`. Would that have worked?

    Or, taking an entirely different approach, would `box-sizing: border-box` have worked? (That does expose a slightly different developer-facing API, but that seems like it might be justified in this case if it helps the restrictions here make sense.)

    David Baron

    (That said, I think it would help me understand the general approach here if I could see a statement of what you're intending to allow vs. forbid with this change.)

    Anders Hartvoll Ruud

    I can't really recommend implementing your own interpretation of "padding", it must surely cause some inconsistencies with other parts of CSS.

    I assume we can't just impose some limit on the padding (in `em`) in this case?

    Andy Paicu

    Vertical padding could be limited by `em` but horizontal padding max size depends on the content-size, which is why we implemented `min|max-width` with `calc-size`.

    So ignoring height for a second, since that can be bound by `1em`:

    The ultimate goal is to ensure that whatever padding is added to the element, the total width (border+padding+content) doesn't run over the `max-width` (which is 3*`fit-content` as defined in the previous CL). Also as an addition, the `span` element should always be centered in the `permission` element so padding should not be allowed to push the span to the side or out of the bounds of the `permission` element. That is why I basically want to put an upper bound on `padding-left|right` which is equal to the equivalent of k * `fit-content`. (Right now k = 1, but that could change in the future)

    I have tried playing around with `box-sizing: border-box` but that doesn't seem to put actual limits on the padding width: if `padding-left` + `padding-right` > `max-width` the element grows over max-width anyways. I've played around both with the `<permission>` element and divs and spans, but like I said I couldn't get it to work, if you have a working example I would be more than happy to chase that approach further. (I've also found various conflicting information on the web on whether `box-sizing: border-box` interacts correctly with `max-width` so I'm not exactly sure if it works reliably).

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Anders Hartvoll Ruud
    • David Baron
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I19e6f9ff6fcd11073b1da0efaa68f743ea3886bb
    Gerrit-Change-Number: 5458152
    Gerrit-PatchSet: 5
    Gerrit-Owner: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Reviewer: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: David Baron <dba...@chromium.org>
    Gerrit-CC: Alexis Menard <alexis...@intel.com>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-Attention: David Baron <dba...@chromium.org>
    Gerrit-Attention: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Comment-Date: Wed, 17 Apr 2024 09:39:06 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: David Baron <dba...@chromium.org>
    Comment-In-Reply-To: Andy Paicu <andy...@chromium.org>
    Comment-In-Reply-To: Anders Hartvoll Ruud <and...@chromium.org>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    David Baron (Gerrit)

    unread,
    Apr 17, 2024, 9:28:32 AMApr 17
    to Andy Paicu, Anders Hartvoll Ruud, David Baron, Blink W3C Test Autoroller, Tricium, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
    Attention needed from Anders Hartvoll Ruud and Andy Paicu

    David Baron added 1 comment

    Patchset-level comments
    Andy Paicu . unresolved

    Hello Anders and David, PTAL.

    David, am I right about the fact that `calc-size` can't be used in padding?
    Anders, PTAL at the other CSS stuff.

    David Baron

    You're correct that `calc-size()` can't be used in `padding`. But I'm not entirely sure what it is that you wanted to do with it. `calc()` can be used in `padding`. Would that have worked?

    Or, taking an entirely different approach, would `box-sizing: border-box` have worked? (That does expose a slightly different developer-facing API, but that seems like it might be justified in this case if it helps the restrictions here make sense.)

    David Baron

    (That said, I think it would help me understand the general approach here if I could see a statement of what you're intending to allow vs. forbid with this change.)

    Anders Hartvoll Ruud

    I can't really recommend implementing your own interpretation of "padding", it must surely cause some inconsistencies with other parts of CSS.

    I assume we can't just impose some limit on the padding (in `em`) in this case?

    Andy Paicu

    Vertical padding could be limited by `em` but horizontal padding max size depends on the content-size, which is why we implemented `min|max-width` with `calc-size`.

    So ignoring height for a second, since that can be bound by `1em`:

    The ultimate goal is to ensure that whatever padding is added to the element, the total width (border+padding+content) doesn't run over the `max-width` (which is 3*`fit-content` as defined in the previous CL). Also as an addition, the `span` element should always be centered in the `permission` element so padding should not be allowed to push the span to the side or out of the bounds of the `permission` element. That is why I basically want to put an upper bound on `padding-left|right` which is equal to the equivalent of k * `fit-content`. (Right now k = 1, but that could change in the future)

    I have tried playing around with `box-sizing: border-box` but that doesn't seem to put actual limits on the padding width: if `padding-left` + `padding-right` > `max-width` the element grows over max-width anyways. I've played around both with the `<permission>` element and divs and spans, but like I said I couldn't get it to work, if you have a working example I would be more than happy to chase that approach further. (I've also found various conflicting information on the web on whether `box-sizing: border-box` interacts correctly with `max-width` so I'm not exactly sure if it works reliably).

    David Baron

    What are the underlying goals of the minimum and maximum sizes here? Is the purpose of the minimum size (for both width and height) is to ensure that the text inside the element is visible? What's the purpose of the maximum size?

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Anders Hartvoll Ruud
    • Andy Paicu
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I19e6f9ff6fcd11073b1da0efaa68f743ea3886bb
    Gerrit-Change-Number: 5458152
    Gerrit-PatchSet: 6
    Gerrit-Owner: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Reviewer: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: David Baron <dba...@chromium.org>
    Gerrit-CC: Alexis Menard <alexis...@intel.com>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-Attention: Andy Paicu <andy...@chromium.org>
    Gerrit-Attention: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Comment-Date: Wed, 17 Apr 2024 13:28:23 +0000
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Andy Paicu (Gerrit)

    unread,
    Apr 17, 2024, 9:57:03 AMApr 17
    to Anders Hartvoll Ruud, David Baron, Blink W3C Test Autoroller, Tricium, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
    Attention needed from Anders Hartvoll Ruud and David Baron

    Andy Paicu added 1 comment

    Patchset-level comments
    Andy Paicu . unresolved

    Hello Anders and David, PTAL.

    David, am I right about the fact that `calc-size` can't be used in padding?
    Anders, PTAL at the other CSS stuff.

    David Baron

    You're correct that `calc-size()` can't be used in `padding`. But I'm not entirely sure what it is that you wanted to do with it. `calc()` can be used in `padding`. Would that have worked?

    Or, taking an entirely different approach, would `box-sizing: border-box` have worked? (That does expose a slightly different developer-facing API, but that seems like it might be justified in this case if it helps the restrictions here make sense.)

    David Baron

    (That said, I think it would help me understand the general approach here if I could see a statement of what you're intending to allow vs. forbid with this change.)

    Anders Hartvoll Ruud

    I can't really recommend implementing your own interpretation of "padding", it must surely cause some inconsistencies with other parts of CSS.

    I assume we can't just impose some limit on the padding (in `em`) in this case?

    Andy Paicu

    Vertical padding could be limited by `em` but horizontal padding max size depends on the content-size, which is why we implemented `min|max-width` with `calc-size`.

    So ignoring height for a second, since that can be bound by `1em`:

    The ultimate goal is to ensure that whatever padding is added to the element, the total width (border+padding+content) doesn't run over the `max-width` (which is 3*`fit-content` as defined in the previous CL). Also as an addition, the `span` element should always be centered in the `permission` element so padding should not be allowed to push the span to the side or out of the bounds of the `permission` element. That is why I basically want to put an upper bound on `padding-left|right` which is equal to the equivalent of k * `fit-content`. (Right now k = 1, but that could change in the future)

    I have tried playing around with `box-sizing: border-box` but that doesn't seem to put actual limits on the padding width: if `padding-left` + `padding-right` > `max-width` the element grows over max-width anyways. I've played around both with the `<permission>` element and divs and spans, but like I said I couldn't get it to work, if you have a working example I would be more than happy to chase that approach further. (I've also found various conflicting information on the web on whether `box-sizing: border-box` interacts correctly with `max-width` so I'm not exactly sure if it works reliably).

    David Baron

    What are the underlying goals of the minimum and maximum sizes here? Is the purpose of the minimum size (for both width and height) is to ensure that the text inside the element is visible? What's the purpose of the maximum size?

    Andy Paicu

    Minimum is to ensure the text is inside and is visible indeed.

    Maximum is to ensure that the text is clearly labeling the button, otherwise you can imagine a huge button stretched over the entire view-port and the button text being just a relatively small surface area inside that button. It would be possible for users to not perceive it as the label to a button.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Anders Hartvoll Ruud
    • David Baron
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I19e6f9ff6fcd11073b1da0efaa68f743ea3886bb
    Gerrit-Change-Number: 5458152
    Gerrit-PatchSet: 6
    Gerrit-Owner: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Reviewer: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: David Baron <dba...@chromium.org>
    Gerrit-CC: Alexis Menard <alexis...@intel.com>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-Attention: David Baron <dba...@chromium.org>
    Gerrit-Attention: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Comment-Date: Wed, 17 Apr 2024 13:56:51 +0000
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    David Baron (Gerrit)

    unread,
    Apr 17, 2024, 4:05:43 PMApr 17
    to Andy Paicu, Anders Hartvoll Ruud, David Baron, Blink W3C Test Autoroller, Tricium, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
    Attention needed from Anders Hartvoll Ruud and Andy Paicu

    David Baron added 1 comment

    Patchset-level comments
    Andy Paicu . unresolved

    Hello Anders and David, PTAL.

    David, am I right about the fact that `calc-size` can't be used in padding?
    Anders, PTAL at the other CSS stuff.

    David Baron

    You're correct that `calc-size()` can't be used in `padding`. But I'm not entirely sure what it is that you wanted to do with it. `calc()` can be used in `padding`. Would that have worked?

    Or, taking an entirely different approach, would `box-sizing: border-box` have worked? (That does expose a slightly different developer-facing API, but that seems like it might be justified in this case if it helps the restrictions here make sense.)

    David Baron

    (That said, I think it would help me understand the general approach here if I could see a statement of what you're intending to allow vs. forbid with this change.)

    Anders Hartvoll Ruud

    I can't really recommend implementing your own interpretation of "padding", it must surely cause some inconsistencies with other parts of CSS.

    I assume we can't just impose some limit on the padding (in `em`) in this case?

    Andy Paicu

    Vertical padding could be limited by `em` but horizontal padding max size depends on the content-size, which is why we implemented `min|max-width` with `calc-size`.

    So ignoring height for a second, since that can be bound by `1em`:

    The ultimate goal is to ensure that whatever padding is added to the element, the total width (border+padding+content) doesn't run over the `max-width` (which is 3*`fit-content` as defined in the previous CL). Also as an addition, the `span` element should always be centered in the `permission` element so padding should not be allowed to push the span to the side or out of the bounds of the `permission` element. That is why I basically want to put an upper bound on `padding-left|right` which is equal to the equivalent of k * `fit-content`. (Right now k = 1, but that could change in the future)

    I have tried playing around with `box-sizing: border-box` but that doesn't seem to put actual limits on the padding width: if `padding-left` + `padding-right` > `max-width` the element grows over max-width anyways. I've played around both with the `<permission>` element and divs and spans, but like I said I couldn't get it to work, if you have a working example I would be more than happy to chase that approach further. (I've also found various conflicting information on the web on whether `box-sizing: border-box` interacts correctly with `max-width` so I'm not exactly sure if it works reliably).

    David Baron

    What are the underlying goals of the minimum and maximum sizes here? Is the purpose of the minimum size (for both width and height) is to ensure that the text inside the element is visible? What's the purpose of the maximum size?

    Andy Paicu

    Minimum is to ensure the text is inside and is visible indeed.

    Maximum is to ensure that the text is clearly labeling the button, otherwise you can imagine a huge button stretched over the entire view-port and the button text being just a relatively small surface area inside that button. It would be possible for users to not perceive it as the label to a button.

    David Baron

    Would it be sufficient to:

    • use `content-box` sizing, and
    • limit the `padding` to a maximum of `2em` or so?

    (That is, would that be enough to avoid having to manipulate the `width` constraints in order to support `padding`?)

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Anders Hartvoll Ruud
    • Andy Paicu
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I19e6f9ff6fcd11073b1da0efaa68f743ea3886bb
    Gerrit-Change-Number: 5458152
    Gerrit-PatchSet: 6
    Gerrit-Owner: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Reviewer: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: David Baron <dba...@chromium.org>
    Gerrit-CC: Alexis Menard <alexis...@intel.com>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-Attention: Andy Paicu <andy...@chromium.org>
    Gerrit-Attention: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Comment-Date: Wed, 17 Apr 2024 20:05:32 +0000
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Andy Paicu (Gerrit)

    unread,
    Apr 18, 2024, 3:39:22 AMApr 18
    to Anders Hartvoll Ruud, David Baron, Blink W3C Test Autoroller, Tricium, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
    Attention needed from Anders Hartvoll Ruud and David Baron

    Andy Paicu added 1 comment

    Patchset-level comments
    Andy Paicu

    I think we could do something like that where we guestimate how much padding is appropriate at most (`2em` is too little probably more like `5em`), but I would rather be able to set a limit that is based on content length since that can vary widely based on i18n and permission status.

    However, if there is no relatively straightforward solution (e.g. to support `calc-size` in `padding`, which likely has implications I don't understand), then putting a static limit on it seems like the next best thing.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Anders Hartvoll Ruud
    • David Baron
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I19e6f9ff6fcd11073b1da0efaa68f743ea3886bb
    Gerrit-Change-Number: 5458152
    Gerrit-PatchSet: 6
    Gerrit-Owner: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Reviewer: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: David Baron <dba...@chromium.org>
    Gerrit-CC: Alexis Menard <alexis...@intel.com>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-Attention: David Baron <dba...@chromium.org>
    Gerrit-Attention: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Comment-Date: Thu, 18 Apr 2024 07:39:08 +0000
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    David Baron (Gerrit)

    unread,
    Apr 18, 2024, 9:30:36 AMApr 18
    to Andy Paicu, Anders Hartvoll Ruud, David Baron, Blink W3C Test Autoroller, Tricium, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
    Attention needed from Anders Hartvoll Ruud and Andy Paicu

    David Baron added 1 comment

    Patchset-level comments
    David Baron

    I don't think supporting `calc-size()` in `padding` is plausible, since what you're asking for there is not `fit-content` for `padding` but `fit-content` for an entirely different property (`width`), which would be a pretty major architectural change (having `calc()` or `calc-size()` expressions accept values that aren't valid for the property).

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Anders Hartvoll Ruud
    • Andy Paicu
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I19e6f9ff6fcd11073b1da0efaa68f743ea3886bb
    Gerrit-Change-Number: 5458152
    Gerrit-PatchSet: 6
    Gerrit-Owner: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Reviewer: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: David Baron <dba...@chromium.org>
    Gerrit-CC: Alexis Menard <alexis...@intel.com>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-Attention: Andy Paicu <andy...@chromium.org>
    Gerrit-Attention: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Comment-Date: Thu, 18 Apr 2024 13:30:23 +0000
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Andy Paicu (Gerrit)

    unread,
    Apr 22, 2024, 4:25:39 PMApr 22
    to David Baron, Anders Hartvoll Ruud, Blink W3C Test Autoroller, Tricium, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
    Attention needed from Anders Hartvoll Ruud and David Baron

    Andy Paicu added 2 comments

    Patchset-level comments
    File-level comment, Patchset 5:
    Andy Paicu . resolved
    Andy Paicu

    Acknowledged, I have switched to fixed sizes for the max bounds. Thank you for confirming.

    File-level comment, Patchset 8 (Latest):
    Andy Paicu . resolved

    Hi Anders, PTAL

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Anders Hartvoll Ruud
    • David Baron
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I19e6f9ff6fcd11073b1da0efaa68f743ea3886bb
    Gerrit-Change-Number: 5458152
    Gerrit-PatchSet: 8
    Gerrit-Owner: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Reviewer: Andy Paicu <andy...@chromium.org>
    Gerrit-CC: Alexis Menard <alexis...@intel.com>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-CC: David Baron <dba...@chromium.org>
    Gerrit-Attention: David Baron <dba...@chromium.org>
    Gerrit-Attention: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Comment-Date: Mon, 22 Apr 2024 20:25:29 +0000
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Anders Hartvoll Ruud (Gerrit)

    unread,
    Apr 23, 2024, 5:42:05 AMApr 23
    to Andy Paicu, David Baron, Blink W3C Test Autoroller, Tricium, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
    Attention needed from Andy Paicu and David Baron

    Anders Hartvoll Ruud voted and added 4 comments

    Votes added by Anders Hartvoll Ruud

    Code-Review+1

    4 comments

    Patchset-level comments
    Anders Hartvoll Ruud

    👍

    Commit Message
    Line 13, Patchset 8 (Latest):the control of the site author), this CL provides a mechanism that
    sets the width/height based on padding.
    Anders Hartvoll Ruud . unresolved

    Not accurate anymore?

    File third_party/blink/renderer/core/html/resources/permission.css
    Line 14, Patchset 8 (Latest): box-sizing: border-box !important;
    Anders Hartvoll Ruud . unresolved

    This is intentional, right? (It's not mentioned in the commit message).

    File third_party/blink/web_tests/external/wpt/html/semantics/permission-element/bounded-sizes-reftest.tentative.html
    Line 50, Patchset 8 (Latest): /* There is a slight misalignment of the text (by 1px) when using
    padding vs using width/height. Since this test's purpose is to
    check that the bounds are identical, the color and background-color
    are set to the same value to cover the slight text misalignment */
    color:black;
    background-color: black;
    Anders Hartvoll Ruud . unresolved

    Is this still relevant?

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Andy Paicu
    • David Baron
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I19e6f9ff6fcd11073b1da0efaa68f743ea3886bb
    Gerrit-Change-Number: 5458152
    Gerrit-PatchSet: 8
    Gerrit-Owner: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Reviewer: Andy Paicu <andy...@chromium.org>
    Gerrit-CC: Alexis Menard <alexis...@intel.com>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-CC: David Baron <dba...@chromium.org>
    Gerrit-Attention: David Baron <dba...@chromium.org>
    Gerrit-Attention: Andy Paicu <andy...@chromium.org>
    Gerrit-Comment-Date: Tue, 23 Apr 2024 09:41:55 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Andy Paicu (Gerrit)

    unread,
    Apr 23, 2024, 6:19:30 AMApr 23
    to Anders Hartvoll Ruud, David Baron, Blink W3C Test Autoroller, Tricium, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
    Attention needed from Anders Hartvoll Ruud

    Andy Paicu added 3 comments

    Commit Message
    Line 13, Patchset 8 (Latest):the control of the site author), this CL provides a mechanism that
    sets the width/height based on padding.
    Anders Hartvoll Ruud . resolved

    Not accurate anymore?

    Andy Paicu

    Done

    File third_party/blink/renderer/core/html/resources/permission.css
    Line 14, Patchset 8 (Latest): box-sizing: border-box !important;
    Anders Hartvoll Ruud . resolved

    This is intentional, right? (It's not mentioned in the commit message).

    Andy Paicu

    Done, augmented commit message.

    File third_party/blink/web_tests/external/wpt/html/semantics/permission-element/bounded-sizes-reftest.tentative.html
    Line 50, Patchset 8 (Latest): /* There is a slight misalignment of the text (by 1px) when using
    padding vs using width/height. Since this test's purpose is to
    check that the bounds are identical, the color and background-color
    are set to the same value to cover the slight text misalignment */
    color:black;
    background-color: black;
    Anders Hartvoll Ruud . unresolved

    Is this still relevant?

    Andy Paicu

    Unfortunately, yes.

    The "actual" element (in this file) uses padding and the "expected" element (in the reference file) uses fixed width/height to achieve the same overall result. However there is a slight misalignment of the text (1px) between the 2 methods, it seems the padding method results in the text being 1px to the left vs fixed width reference (the overall bounds e.g. border are still identical). This seemed largely superfluous so I went with this approach to make them visually identical (after various unsuccessful attempts to fix it via CSS).

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Anders Hartvoll Ruud
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I19e6f9ff6fcd11073b1da0efaa68f743ea3886bb
    Gerrit-Change-Number: 5458152
    Gerrit-PatchSet: 8
    Gerrit-Owner: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Reviewer: Andy Paicu <andy...@chromium.org>
    Gerrit-CC: Alexis Menard <alexis...@intel.com>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-CC: David Baron <dba...@chromium.org>
    Gerrit-Attention: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Comment-Date: Tue, 23 Apr 2024 10:19:19 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Anders Hartvoll Ruud (Gerrit)

    unread,
    Apr 23, 2024, 7:00:19 AMApr 23
    to Andy Paicu, David Baron, Blink W3C Test Autoroller, Tricium, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
    Attention needed from Andy Paicu

    Anders Hartvoll Ruud added 1 comment

    File third_party/blink/web_tests/external/wpt/html/semantics/permission-element/bounded-sizes-reftest.tentative.html
    Line 50, Patchset 8 (Latest): /* There is a slight misalignment of the text (by 1px) when using
    padding vs using width/height. Since this test's purpose is to
    check that the bounds are identical, the color and background-color
    are set to the same value to cover the slight text misalignment */
    color:black;
    background-color: black;
    Anders Hartvoll Ruud . unresolved

    Is this still relevant?

    Andy Paicu

    Unfortunately, yes.

    The "actual" element (in this file) uses padding and the "expected" element (in the reference file) uses fixed width/height to achieve the same overall result. However there is a slight misalignment of the text (1px) between the 2 methods, it seems the padding method results in the text being 1px to the left vs fixed width reference (the overall bounds e.g. border are still identical). This seemed largely superfluous so I went with this approach to make them visually identical (after various unsuccessful attempts to fix it via CSS).

    Anders Hartvoll Ruud

    Can you not just using `padding` in the ref as well?

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Andy Paicu
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I19e6f9ff6fcd11073b1da0efaa68f743ea3886bb
    Gerrit-Change-Number: 5458152
    Gerrit-PatchSet: 8
    Gerrit-Owner: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Reviewer: Andy Paicu <andy...@chromium.org>
    Gerrit-CC: Alexis Menard <alexis...@intel.com>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-CC: David Baron <dba...@chromium.org>
    Gerrit-Attention: Andy Paicu <andy...@chromium.org>
    Gerrit-Comment-Date: Tue, 23 Apr 2024 11:00:05 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Andy Paicu <andy...@chromium.org>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Andy Paicu (Gerrit)

    unread,
    Apr 23, 2024, 7:25:25 AMApr 23
    to Anders Hartvoll Ruud, David Baron, Blink W3C Test Autoroller, Tricium, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
    Attention needed from Anders Hartvoll Ruud

    Andy Paicu added 1 comment

    File third_party/blink/web_tests/external/wpt/html/semantics/permission-element/bounded-sizes-reftest.tentative.html
    Line 50, Patchset 8 (Latest): /* There is a slight misalignment of the text (by 1px) when using
    padding vs using width/height. Since this test's purpose is to
    check that the bounds are identical, the color and background-color
    are set to the same value to cover the slight text misalignment */
    color:black;
    background-color: black;
    Anders Hartvoll Ruud . unresolved

    Is this still relevant?

    Andy Paicu

    Unfortunately, yes.

    The "actual" element (in this file) uses padding and the "expected" element (in the reference file) uses fixed width/height to achieve the same overall result. However there is a slight misalignment of the text (1px) between the 2 methods, it seems the padding method results in the text being 1px to the left vs fixed width reference (the overall bounds e.g. border are still identical). This seemed largely superfluous so I went with this approach to make them visually identical (after various unsuccessful attempts to fix it via CSS).

    Anders Hartvoll Ruud

    Can you not just using `padding` in the ref as well?

    Andy Paicu

    If we did that we would no longer be testing anything at all. The 2 elements would be identical style-wise so they would of-course render identically.

    I'm trying to ensure that padding actually takes effect so, for example: if padding-left is 10px (and width is auto) then the apparent visual width of the button should be text + 10px on each side. The only way I could think of doing this is to calculate the overall necessary width (fit-content + 2*padding) and set that on the reference element. Then the one with padding should look identical to the one that has the size set fit-content + 2*padding.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Anders Hartvoll Ruud
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I19e6f9ff6fcd11073b1da0efaa68f743ea3886bb
    Gerrit-Change-Number: 5458152
    Gerrit-PatchSet: 8
    Gerrit-Owner: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Reviewer: Andy Paicu <andy...@chromium.org>
    Gerrit-CC: Alexis Menard <alexis...@intel.com>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-CC: David Baron <dba...@chromium.org>
    Gerrit-Attention: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Comment-Date: Tue, 23 Apr 2024 11:25:14 +0000
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Anders Hartvoll Ruud (Gerrit)

    unread,
    Apr 23, 2024, 9:11:44 AMApr 23
    to Andy Paicu, David Baron, Blink W3C Test Autoroller, Tricium, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
    Attention needed from Andy Paicu

    Anders Hartvoll Ruud added 1 comment

    File third_party/blink/web_tests/external/wpt/html/semantics/permission-element/bounded-sizes-reftest.tentative.html
    Line 50, Patchset 8 (Latest): /* There is a slight misalignment of the text (by 1px) when using
    padding vs using width/height. Since this test's purpose is to
    check that the bounds are identical, the color and background-color
    are set to the same value to cover the slight text misalignment */
    color:black;
    background-color: black;
    Anders Hartvoll Ruud . resolved

    Is this still relevant?

    Andy Paicu

    Unfortunately, yes.

    The "actual" element (in this file) uses padding and the "expected" element (in the reference file) uses fixed width/height to achieve the same overall result. However there is a slight misalignment of the text (1px) between the 2 methods, it seems the padding method results in the text being 1px to the left vs fixed width reference (the overall bounds e.g. border are still identical). This seemed largely superfluous so I went with this approach to make them visually identical (after various unsuccessful attempts to fix it via CSS).

    Anders Hartvoll Ruud

    Can you not just using `padding` in the ref as well?

    Andy Paicu

    If we did that we would no longer be testing anything at all. The 2 elements would be identical style-wise so they would of-course render identically.

    I'm trying to ensure that padding actually takes effect so, for example: if padding-left is 10px (and width is auto) then the apparent visual width of the button should be text + 10px on each side. The only way I could think of doing this is to calculate the overall necessary width (fit-content + 2*padding) and set that on the reference element. Then the one with padding should look identical to the one that has the size set fit-content + 2*padding.

    Anders Hartvoll Ruud

    Oh yeah, I forgot that it's a `<permission>` element on both sides, i.e. both sides are subjected to the same adjustments. :-)

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Andy Paicu
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I19e6f9ff6fcd11073b1da0efaa68f743ea3886bb
    Gerrit-Change-Number: 5458152
    Gerrit-PatchSet: 8
    Gerrit-Owner: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Reviewer: Andy Paicu <andy...@chromium.org>
    Gerrit-CC: Alexis Menard <alexis...@intel.com>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-CC: David Baron <dba...@chromium.org>
    Gerrit-Attention: Andy Paicu <andy...@chromium.org>
    Gerrit-Comment-Date: Tue, 23 Apr 2024 13:11:32 +0000
    satisfied_requirement
    open
    diffy

    David Baron (Gerrit)

    unread,
    Apr 23, 2024, 9:35:24 AMApr 23
    to Andy Paicu, David Baron, Anders Hartvoll Ruud, Blink W3C Test Autoroller, Tricium, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
    Attention needed from Andy Paicu

    David Baron voted and added 2 comments

    Votes added by David Baron

    Code-Review+1

    2 comments

    Patchset-level comments
    File-level comment, Patchset 8 (Latest):
    David Baron . resolved

    LGTM as well, although I think it's worth noting that if the end goal is to get this feature shipped in multiple engines, it would be good to get feedback on this sort of approach from the others sooner rather than later, since it's possible that this sort of adjustment mechanism (which is unusual and doesn't match normal UA style adjustments) might not be feasible in other engines (particularly Gecko, since there isn't a common origin for the codebase).

    File third_party/blink/web_tests/external/wpt/html/semantics/permission-element/bounded-sizes-reftest.tentative.html
    Line 50, Patchset 8 (Latest): /* There is a slight misalignment of the text (by 1px) when using
    padding vs using width/height. Since this test's purpose is to
    check that the bounds are identical, the color and background-color
    are set to the same value to cover the slight text misalignment */
    color:black;
    background-color: black;
    Anders Hartvoll Ruud . resolved

    Is this still relevant?

    Andy Paicu

    Unfortunately, yes.

    The "actual" element (in this file) uses padding and the "expected" element (in the reference file) uses fixed width/height to achieve the same overall result. However there is a slight misalignment of the text (1px) between the 2 methods, it seems the padding method results in the text being 1px to the left vs fixed width reference (the overall bounds e.g. border are still identical). This seemed largely superfluous so I went with this approach to make them visually identical (after various unsuccessful attempts to fix it via CSS).

    Anders Hartvoll Ruud

    Can you not just using `padding` in the ref as well?

    Andy Paicu

    If we did that we would no longer be testing anything at all. The 2 elements would be identical style-wise so they would of-course render identically.

    I'm trying to ensure that padding actually takes effect so, for example: if padding-left is 10px (and width is auto) then the apparent visual width of the button should be text + 10px on each side. The only way I could think of doing this is to calculate the overall necessary width (fit-content + 2*padding) and set that on the reference element. Then the one with padding should look identical to the one that has the size set fit-content + 2*padding.

    Anders Hartvoll Ruud

    Oh yeah, I forgot that it's a `<permission>` element on both sides, i.e. both sides are subjected to the same adjustments. :-)

    David Baron

    I'm curious if `width: max-content` or `width: min-content` might have produced better results than `width: fit-content`... but it probably doesn't make a difference.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Andy Paicu
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I19e6f9ff6fcd11073b1da0efaa68f743ea3886bb
    Gerrit-Change-Number: 5458152
    Gerrit-PatchSet: 8
    Gerrit-Owner: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Reviewer: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: David Baron <dba...@chromium.org>
    Gerrit-CC: Alexis Menard <alexis...@intel.com>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-Attention: Andy Paicu <andy...@chromium.org>
    Gerrit-Comment-Date: Tue, 23 Apr 2024 13:35:11 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    satisfied_requirement
    open
    diffy

    Andy Paicu (Gerrit)

    unread,
    Apr 23, 2024, 10:56:03 AMApr 23
    to David Baron, Anders Hartvoll Ruud, Blink W3C Test Autoroller, Tricium, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org

    Andy Paicu voted and added 3 comments

    Votes added by Andy Paicu

    Commit-Queue+2

    3 comments

    Patchset-level comments
    David Baron . resolved

    LGTM as well, although I think it's worth noting that if the end goal is to get this feature shipped in multiple engines, it would be good to get feedback on this sort of approach from the others sooner rather than later, since it's possible that this sort of adjustment mechanism (which is unusual and doesn't match normal UA style adjustments) might not be feasible in other engines (particularly Gecko, since there isn't a common origin for the codebase).

    Andy Paicu

    We are in constant discussion with other vendors, including about the styling restrictions. I can't say the feedback is exactly what I was hoping for, but communication is happening.

    Andy Paicu . resolved

    Thank you both.

    File third_party/blink/web_tests/external/wpt/html/semantics/permission-element/bounded-sizes-reftest.tentative.html
    Line 50, Patchset 8 (Latest): /* There is a slight misalignment of the text (by 1px) when using
    padding vs using width/height. Since this test's purpose is to
    check that the bounds are identical, the color and background-color
    are set to the same value to cover the slight text misalignment */
    color:black;
    background-color: black;
    Anders Hartvoll Ruud . resolved

    Is this still relevant?

    Andy Paicu

    Unfortunately, yes.

    The "actual" element (in this file) uses padding and the "expected" element (in the reference file) uses fixed width/height to achieve the same overall result. However there is a slight misalignment of the text (1px) between the 2 methods, it seems the padding method results in the text being 1px to the left vs fixed width reference (the overall bounds e.g. border are still identical). This seemed largely superfluous so I went with this approach to make them visually identical (after various unsuccessful attempts to fix it via CSS).

    Anders Hartvoll Ruud

    Can you not just using `padding` in the ref as well?

    Andy Paicu

    If we did that we would no longer be testing anything at all. The 2 elements would be identical style-wise so they would of-course render identically.

    I'm trying to ensure that padding actually takes effect so, for example: if padding-left is 10px (and width is auto) then the apparent visual width of the button should be text + 10px on each side. The only way I could think of doing this is to calculate the overall necessary width (fit-content + 2*padding) and set that on the reference element. Then the one with padding should look identical to the one that has the size set fit-content + 2*padding.

    Anders Hartvoll Ruud

    Oh yeah, I forgot that it's a `<permission>` element on both sides, i.e. both sides are subjected to the same adjustments. :-)

    David Baron

    I'm curious if `width: max-content` or `width: min-content` might have produced better results than `width: fit-content`... but it probably doesn't make a difference.

    Andy Paicu

    I have tried `min-content`, but not `max-content` but I doubt that would work since making the width bigger would automatically cause pixel differences.

    Open in Gerrit

    Related details

    Attention set is empty
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I19e6f9ff6fcd11073b1da0efaa68f743ea3886bb
    Gerrit-Change-Number: 5458152
    Gerrit-PatchSet: 8
    Gerrit-Owner: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Reviewer: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: David Baron <dba...@chromium.org>
    Gerrit-CC: Alexis Menard <alexis...@intel.com>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-Comment-Date: Tue, 23 Apr 2024 14:55:49 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    Comment-In-Reply-To: David Baron <dba...@chromium.org>
    satisfied_requirement
    open
    diffy

    Chromium LUCI CQ (Gerrit)

    unread,
    Apr 23, 2024, 11:00:01 AMApr 23
    to Andy Paicu, David Baron, Anders Hartvoll Ruud, Blink W3C Test Autoroller, Tricium, Alexis Menard, chromium...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org

    Chromium LUCI CQ submitted the change

    Change information

    Commit message:
    [PEPC] Provide partial padding support to allow for common use case

    Removal of 'padding' support from the permission element has removed
    support for a common use case that used padding in conjunction with
    'width: auto'. Since it is very difficult to provide a fixed width
    for the permission element (as the text can change and is not under

    the control of the site author), this CL provides a mechanism that
    sets the width/height based on padding.

    We don't want to simply support padding unchecked since it can be
    used to push the text outside of the permission element area, and
    creating a bounded expression is very difficult because 'calc-size'
    is not supported for padding lengths.

    Also do a quick cleanup of vertical-align.
    Bug: 1462930
    Fixed: 335835011
    Change-Id: I19e6f9ff6fcd11073b1da0efaa68f743ea3886bb
    Reviewed-by: Anders Hartvoll Ruud <and...@chromium.org>
    Commit-Queue: Andy Paicu <andy...@chromium.org>
    Reviewed-by: David Baron <dba...@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#1291274}
    Files:
    • M third_party/blink/renderer/core/css/css_properties.json5
    • M third_party/blink/renderer/core/html/html_permission_element.cc
    • M third_party/blink/renderer/core/html/resources/permission.css
    • M third_party/blink/web_tests/external/wpt/html/semantics/permission-element/bounded-sizes-reftest-ref.html
    • M third_party/blink/web_tests/external/wpt/html/semantics/permission-element/bounded-sizes-reftest.tentative.html
    • M third_party/blink/web_tests/external/wpt/html/semantics/permission-element/bounded-sizes.tentative.html
    Change size: M
    Delta: 6 files changed, 115 insertions(+), 6 deletions(-)
    Branch: refs/heads/main
    Submit Requirements:
    • requirement satisfiedCode-Review: +1 by Anders Hartvoll Ruud, +1 by David Baron
    Open in Gerrit
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: merged
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I19e6f9ff6fcd11073b1da0efaa68f743ea3886bb
    Gerrit-Change-Number: 5458152
    Gerrit-PatchSet: 9
    Gerrit-Owner: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Reviewer: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>
    Gerrit-Reviewer: David Baron <dba...@chromium.org>
    Gerrit-CC: Alexis Menard <alexis...@intel.com>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    open
    diffy
    satisfied_requirement

    Blink W3C Test Autoroller (Gerrit)

    unread,
    Apr 23, 2024, 11:28:32 AMApr 23
    to Andy Paicu, Chromium LUCI CQ, David Baron, Anders Hartvoll Ruud, Tricium, Alexis Menard, chromium...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org

    Message from Blink W3C Test Autoroller

    The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/45739

    Open in Gerrit

    Related details

    Attention set is empty
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I19e6f9ff6fcd11073b1da0efaa68f743ea3886bb
    Gerrit-Change-Number: 5458152
    Gerrit-PatchSet: 9
    Gerrit-Owner: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Reviewer: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>
    Gerrit-Reviewer: David Baron <dba...@chromium.org>
    Gerrit-CC: Alexis Menard <alexis...@intel.com>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-Comment-Date: Tue, 23 Apr 2024 15:28:21 +0000
    Gerrit-HasComments: No
    Gerrit-Has-Labels: No
    satisfied_requirement
    open
    diffy
    Reply all
    Reply to author
    Forward
    0 new messages