[PEPC] Relax 'display' value restrictions [chromium/src : main]
Andy Paicu (Gerrit)
Andy Paicu voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Andy Paicu
- Mason Freed
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Paicu (Gerrit)
Andy Paicu voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Andy Paicu
- Mason Freed
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Paicu (Gerrit)
Andy Paicu voted![]( "Open in Gerrit")
| Code-Review | +0 |
| Commit-Queue | +1 |
Related details
- Andy Paicu
- Mason Freed
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 4 comments![]( "Open in Gerrit")
Code mostly LGTM just a question about the lists
constexpr EDisplay kValidDisplayStyles[] = {With such a big list, it'd be good to link to a spec or something explaining what is in, and perhaps more importantly, what is missing from this list.
In fact, would it not be better to instead list what's *not* allowed?
". Only block, flex, inline-* and masonry values are supported."}));Reflecting the above comment - it'd be more helpful to name the things that are forbidden.
testDisplayValue("block", "");
testDisplayValue("inline-block", "");
testDisplayValue("inline", "style_invalid");Perhaps this should be a more complete list?
Related details
- Andy Paicu
Code-Coverage
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 3 comments![]( "Open in Gerrit")
Votes added by Andy Paicu
| Commit-Queue | +1 |
3 comments
With such a big list, it'd be good to link to a spec or something explaining what is in, and perhaps more importantly, what is missing from this list.
In fact, would it not be better to instead list what's *not* allowed?
Done
". Only block, flex, inline-* and masonry values are supported."}));Reflecting the above comment - it'd be more helpful to name the things that are forbidden.
Done
testDisplayValue("block", "");
testDisplayValue("inline-block", "");
testDisplayValue("inline", "style_invalid");Perhaps this should be a more complete list?
- Andy Paicu
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 3 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
3 comments
Nice, thank you for the changes. LGTM.
constexpr EDisplay kValidDisplayStyles[] = {Andy PaicuWith such a big list, it'd be good to link to a spec or something explaining what is in, and perhaps more importantly, what is missing from this list.
In fact, would it not be better to instead list what's *not* allowed?
Done
Ha - guess the "deny" list is slightly larger, mostly because of kTable*. Still, thanks for making this change, I think it's easier to understand.
I wonder if you could use IsDisplayTableType() from ComputedStyle? Ok if not.
". Only block, flex, inline-* and masonry values are supported."}));Andy PaicuReflecting the above comment - it'd be more helpful to name the things that are forbidden.
Done
- Andy Paicu
- 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)
Message from Andy Paicu![]( "Open in Gerrit")
- Andy Paicu
- Mason Freed
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 2 comments![]( "Open in Gerrit")
Votes added by Andy Paicu
| Commit-Queue | +1 |
2 comments
@mas...@chromium.org since in the meantime a dependent CL was reverted (more details in the description) and this CL re-adds it (and also fixes the issue in the process), please re-review.
@tun...@chromium.org please verify that this new version will not run into the same issue as the CL which was reverted
constexpr EDisplay kValidDisplayStyles[] = {Andy PaicuWith such a big list, it'd be good to link to a spec or something explaining what is in, and perhaps more importantly, what is missing from this list.
In fact, would it not be better to instead list what's *not* allowed?
Mason FreedDone
Ha - guess the "deny" list is slightly larger, mostly because of kTable*. Still, thanks for making this change, I think it's easier to understand.
I wonder if you could use IsDisplayTableType() from ComputedStyle? Ok if not.
- Andy Paicu
- Mason Freed
- Thomas Nguyen
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Nguyen (Gerrit)
Thomas Nguyen voted and added 1 comment![]( "Open in Gerrit")
Votes added by Thomas Nguyen
| Code-Review | +1 |
1 comment
LayoutObject* layout_object = GetLayoutObject();off topic: as you mentioned, layout_object is not guaranteed not null. Looks like this does not 100% correct anymore, and I am also curious if the IntersectionObserver is still working with display:inline or display:content.
If IO does not work at all, I think we have to restrict the display values.
Related details
- Andy Paicu
- Andy Paicu
- Mason Freed
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")
LayoutObject* layout_object = GetLayoutObject();off topic: as you mentioned, layout_object is not guaranteed not null. Looks like this does not 100% correct anymore, and I am also curious if the IntersectionObserver is still working with display:inline or display:content.
If IO does not work at all, I think we have to restrict the display values.
Both display: inline and display: content make the permission element invalid. I went through all options manually and made all the display values that don't result in a LayoutBox to be invalid.
Related details
- Andy Paicu
- Mason Freed
- Thomas Nguyen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 2 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
2 comments
Still LGTM
Related details
- Andy Paicu
- Andy Paicu
- Thomas Nguyen
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")
LayoutObject* layout_object = GetLayoutObject();Andy Paicuoff topic: as you mentioned, layout_object is not guaranteed not null. Looks like this does not 100% correct anymore, and I am also curious if the IntersectionObserver is still working with display:inline or display:content.
If IO does not work at all, I think we have to restrict the display values.
Both display: inline and display: content make the permission element invalid. I went through all options manually and made all the display values that don't result in a LayoutBox to be invalid.
I'll mark as done, let me know if my answer is not sufficient.
Related details
- Andy Paicu
- Thomas Nguyen
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Paicu (Gerrit)
Andy Paicu voted and added 1 comment![]( "Open in Gerrit")
Votes added by Andy Paicu
| Commit-Queue | +2 |
1 comment
thank you
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[PEPC] Relax 'display' value restrictions
There should be more allowed properties besides 'inline-block' which
work for the permission element.
This CL now also re-adds the reverted CL
https://chromium-review.googlesource.com/c/chromium/src/+/6551238. That
CL was reverted as it was causing issues with Zoom and Meet who were
using display: flex and display: block (not deliberately, but
inherited). This CL does also allow flex and block as values so it
should be safe to re-add.
- M third_party/blink/renderer/core/html/html_permission_element.cc
- M third_party/blink/renderer/core/html/html_permission_element.h
- M third_party/blink/renderer/core/html/html_permission_element_test.cc
- D third_party/blink/web_tests/external/wpt/html/semantics/permission-element/display-css-property-reftest-ref.html
- D third_party/blink/web_tests/external/wpt/html/semantics/permission-element/display-css-property-reftest.tentative.html
- D third_party/blink/web_tests/external/wpt/html/semantics/permission-element/display-css-property.tentative.html
- A third_party/blink/web_tests/external/wpt/html/semantics/permission-element/display-values.tentative.html
- M tools/metrics/histograms/metadata/blink/enums.xml
Code-Review: +1 by Mason Freed, +1 by Thomas Nguyen
| 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/53382
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |