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

Inputting TextInput answers in landscape auto-closes keyboard #1769

Closed
BenHenning opened this issue Sep 2, 2020 · 12 comments · Fixed by #2683
Closed

Inputting TextInput answers in landscape auto-closes keyboard #1769

BenHenning opened this issue Sep 2, 2020 · 12 comments · Fixed by #2683
Assignees
Labels
Priority: Essential This work item must be completed for its milestone. Z-ibt Temporary label for Ben to keep track of issues he's triaged.

Comments

@BenHenning
Copy link
Member

This is the same bug as #1767 except after its cause PR is reverted, this still repros but only for the TextInput interaction which indicates it may have a different root cause.

@BenHenning BenHenning added Type: Bug Priority: Essential This work item must be completed for its milestone. labels Sep 2, 2020
@BenHenning BenHenning added this to the Alpha milestone Sep 2, 2020
@BenHenning BenHenning self-assigned this Sep 2, 2020
@BenHenning
Copy link
Member Author

Note that some tests in #1630 have been disabled until this issue is resolved.

@BenHenning
Copy link
Member Author

Interestingly, this sometimes comes across as initially working (e.g. keyboard does not automatically dismiss), but dismissing the keyboard manually then re-selecting the input box can trigger automatic dismissal.

@BenHenning
Copy link
Member Author

This seems to not be a regression--I'm able to repro it as far back as 6722988, and gave up looking it farther. Since we know this is specific to my device, making the choice to push this to beta. It's a bad bug for users to hit, but if they do they can work around it by rotating their device to portrait.

@BenHenning BenHenning removed their assignment Sep 2, 2020
@BenHenning BenHenning modified the milestones: Alpha, Beta Sep 2, 2020
BenHenning added a commit that referenced this issue Sep 3, 2020
)

* Introduce test coroutine dispatchers support in Espresso.

This piggybacks off of the solution introduced in #1276 for Robolectric.
That PR allows Robolectric tests in app module to share dependencies
with production components by creating a test application & telling
Robolectric to use it instead of OppiaApplication via a @config
annotation.

This PR achieves the same thing by using a custom test runner that reads
the same annotation and forces Espresso & instrumentation to use the
test application instead of the default OppiaApplication. This setup
will be easier once #59 is finished since we can specify the application
in per-test manifests that both Robolectric & Espresso will respect.

Note that while this lets the same test coroutine dispatchers work in
both production & test code for Espresso, some additional work was
needed to ensure the coroutines behave correctly. In particular, the
coroutines use a fake system clock in Robolectric that requires explicit
synchronization points in the tests to allow the clock to move forward
& properly coordinate execution between background & main thread
operations. However, in Espresso, since everything is using a real clock
an idling resource is the preferred way to synchronize execution: it
allows the background coroutines to operate in real-time much like they
would in production, and then notify Espresso when completed. The test
dispatchers API is now setup to support both synchronization mechanisms
for both Robolectric & Espresso (the idling resource does nothing on
Robolectric and the force synchronization effectively does nothing on
Espresso).

The first test being demonstrated as now stable is SplashActivityTest
(as part of downstream work in #1397.

* Revert "Fixes #941: Add radar effect in Hints and solution (#1475)"

This reverts commit 41eb10b.

* Stabilize StateFragmentTest such that it passes on both Robolectric and
Espresso.

Note that some issues were found during this: #1612 (#1611 was found a
few weeks ago, but it also affects these tests). To ensure the tests can
still be run, a @runon annotation was added to allow tests to target
specific test platforms. The tests that currently fail on Robolectric
due to #1611 and #1612 are disabled for that platform. The test suite as
a whole has been verified to pass in its current state on both
Robolectric and Espresso (on a Pixel XL).

The aim of this PR is to actually enable critical state fragment tests
in CI, so both StateFragmentTest and StateFragmentLocalTest are being
enabled in GitHub actions.

* Enable StateFragmentTest (Robolectric) & StateFragmentLocalTest for CI.

* Add thorough documentation for new dispatchers.

* Clean up comments & add additional documentation.

* Fix lint errors.

* Fix broken test after changes to FakeSystemClock.

* Fix linter errors.

* Use a custom executor service for Glide requests that coordinates with
Oppia's test dispatchers. Note that this does not actually introduce the
service--that will happen in a new branch.

* Introduce new executor service which allows interop with Kotlin
coroutines, plus a test to verify that it fundamentally follows one
interpretation of ExecutorService's API.

* Fix flaky timeout tests by improving cancellation cooperation for
invokeAny() and provide longer timeouts for tests that are
CPU-sensitive.

* Add documentation & clean up unused code.

* Lint fixes.

* Significantly reorganize invokeAll() to try and make it more cooperative
for cancellation, and increase timeout times in tests to reduce
flakiness for time-sensitive tests. Some tests are remaining flaky, so
ignoring those.

Re-add maybeWithTimeoutOrNull since it actually was needed.

* Lint fixes.

* Post-merge module fixes.

* Post-merge fixes with ratio input & add a TODO to improve speed of the
new coroutine executor service.

* Revert "Fixes part of #40 & #42: Generalisation Highfi Mobile Portrait + Landscape - Buttons (#1653)"

This reverts commit 1bb1ffa.

* Ensure terminated tasks do not interfere with one another (timeouts
should happen individually for each task during termination). This fixes
a failure observed in StateFragmentLocalTest in #1630.

* Ignore failing tests until #1769 is resolved.

* Fix awaitTermination & improve test. Improve stack trace for test
dispatcher timeouts.

* Fix slow & broken tests in Robolectric for StateFragmentLocalTest.

* Add missing deps for StateFragmentLocalTest.

* Address reviewer comments.
BenHenning added a commit that referenced this issue Sep 3, 2020
* Add support for showing concept cards in feedback, and add a concept
card as one of the remediation pathways for 'the meaning of equal parts'
lesson.

* Introduce test coroutine dispatchers support in Espresso.

This piggybacks off of the solution introduced in #1276 for Robolectric.
That PR allows Robolectric tests in app module to share dependencies
with production components by creating a test application & telling
Robolectric to use it instead of OppiaApplication via a @config
annotation.

This PR achieves the same thing by using a custom test runner that reads
the same annotation and forces Espresso & instrumentation to use the
test application instead of the default OppiaApplication. This setup
will be easier once #59 is finished since we can specify the application
in per-test manifests that both Robolectric & Espresso will respect.

Note that while this lets the same test coroutine dispatchers work in
both production & test code for Espresso, some additional work was
needed to ensure the coroutines behave correctly. In particular, the
coroutines use a fake system clock in Robolectric that requires explicit
synchronization points in the tests to allow the clock to move forward
& properly coordinate execution between background & main thread
operations. However, in Espresso, since everything is using a real clock
an idling resource is the preferred way to synchronize execution: it
allows the background coroutines to operate in real-time much like they
would in production, and then notify Espresso when completed. The test
dispatchers API is now setup to support both synchronization mechanisms
for both Robolectric & Espresso (the idling resource does nothing on
Robolectric and the force synchronization effectively does nothing on
Espresso).

The first test being demonstrated as now stable is SplashActivityTest
(as part of downstream work in #1397.

* Revert "Fixes #941: Add radar effect in Hints and solution (#1475)"

This reverts commit 41eb10b.

* Stabilize StateFragmentTest such that it passes on both Robolectric and
Espresso.

Note that some issues were found during this: #1612 (#1611 was found a
few weeks ago, but it also affects these tests). To ensure the tests can
still be run, a @runon annotation was added to allow tests to target
specific test platforms. The tests that currently fail on Robolectric
due to #1611 and #1612 are disabled for that platform. The test suite as
a whole has been verified to pass in its current state on both
Robolectric and Espresso (on a Pixel XL).

The aim of this PR is to actually enable critical state fragment tests
in CI, so both StateFragmentTest and StateFragmentLocalTest are being
enabled in GitHub actions.

* Enable StateFragmentTest (Robolectric) & StateFragmentLocalTest for CI.

* Add thorough documentation for new dispatchers.

* Clean up comments & add additional documentation.

* Fix lint errors.

* Fix broken test after changes to FakeSystemClock.

* Fix linter errors.

* Update test lesson to include references to concept cards.

* Lint fixes & use HtmlCompat instead of Html.

* Add support for the newer & finalized tag format.

* Lint fixes.

* Use a custom executor service for Glide requests that coordinates with
Oppia's test dispatchers. Note that this does not actually introduce the
service--that will happen in a new branch.

* Introduce new executor service which allows interop with Kotlin
coroutines, plus a test to verify that it fundamentally follows one
interpretation of ExecutorService's API.

* Fix flaky timeout tests by improving cancellation cooperation for
invokeAny() and provide longer timeouts for tests that are
CPU-sensitive.

* Add documentation & clean up unused code.

* Lint fixes.

* Significantly reorganize invokeAll() to try and make it more cooperative
for cancellation, and increase timeout times in tests to reduce
flakiness for time-sensitive tests. Some tests are remaining flaky, so
ignoring those.

Re-add maybeWithTimeoutOrNull since it actually was needed.

* Lint fixes.

* Post-merge module fixes.

* Post-merge fixes with ratio input & add a TODO to improve speed of the
new coroutine executor service.

* Revert "Fixes part of #40 & #42: Generalisation Highfi Mobile Portrait + Landscape - Buttons (#1653)"

This reverts commit 1bb1ffa.

* Ensure terminated tasks do not interfere with one another (timeouts
should happen individually for each task during termination). This fixes
a failure observed in StateFragmentLocalTest in #1630.

* Ignore failing tests until #1769 is resolved.

* Fix awaitTermination & improve test. Improve stack trace for test
dispatcher timeouts.

* Fix slow & broken tests in Robolectric for StateFragmentLocalTest.

* Add missing deps for StateFragmentLocalTest.

* Address TODOs (including adding support for list tags which replaces the
old handler & adds nested custom tag support), and add tests.

* Lint fixes.

* Address reviewer comments.

* Address review comments. Fix new concept card tests on Espresso & add
landscape versions (required configuring hints to show quickly to avoid
delaying the test, and fixing a bug in the espresso test dispatcher).
Add support for disabling concept cards of they aren't enabled for
parsing particular HTML (the default behavior is to ignore the custom
tag).

* Add support for concept cards in questions. Note that it's not clear how
to test verifying that pressing the exit button closes the concept card
since the exit button is part of the dialog's toolbar.

* Fix image-breaking duplicated code in HtmlParser, fix a paragraph
parsing issue in BulletTagHandler, and add tests for
CustomHtmlContentHandler.

* Lint fixes.
prayutsu pushed a commit to prayutsu/oppia-android that referenced this issue Sep 3, 2020
…#1764] (oppia#1630)

* Introduce test coroutine dispatchers support in Espresso.

This piggybacks off of the solution introduced in oppia#1276 for Robolectric.
That PR allows Robolectric tests in app module to share dependencies
with production components by creating a test application & telling
Robolectric to use it instead of OppiaApplication via a @config
annotation.

This PR achieves the same thing by using a custom test runner that reads
the same annotation and forces Espresso & instrumentation to use the
test application instead of the default OppiaApplication. This setup
will be easier once oppia#59 is finished since we can specify the application
in per-test manifests that both Robolectric & Espresso will respect.

Note that while this lets the same test coroutine dispatchers work in
both production & test code for Espresso, some additional work was
needed to ensure the coroutines behave correctly. In particular, the
coroutines use a fake system clock in Robolectric that requires explicit
synchronization points in the tests to allow the clock to move forward
& properly coordinate execution between background & main thread
operations. However, in Espresso, since everything is using a real clock
an idling resource is the preferred way to synchronize execution: it
allows the background coroutines to operate in real-time much like they
would in production, and then notify Espresso when completed. The test
dispatchers API is now setup to support both synchronization mechanisms
for both Robolectric & Espresso (the idling resource does nothing on
Robolectric and the force synchronization effectively does nothing on
Espresso).

The first test being demonstrated as now stable is SplashActivityTest
(as part of downstream work in oppia#1397.

* Revert "Fixes oppia#941: Add radar effect in Hints and solution (oppia#1475)"

This reverts commit 41eb10b.

* Stabilize StateFragmentTest such that it passes on both Robolectric and
Espresso.

Note that some issues were found during this: oppia#1612 (oppia#1611 was found a
few weeks ago, but it also affects these tests). To ensure the tests can
still be run, a @runon annotation was added to allow tests to target
specific test platforms. The tests that currently fail on Robolectric
due to oppia#1611 and oppia#1612 are disabled for that platform. The test suite as
a whole has been verified to pass in its current state on both
Robolectric and Espresso (on a Pixel XL).

The aim of this PR is to actually enable critical state fragment tests
in CI, so both StateFragmentTest and StateFragmentLocalTest are being
enabled in GitHub actions.

* Enable StateFragmentTest (Robolectric) & StateFragmentLocalTest for CI.

* Add thorough documentation for new dispatchers.

* Clean up comments & add additional documentation.

* Fix lint errors.

* Fix broken test after changes to FakeSystemClock.

* Fix linter errors.

* Use a custom executor service for Glide requests that coordinates with
Oppia's test dispatchers. Note that this does not actually introduce the
service--that will happen in a new branch.

* Introduce new executor service which allows interop with Kotlin
coroutines, plus a test to verify that it fundamentally follows one
interpretation of ExecutorService's API.

* Fix flaky timeout tests by improving cancellation cooperation for
invokeAny() and provide longer timeouts for tests that are
CPU-sensitive.

* Add documentation & clean up unused code.

* Lint fixes.

* Significantly reorganize invokeAll() to try and make it more cooperative
for cancellation, and increase timeout times in tests to reduce
flakiness for time-sensitive tests. Some tests are remaining flaky, so
ignoring those.

Re-add maybeWithTimeoutOrNull since it actually was needed.

* Lint fixes.

* Post-merge module fixes.

* Post-merge fixes with ratio input & add a TODO to improve speed of the
new coroutine executor service.

* Revert "Fixes part of oppia#40 & oppia#42: Generalisation Highfi Mobile Portrait + Landscape - Buttons (oppia#1653)"

This reverts commit 1bb1ffa.

* Ensure terminated tasks do not interfere with one another (timeouts
should happen individually for each task during termination). This fixes
a failure observed in StateFragmentLocalTest in oppia#1630.

* Ignore failing tests until oppia#1769 is resolved.

* Fix awaitTermination & improve test. Improve stack trace for test
dispatcher timeouts.

* Fix slow & broken tests in Robolectric for StateFragmentLocalTest.

* Add missing deps for StateFragmentLocalTest.

* Address reviewer comments.
prayutsu pushed a commit to prayutsu/oppia-android that referenced this issue Sep 3, 2020
* Add support for showing concept cards in feedback, and add a concept
card as one of the remediation pathways for 'the meaning of equal parts'
lesson.

* Introduce test coroutine dispatchers support in Espresso.

This piggybacks off of the solution introduced in oppia#1276 for Robolectric.
That PR allows Robolectric tests in app module to share dependencies
with production components by creating a test application & telling
Robolectric to use it instead of OppiaApplication via a @config
annotation.

This PR achieves the same thing by using a custom test runner that reads
the same annotation and forces Espresso & instrumentation to use the
test application instead of the default OppiaApplication. This setup
will be easier once oppia#59 is finished since we can specify the application
in per-test manifests that both Robolectric & Espresso will respect.

Note that while this lets the same test coroutine dispatchers work in
both production & test code for Espresso, some additional work was
needed to ensure the coroutines behave correctly. In particular, the
coroutines use a fake system clock in Robolectric that requires explicit
synchronization points in the tests to allow the clock to move forward
& properly coordinate execution between background & main thread
operations. However, in Espresso, since everything is using a real clock
an idling resource is the preferred way to synchronize execution: it
allows the background coroutines to operate in real-time much like they
would in production, and then notify Espresso when completed. The test
dispatchers API is now setup to support both synchronization mechanisms
for both Robolectric & Espresso (the idling resource does nothing on
Robolectric and the force synchronization effectively does nothing on
Espresso).

The first test being demonstrated as now stable is SplashActivityTest
(as part of downstream work in oppia#1397.

* Revert "Fixes oppia#941: Add radar effect in Hints and solution (oppia#1475)"

This reverts commit 41eb10b.

* Stabilize StateFragmentTest such that it passes on both Robolectric and
Espresso.

Note that some issues were found during this: oppia#1612 (oppia#1611 was found a
few weeks ago, but it also affects these tests). To ensure the tests can
still be run, a @runon annotation was added to allow tests to target
specific test platforms. The tests that currently fail on Robolectric
due to oppia#1611 and oppia#1612 are disabled for that platform. The test suite as
a whole has been verified to pass in its current state on both
Robolectric and Espresso (on a Pixel XL).

The aim of this PR is to actually enable critical state fragment tests
in CI, so both StateFragmentTest and StateFragmentLocalTest are being
enabled in GitHub actions.

* Enable StateFragmentTest (Robolectric) & StateFragmentLocalTest for CI.

* Add thorough documentation for new dispatchers.

* Clean up comments & add additional documentation.

* Fix lint errors.

* Fix broken test after changes to FakeSystemClock.

* Fix linter errors.

* Update test lesson to include references to concept cards.

* Lint fixes & use HtmlCompat instead of Html.

* Add support for the newer & finalized tag format.

* Lint fixes.

* Use a custom executor service for Glide requests that coordinates with
Oppia's test dispatchers. Note that this does not actually introduce the
service--that will happen in a new branch.

* Introduce new executor service which allows interop with Kotlin
coroutines, plus a test to verify that it fundamentally follows one
interpretation of ExecutorService's API.

* Fix flaky timeout tests by improving cancellation cooperation for
invokeAny() and provide longer timeouts for tests that are
CPU-sensitive.

* Add documentation & clean up unused code.

* Lint fixes.

* Significantly reorganize invokeAll() to try and make it more cooperative
for cancellation, and increase timeout times in tests to reduce
flakiness for time-sensitive tests. Some tests are remaining flaky, so
ignoring those.

Re-add maybeWithTimeoutOrNull since it actually was needed.

* Lint fixes.

* Post-merge module fixes.

* Post-merge fixes with ratio input & add a TODO to improve speed of the
new coroutine executor service.

* Revert "Fixes part of oppia#40 & oppia#42: Generalisation Highfi Mobile Portrait + Landscape - Buttons (oppia#1653)"

This reverts commit 1bb1ffa.

* Ensure terminated tasks do not interfere with one another (timeouts
should happen individually for each task during termination). This fixes
a failure observed in StateFragmentLocalTest in oppia#1630.

* Ignore failing tests until oppia#1769 is resolved.

* Fix awaitTermination & improve test. Improve stack trace for test
dispatcher timeouts.

* Fix slow & broken tests in Robolectric for StateFragmentLocalTest.

* Add missing deps for StateFragmentLocalTest.

* Address TODOs (including adding support for list tags which replaces the
old handler & adds nested custom tag support), and add tests.

* Lint fixes.

* Address reviewer comments.

* Address review comments. Fix new concept card tests on Espresso & add
landscape versions (required configuring hints to show quickly to avoid
delaying the test, and fixing a bug in the espresso test dispatcher).
Add support for disabling concept cards of they aren't enabled for
parsing particular HTML (the default behavior is to ignore the custom
tag).

* Add support for concept cards in questions. Note that it's not clear how
to test verifying that pressing the exit button closes the concept card
since the exit button is part of the dialog's toolbar.

* Fix image-breaking duplicated code in HtmlParser, fix a paragraph
parsing issue in BulletTagHandler, and add tests for
CustomHtmlContentHandler.

* Lint fixes.
@rt4914 rt4914 added the SLoP-25 label Sep 24, 2020
@anandwana001
Copy link
Contributor

Keyboard closing in the landscape in espresoo will get solved using,

onView(withId(interactionViewId))
  .perform(typeText(text))
onView(isRoot()).perform(closeSoftKeyboard())
testCoroutineDispatchers.runCurrent()

but not in robolectric. I think we might need to create custom inputs here like -

the major issue to solve here is dragging doesn't work in Robolectric, might be timing issue, will look into deep, just a thought as of now.

@BenHenning
Copy link
Member Author

Note that this issue is actually happening in production builds so far as I can remember.

@yurilev
Copy link
Contributor

yurilev commented Feb 3, 2021

Hi, can I work on this?

@anandwana001
Copy link
Contributor

Hi, @yurilev any update on this?

@BenHenning
Copy link
Member Author

@yurilev are you sure these are related? From my experience it seems like the auto-closing happens on real Android devices (& potentially not on emulators--I forget). #1612 is describing an issue that only occurs in Robolectric due to missing functionality in the Robolectric test framework.

@yurilev
Copy link
Contributor

yurilev commented Feb 10, 2021

One sec, I'm writing the full reply with the context.

@yurilev
Copy link
Contributor

yurilev commented Feb 10, 2021

Hi @anandwana001 and @BenHenning,

I've tried reproducing again (looked at #1767 as well), unsuccessfully. I tested on:

  • About 4 mobile emulators, all running API 29. In case screen size could be related, with screens ranging from 2.7" 240x320 to 6.3" Pixel 4 XL. And one tablet.
  • Physical Pixel 2 running API 29 and Nexus 5 running API 23.

Per this comment, dismissed the keyboard in a number of different ways.

None of these reproduced the issue.

Moreover, the two skipped tests in StateFregmentTest are now passing in Espresso with no issue. PR #2165 probably fixed them, on the same day @BenHenning last commented he reproduced it.

I'll send a PR tomorrow to re-enable these tests in Espresso. #1612 covers Robolectic (there are preexisting TODOs for it there), I'll try looking into that next.

@BenHenning - unless you can still reproduce it on your device or have other suggestions for testing, I think this should be closed.

@BenHenning
Copy link
Member Author

Nice findings @yurilev. Confirming that I also can't repro the issue anymore. Bit surprised to hear that the fix correlates to #1630 since I recall reproing this after that PR, but maybe something else later on fixed the problem in conjunction with that PR.

@yurilev
Copy link
Contributor

yurilev commented Feb 11, 2021

The bit about #1630 was conjecture on my part – the changes made seemed like they could fix it, and the timeline fits. I can try and test with the version prior if you want me to confirm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Essential This work item must be completed for its milestone. Z-ibt Temporary label for Ben to keep track of issues he's triaged.
Development

Successfully merging a pull request may close this issue.

5 participants