Treat static video controls as replaced normal-flow stacking [chromium/src : main]
jj (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Xianzhu Wang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xianzhu Wang (Gerrit)
Xianzhu Wang added 4 comments![]( "Open in Gerrit")
+Mason for the style and html parts of this CL.
if (child_layer->GetLayoutObject().IsSVGForeignObject()) {I would like a comment here. I'm not very sure how that works. Can you create a test case with a stacked child under an SVG foreign object, and check the stack when the stacked child is hit tested? The stack will help us put correct comment here.
if (child->GetLayoutObject().IsSVGForeignObject()) {I would like a comment here. I'm not very sure how that works. Can you create a test case with a stacked child under an SVG foreign object, and check the stack when the stacked child is painted? The stack will help us put correct comment here.
Related details
- Mason Freed
- jj
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| 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
third_party/blink/renderer/core/css/resolver/style_adjuster.cc and third_party/blink/renderer/core/html/media/html_video_element_test.cc
LGTM
element && element->FastHasAttribute(html_names::kControlsAttr);You shouldn't have to check `element` here, if `is_video_element` is true.
The difference between these elements and regular stacking contexts is thatnit: line wrapping
Related details
- jj
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
jj (Gerrit)
jj voted and added 3 comments![]( "Open in Gerrit")
Votes added by jj
| Commit-Queue | +1 |
3 comments
element && element->FastHasAttribute(html_names::kControlsAttr);You shouldn't have to check `element` here, if `is_video_element` is true.
Good catch, thanks!
Done
The difference between these elements and regular stacking contexts is thatjjnit: line wrapping
Done
Related details
- Xianzhu Wang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
jj (Gerrit)
jj added 1 comment![]( "Open in Gerrit")
if (child_layer->GetLayoutObject().IsSVGForeignObject()) {I would like a comment here. I'm not very sure how that works. Can you create a test case with a stacked child under an SVG foreign object, and check the stack when the stacked child is hit tested? The stack will help us put correct comment here.
My understanding of the original conditions was that foreign objects had hit tests and painting handled elsewhere (with the hit tests applying some coordinate transformations) and the skip prevented them from being tested/painted twice. I narrowed it to just foreign objects, since video controls do not have any special handling for hit tests/painting.
I did a small test for this: https://gist.github.com/556a1a1e2845c11cc032493bb373a836
The normal iteration is skipped and instead hit tested by LayoutSVGForeignObject::NodeAtPointFromSVG. The other `if(...)` should be a similar situation, where the specific logic is handled instead by SVGForeignObjectPainter.
jj (Gerrit)
jj added 1 comment![]( "Open in Gerrit")
By the way, do you think it would be appropriate to killswitch this change too? The other changes were killswitched, and I think this could be breaking enough to warrant flagging as well.
Related details
- Xianzhu Wang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xianzhu Wang (Gerrit)
Xianzhu Wang added 2 comments![]( "Open in Gerrit")
bool is_replaced_normal_flow_video =Please put it behind a killswitch (can be same flag as the previous CL).
if (child_layer->GetLayoutObject().IsSVGForeignObject()) {jjI would like a comment here. I'm not very sure how that works. Can you create a test case with a stacked child under an SVG foreign object, and check the stack when the stacked child is hit tested? The stack will help us put correct comment here.
My understanding of the original conditions was that foreign objects had hit tests and painting handled elsewhere (with the hit tests applying some coordinate transformations) and the skip prevented them from being tested/painted twice. I narrowed it to just foreign objects, since video controls do not have any special handling for hit tests/painting.
I did a small test for this: https://gist.github.com/556a1a1e2845c11cc032493bb373a836
The normal iteration is skipped and instead hit tested by LayoutSVGForeignObject::NodeAtPointFromSVG. The other `if(...)` should be a similar situation, where the specific logic is handled instead by SVGForeignObjectPainter.
Thanks for investigation. I would like a comment here like:
```
// Hit-testing of whole subtree of an SVG foreignObject, including
// stacked children, are handled by LayoutSVGForeignObject, so don't
// hit test stacked children here.
```
and a similar comment in paint_layer_painter.cc.
Related details
- jj
jj (Gerrit)
jj added 4 comments![]( "Open in Gerrit")
By the way, do you think it would be appropriate to killswitch this change too? The other changes were killswitched, and I think this could be breaking enough to warrant flagging as well.
Resolving.
Please put it behind a killswitch (can be same flag as the previous CL).
Done
if (child_layer->GetLayoutObject().IsSVGForeignObject()) {jjI would like a comment here. I'm not very sure how that works. Can you create a test case with a stacked child under an SVG foreign object, and check the stack when the stacked child is hit tested? The stack will help us put correct comment here.
Xianzhu WangMy understanding of the original conditions was that foreign objects had hit tests and painting handled elsewhere (with the hit tests applying some coordinate transformations) and the skip prevented them from being tested/painted twice. I narrowed it to just foreign objects, since video controls do not have any special handling for hit tests/painting.
I did a small test for this: https://gist.github.com/556a1a1e2845c11cc032493bb373a836
The normal iteration is skipped and instead hit tested by LayoutSVGForeignObject::NodeAtPointFromSVG. The other `if(...)` should be a similar situation, where the specific logic is handled instead by SVGForeignObjectPainter.
Thanks for investigation. I would like a comment here like:
```
// Hit-testing of whole subtree of an SVG foreignObject, including
// stacked children, are handled by LayoutSVGForeignObject, so don't
// hit test stacked children here.
```
and a similar comment in paint_layer_painter.cc.
Done
if (child->GetLayoutObject().IsSVGForeignObject()) {jjI would like a comment here. I'm not very sure how that works. Can you create a test case with a stacked child under an SVG foreign object, and check the stack when the stacked child is painted? The stack will help us put correct comment here.
Done
Related details
- Mason Freed
- Xianzhu Wang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xianzhu Wang (Gerrit)
Xianzhu Wang voted Code-Review+1![]( "Open in Gerrit")
- Mason Freed
- jj
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| 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/58304.
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
- Mason Freed
- jj
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
1 comment
third_party/blink/renderer/core/css/resolver/style_adjuster.cc and third_party/blink/renderer/core/html/media/html_video_element_test.cc
still LGTM
Related details
- jj
jj (Gerrit)
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Treat static video controls as replaced normal-flow stacking
Static video controls create stacked paint layers, which paint after
inline siblings, causing positioned elements (e.g. buttons) placed on
top of the video to be painted over by the controls.
Fix this by treating static <video controls> as a replaced normal-flow
stacking element.
The hit-test and paint child-skip in PaintLayer is narrowed from
IsReplacedNormalFlowStacking() to IsSVGForeignObject(), since the skip
logic relies on SVG-specific coordinate transforms that don't apply to
video.
- M third_party/blink/renderer/core/css/resolver/style_adjuster.cc
- M third_party/blink/renderer/core/html/media/html_video_element_test.cc
- M third_party/blink/renderer/core/layout/layout_video.cc
- M third_party/blink/renderer/core/layout/layout_video.h
- M third_party/blink/renderer/core/paint/README.md
- M third_party/blink/renderer/core/paint/paint_layer.cc
- M third_party/blink/renderer/core/paint/paint_layer_painter.cc
- M third_party/blink/renderer/core/paint/paint_layer_test.cc
- A third_party/blink/web_tests/external/wpt/html/rendering/replaced-elements/embedded-content/video-controls-stacking-hit-test.html
- A third_party/blink/web_tests/external/wpt/html/rendering/replaced-elements/embedded-content/video-controls-stacking-ref.html
- A third_party/blink/web_tests/external/wpt/html/rendering/replaced-elements/embedded-content/video-controls-stacking.html
- M third_party/blink/web_tests/platform/linux/virtual/dark-color-scheme/media/video-focus-ring-expected.png
- M third_party/blink/web_tests/platform/mac/virtual/dark-color-scheme/media/video-focus-ring-expected.png
- M third_party/blink/web_tests/platform/win/virtual/dark-color-scheme/media/video-focus-ring-expected.png
Code-Review: +1 by Xianzhu Wang, +1 by Mason Freed
| 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/58304
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |