TextField text backgroundColor covers text selection color - #96276

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.

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.

Gold has detected about 11 new digest(s) on patchset 1.
View them at https://flutter-gold.skia.org/cl/github/96276

@JsouLiang
This PR requires a test and this is also failing other tests in editable_test.dart

03:39 +5772 ~14 -4: /b/s/w/ir/x/w/flutter/packages/flutter/test/rendering/editable_test.dart: custom painters reusing the same painter [E]                                                             
  Expected: Object or closure painting:
            'a rectangle with Color(0x12345678), Rect.fromLTRB(1.0, 1.0, 1.0, 1.0)'
            'drawParagraph(..., ...)'
            'a rectangle with Color(0x12345678), Rect.fromLTRB(1.0, 1.0, 1.0, 1.0)'
    Actual: <Closure: (Canvas) => void>
     Which: did not match the pattern.
            It called 1 other method on the canvas, the first of which was drawRect(Rect.fromLTRB(1.0, 1.0, 1.0, 1.0), Paint(Color(0x12345678))), but did not call drawParagraph() at the time where drawParagraph(..., ...) was expected.
            The first method that was called when the call to drawParagraph() was expected, drawRect(Rect.fromLTRB(1.0, 1.0, 1.0, 1.0), Paint(Color(0x12345678))), was called with the following stack:
              #0      TestRecordingCanvas.noSuchMethod (file:///b/s/w/ir/x/w/flutter/packages/flutter/test/rendering/recording_canvas.dart:87:70)

@JsouLiang This PR requires a test and this is also failing other tests in editable_test.dart

03:39 +5772 ~14 -4: /b/s/w/ir/x/w/flutter/packages/flutter/test/rendering/editable_test.dart: custom painters reusing the same painter [E]                                                             
  Expected: Object or closure painting:
            'a rectangle with Color(0x12345678), Rect.fromLTRB(1.0, 1.0, 1.0, 1.0)'
            'drawParagraph(..., ...)'
            'a rectangle with Color(0x12345678), Rect.fromLTRB(1.0, 1.0, 1.0, 1.0)'
    Actual: <Closure: (Canvas) => void>
     Which: did not match the pattern.
            It called 1 other method on the canvas, the first of which was drawRect(Rect.fromLTRB(1.0, 1.0, 1.0, 1.0), Paint(Color(0x12345678))), but did not call drawParagraph() at the time where drawParagraph(..., ...) was expected.
            The first method that was called when the call to drawParagraph() was expected, drawRect(Rect.fromLTRB(1.0, 1.0, 1.0, 1.0), Paint(Color(0x12345678))), was called with the following stack:
              #0      TestRecordingCanvas.noSuchMethod (file:///b/s/w/ir/x/w/flutter/packages/flutter/test/rendering/recording_canvas.dart:87:70)

OK, i will fix that

I use an alternative implement: I change the Selection Highlight Painter from BackgroundPainters to ForegroundPainters. I think the Selection Highlight should be paint in the top.

@justinmc hey, I run the test, found some test generate image have some diff, please help me to check that image show which is right?

(from triage): @JsouLiang looks like there are some failing tests on this one. Can you take a look at those?

(from triage): @JsouLiang looks like there are some failing tests on this one. Can you take a look at those?

yeah, I had look at those, and I think I may be change the flutter golden image

cc @Piinks

Hi @JsouLiang it looks like there aren't any golden file changes here currently. The failing test is not a golden file test:

01:27 +1626 ~2: /b/s/w/ir/x/w/flutter/packages/flutter/test/widgets/editable_text_test.dart: iOS autocorrection rectangle should appear on demand and dismiss when the text changes or when focus is lost
══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
The following TestFailure was thrown running a test:
Expected: Object or closure painting:
          'an "everything" step'
  Actual: RenderEditable:<RenderEditable#c6b1b>
   Which: did not match the pattern.
          It painted something that the predicate passed to an "everything" step in the paint
pattern considered incorrect:
                Expected: autocorrection rect not visible, found: Rect.fromLTRB(0.0, 0.0, 14.0,
14.0)

          The stack of the offending call was:
            #0      TestRecordingCanvas.noSuchMethod
(file:///b/s/w/ir/x/w/flutter/packages/flutter/test/rendering/recording_canvas.dart:87:70)
            #1      TestRecordingCanvas.drawRect (dart:ui/painting.dart:4394:8)
            #2      _TextHighlightPainter.paint
(package:flutter/src/rendering/editable.dart:2752:14)
            #3      _CompositeRenderEditablePainter.paint
(package:flutter/src/rendering/editable.dart:2966:15)
            #4      _RenderEditableCustomPaint.paint
(package:flutter/src/rendering/editable.dart:2611:15)
            #5      TestRecordingPaintingContext.paintChild
(file:///b/s/w/ir/x/w/flutter/packages/flutter/test/rendering/recording_canvas.dart:101:11)
            #6      RenderEditable._paintContents
(package:flutter/src/rendering/editable.dart:2490:15)
            #7      RenderEditable.paint (package:flutter/src/rendering/editable.dart:2532:7)
            #8      _evaluatePainter
(file:///b/s/w/ir/x/w/flutter/packages/flutter/test/rendering/mock_canvas.dart:516:20)
            #9      _TestRecordingCanvasMatcher.matches
(file:///b/s/w/ir/x/w/flutter/packages/flutter/test/rendering/mock_canvas.dart:533:12)
            #10     _expect (package:test_api/src/expect/expect.dart:126:30)
            #11     expect (package:test_api/src/expect/expect.dart:46:3)
            #12     expect (package:flutter_test/src/widget_tester.dart:455:16)
            #13     main.<anonymous closure>.verifyAutocorrectionRectVisibility
(file:///b/s/w/ir/x/w/flutter/packages/flutter/test/widgets/editable_text_test.dart:2110:9)
            #14     main.<anonymous closure>
(file:///b/s/w/ir/x/w/flutter/packages/flutter/test/widgets/editable_text_test.dart:2149:7)
            <asynchronous suspension>
            <asynchronous suspension>
            (elided one frame from package:stack_trace)
          The complete display list was:
            * drawParagraph(Instance of 'Paragraph', Offset(0.0, 0.0))
            * drawRect(Rect.fromLTRB(0.0, 0.0, 14.0, 14.0), Paint(Color(0xffff0000)))

When the exception was thrown, this was the stack:
#4      main.<anonymous closure>.verifyAutocorrectionRectVisibility (file:///b/s/w/ir/x/w/flutter/packages/flutter/test/widgets/editable_text_test.dart:2110:9)
#5      main.<anonymous closure> (file:///b/s/w/ir/x/w/flutter/packages/flutter/test/widgets/editable_text_test.dart:2149:7)
<asynchronous suspension>
<asynchronous suspension>
(elided one frame from package:stack_trace)

This was caught by the test expectation on the following line:
  file:///b/s/w/ir/x/w/flutter/packages/flutter/test/widgets/editable_text_test.dart line 2110
The test description was:
  iOS autocorrection rectangle should appear on demand and dismiss when the text changes or when
  focus is lost
════════════════════════════════════════════════════════════════════════════════════════════════════

01:27 +1627 ~2 -1: /b/s/w/ir/x/w/flutter/packages/flutter/test/widgets/scrollable_test.dart: ... vertical scrollables are scrolled when activated via keyboard. (variant: ui.KeyData then RawKeyEvent) 
01:27 +1628 ~2 -1: /b/s/w/ir/x/w/flutter/packages/flutter/test/widgets/scrollable_test.dart: ... vertical scrollables are scrolled when activated via keyboard. (variant: ui.KeyData then RawKeyEvent) 
01:27 +1629 ~2 -1: /b/s/w/ir/x/w/flutter/packages/flutter/test/widgets/scrollable_test.dart: ... vertical scrollables are scrolled when activated via keyboard. (variant: ui.KeyData then RawKeyEvent) 
01:27 +1629 ~2 -1: /b/s/w/ir/x/w/flutter/packages/flutter/test/widgets/editable_text_test.dart: iOS autocorrection rectangle should appear on demand and dismiss when the text changes or when focus is lost [E]
  Test failed. See exception logs above.
  The test description was: iOS autocorrection rectangle should appear on demand and dismiss when the text changes or when focus is lost
  

@Piinks hey, for editable_text_test.dart I run my local it's passed😂

.

@justinmc hey, thank you for review my pr. I think the test failed is cause of the 'Selected highlight' is paint the topmost level. If you agree with my changes, I will try to modify the test.

@JsouLiang Can you give me a screenshot or an app to reproduce the problem? At a high level it makes sense to move these painters to the foreground if it does fix the bug and doesn't break autocorrect or anything, but I want to make sure I understand what is being fixed.

Sorry, I missed that this was a fix for #96112, I see the screenshots and example there.

@LongCatIsLooong You mentioned that the solution for this problem was blocked on the SkParagraph migration at the time (here). Does that mean we need a deeper solution than this PR?

Gold has detected about 8 new digest(s) on patchset 1.
View them at https://flutter-gold.skia.org/cl/github/96276

@justinmc this doesn't seem to be a fix for #96112? But it can be used as a workaround to get past that issue.

@justinmc this doesn't seem to be a fix for #96112? But it can be used as a workaround to get past that issue.

Does it need to change the SkParagraph background-color and text render levels？

Gold has detected about 8 new digest(s) on patchset 1.
View them at https://flutter-gold.skia.org/cl/github/96276

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #96276 at sha f298a9f5f7cb70b335b8219a6856efb61719440f

@JsouLiang I want to try running this code, can you tell me how to reproduce the problem that this fixes?

Golden file changes are available for triage from new commit, Click here to view.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #96276 at sha 4e10bb15622064aa1b001b09bf74bbd912301e4e

@JsouLiang Taking a look at the most recent goldens failures, it looks like now the selection is being drawn in front of the text, and it has changed the color of the text slightly, which probably isn't what we want.

(from triage): @JsouLiang Do you still have plans to follow up on the feedback given above?

@JsouLiang I want to try running this code, can you tell me how to reproduce the problem that this fixes?

Sorry, late reply.
You can run this code to reproduce this issue:

import 'package:flutter/material.dart';

void main() {
  runApp(const MyApp());
}

class MyApp extends StatelessWidget {
  const MyApp({Key? key}) : super(key: key);

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      home: const MyHomePage(),
    );
  }
}

class MyHomePage extends StatefulWidget {
  const MyHomePage({Key? key}) : super(key: key);

  @override
  State<MyHomePage> createState() => _MyHomePageState();
}

class _MyHomePageState extends State<MyHomePage> {
  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Center(
        child: TextField(
          style: new TextStyle(backgroundColor: Colors.yellow),
        ),
      ),
    );
  }
}

And I found the golden is has diff, but I don't know if we need to change the golden test in this scenario, or is there a problem with the code logic.

You can reproduce this issue and check the selection paint logic.
Thanks
cc @justinmc

Golden file changes are available for triage from new commit, Click here to view.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #96276 at sha 1be648de8f2853ac3aaf028a72452b5c49d37389

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

(Triage) @JsouLiang what is the status of this PR?

@JsouLiang Using the code you gave in #96276 (comment), I was able to reproduce the problem on master, but after checking out this branch, it doesn't seem to fix it!

Can you take a look and see if it's working for you?

Golden file changes are available for triage from new commit, Click here to view.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #96276 at sha 2c59ead

Hello gentlemen, may I ask is there any progress on this issues? I have been waiting a long time to get a solution

I just pulled the branch and ran it locally and this still doesn't seem to fix the problem. Same result as before using the code in #96276 (comment):

@JsouLiang Am I doing something wrong or is this not working?

So sorry replay too late.

I run this code #96276 (comment) on my branch and this is my behavior:

@justinmc

@JsouLiang It looks like you're on iOS, can you try on other platforms? I just tried the same code again on Linux and on an Android emulator, and both look wrong. The selection doesn't show above the yellow highlight.

(Triage) @JsouLiang Do you still have plans to come back to this PR and address the feedback from above?

Closing this as stale. If @JsouLiang or anyone else wants to work on this again, please let me know and I'm happy to keep helping with code review!

any updates on this? I am still facing this issue.