Bring back paste button hide behavior 3#57139
Bring back paste button hide behavior 3#57139fluttergithubbot merged 4 commits intoflutter:masterfrom
Conversation
…" (flutter#56951)" This reverts commit c969b8a.
fluttergithubbot
added
a: accessibility
|
Wondering: Maybe the previous run of the integration test is affecting the next? E.g. during the previous run you copied something into the text field. So during the next run, the clipboard doesn't appear empty anymore and the test (expecting it to be empty) fails. With this change, the test will now fail if the clipboard happens to be empty at the start of the test. So, I think it is still flaky. Can we set the clipboard state to a known state at the beginning of the test? Ideally, we would clear it to test both cases: a full and an empty clipboard. Is there a command to clear the textfield that we could execute at the beginning of the test? |
|
I bet you're right! It seemed to be flaky, but when I ran it over and over it again it always gave the same result (because the clipboard status didn't change between runs). I assumed it would start empty like in the unit tests, but I'll look for a way to clear it manually. |
|
@goderbauer I've updated it to copy some text into the clipboard for the relevant integration tests and it seems to work. It passes whether or not the clipboard is empty before running the test. I found no reliable way to clear the clipboard except for restarting the device. So, I'm not testing the case where there is nothing in the clipboard, I'm just making sure there is definitely something pasteable on the clipboard. When I do that, the integration tests pass without any further migration needed, as I would expect. |
|
|
||
| // The text selection menu and related semantics vary depending on if the | ||
| // clipboard contents are pasteable. Copy some text into the clipboard to | ||
| // make sure these tests always run with pasteable content in the clipboard. |
There was a problem hiding this comment.
Maybe add that ideally we would want to run the tests with an empty clipboard as well, but there is no reliable way to clear the clipboard.
|
@justinmc @goderbauer We caught it in our integration tests: https://ci.chromium.org/p/flutter/builders/try/Linux%20Web%20Engine/4099? These tests only run on the engine side so we were able to catch it only now. Overall getData means paste, reading the clipboard on web, which requires permission and should only be done with user permission. Calling getData will bring a browser popup window and will ask user for permission. It is not the best practice to ask user for their clipboard's access without an explicit event that might trigger it. Let's discuss a solution that will also work for web before remerging this PR. |
…)" (flutter#57286)" This reverts commit 32547dc.
5 participants
First Try
I wrote the original PR #54902 and merged it when it was green.
The integration test android_semantics_testing failed after it was merged, so it was reverted. I ran the integration test locally and found that it did fail in the same way locally (using a physical Pixel 2).
Second Try
I fixed the integration test failure by updating the expected semantics options, and I added an extra case that specifically tests the behavior in this PR. Then I created a new PR with this changed reverting the revert and merged it.
The integration test passed and the build went green after the merge. However, during the next roll, there were errors in google3 with a stacktrace pointing to my code, so it was reverted.
Third Try
I identified and fixed the problem in my code. Then I created and merged a PR with this change reverting the previous reversion.
I didn't rerun the integration test at the time, because nothing should have changed related to it, but running it on this branch now I see that it fails again. The failure seems to have nothing to do with the change that I made in this PR. I'm guessing it is related to something that changed in master in the time between the two PRs.
This Fourth Try
I undid my original changes to the integration test since it now seems to pass without those changes. I kept my extra case for this PR, though. The integration test now passes locally.