Fix that RenderEditable (TextField) ignores offset in painting, making text selections shifted when offset is nonzero #109287

fzyzcjy · 2022-08-10T05:54:50Z

I will start adding tests once flutter team thinks this PR is acceptable, since I am not sure whether the team like to fix it, and if the team does not like it I will not waste time on it :)

When implementing RenderBox.paint, we know we should respect the offset parameter. However, even though _paintContents respects it, the _paintHandleLayers does not - instead it completely ignores the offset parameter. This is logically wrong as it does not respect the definition and requirements of the function.

Luckily (or unluckily), currently it does not have visible harm yet. This is because RenderEditable is used by _Editable and future by EditableText. And, the _Editable's ancestors widgets do not provide any offset (because the widgets in the same Layer paint child without shifting). Thus, offset parameter is always zero in the current version of Flutter.

However, I am still proposing this PR because of the following reasons:

It is logically wrong (violates definition of paint), so by definition it is a bug and we should fix it.
It may be a visible bug in future releases. With adding more functionality to text fields, surely we cannot guarantee RenderEditable's ancestors always give a zero offset. (If we think so, I should immediately raise another PR with assert(offset==Offset.zero) to enforce it.)
Users who need deep customization may directly use RenderEditable (since it is public), and even copy some of the source code and hack it (e.g. me). With their customizations, it is highly possible that the offset is no longer always zero, then they will find suddenly the text selections shifted weirdly. (Disclaimer: That is my case, and why I find this bug)

I have not come up with a way to make tests about this, because _Editable is private, and as mentioned above, it is logically wrong but not visible now.
However, this PR does not add any new logic, so making existing tests pass IMHO may be sufficient.

Close #109289

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

P.S. https://discord.com/channels/608014603317936148/608018585025118217/1006808038914654218

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flutter-dashboard · 2022-08-10T06:06:31Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

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.

fzyzcjy · 2022-08-12T14:19:32Z

@justinmc Hi, as discussed by @Hixie at here:

@Hixie : seems fine to me but @justin McCandless (justinmc) is the person we should really ask

So I wonder whether this PR is ok for you? If so I will spend time adding tests.

justinmc · 2022-08-12T22:06:59Z

As discussed on Discord, the approach in this PR looks good to me. Please tag me or request a review from me when you finish the tests.

fzyzcjy · 2022-08-13T00:45:25Z

@justinmc New tests added and passed :)

fzyzcjy · 2022-08-15T07:28:15Z

@justinmc Hi, is there any updates?

justinmc

LGTM 👍

Thanks for fixing this and for adding the test! Just one nit below and a question. Maybe we can make it more clear where the 14 value comes from in the test if possible, otherwise no big deal.

justinmc · 2022-08-15T16:41:25Z

packages/flutter/test/rendering/editable_test.dart

+
+    final List<LeaderLayer> leaderLayers = context.pushedLayers.whereType<LeaderLayer>().toList();
+    expect(leaderLayers, hasLength(1), reason: '_paintHandleLayers will paint a LeaderLayer');
+    expect(leaderLayers.single.offset, const Offset(0, 14) + paintOffset, reason: 'offset should respect paintOffset');


Nit: 0.0 and 14.0 for doubles.

Also, where does that 14 come from exactly?

Also, where does that 14 come from exactly?

LeaderLayer(link: endHandleLayerLink, offset: endPoint + offset),

That endPoint.

That is fontsize. I have make it more explicit now. (Will see whether CI is happy)

CI looks happy about that

That looks much more obvious about what's happening, thank you!

Renzo-Olivares

LGTM

fzyzcjy · 2022-08-16T06:47:15Z

All tests have passed

…ing, making text selections shifted when offset is nonzero (flutter/flutter#109287)

…g, making text selections shifted when offset is nonzero (flutter/flutter#109287)

Update editable.dart

2e94e4b

flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Aug 10, 2022

fzyzcjy changed the title ~~Fix that RenderEditable (TextField) ignores offset in painting~~ Fix that RenderEditable (TextField) ignores offset in painting, making text selections shifted when offset is nonzero Aug 10, 2022

fzyzcjy marked this pull request as ready for review August 10, 2022 06:05

fzyzcjy mentioned this pull request Aug 10, 2022

RenderEditable (TextField) ignores offset in painting, making text selections shifted when offset is nonzero #109289

Closed

HansMuller requested a review from justinmc August 12, 2022 22:14

fzyzcjy added 5 commits August 13, 2022 07:26

Update editable_test.dart

52cf214

Update rendering_tester.dart

b96412e

Update editable_test.dart

0947320

Update rendering_tester.dart

b0a4ad5

Update editable_test.dart

04768dd

justinmc approved these changes Aug 15, 2022

View reviewed changes

justinmc requested review from Renzo-Olivares and LongCatIsLooong August 15, 2022 16:42

Renzo-Olivares approved these changes Aug 15, 2022

View reviewed changes

fzyzcjy added 3 commits August 16, 2022 06:23

nit: add .0 postfix to make it double

f746b47

Update editable_test.dart

36ef8fe

Update editable_test.dart

b994480

justinmc added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 16, 2022

justinmc merged commit 0e85609 into flutter:master Aug 16, 2022

This was referenced Aug 17, 2022

Roll Flutter from 068fb57d5f82 to cb936c51f8b2 (22 revisions) flutter/packages#2469

Merged

Roll Flutter from c873c2100ad7 to cb936c51f8b2 (40 revisions) flutter/plugins#6285

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2022

0e85609cf Fix that RenderEditable (TextField) ignores offset in paint…

c713f05

…ing, making text selections shifted when offset is nonzero (flutter/flutter#109287)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 17, 2022

0e85609 Fix that RenderEditable (TextField) ignores offset in paintin…

4b32fc4

…g, making text selections shifted when offset is nonzero (flutter/flutter#109287)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix that RenderEditable (TextField) ignores offset in painting, making text selections shifted when offset is nonzero #109287

Fix that RenderEditable (TextField) ignores offset in painting, making text selections shifted when offset is nonzero #109287

fzyzcjy commented Aug 10, 2022 •

edited

flutter-dashboard bot commented Aug 10, 2022

fzyzcjy commented Aug 12, 2022

justinmc commented Aug 12, 2022

fzyzcjy commented Aug 13, 2022

fzyzcjy commented Aug 15, 2022

justinmc left a comment

justinmc Aug 15, 2022

fzyzcjy Aug 15, 2022

fzyzcjy Aug 15, 2022

fzyzcjy Aug 16, 2022

justinmc Aug 16, 2022

Renzo-Olivares left a comment

fzyzcjy commented Aug 16, 2022 •

edited

Fix that RenderEditable (TextField) ignores offset in painting, making text selections shifted when offset is nonzero #109287

Fix that RenderEditable (TextField) ignores offset in painting, making text selections shifted when offset is nonzero #109287

Conversation

fzyzcjy commented Aug 10, 2022 • edited

Pre-launch Checklist

flutter-dashboard bot commented Aug 10, 2022

fzyzcjy commented Aug 12, 2022

justinmc commented Aug 12, 2022

fzyzcjy commented Aug 13, 2022

fzyzcjy commented Aug 15, 2022

justinmc left a comment

Choose a reason for hiding this comment

justinmc Aug 15, 2022

Choose a reason for hiding this comment

fzyzcjy Aug 15, 2022

Choose a reason for hiding this comment

fzyzcjy Aug 15, 2022

Choose a reason for hiding this comment

fzyzcjy Aug 16, 2022

Choose a reason for hiding this comment

justinmc Aug 16, 2022

Choose a reason for hiding this comment

Renzo-Olivares left a comment

Choose a reason for hiding this comment

fzyzcjy commented Aug 16, 2022 • edited

fzyzcjy commented Aug 10, 2022 •

edited

fzyzcjy commented Aug 16, 2022 •

edited