Fix selection highlight artifacts for faded selectable text#183628
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a rendering artifact for faded selectable text by separating the painting of selection highlights from the text fading layer. The refactoring of _SelectableFragment into paintSelection and paintHandles is a good improvement for code clarity. I have one suggestion to improve the robustness of the canvas state management during painting.
| if (_needsClipping) { | ||
| context.canvas.save(); | ||
| context.canvas.clipRect(offset & size); | ||
| } | ||
| for (final _SelectableFragment fragment in _lastSelectableFragments!) { | ||
| fragment.paintSelection(context, offset); | ||
| } | ||
| if (_needsClipping) { | ||
| context.canvas.restore(); | ||
| } |
There was a problem hiding this comment.
To ensure the canvas state is always restored even if an exception occurs during painting the selection, it's safer to wrap the painting logic in a try...finally block. This prevents leaving the canvas in a corrupted state if paintSelection throws an exception.
if (_needsClipping) {
context.canvas.save();
context.canvas.clipRect(offset & size);
}
try {
for (final _SelectableFragment fragment in _lastSelectableFragments!) {
fragment.paintSelection(context, offset);
}
} finally {
if (_needsClipping) {
context.canvas.restore();
}
}|
Hi @ikramhasan, thank you for the fix! The approach looks good to me but I had a question. Also before moving forward with this change it will need a test to verify the expected behavior. |
|
|
||
| if (_lastSelectableFragments != null) { | ||
| for (final _SelectableFragment fragment in _lastSelectableFragments!) { | ||
| fragment.paintHandles(context, offset); |
There was a problem hiding this comment.
Is there a reason why we separate the handles from the selection painting now? It makes sense in that we probably do not want to clip the handles when painting them but curious if there is another reason.
|
Hi @Renzo-Olivares sorry for the late reply. I was on my Eid vacation.
It was split as I needed a way to paint the selection and handle separately. The previous
Sure. It should be a golden test right? |
|
@ikramhasan yeah a golden test feels appropriate here. |
Renzo-Olivares
left a comment
There was a problem hiding this comment.
Overall change LGTM, but we still need a test to land this.
|
Updated with a test @Renzo-Olivares |
|
Hey @Renzo-Olivares could you have another look at this one? |
Renzo-Olivares
left a comment
There was a problem hiding this comment.
LGTM w/ some small comments.
|
Updated the test with your feedback @Renzo-Olivares |
|
autosubmit label was removed for flutter/flutter/183628, because - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
Google testing failed but seems unrelated. Going to re-run. |
flutter/flutter@0541913...1ceffd1 2026-05-14 ikramhasan.dev@gmail.com Fix selection highlight artifacts for faded selectable text (flutter/flutter#183628) 2026-05-14 46920873+gabrimatic@users.noreply.github.com Fix nested Text.rich semantics order (flutter/flutter#186116) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC bmparr@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…#11719) flutter/flutter@0541913...1ceffd1 2026-05-14 ikramhasan.dev@gmail.com Fix selection highlight artifacts for faded selectable text (flutter/flutter#183628) 2026-05-14 46920873+gabrimatic@users.noreply.github.com Fix nested Text.rich semantics order (flutter/flutter#186116) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC bmparr@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Summary
Fixes a rendering artifact when selecting text inside
SelectionAreafor aText/RichTextthat usesTextOverflow.fade.Previously,
RenderParagraphpainted selection highlights into the same saveLayer used for the fade overflow shader. That caused the fade compositing path to affect the selection rects and produced dark border/line artifacts during selection.This change separates selection highlight painting from the faded text layer so the fade shader only applies to the text content.
Fixes: #182776
Changes
RenderParagraphselection highlights outside the overflow fade saveLayerScreenshots