[PEPC] Restrict Permission icon's height and margin. [chromium/src : main]
Andy Paicu (Gerrit)
Andy Paicu added 6 comments![]( "Open in Gerrit")
static const CalculationExpressionNode* BuildLengthBoundExpr(These 2 functions can be internal-only (defined in an anonymous namespace in the cc file), I think only the function above is actually called.
Element::AdjustStyle(builder);What about other properties like stroke-width? Should they have a max size to not interfere with the test?
builder.SetWidth(I believe that `width` was supposed to just duplicate height, you can do this by setting it here (both for width and minwidth)
Also when adding this, it would be good set a console warning if we have a width here that is not auto and not already equal to height.
"the min/max width and height of the permission element";"of the permission element's icon"
also only the height can be set I believe, the width is supposed to copy it.
A standard permission element of type location, with 50px icon height.This explanation should go in the reftest. Also same comment about setting the value to the precise expected value as below.
margin-inline-end: 80px;if you define the font-size of the permission element you should be able to precisely set the precise value of 3em and not need to rely on the clamping in this reference file as well.
Related details
- Ravjit Uppal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ravjit Uppal (Gerrit)
Ravjit Uppal added 6 comments![]( "Open in Gerrit")
static const CalculationExpressionNode* BuildLengthBoundExpr(These 2 functions can be internal-only (defined in an anonymous namespace in the cc file), I think only the function above is actually called.
Done
Element::AdjustStyle(builder);What about other properties like stroke-width? Should they have a max size to not interfere with the test?
How can it affect the test? The default for stroke-width is 1px unless it is explicitly set.
I believe that `width` was supposed to just duplicate height, you can do this by setting it here (both for width and minwidth)
Also when adding this, it would be good set a console warning if we have a width here that is not auto and not already equal to height.
Done
"the min/max width and height of the permission element";"of the permission element's icon"
also only the height can be set I believe, the width is supposed to copy it.
Done
A standard permission element of type location, with 50px icon height.This explanation should go in the reftest. Also same comment about setting the value to the precise expected value as below.
Done
if you define the font-size of the permission element you should be able to precisely set the precise value of 3em and not need to rely on the clamping in this reference file as well.
- Andy Paicu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Paicu (Gerrit)
Andy Paicu added 1 comment![]( "Open in Gerrit")
Element::AdjustStyle(builder);Ravjit UppalWhat about other properties like stroke-width? Should they have a max size to not interfere with the test?
How can it affect the test? The default for stroke-width is 1px unless it is explicitly set.
Sorry, typo: I meant to say "text". As in if you set a large value for stoke-width will it end up covering the text of the permission element.
Related details
- Ravjit Uppal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ravjit Uppal (Gerrit)
Ravjit Uppal added 1 comment![]( "Open in Gerrit")
Element::AdjustStyle(builder);Ravjit UppalWhat about other properties like stroke-width? Should they have a max size to not interfere with the test?
Andy PaicuHow can it affect the test? The default for stroke-width is 1px unless it is explicitly set.
Sorry, typo: I meant to say "text". As in if you set a large value for stoke-width will it end up covering the text of the permission element.
I tested. The SVG doesn't render outside it's max-height and width. Setting a large stroke-width doesn't interfere with the text, it just makes the icon illegible.
Related details
- Andy Paicu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Paicu (Gerrit)
Andy Paicu voted and added 7 comments![]( "Open in Gerrit")
Votes added by Andy Paicu
| Code-Review | +1 |
7 comments
Element::AdjustStyle(builder);Ravjit UppalWhat about other properties like stroke-width? Should they have a max size to not interfere with the test?
Andy PaicuHow can it affect the test? The default for stroke-width is 1px unless it is explicitly set.
Ravjit UppalSorry, typo: I meant to say "text". As in if you set a large value for stoke-width will it end up covering the text of the permission element.
I tested. The SVG doesn't render outside it's max-height and width. Setting a large stroke-width doesn't interfere with the text, it just makes the icon illegible.
Acknowledged
LOG(WARNING) << warning;I don't think we need this LOG, just the console message.
builder.SetMarginRight(AdjustedBoundedLengthWrapper(and SetMarginLeft(0)
LOG(WARNING) << warning;I don't think we need this LOG, just the console message.
height: var(--height);I'm confused why we don't set the height directly to 15px here.
Related details
- Ravjit Uppal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ravjit Uppal (Gerrit)
Ravjit Uppal added 6 comments![]( "Open in Gerrit")
LOG(WARNING) << warning;I don't think we need this LOG, just the console message.
Done
builder.SetMarginRight(AdjustedBoundedLengthWrapper(Ravjit Uppaland SetMarginLeft(0)
Done
builder.MarginLeft(),Ravjit Uppaland SetMarginRight(0)
Done
LOG(WARNING) << warning;I don't think we need this LOG, just the console message.
Done
height: 1.3em;I would put `width: auto` here.
Done
height: var(--height);I'm confused why we don't set the height directly to 15px here.
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 6 comments![]( "Open in Gerrit")
Want to put a `//` on this line so I can tell that the first paragraph is talking about AdjustedBoundedLength? Right now it looks like it might be documenting the HTMLPermissionElementUtils class.
How similar is this to the removed code in html_permission_element.cc? Is there a small portion that is changed that I could focus on?
// A bool that tracks whether a specific console message was sent already to
// ensure it's not sent again.
bool length_console_error_sent_ = false;
// A bool that tracks whether a specific console message was sent already to
// ensure it's not sent again.
bool width_console_error_sent_ = false;The DevTools frontend already has logic to deduplicate and combine similar console messages. If the affected scenario emits a couple messages, I might consider removing this. If it gets sent hundreds or thousands of times though, I think this is a good idea for performance reasons and we should keep it.
LOG(WARNING) << warning;Ravjit UppalI don't think we need this LOG, just the console message.
Done
I still see LOG(WARNING) here, and I'm not sure the other comments in this file have actually been addressed?
height: 1.3em;Ravjit UppalI would put `width: auto` here.
Done
I see `width: 1.3em` removed, but I don't see a `width: auto`. Is `width: auto` the default value?
height: var(--height);Ravjit UppalI'm confused why we don't set the height directly to 15px here.
Fixed.
I over engineered it xD
- Ravjit Uppal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ravjit Uppal (Gerrit)
Ravjit Uppal added 6 comments![]( "Open in Gerrit")
Want to put a `//` on this line so I can tell that the first paragraph is talking about AdjustedBoundedLength? Right now it looks like it might be documenting the HTMLPermissionElementUtils class.
Done
How similar is this to the removed code in html_permission_element.cc? Is there a small portion that is changed that I could focus on?
It is exactly the same. It is just moved to a util class.
// A bool that tracks whether a specific console message was sent already to
// ensure it's not sent again.
bool length_console_error_sent_ = false;
// A bool that tracks whether a specific console message was sent already to
// ensure it's not sent again.
bool width_console_error_sent_ = false;The DevTools frontend already has logic to deduplicate and combine similar console messages. If the affected scenario emits a couple messages, I might consider removing this. If it gets sent hundreds or thousands of times though, I think this is a good idea for performance reasons and we should keep it.
AdjustStyle gets called very often while navigating the page. Let's keep this?
Ravjit UppalI don't think we need this LOG, just the console message.
Joey ArharDone
I still see LOG(WARNING) here, and I'm not sure the other comments in this file have actually been addressed?
Ah the latest changes were not pushed.
Done
Ravjit UppalI would put `width: auto` here.
Joey ArharDone
I see `width: 1.3em` removed, but I don't see a `width: auto`. Is `width: auto` the default value?
Done
Ravjit UppalI'm confused why we don't set the height directly to 15px here.
Joey ArharFixed.
I over engineered it xD
I still see `var(--height)` instead of `15px`
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar voted and added 1 comment![]( "Open in Gerrit")
Votes added by Joey Arhar
| Code-Review | +1 |
1 comment
// A bool that tracks whether a specific console message was sent already to
// ensure it's not sent again.
bool length_console_error_sent_ = false;
// A bool that tracks whether a specific console message was sent already to
// ensure it's not sent again.
bool width_console_error_sent_ = false;Ravjit UppalThe DevTools frontend already has logic to deduplicate and combine similar console messages. If the affected scenario emits a couple messages, I might consider removing this. If it gets sent hundreds or thousands of times though, I think this is a good idea for performance reasons and we should keep it.
AdjustStyle gets called very often while navigating the page. Let's keep this?
- Ravjit Uppal
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
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/53632.
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
Related details
- Ravjit Uppal
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ravjit Uppal (Gerrit)
Ravjit Uppal voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[PEPC] Restrict Permission icon's height and margin.
Permission icon's height or width should not exceed 1.5em The
margin-inline-end should not exceed 3em. Other margins are immutable.
DD: go/permission-icon-dd
- M third_party/blink/renderer/core/html/build.gni
- M third_party/blink/renderer/core/html/html_permission_element.cc
- M third_party/blink/renderer/core/html/html_permission_element.h
- A third_party/blink/renderer/core/html/html_permission_element_utils.cc
- A third_party/blink/renderer/core/html/html_permission_element_utils.h
- M third_party/blink/renderer/core/html/html_permission_icon_element.cc
- M third_party/blink/renderer/core/html/html_permission_icon_element.h
- M third_party/blink/renderer/core/html/resources/permission.css
- A third_party/blink/web_tests/external/wpt/html/semantics/permission-element/permission-icon/icon-css-property-height-bounded-reftest-ref.html
- A third_party/blink/web_tests/external/wpt/html/semantics/permission-element/permission-icon/icon-css-property-height-bounded-reftest.html
- M third_party/blink/web_tests/external/wpt/html/semantics/permission-element/permission-icon/icon-css-property-height-reftest.html
- A third_party/blink/web_tests/external/wpt/html/semantics/permission-element/permission-icon/icon-css-property-margin-bounded-reftest-ref.html
- A third_party/blink/web_tests/external/wpt/html/semantics/permission-element/permission-icon/icon-css-property-margin-bounded-reftest.html
- M third_party/blink/web_tests/external/wpt/html/semantics/permission-element/permission-icon/icon-css-property-max-height-reftest-ref.html
- A third_party/blink/web_tests/external/wpt/html/semantics/permission-element/permission-icon/icon-css-property-min-height-reftest-ref.html
- M third_party/blink/web_tests/external/wpt/html/semantics/permission-element/permission-icon/icon-css-property-min-height-reftest.html
Code-Review: +1 by Andy Paicu, +1 by Joey Arhar
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/53632
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |