Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Bug on waitUntilVisible() and visible #2464

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

FritzMatthaeus
Copy link

This fix is related to the issue #2463

The methods waitUntilVisible() and the getter visible default to Alignment.center when calling hitTestable() on the Flutter test framework. In case the tested Widget has an empty SizedBox in it's middle, both methods will fail. This can be fixed by allowing to pass an Alignment.

Example:

class ElementB extends StatelessWidget {
  ElementB() : super(key: Keys.elementB);

  @override
  Widget build(BuildContext context) {
    return const Column(
      children: [
        Text(
          'Element B',
        ),
        SizedBox(height: 48)
      ],
    );
  }
}

Calling $(Keys.elementB).waitUntilVisible() on this widget will fail as the SizedBox will cover the middle of the Widget.
Calling $(Keys.elementB).waitUntilVisible(alignment: Alignment.topCenter) on this widget will pass.

The visible getter has to be refactored as a function to be able to pass an Alignment parameter: visible({Alignment alignment = Alignment.center})

An alternative Approach would be to evaluate the finder for all possible Alignments when calling waitUntilVisible() and visible. A separate Pull Request will be created for that solution. You are in a better position to decide which way would suit best for the package.

This commit adds an Alignment parameter to the visibility related methods.
The default setting by Flutter of Alignment.center fails, if hitting a SizedBox.
This can be fixed by specifying an Alignment.
Copy link

docs-page bot commented Dec 21, 2024

To view this pull requests documentation preview, visit the following URL:

docs.page/leancodepl/patrol~2464

Documentation is deployed and generated using docs.page.

this addresses the same issue leancodepl#2464 as the previous commit but with a different solution
@FritzMatthaeus
Copy link
Author

the second commit offers an alternative solution to the one of the first commit. Here the API remains untouched but we check all possible Alignments for a possible hit test:

/// Waits until [finder] finds at least one visible widget.
  ///
  /// Throws a [WaitUntilVisibleTimeoutException] if more time than specified by
  /// the timeout passed and no widgets were found.
  Future<PatrolFinder> waitUntilVisible(
    Finder finder, {
    Duration? timeout,
    bool enablePatrolLog = true,
  }) {
    return TestAsyncUtils.guard(
      () => wrapWithPatrolLog(
        action: 'waitUntilVisible',
        finder: finder,
        color: AnsiCodes.cyan,
        enablePatrolLog: enablePatrolLog,
        function: () async {
          final duration = timeout ?? config.visibleTimeout;
          final end = tester.binding.clock.now().add(duration);
          final hitTestableFinders = alignments.map((alignment) => finder.hitTestable(at: alignment));
          final hitTestableEvaluations = hitTestableFinders.map((finder) => finder.evaluate());
          while (hitTestableEvaluations.map((result) => result.isNotEmpty).firstOrNull == null) {
            final now = tester.binding.clock.now();
            if (now.isAfter(end)) {
              throw WaitUntilVisibleTimeoutException(
                finder: finder,
                duration: duration,
              );
            }            
            await tester.pump(const Duration(milliseconds: 100));
          }
          return PatrolFinder(
            finder: hitTestableFinders.firstWhere((finder) => finder.evaluate().isNotEmpty),
            tester: this,
          );
        },
      ),
    );
  }

Looking forward to your feedback.

@FritzMatthaeus
Copy link
Author

Both solutions can be tested on this example: Patrol Example

@mateuszwojtczak
Copy link
Contributor

Hi @FritzMatthaeus! Welcome to the contributor lounge. 🎉

The idea of being "visible" is very hard to set in stone. We thought hit-testability is the closest we can get to visibility, since most of the time users want to interact with something if they wait for it to be visible. Thus, we check for hitTestable() evaluations.

In your case, the Column is a widget that takes all available height and is not responding to hit tests. So, when the default alignment is Alignment.center, then it hits a center of a Column that is "transparent" to hit tests and it can't find any hit-testable widgets.

Your first commit provides a nice fix of just passing by the alignment parameter to the user, so that they're in full control of where the hit test happens.

Your second commit tries to hit-test in 9 points which can lead to more unexpected issues for other users. Still, it won't cover all the cases because if the child is moved by a pixel (e.g. with a padding), then the hit-test fails.

That said, we think your first commit is the best fix for this issue right now.

I have generated a golden test image from your ElementA example (Column is in blue and doesn't react to hit tests, but it's center is there).

issue_test

@FritzMatthaeus
Copy link
Author

Hi @mateuszwojtczak,

i fully agree with your decision. This PR has been the result of my learning curve on "visible" and "hitTestable". My final learning is, to be aware of Columns and Rows when testing "visibility". Maybe this should be added to the docs somewhere. I'll think about it and maybe i come up with a PR on the docs.

Have good year 2025 and thank you for your work on this package!

@pdenert
Copy link
Collaborator

pdenert commented Jan 2, 2025

Hi @FritzMatthaeus, maybe it wasn't obvious from @mateuszwojtczak answer, but we would like to merge your first commit, as it extends user possibilities, so it would be nice to have it in Patrol.
You can reopen the PR and restore changes from first commit or create new one. If you don't have time for that, let us know and we'll do it by ourself.

Thanks again for your contribution.

update dart doc
avoid breaking change on visible
add tests
update dart doc
remove alignments
@FritzMatthaeus FritzMatthaeus reopened this Jan 2, 2025
@FritzMatthaeus
Copy link
Author

Hi @pdenert,

sorry, i do not have much experience and routine on Pull Requests. As you requested, i have reverted to the Alignment parameter and added some dart doc and a test. I took me some merges, etc, etc. ;)

Note that i thought it might be no good idea to create a breaking change by turning visible from a getter into a method. Therefore i created a new method isVisibleAt taking in a parameter. visible is still available but references isVisibleAt internally.

I hope that this time the PR is up to your needs.

Best regards,
Fritz

Copy link
Collaborator

@pdenert pdenert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good job! Just few organizing things:

  1. Please format changed files. You can use dart format ./packages/patrol_finders
  2. We try to stick with the standard flutter max 80 chars per line. Please change your comments to fit this requirement.
  3. Add new unreleased entry to patrol_finders changelog. See example here: https://github.com/leancodepl/patrol/pull/2425/files#diff-e17f3235f9e47a742b45c5a87b2ce49cb726b37d26fb9d478ce3880c6e61ba98

add changelog entry
format files and comments
@FritzMatthaeus
Copy link
Author

Very good job! Just few organizing things:

  1. Please format changed files. You can use dart format ./packages/patrol_finders
  2. We try to stick with the standard flutter max 80 chars per line. Please change your comments to fit this requirement.
  3. Add new unreleased entry to patrol_finders changelog. See example here: https://github.com/leancodepl/patrol/pull/2425/files#diff-e17f3235f9e47a742b45c5a87b2ce49cb726b37d26fb9d478ce3880c6e61ba98

I did as requested. Thanks for guiding me through this process. Crash course in good package maintainance for me ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants