Fix time datalist Other picker when popup opens upward [chromium/src : main]
Helmut Januschka (Gerrit)
Helmut Januschka voted and added 1 comment![]( "Open in Gerrit")
Votes added by Helmut Januschka
| Commit-Queue | +1 |
1 comment
dba...@chromium.org, thanks in advance for your review and time.
I was able to reproduce this on my Windows machine, and this CL fixes the issue for me. On an unpatched debug build, clicking "Other" can hit:
```
FATAL:cc/layers/nine_patch_generator.cc:75
DCHECK failed: output_bounds_.width() >= border_.width() (1 vs. 4)
```
Top of the stack:
```
cc::NinePatchGenerator::CheckGeometryLimitations
cc::NinePatchLayerImpl::AppendQuads
cc::LayerTreeHostImpl::CalculateRenderPasses
cc::LayerTreeHostImpl::PrepareToDraw
cc::SingleThreadProxy::DoComposite
```
The issue appears to be that the current "Other" path calls hideWindow(), which resizes the page popup to 1x1, and on Windows that can go through the nine-patch compositor path where the border is wider than the output bounds. openCalendarPicker() already replaces the picker contents and resizes the popup to the real picker size, so i think its safe to keep hideWindow removed.
guess on other platforms a 1x1 might be silently hidden/dismissed?
please let me know if you want me to change anything
Related details
- David Baron
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. |
David Baron (Gerrit)
David Baron added 2 comments![]( "Open in Gerrit")
Open the time picker directly instead of hiding the suggestion popup to a 1x1 intermediate window.Did you make any attempts to test the bugs that were being fixed in https://chromium.googlesource.com/chromium/src/+/a0433141504575df36b4d1d3646fe6424077ed86%5E%21 and https://chromium.googlesource.com/chromium/src/+/79ec2e579e87f6bf28dcbf0e4cb602290288a4df%5E%21 ?
hideWindow();This was the only call to `hideWindow()` so you should at least remove `hideWindow()` and maybe also the `isWindowHidden()` and the `didHide` event dispatching in `picker_common.js`. I think there's also a related leftover `removeEventListener` in `list_picker.js`.
Related details
- Helmut Januschka
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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
Open the time picker directly instead of hiding the suggestion popup to a 1x1 intermediate window.Did you make any attempts to test the bugs that were being fixed in https://chromium.googlesource.com/chromium/src/+/a0433141504575df36b4d1d3646fe6424077ed86%5E%21 and https://chromium.googlesource.com/chromium/src/+/79ec2e579e87f6bf28dcbf0e4cb602290288a4df%5E%21 ?
thanks david, i'v been building a few test cases around the commits you mentioned (thanks), and it seems i'v been jumping head first into a can of worms, there is edge cases and craziness on every platform, i'll put the CL into WIP, and try to lock myself in over the weekend 😆 (seems to be windows with it's 4x4 glitch is the smallest problem)
Related details
- David Baron
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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 4 comments![]( "Open in Gerrit")
dba...@chromium.org, thanks in advance for your review and time.
I was able to reproduce this on my Windows machine, and this CL fixes the issue for me. On an unpatched debug build, clicking "Other" can hit:
```
FATAL:cc/layers/nine_patch_generator.cc:75
DCHECK failed: output_bounds_.width() >= border_.width() (1 vs. 4)
```Top of the stack:
```
cc::NinePatchGenerator::CheckGeometryLimitations
cc::NinePatchLayerImpl::AppendQuads
cc::LayerTreeHostImpl::CalculateRenderPasses
cc::LayerTreeHostImpl::PrepareToDraw
cc::SingleThreadProxy::DoComposite
```The issue appears to be that the current "Other" path calls hideWindow(), which resizes the page popup to 1x1, and on Windows that can go through the nine-patch compositor path where the border is wider than the output bounds. openCalendarPicker() already replaces the picker contents and resizes the popup to the real picker size, so i think its safe to keep hideWindow removed.
guess on other platforms a 1x1 might be silently hidden/dismissed?
please let me know if you want me to change anything
PS2 is ready for review. I reworked this to avoid the 1x1 hide-window transition entirely and to preserve the old flicker/transition behavior that the earlier fixes were trying to protect.
I also recorded current Chrome stable on macOS showing the edge cases still broken with my sampler page:
https://static.januschka.com/i-348520442/edge-cases.html
Video:
https://www.youtube.com/watch?v=K4t8jc4UUfQ
Trybots are green, and I manually tested across platforms. PTAL.
PS2 is ready for review. I reworked this to avoid the 1x1 hide-window transition entirely and fix the root cause.
While testing the sampler here:
https://static.januschka.com/i-348520442/edge-cases.html
I noticed this is currently broken even on stable on macOS. Video:
https://www.youtube.com/watch?v=K4t8jc4UUfQ
With the current PS2, trybots are green, and I manually tested across platforms. PTAL.
Let me know if you want me to address anything.
Open the time picker directly instead of hiding the suggestion popup to a 1x1 intermediate window.Helmut JanuschkaDid you make any attempts to test the bugs that were being fixed in https://chromium.googlesource.com/chromium/src/+/a0433141504575df36b4d1d3646fe6424077ed86%5E%21 and https://chromium.googlesource.com/chromium/src/+/79ec2e579e87f6bf28dcbf0e4cb602290288a4df%5E%21 ?
thanks david, i'v been building a few test cases around the commits you mentioned (thanks), and it seems i'v been jumping head first into a can of worms, there is edge cases and craziness on every platform, i'll put the CL into WIP, and try to lock myself in over the weekend 😆 (seems to be windows with it's 4x4 glitch is the smallest problem)
ok it should work now, and mentioned bugs should not regress.
hideWindow();This was the only call to `hideWindow()` so you should at least remove `hideWindow()` and maybe also the `isWindowHidden()` and the `didHide` event dispatching in `picker_common.js`. I think there's also a related leftover `removeEventListener` in `list_picker.js`.
Done in PS2. I removed the now-unused `hideWindow()` path, the related hidden-window state/event handling, the obsolete test for it, and the leftover listener cleanup in `list_picker.js`.
Related details
- David Baron
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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
Helmut Januschkadba...@chromium.org, thanks in advance for your review and time.
I was able to reproduce this on my Windows machine, and this CL fixes the issue for me. On an unpatched debug build, clicking "Other" can hit:
```
FATAL:cc/layers/nine_patch_generator.cc:75
DCHECK failed: output_bounds_.width() >= border_.width() (1 vs. 4)
```Top of the stack:
```
cc::NinePatchGenerator::CheckGeometryLimitations
cc::NinePatchLayerImpl::AppendQuads
cc::LayerTreeHostImpl::CalculateRenderPasses
cc::LayerTreeHostImpl::PrepareToDraw
cc::SingleThreadProxy::DoComposite
```The issue appears to be that the current "Other" path calls hideWindow(), which resizes the page popup to 1x1, and on Windows that can go through the nine-patch compositor path where the border is wider than the output bounds. openCalendarPicker() already replaces the picker contents and resizes the popup to the real picker size, so i think its safe to keep hideWindow removed.
guess on other platforms a 1x1 might be silently hidden/dismissed?
please let me know if you want me to change anything
PS2 is ready for review. I reworked this to avoid the 1x1 hide-window transition entirely and to preserve the old flicker/transition behavior that the earlier fixes were trying to protect.
I also recorded current Chrome stable on macOS showing the edge cases still broken with my sampler page:
https://static.januschka.com/i-348520442/edge-cases.htmlVideo:
https://www.youtube.com/watch?v=K4t8jc4UUfQTrybots are green, and I manually tested across platforms. PTAL.
outdated!
Related details
- David Baron
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |