Fix selection handles hidden when line-height exceeds input height [chromium/src : main]
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
dear reviewers thanks in advance for your time and patience,
this fixes the bug, see screenshots in the issue.
please let me know if you want me to address anything!
Related details
- Florin Malita
- Jonathan Ross
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jonathan Ross (Gerrit)
Jonathan Ross added 1 comment![]( "Open in Gerrit")
+pdr@ who reviewed the original CL: https://chromium-review.googlesource.com/c/chromium/src/+/6828942 as they have more context on this class
Related details
- Florin Malita
- Helmut Januschka
- 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 added 3 comments![]( "Open in Gerrit")
Fix selection handles hidden when line-height exceeds input heightDid you check if this works end-to-end? I think the code in ComputeViewportSelectionBound will hide these handles? Or maybe that code won't for some reason?
// Use the full selection edge for visibility testing. The handle should beYou removed the comment about ComputeViewportSelectionBound. Is that still important to keep?
gfx::Rect sample = gfx::BoundingRect(bound.edge_start, bound.edge_end);Can you wrap this in a feature check of some kind (similar to the SelectionHandleWithBottomClippedEnabled code you removed)? We've had issues with regressions in this area, and having a runtime enabled feature will let us revert back to the code before your change in an emergency situation.
If this change is entirely in blink/, a RuntimeEnabledFeature is easiest. If this needs to touch cc code too, cc/base/features.h may be the best.
Related details
- Florin Malita
- 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 3 comments![]( "Open in Gerrit")
Fix selection handles hidden when line-height exceeds input heightDid you check if this works end-to-end? I think the code in ComputeViewportSelectionBound will hide these handles? Or maybe that code won't for some reason?
tried with and without patch, see screenshot in issue (https://issues.chromium.org/issues/451833352)
// Use the full selection edge for visibility testing. The handle should beYou removed the comment about ComputeViewportSelectionBound. Is that still important to keep?
sorry restored the ComputeViewportSelectionBound() reference and clarified the intent.
gfx::Rect sample = gfx::BoundingRect(bound.edge_start, bound.edge_end);Can you wrap this in a feature check of some kind (similar to the SelectionHandleWithBottomClippedEnabled code you removed)? We've had issues with regressions in this area, and having a runtime enabled feature will let us revert back to the code before your change in an emergency situation.
If this change is entirely in blink/, a RuntimeEnabledFeature is easiest. If this needs to touch cc code too, cc/base/features.h may be the best.
- Florin Malita
- 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
Related details
- Florin Malita
- Helmut Januschka
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Florin Malita
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 selection handles hidden when line-height exceeds input height
Use the full selection edge for visibility testing instead of a 1px sample
near edge_end, so handles show when any part of the edge is within the clip.
- M third_party/blink/renderer/core/paint/selection_bounds_recorder_test.cc
- M third_party/blink/renderer/platform/graphics/compositing/paint_chunks_to_cc_layer.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. |
Xianzhu Wang (Gerrit)
Xianzhu Wang added 1 comment![]( "Open in Gerrit")
sample = gfx::BoundingRect(bound.edge_start, bound.edge_end);This behavior will be inconsistent with the CC side behavior, which will make the visibility of selection handles along composited layer bounds behaves differently. For example, in the composited code path, selections with only the top half visible won't have selection handles. Also when only the top of the selection is visible, the selection handle will have a distance to the visible selection, which looks weird.
I think at least we need to keep the two code paths behave the same, i.e. also implement this behavior in cc.
Related details
- Helmut Januschka
Code-Coverage
Code-Owners
Code-Review
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")
sample = gfx::BoundingRect(bound.edge_start, bound.edge_end);This behavior will be inconsistent with the CC side behavior, which will make the visibility of selection handles along composited layer bounds behaves differently. For example, in the composited code path, selections with only the top half visible won't have selection handles. Also when only the top of the selection is visible, the selection handle will have a distance to the visible selection, which looks weird.
I think at least we need to keep the two code paths behave the same, i.e. also implement this behavior in cc.
@wangx...@chromium.org oh, thanks for the explanation, will do a follow up!
Related details
Code-Coverage
Code-Owners
Code-Review
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")
sample = gfx::BoundingRect(bound.edge_start, bound.edge_end);Helmut JanuschkaThis behavior will be inconsistent with the CC side behavior, which will make the visibility of selection handles along composited layer bounds behaves differently. For example, in the composited code path, selections with only the top half visible won't have selection handles. Also when only the top of the selection is visible, the selection handle will have a distance to the visible selection, which looks weird.
I think at least we need to keep the two code paths behave the same, i.e. also implement this behavior in cc.
@wangx...@chromium.org oh, thanks for the explanation, will do a follow up!
follow up: https://chromium-review.googlesource.com/c/chromium/src/+/7614280
test APK's and screenshots are here: https://static.januschka.com/issue-451833352/
Xianzhu Wang (Gerrit)
Xianzhu Wang added 1 comment![]( "Open in Gerrit")
return start < end ? -1 : start > end ? 1 : 0;Another method is to change `-1` and `1` here to something like `4` to cover large device scale factors. That would be a simplication of my original idea to plumb device scale factor here. I think this is better than the new behavior with gap between the selection and the selection handle when the bottom of the text is clipped. Is there any chance you could test this method?
Related details
- Helmut Januschka
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |