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

LocalUiController does not inject events to the correct Window #6741

Closed
wesalvaro opened this issue Sep 28, 2021 · 10 comments
Closed

LocalUiController does not inject events to the correct Window #6741

wesalvaro opened this issue Sep 28, 2021 · 10 comments

Comments

@wesalvaro
Copy link
Contributor

wesalvaro commented Sep 28, 2021

Description

LocalUiController always selects the top-most Window to deliver MotionEvents and KeyEvents. The top-most Window is not the correct heuristic to use as it can be non-interactable. This creates confusing issues when interacting with the main application when a tooltip-like PopupWindow is showing. Events seem to simply disappear/never be delivered.

I would propose improving the heuristic in the LocalUiController. At least to take into account the Window flags (e.g. FLAG_NOT_TOUCHABLE and FLAG_NOT_FOCUSABLE). Ideally we could further look at which Window should receive the event; the WindowManager.LayoutParams do contain location and sizing information but that would require measuring the Window contents (as in the case of WRAP_CONTENT).

The current workaround is to:

  1. Realize the problem
  2. Verify the behavior is erroneous
  3. Create one's own UiController that may look like:
    private class WorkaroundUiController(private val view: View) : UiController {
      override fun injectMotionEvent(event: MotionEvent?): Boolean {
        requireNotNull(event)
        view.rootView.dispatchTouchEvent(event)
        return true
      }
    
      override fun injectKeyEvent(event: KeyEvent?): Boolean {
        requireNotNull(event)
        view.rootView.dispatchKeyEvent(event)
        return true
      }
    
      override fun injectString(str: String?): Boolean {
        TODO("Not yet implemented")
      }
    
      override fun loopMainThreadUntilIdle() = idleMainLooper()
      override fun loopMainThreadForAtLeast(millis: Long) = idleMainLooper(millis, MILLISECONDS)
    }
    
  4. Create one's own ViewAction that may look like:
    fun click() = viewAction("click in the view's window") {
      _, view ->
      ViewActions.click().perform(WorkaroundUiController(view), view)
    }
    

Steps to Reproduce

  1. Show a small, non-focusable, non-touchable PopupWindow.
  2. Use Espresso to click on a view in the main application Window:
    onView(withId(R.id.foo)).perform(click())
    

Expected: Click is delivered to the main application Window
Observed: Click is delivered to the PopupWindow

The same can be said for a variety of situations.

Robolectric & Android Version

Current Robolectric.
Android Version does not apply.

Link to a public git repo demonstrating the problem:

Internal b/199994449 has more details, discussion, and examples.

@utzcoz
Copy link
Member

utzcoz commented Sep 28, 2021

Ideally we could further look at which Window should receive the event; the WindowManager.LayoutParams do contain location and sizing information but that would require measuring the Window contents (as in the case of WRAP_CONTENT).

It is also useful for multi-window occasion, including split screen and freeform. From network information, there are so many fold and tablets devices will leverage split screen to enhance Android experience, and in my opinion supporting for those occasion is very important for Robolectric. @wesalvaro Looking forward your PR to improve it. cc @hoisie .

The Robolectric has a module called ctesque to test occasion for Robolectric and emulator both to test behavior fidelity of Robolectric. It is useful to test improved simulation.

@hoisie
Copy link
Contributor

hoisie commented Sep 28, 2021

👍 to adding some heuristics to LocalUiController to select a more appropriate window. @wesalvaro would you be able to send a PR for that change?

@wesalvaro
Copy link
Contributor Author

lol. I'm not sure what's up with that copybara patch.
Doesn't look like what I thought it would...

@utzcoz
Copy link
Member

utzcoz commented Sep 29, 2021

lol. I'm not sure what's up with that copybara patch. Doesn't look like what I thought it would...

If you add your Google email to your GitHub account, I think the GitHub will show you as author. What I learnt from those copybara PRs is that it will copy and send a PR if internal has a commit, and it contains previous commits copied by copybara to Robolectric GitHub with PRs, that not been merged. So your related commit contains many changes that not you did. @hoisie will merge those PRs with sent sequence one by one, and you PRs will contain your change only when it is merged. For more details about copybara, I think @hoisie can provide more insights.

And @wesalvaro could you add Fix: this issue link to link that PR to this issue? When it is merged, this issue will be closed automatically.

@hoisie
Copy link
Contributor

hoisie commented Sep 29, 2021

@wesalvaro sorry about that, the way we use Copybara is totally broken. The PRs from copybra are effectively meaningless, it displays the last N commits since the last time Google contributions are synced to GitHub. It is high on our TODO list to improve this situation.

@wesalvaro
Copy link
Contributor Author

@utzcoz
Interesting! I do have my email address setup on my account... Hmm...
I intentionally left off the Fix because I'd also like to try a more intelligent heuristic based on the size/location as well before calling this bug fixed. As-is with my tweak, it'll allow for a workaround on the side of the developer (having the correct flags set) but it's not a great final solution.

@utzcoz
Copy link
Member

utzcoz commented Sep 30, 2021

@wesalvaro https://github.com/robolectric/robolectric/commit/52cc2fd6832f723fc33530250501fbd328c6b1f6.patch from this commit message, it shows author is Googler noreply@google.com. Maybe it is a problem of copybara. If the author email is your linked emails at GitHub account, it will show you at GitHub commit page and other locations at GitHub.

@wesalvaro
Copy link
Contributor Author

@utzcoz Maybe it's because that's a compound patch with more than just my change?

copybara-service bot pushed a commit that referenced this issue Nov 24, 2021
copybara-service bot pushed a commit that referenced this issue Nov 24, 2021
copybara-service bot pushed a commit that referenced this issue Nov 24, 2021
@utzcoz
Copy link
Member

utzcoz commented Nov 24, 2021

@utzcoz Maybe it's because that's a compound patch with more than just my change?

Sorry, I don't know details about copybara-service. Maybe @hoisie can help explain this problem for you.

hoisie pushed a commit that referenced this issue Nov 28, 2021
hoisie added a commit that referenced this issue Nov 28, 2021
Check Window flags when selecting root view in LocalUiController. #6741
@utzcoz
Copy link
Member

utzcoz commented Jan 7, 2022

@wesalvaro Looks like related PR is merged. I will close this issue. If you have any problem about it, feel free to reopen it. Thanks.

@utzcoz utzcoz closed this as completed Jan 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants