Make select popup anchor stay within web contents [chromium/src : main]
Mason Freed (Gerrit)
Mason Freed added 3 comments![]( "Open in Gerrit")
gfx::Rect viewport_rect(This looks like just the size (at offset 0,0) of the viewport, and (maybe?) isn't scaled by dpr. How does this work? Also, what happens if the intersection is null (no overlap)?
<style>
body { margin: 0; }
select {
position: fixed;
left: -50px;
top: -50px;
width: 200px;
height: 200px;
}
</style>I think it'd be good to add a few more test cases, like the select is off screen down and to the right. Or has zero size. And test with different device pixel ratios, to make sure that math checks out. Etc.
// Make select popup anchor stay within web contents to preventPlease add a bug link, and the milestones when this shipped and can be removed.
Related details
- Joey Arhar
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. |
Joey Arhar (Gerrit)
Joey Arhar added 2 comments![]( "Open in Gerrit")
gfx::Rect viewport_rect(This looks like just the size (at offset 0,0) of the viewport, and (maybe?) isn't scaled by dpr. How does this work? Also, what happens if the intersection is null (no overlap)?
Also, what happens if the intersection is null (no overlap)?
This code prevents the popup from showing at all when there is no overlap. I manually tested and verified it: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/forms/select_type.cc;l=754-761;drc=d6c78bad90b464eac484e75021d86c69d054021f
With this in mind, I added a CHECK to make sure that the intersection is not null.
This looks like just the size (at offset 0,0) of the viewport
Based on my manual testing, this does correctly move the macos popup to stay within the web contents in the problematic case where the select element's button is mostly outside of the viewport. So I think that this is the right thing.
and (maybe?) isn't scaled by dpr
I added tests with dpr and manually tested on my mac with dpr of 1.0 and 2.0 by using the builtin screen and an external monitor, and the normal case and the partially outside the viewport select case both look correct to me.
I also tested pinch zoom and panning to move the select element to the bottom right corner of the viewport, and the popup is still in the correct spot.
After adding more comprehensive tests, I found that clipping the select in the bottom right corner had an issue with dpr which ai fixed by adding dpr logic to the early return in MenuListSelectType::ShowPopup which matches the dpr calculations in externalpopupmenu.
<style>
body { margin: 0; }
select {
position: fixed;
left: -50px;
top: -50px;
width: 200px;
height: 200px;
}
</style>I think it'd be good to add a few more test cases, like the select is off screen down and to the right. Or has zero size. And test with different device pixel ratios, to make sure that math checks out. Etc.
- Mason Freed
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 5 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
5 comments
Thanks for the expanded test. Still some things to fix, but I'll mark it +1.
gfx::Rect viewport_rect(Joey ArharThis looks like just the size (at offset 0,0) of the viewport, and (maybe?) isn't scaled by dpr. How does this work? Also, what happens if the intersection is null (no overlap)?
Also, what happens if the intersection is null (no overlap)?
This code prevents the popup from showing at all when there is no overlap. I manually tested and verified it: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/forms/select_type.cc;l=754-761;drc=d6c78bad90b464eac484e75021d86c69d054021f
With this in mind, I added a CHECK to make sure that the intersection is not null.
This looks like just the size (at offset 0,0) of the viewport
Based on my manual testing, this does correctly move the macos popup to stay within the web contents in the problematic case where the select element's button is mostly outside of the viewport. So I think that this is the right thing.
and (maybe?) isn't scaled by dpr
I added tests with dpr and manually tested on my mac with dpr of 1.0 and 2.0 by using the builtin screen and an external monitor, and the normal case and the partially outside the viewport select case both look correct to me.
I also tested pinch zoom and panning to move the select element to the bottom right corner of the viewport, and the popup is still in the correct spot.
After adding more comprehensive tests, I found that clipping the select in the bottom right corner had an issue with dpr which ai fixed by adding dpr logic to the early return in MenuListSelectType::ShowPopup which matches the dpr calculations in externalpopupmenu.
Great! Thanks. The CHECK scared me a little, but I see that ShowPopup should keep that from getting hit.
#ifdef OS_ANDROIDWhat's with these android differences? That feels a little weird.
left: )HTML" +This is kind of hard to parse - maybe use base::StringPrintf instead?
// https://issues.chromium.org/issues/511869411Can you use the normal `TODO(crbug...)` convention. And shipped in 150, removed in 152.
Related details
- Joey Arhar
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. |
Joey Arhar (Gerrit)
Joey Arhar voted and added 4 comments![]( "Open in Gerrit")
Votes added by Joey Arhar
| Code-Review | +1 |
| Commit-Queue | +2 |
4 comments
What's with these android differences? That feels a little weird.
i added a comment explaining why it is there, it just corresponds to an existing os_android check in external_popup_menu.cc, and the next test in this file does a similar thing but disables the test completely on andorid
This is kind of hard to parse - maybe use base::StringPrintf instead?
Done
// Make select popup anchor stay within web contents to preventPlease add a bug link, and the milestones when this shipped and can be removed.
Done
Can you use the normal `TODO(crbug...)` convention. And shipped in 150, removed in 152.
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |