1290823 - Credentials input screen not displayed when input box is tapped

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Logins, Passwords and Form Fill, defect, P1)

Description

[u549602]

Environment: 
Device: Samsung Galaxy S6 Edge  (Android 6.0 );
Build: Nightly 50.0a1 (2016-07-31);

Steps to reproduce:
1. Open Nightly in portrait mode or landscape
2. Go to Create a Firefox account screen
3. Tap on any input box (Email, Password, Age, newsletter checkbox)

Expected result:
Input screen should be displayed 

Actual result:
Nothing happens, Input boxes appear as grayed out

Notes:
Please note that after switching device orientation, the Input boxes become responsive upon tapping them.

This issue occurred on several devices, but not all, some of them: Samsung Galaxy S6 Edge (Android 6.0), Nexus 6 (Android 6.0),Alcatel One plus (Android 5.1.1), Xiaomi miPad  2 Android (5.1)

good/bad build:  http://hg.mozilla.org/mozilla-central/pushloghtml (30.07 to 31.07)

For further details please check : https://www.youtube.com/watch?v=d-HP3Xnic4k&feature=youtu.be

Comment 1

[u549602]

After further investigation it seems that this issue can be reproduced on several pages as buttons become unresponsive (e.g about:config option toggle, "Add to Firefox" button, browse add-ons in the add-ons page, open a document with a single tap in about:downloads, several log-in forms)

Comment 2

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hello Mark, Snorp, Mike, SoftVision has recommended blocking aurora updates on Fennec 50 on this issue. Could you please help investigate the impact? Do you agree with SV's recommendation? Will appreciate a prompt reply. Thanks!

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Especially comment 1 sounds like a platform bug. Let's see what snorp says.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

I don't think I understand the problem, or maybe I'm just not seeing it? Forms seem to be working fine here.

How do I get to the 'create a firefox account' screen?

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #4)
> I don't think I understand the problem, or maybe I'm just not seeing it?
> Forms seem to be working fine here.

There's a youtube video in comment 0, fwiw.

> How do I get to the 'create a firefox account' screen?

3-dot Settings -> Sign in.

I can reproduce on my GS4, Android 4.4, latest api 15 build from archive.mozilla.org.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Looking at the source of sync sign-in, the form is displayed within an iframe – could this be related?

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

Ah, I was already signed in so didn't see that. I can repro this on a local build on the signin page, though.

Mihai, can we get a narrower regression range? I don't see anything jumping out at me.

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked blocking for Fx50. I hope we can get a fix landed soon as this bug is currently blocking us from turning on Aurora updates on Fennec 50.

Comment 9

[u549602]

Hi all,
I've narrowed it down to the good/bad tinder builds 50.0a1(NIGHTLY) (GOOD http://ftp.mozilla.org/pub/mobile/tinderbox-builds/mozilla-central-android-api-15/1469888187/ and BAD http://ftp.mozilla.org/pub/mobile/tinderbox-builds/mozilla-central-android-api-15/1469888545/ ) with the following pushlog http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c3565c8b1cdb575db1c80c7791984a6490598b84&tochange=e5859dfe0bcbd40f4e33f4a633f73ea3473a7849 . 

These both builds are from July 30th.
The range was found on a Samsung Galaxy S6 Edge with 6.0 and on a Nexus 9 tablet with Android 6.0.1

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

mozregression should be able to figure out if it happened on inbound or fx-team, can we try to get that?

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

I bisected myself, and it looks like bug 1250024 is the culprit. Specifically, https://hg.mozilla.org/integration/mozilla-inbound/rev/3b0313fd4ce6

Kats, is out until the 14th. Botond, can you take a look? Maybe Randall?

Comment 12

[Randall Barker [:rbarker]]

I can reproduce on Nexus 5X and Galaxy S4 but not Nexus 4. Wonder if it is related to screen size (S4:1080x1920 pixels vs. N4:768x1280 pixels)? Hopefully :botond can shed some light on the issue, but I'll take a look in the mean time.

Comment 13

[Botond Ballo [:botond]]

Since bug 1250024 is concerned with the device pixel ratio, it probably requires a phone with a HiDPI display to reproduce. I don't think I have one :/ I'm doing an Android build right now to try to repro on the device I do have (which is not HiDPI), but chances are it won't reproduce there.

Comment 14

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #13)
> I'm doing an Android build right now to try to repro on the device I do
> have (which is not HiDPI), but chances are it won't reproduce there.

Turns out I *can* reproduce the problem on my device, after all. I'm looking into it.

Comment 15

[Botond Ballo [:botond]]

It looks like the regressing patch added an early-return path to AndroidContentController::HandleTap(), which is being hit during the affected STR, and results in ChromeProcessController::HandleTap() not being called, and as such the tap event never getting dispatched.

Comment 16

[Botond Ballo [:botond]]

Attachment: Bug 1290823 - Factor out the code for dispatching a single tap event to observers into its own function.

This is not just a refactoring. It ensures that the early return in the
factored-out code only skips the dispatch to observers, *not* the
additional processing by ChromeProcessController.

Review commit: https://reviewboard.mozilla.org/r/69808/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69808/

Comment 17

[Botond Ballo [:botond]]

I believe the intention was for the early return to skip the code inside the "if" block in AndroidContentController::HandleTap() (which dispatched the tap event to observers), but *not* the call to ChromeProcessController::HandleTap().

The attached patch factors out a function to get the early return to skip only what was intended.

It's probably worth looking into *why* the early return is being hit, as it seems unexpected to me, but this patch should at least fix the regression and restore the previous behaviour.

Comment 18

[Botond Ballo [:botond]]

Actually, I misread the code. Prior to bug 1250024, not having a pres shell (the condition of the early return) wouldn't result in the Gesture:SingleTap observer message not being dispatched, only in the scale-to-resolution (if any) not being unapplied from the coordinates of the point being dispatched.

This means that, while the patch I posted will help (it fixes this particular STR for me locally), it may not solve the regression in its entirety. A full solution will require investigating why we don't have a pres shell.

I will resume this investigation on Monday.

Comment 19

[Botond Ballo [:botond]]

The reason we don't have a pres shell is that we are obtaining the pres shell via GetRootContentDocumentPresShellForContent(), but we're in a chrome document (believable for the Firefox Accounts sign-in page).

I believe the correct thing to do here is to use the pres shell of whatever document the content is in, rather than the RCD pres shell, to obtain the device scale, and then the RCD pres shell, if any, to obtain a resolution (and not bail if there's no RCD pres shell because we're in chrome).

Comment 20

[Botond Ballo [:botond]]

Attachment: Bug 1290823 - Introduce a helper APZCCallbackHelper::GetPresContextForContent().

And use it in APZCCallbackHelper::GetRootContentDocumentPresShellForContent().

Review commit: https://reviewboard.mozilla.org/r/70088/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/70088/

Comment 21

[Botond Ballo [:botond]]

Attachment: Bug 1290823 - Send Gesture:SingleTap even if we're in a chrome document.

Review commit: https://reviewboard.mozilla.org/r/70090/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/70090/

Comment 22

[Botond Ballo [:botond]]

Comment on attachment 8778517 [details]
Bug 1290823 - Factor out the code for dispatching a single tap event to observers into its own function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69808/diff/1-2/

Comment 23

[Randall Barker [:rbarker]]

Comment on attachment 8779027 [details]
Bug 1290823 - Introduce a helper APZCCallbackHelper::GetPresContextForContent().

https://reviewboard.mozilla.org/r/70088/#review67308

Comment 24

[Randall Barker [:rbarker]]

Comment on attachment 8779028 [details]
Bug 1290823 - Send Gesture:SingleTap even if we're in a chrome document.

https://reviewboard.mozilla.org/r/70090/#review67310

Comment 25

[Randall Barker [:rbarker]]

Comment on attachment 8778517 [details]
Bug 1290823 - Factor out the code for dispatching a single tap event to observers into its own function.

https://reviewboard.mozilla.org/r/69808/#review67300

::: widget/android/AndroidContentController.h:52
(Diff revision 2)
>      static void NotifyDefaultPrevented(mozilla::layers::IAPZCTreeManager* aManager,
>                                         uint64_t aInputBlockId, bool aDefaultPrevented);
>  private:
>      nsWindow* mAndroidWindow;
> +
> +    void DispatchSingleTapToObservers(const LayoutDevicePoint& aPoint, 

Looks like some trailing white space snuck into the patch.

::: widget/android/AndroidContentController.cpp:46
(Diff revision 2)
>      }
>  
>      aManager->ContentReceivedInputBlock(aInputBlockId, aDefaultPrevented);
>  }
>  
> -void
> +void 

More trailing white space. :)

Comment 26

[Botond Ballo [:botond]]

Comment on attachment 8778517 [details]
Bug 1290823 - Factor out the code for dispatching a single tap event to observers into its own function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69808/diff/2-3/

Comment 27

[Botond Ballo [:botond]]

Comment on attachment 8779027 [details]
Bug 1290823 - Introduce a helper APZCCallbackHelper::GetPresContextForContent().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70088/diff/1-2/

Comment 28

[Botond Ballo [:botond]]

Comment on attachment 8779028 [details]
Bug 1290823 - Send Gesture:SingleTap even if we're in a chrome document.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70090/diff/1-2/

Comment 29

[Botond Ballo [:botond]]

Fixed trailing whitespace (that's what I get for not using my usual editor).

Comment 30

[Daniel Holbert [:dholbert]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #11)
> I bisected myself, and it looks like bug 1250024 is the culprit.

(I assume you meant to set this as "blocking" that bug [as is the conventional way to mark regressions], rather than "depends-on" that bug. --> Updating dependency.)

Comment 31

[Randall Barker [:rbarker]]

Comment on attachment 8778517 [details]
Bug 1290823 - Factor out the code for dispatching a single tap event to observers into its own function.

https://reviewboard.mozilla.org/r/69808/#review67320

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6ee3b3be309b
Factor out the code for dispatching a single tap event to observers into its own function. r=rbarker
https://hg.mozilla.org/integration/autoland/rev/7faf4ab858f3
Introduce a helper APZCCallbackHelper::GetPresContextForContent(). r=rbarker
https://hg.mozilla.org/integration/autoland/rev/511617938a50
Send Gesture:SingleTap even if we're in a chrome document. r=rbarker

Comment 33

[Botond Ballo [:botond]]

Comment on attachment 8778517 [details]
Bug 1290823 - Factor out the code for dispatching a single tap event to observers into its own function.

https://reviewboard.mozilla.org/r/69806/#review67382

Comment 35

[Botond Ballo [:botond]]

(In reply to Roberto from comment #34)
> Comment on attachment 8779028 [details]
> Bug 1290823 - Send Gesture:SingleTap even if we're in a chrome document.
> 
> Mmmm nice...

Care to elaborate? The patch is just restoring the behaviour to what it was prior to bug 1250024, it's not introducing any new behaviour.

Comment 36

[Daniel Holbert [:dholbert]]

(Ignore comment 34; that user seems to be trolling / messing around, based on another recent bug comment of his. Roberto, please stop.)

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6ee3b3be309b
https://hg.mozilla.org/mozilla-central/rev/7faf4ab858f3
https://hg.mozilla.org/mozilla-central/rev/511617938a50

Comment 40

[Botond Ballo [:botond]]

:Ninu, could you confirm that you no longer see the issue in the latest Nightly? If so I'll request uplift to 50.

Comment 42

[u549602]

Verified as fixed on the latest Nightly build (51.0a1 / 2016-08-10). 
This issue was tested using a Samsung Galaxy S6 EDGE (Android 6.0), all depending and dupes of this issue were verified to make sure the fix was correctly implemented and I can confirm that it works.
Thanks for the effort, waiting for Aurora uplift.

Comment 44

[Botond Ballo [:botond]]

Comment on attachment 8778517 [details]
Bug 1290823 - Factor out the code for dispatching a single tap event to observers into its own function.

Great, thanks a lot for testing!

This uplift request applies to all three patches in this bug:

Approval Request Comment
[Feature/regressing bug #]:
  Bug 1250024

[User impact if declined]:
  Tapping in some settings pages (e.g. Firefox accounts login,
  about:config), forms, and dialogs brought up by some websites
  does not work.

[Describe test coverage new/current, TreeHerder]:
  QA verified the fix fixes this bug and all its duplicates
  (see comment 42).

[Risks and why]: 
  Low, this is a small and straightforward change.

[String/UUID change made/needed]:
  None

Comment 45

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8778517 [details]
Bug 1290823 - Factor out the code for dispatching a single tap event to observers into its own function.

Fixes a recent blocking regression in 50, let's uplift to Aurora.

Comment 46

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c5d62f297f9a
https://hg.mozilla.org/releases/mozilla-aurora/rev/285042c2d340
https://hg.mozilla.org/releases/mozilla-aurora/rev/3237f302d678

Comment 47

[Kartikaya Gupta (email:[e-mail])]

Thanks for fixing this!

Comment 48

[Alex Caziuc]

Verified as fixed using the steps provided by Ninu in description with Galaxy Note 5(6.0.1) on Aurora 50.0a2 (2016-08-31)