Fix image centering when viewport meta is disabled on mobile [chromium/src : main]
Perry (Gerrit)
Perry voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Perry (Gerrit)
Perry added 1 comment![]( "Open in Gerrit")
Related details
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 2 comments![]( "Open in Gerrit")
based on the bug comments I think that pdr has more context on this
Do you think we could make a WPT in web_tests/external/wpt? I imagine that this is something that other browsers would care about too, right?
Related details
- Perry
- Philip Rogers
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. |
Philip Rogers (Gerrit)
Philip Rogers added 1 comment![]( "Open in Gerrit")
Do you think we could make a WPT in web_tests/external/wpt? I imagine that this is something that other browsers would care about too, right?
Unfortunately we don't really have mobile coverage in WPT.
Related details
- Joey Arhar
- Perry
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Rogers (Gerrit)
Philip Rogers added 3 comments![]( "Open in Gerrit")
int layout_width =I think this change is good, but because of low test coverage, I'd like to guard this change with an enabled-by-default RuntimeEnabledFeature which we can force-disable in the event that we break some important image usecase.
Can you please change this to roughly:
```
int width = (RuntimeEnabledFeature::ImageDocumentUseLayoutWidthEnabled() ? View()->GetLayoutSize().width() : GetFrame()->GetPage()->GetVisualViewport().Size().width()) / GetFrame()->LayoutZoomFactor();
```
Then add the following in `third_party/blink/renderer/platform/runtime_enabled_features.json5`:
```
{
name: "ImageDocumentUseLayoutWidth",
status: "stable",
},
```
TEST_F(ImageDocumentViewportTest, DivWidthOnMobileWithDisabledViewportMeta) {Can you add two additional testcases that are the same (verify centering) except the images are 10000x1 pixels and 1x10000 pixels? There is probably a better way to include the images than the `JpegImage` approach.
EXPECT_EQ(465, rect->x());This isn't obviously verifying centering to me. Can you add a comment about where this comes from? E.g., is it (980 / 2) - (50 / 2)?
Related details
- Joey Arhar
- Perry
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. |
Perry (Gerrit)
Perry added 4 comments![]( "Open in Gerrit")
I think this change is good, but because of low test coverage, I'd like to guard this change with an enabled-by-default RuntimeEnabledFeature which we can force-disable in the event that we break some important image usecase.
Can you please change this to roughly:
```
int width = (RuntimeEnabledFeature::ImageDocumentUseLayoutWidthEnabled() ? View()->GetLayoutSize().width() : GetFrame()->GetPage()->GetVisualViewport().Size().width()) / GetFrame()->LayoutZoomFactor();
```Then add the following in `third_party/blink/renderer/platform/runtime_enabled_features.json5`:
```
{
name: "ImageDocumentUseLayoutWidth",
status: "stable",
},
```
Done
TEST_F(ImageDocumentViewportTest, DivWidthOnMobileWithDisabledViewportMeta) {Can you add two additional testcases that are the same (verify centering) except the images are 10000x1 pixels and 1x10000 pixels? There is probably a better way to include the images than the `JpegImage` approach.
Done
This isn't obviously verifying centering to me. Can you add a comment about where this comes from? E.g., is it (980 / 2) - (50 / 2)?
- Joey Arhar
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Rogers (Gerrit)
Philip Rogers voted and added 1 comment![]( "Open in Gerrit")
Votes added by Philip Rogers
| Code-Review | +1 |
1 comment
- Joey Arhar
- Perry
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Perry (Gerrit)
Perry voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| 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
Fix image centering when viewport meta is disabled on mobile
On mobile, when viewport meta is disabled, the layout width is greater
than the viewport width, resulting in the image not being centered. This
CL uses layout width instead of viewport width to calculate div width,
ensuring that the image is displayed in the center.
- M third_party/blink/renderer/core/html/image_document.cc
- M third_party/blink/renderer/core/html/image_document.h
- M third_party/blink/renderer/core/html/image_document_test.cc
- M third_party/blink/renderer/platform/runtime_enabled_features.json5
Code-Review: +1 by Philip Rogers
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |