[Omnibox] Fixs stale height regression in WebUI popup [chromium/src : main]
Moe Ahmadi (Gerrit)
Moe Ahmadi voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Aviv Kiss
- Keren Zhu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marlon Facey (Gerrit)
Marlon Facey added 1 comment![]( "Open in Gerrit")
The issue that the manual height restoration was fixing is when the popup is opened and then closed and opened again when there are no results. No resized event happens so the popup has the incorrect size. Do we know if that issue is still fixed?
Related details
- Aviv Kiss
- Keren Zhu
- Moe Ahmadi
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. |
Moe Ahmadi (Gerrit)
Moe Ahmadi added 1 comment![]( "Open in Gerrit")
The issue that the manual height restoration was fixing is when the popup is opened and then closed and opened again when there are no results. No resized event happens so the popup has the incorrect size. Do we know if that issue is still fixed?
I don't know. Can you please test with patchset with the known repro steps?
Related details
- Aviv Kiss
- Keren Zhu
- Marlon Facey
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. |
Aviv Kiss (Gerrit)
Aviv Kiss added 1 comment![]( "Open in Gerrit")
Moe AhmadiThe issue that the manual height restoration was fixing is when the popup is opened and then closed and opened again when there are no results. No resized event happens so the popup has the incorrect size. Do we know if that issue is still fixed?
I don't know. Can you please test with patchset with the known repro steps?
+1, removing the additional `OnContentHeightChanged` call will likely bring back the empty popup bug (assuming this resolved that issue, afaik there have not been new reports since this went in).
Related details
- Keren Zhu
- Marlon Facey
- Moe Ahmadi
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. |
Keren Zhu (Gerrit)
Keren Zhu voted and added 1 comment![]( "Open in Gerrit")
Votes added by Keren Zhu
| Code-Review | +1 |
1 comment
Moe AhmadiThe issue that the manual height restoration was fixing is when the popup is opened and then closed and opened again when there are no results. No resized event happens so the popup has the incorrect size. Do we know if that issue is still fixed?
I don't know. Can you please test with patchset with the known repro steps?
I tried wikipedia.org that is supposed to have no suggestion result. The popup shows up successfully. Marlon, is this the issue you were trying to fix in https://crrev.com/c/7276215? If so, it seems no longer an issue.
https://screencast.googleplex.com/cast/NDgyMTU2NzQ4ODkxNzUwNHw4MDBmYzFjOC1kNQ
Related details
- Marlon Facey
- Moe Ahmadi
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. |
Aviv Kiss (Gerrit)
Aviv Kiss added 1 comment![]( "Open in Gerrit")
Moe AhmadiThe issue that the manual height restoration was fixing is when the popup is opened and then closed and opened again when there are no results. No resized event happens so the popup has the incorrect size. Do we know if that issue is still fixed?
Keren ZhuI don't know. Can you please test with patchset with the known repro steps?
I tried wikipedia.org that is supposed to have no suggestion result. The popup shows up successfully. Marlon, is this the issue you were trying to fix in https://crrev.com/c/7276215? If so, it seems no longer an issue.
https://screencast.googleplex.com/cast/NDgyMTU2NzQ4ODkxNzUwNHw4MDBmYzFjOC1kNQ
I think the issue Marlon's CL was attempting to fix was a problem where if the height didn't change between dismissal -> presentation then `OnContentHeightChanged` was never called and the height stayed at 1.
I don't recall the exact repro steps (the ones I was using were somewhat convoluted) but if we remove this code we need to be confident there are no other paths that have no `OnContentHeightChanged` events and can leave the height at 1. We never had consistent steps to reproduce that bug so it's possible there are other codepaths where it can occur.
Marlon Facey (Gerrit)
Marlon Facey voted and added 2 comments![]( "Open in Gerrit")
Votes added by Marlon Facey
| Code-Review | +1 |
2 comments
LGTM
Moe AhmadiThe issue that the manual height restoration was fixing is when the popup is opened and then closed and opened again when there are no results. No resized event happens so the popup has the incorrect size. Do we know if that issue is still fixed?
Keren ZhuI don't know. Can you please test with patchset with the known repro steps?
Aviv KissI tried wikipedia.org that is supposed to have no suggestion result. The popup shows up successfully. Marlon, is this the issue you were trying to fix in https://crrev.com/c/7276215? If so, it seems no longer an issue.
https://screencast.googleplex.com/cast/NDgyMTU2NzQ4ODkxNzUwNHw4MDBmYzFjOC1kNQ
I think the issue Marlon's CL was attempting to fix was a problem where if the height didn't change between dismissal -> presentation then `OnContentHeightChanged` was never called and the height stayed at 1.
I don't recall the exact repro steps (the ones I was using were somewhat convoluted) but if we remove this code we need to be confident there are no other paths that have no `OnContentHeightChanged` events and can leave the height at 1. We never had consistent steps to reproduce that bug so it's possible there are other codepaths where it can occur.
There should be no results but just tested and it looks fine to me: http://screencast/cast/NTIyOTA1MzQ1MDY0OTYwMHw5NzljODY3Ni00Ng
Related details
- Moe Ahmadi
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. |
Marlon Facey (Gerrit)
Marlon Facey added 1 comment![]( "Open in Gerrit")
The issue that the manual height restoration was fixing is when the popup is opened and then closed and opened again when there are no results. No resized event happens so the popup has the incorrect size. Do we know if that issue is still fixed?
Keren ZhuI don't know. Can you please test with patchset with the known repro steps?
Aviv KissI tried wikipedia.org that is supposed to have no suggestion result. The popup shows up successfully. Marlon, is this the issue you were trying to fix in https://crrev.com/c/7276215? If so, it seems no longer an issue.
https://screencast.googleplex.com/cast/NDgyMTU2NzQ4ODkxNzUwNHw4MDBmYzFjOC1kNQ
Marlon FaceyI think the issue Marlon's CL was attempting to fix was a problem where if the height didn't change between dismissal -> presentation then `OnContentHeightChanged` was never called and the height stayed at 1.
I don't recall the exact repro steps (the ones I was using were somewhat convoluted) but if we remove this code we need to be confident there are no other paths that have no `OnContentHeightChanged` events and can leave the height at 1. We never had consistent steps to reproduce that bug so it's possible there are other codepaths where it can occur.
There should be no results but just tested and it looks fine to me: http://screencast/cast/NTIyOTA1MzQ1MDY0OTYwMHw5NzljODY3Ni00Ng
- Aviv Kiss
- Moe Ahmadi
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Moe Ahmadi (Gerrit)
Moe Ahmadi voted and added 1 comment![]( "Open in Gerrit")
Votes added by Moe Ahmadi
| Commit-Queue | +2 |
1 comment
thanks! fingers crossed
Related details
- Aviv Kiss
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
[Omnibox] Fixs stale height regression in WebUI popup
This CL fixes a visual regression introduced by the WebUI Omnibox
auto resize debouncer. crrev.com/c/7410688 delayed resize events to
prevent flickering where a broken initial frame would bleed into the
tab strip with sharp corners, but caused the popup to render using
stale dimensions from the previous session during the debounce delay,
when combined with changes introduced in crrev.com/c/7276215
By removing the manual height restoration in ShowUI(), we allow
the popup to remain at its reset height of 1px until the debouncer
expires and the renderer provides the correct new height.
- M chrome/browser/ui/views/omnibox/omnibox_popup_presenter.cc
- M chrome/browser/ui/views/omnibox/omnibox_popup_webui_base_content.cc
Code-Review: +1 by Keren Zhu, +1 by Marlon Facey
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |