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. |
David Baron (Gerrit)
David Baron added 2 comments![]( "Open in Gerrit")
I'm somewhat uncomfortable with the approach here -- but I'm also not all that familiar with the javascript implementations of these pickers. cc:ing @dan...@microsoft.com who has worked on them more and might have a stronger opinion.
* Returns the popup size this picker will request from adjustHeight() andSo I've looked at this a few times, and it still makes me very uncomfortable to do something like this and duplicate the layout code into another method. This seems to require that anybody making future changes to the picker layouts also update these parallel methods. (There are not even any comments pointing that out -- but I also don't want you to think that that means that good enough comments would make the approach acceptable.)
This seems like too much complexity for what feels like it should be a small-ish bug fix.
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. |
Dan Clark (Gerrit)
Dan Clark added 1 comment![]( "Open in Gerrit")
* Returns the popup size this picker will request from adjustHeight() andSo I've looked at this a few times, and it still makes me very uncomfortable to do something like this and duplicate the layout code into another method. This seems to require that anybody making future changes to the picker layouts also update these parallel methods. (There are not even any comments pointing that out -- but I also don't want you to think that that means that good enough comments would make the approach acceptable.)
This seems like too much complexity for what feels like it should be a small-ish bug fix.
I'm also a little concerned with the added complexity with the duplicated layout calculations.
The big thing I still don't understand about this bug is, why does this only happen when the pickers open above the element? Is there some more scoped thing we can do to make the "above the element" case work like the "below the element" case?
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
* Returns the popup size this picker will request from adjustHeight() andDan ClarkSo I've looked at this a few times, and it still makes me very uncomfortable to do something like this and duplicate the layout code into another method. This seems to require that anybody making future changes to the picker layouts also update these parallel methods. (There are not even any comments pointing that out -- but I also don't want you to think that that means that good enough comments would make the approach acceptable.)
This seems like too much complexity for what feels like it should be a small-ish bug fix.
I'm also a little concerned with the added complexity with the duplicated layout calculations.
The big thing I still don't understand about this bug is, why does this only happen when the pickers open above the element? Is there some more scoped thing we can do to make the "above the element" case work like the "below the element" case?
Thanks you both are right with being concerned. managed to reduce the duplicate layout logic.
For the underlying bug: this only shows up when the picker opens above the element because the suggestion popup starts with a smaller rect, then the calendar is swapped in and asks the browser to resize/reposition the popup.
In the above-anchor case, growing the popup changes its top edge, so there can be a frame where the calendar is painted into the old suggestion-sized surface/position.
Related details
- Dan Clark
- 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 1 comment![]( "Open in Gerrit")
* Returns the popup size this picker will request from adjustHeight() andDan ClarkSo I've looked at this a few times, and it still makes me very uncomfortable to do something like this and duplicate the layout code into another method. This seems to require that anybody making future changes to the picker layouts also update these parallel methods. (There are not even any comments pointing that out -- but I also don't want you to think that that means that good enough comments would make the approach acceptable.)
This seems like too much complexity for what feels like it should be a small-ish bug fix.
Helmut JanuschkaI'm also a little concerned with the added complexity with the duplicated layout calculations.
The big thing I still don't understand about this bug is, why does this only happen when the pickers open above the element? Is there some more scoped thing we can do to make the "above the element" case work like the "below the element" case?
Thanks you both are right with being concerned. managed to reduce the duplicate layout logic.
For the underlying bug: this only shows up when the picker opens above the element because the suggestion popup starts with a smaller rect, then the calendar is swapped in and asks the browser to resize/reposition the popup.
In the above-anchor case, growing the popup changes its top edge, so there can be a frame where the calendar is painted into the old suggestion-sized surface/position.
Sorry for the delay getting back to you -- but I guess my main concern is that I don't think this comment was addressed. There's still a bunch of duplicate layout code right here. I'm not sure what the right approach is (although it seems likely to involve a mechanism that's capable of doing relative-to-bottom positioning), but I don't think the underlying concern here about duplicate layout code has been addressed.
Related details
- Dan Clark
- 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")
* Returns the popup size this picker will request from adjustHeight() andDan ClarkSo I've looked at this a few times, and it still makes me very uncomfortable to do something like this and duplicate the layout code into another method. This seems to require that anybody making future changes to the picker layouts also update these parallel methods. (There are not even any comments pointing that out -- but I also don't want you to think that that means that good enough comments would make the approach acceptable.)
This seems like too much complexity for what feels like it should be a small-ish bug fix.
Helmut JanuschkaI'm also a little concerned with the added complexity with the duplicated layout calculations.
The big thing I still don't understand about this bug is, why does this only happen when the pickers open above the element? Is there some more scoped thing we can do to make the "above the element" case work like the "below the element" case?
David BaronThanks you both are right with being concerned. managed to reduce the duplicate layout logic.
For the underlying bug: this only shows up when the picker opens above the element because the suggestion popup starts with a smaller rect, then the calendar is swapped in and asks the browser to resize/reposition the popup.
In the above-anchor case, growing the popup changes its top edge, so there can be a frame where the calendar is painted into the old suggestion-sized surface/position.
Sorry for the delay getting back to you -- but I guess my main concern is that I don't think this comment was addressed. There's still a bunch of duplicate layout code right here. I'm not sure what the right approach is (although it seems likely to involve a mechanism that's capable of doing relative-to-bottom positioning), but I don't think the underlying concern here about duplicate layout code has been addressed.
Hey David, thanks for sticking with this one. I want to make sure I take the direction you'd actually be happy with before I sink more time in, i see:
[1] Drop the predicted-size code entirely and size the popup from the picker's real layout. The pickers already hide their content across the resize, so I can open the picker inert, let it do its normal single resizeWindow() with its actual DOM size, and reveal after the resize ack. Small, scoped, but it's still "JS papers over a two-step resize."
[2] bottom-edge-anchored repositioning in WebPagePopupImpl so an above-anchor popup keeps its bottom edge fixed across a resize. That's the "correct" fix, but it's a cross-process surface-sync change and feels heavy for what started as a small bug.
happy to go either route, what would you rather see land?
Related details
- Dan Clark
- 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")
* Returns the popup size this picker will request from adjustHeight() andDan ClarkSo I've looked at this a few times, and it still makes me very uncomfortable to do something like this and duplicate the layout code into another method. This seems to require that anybody making future changes to the picker layouts also update these parallel methods. (There are not even any comments pointing that out -- but I also don't want you to think that that means that good enough comments would make the approach acceptable.)
This seems like too much complexity for what feels like it should be a small-ish bug fix.
Helmut JanuschkaI'm also a little concerned with the added complexity with the duplicated layout calculations.
The big thing I still don't understand about this bug is, why does this only happen when the pickers open above the element? Is there some more scoped thing we can do to make the "above the element" case work like the "below the element" case?
David BaronThanks you both are right with being concerned. managed to reduce the duplicate layout logic.
For the underlying bug: this only shows up when the picker opens above the element because the suggestion popup starts with a smaller rect, then the calendar is swapped in and asks the browser to resize/reposition the popup.
In the above-anchor case, growing the popup changes its top edge, so there can be a frame where the calendar is painted into the old suggestion-sized surface/position.
Helmut JanuschkaSorry for the delay getting back to you -- but I guess my main concern is that I don't think this comment was addressed. There's still a bunch of duplicate layout code right here. I'm not sure what the right approach is (although it seems likely to involve a mechanism that's capable of doing relative-to-bottom positioning), but I don't think the underlying concern here about duplicate layout code has been addressed.
Hey David, thanks for sticking with this one. I want to make sure I take the direction you'd actually be happy with before I sink more time in, i see:
[1] Drop the predicted-size code entirely and size the popup from the picker's real layout. The pickers already hide their content across the resize, so I can open the picker inert, let it do its normal single resizeWindow() with its actual DOM size, and reveal after the resize ack. Small, scoped, but it's still "JS papers over a two-step resize."
[2] bottom-edge-anchored repositioning in WebPagePopupImpl so an above-anchor popup keeps its bottom edge fixed across a resize. That's the "correct" fix, but it's a cross-process surface-sync change and feels heavy for what started as a small bug.
happy to go either route, what would you rather see land?
any ideas?