Closed Bug 1290823 Opened 8 years ago Closed 8 years ago

Credentials input screen not displayed when input box is tapped

Categories

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

Product:
Firefox for Android Graveyard
Component:
Logins, Passwords and Form Fill
Platform:
ARM
Android
Type:
defect

Tracking

(firefox47 unaffected, firefox48 unaffected, firefox49 unaffected, fennec50+, firefox50blocking verified, firefox51 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 51
Tracking Flags:
Tracking Status
firefox47 --- unaffected
firefox48 --- unaffected
firefox49 --- unaffected
fennec 50+ ---
firefox50 blocking verified
firefox51 --- verified

People

(Reporter: u549602, Assigned: botond)

References

Details

(Keywords: qablocker, regression)

User Story

Please note that this is the correct pushlog http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2ea3d51ba1bb9f5c3b6921c43ea63f70b4fdf5d2&tochange=e5859dfe0bcbd40f4e33f4a633f73ea3473a7849

Attachments

(3 files)

Bug 1290823 - Factor out the code for dispatching a single tap event to observers into its own function.
58 bytes, text/x-review-board-request
rbarker
: review+
ritu
: approval-mozilla-aurora+
Details
Bug 1290823 - Introduce a helper APZCCallbackHelper::GetPresContextForContent().
58 bytes, text/x-review-board-request
rbarker
: review+
ritu
: approval-mozilla-aurora+
Details
Bug 1290823 - Send Gesture:SingleTap even if we're in a chrome document.
58 bytes, text/x-review-board-request
rbarker
: review+
ritu
: approval-mozilla-aurora+
Details 
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
Keywords: qablocker
Component: General → Logins, Passwords and Form Fill
tracking-fennec: --- → ?
User Story: (updated)
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)
Severity: normal → major
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!
Flags: needinfo?(snorp)
Flags: needinfo?(michael.l.comella)
Flags: needinfo?(mark.finkle)
Especially comment 1 sounds like a platform bug. Let's see what snorp says.
status-firefox51: --- → affected
Flags: needinfo?(michael.l.comella)
Flags: needinfo?(mark.finkle)
Priority: -- → P1
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?
Flags: needinfo?(snorp)
Flags: needinfo?(mihai.ninu)
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
Flags: needinfo?(snorp)
Looking at the source of sync sign-in, the form is displayed within an iframe – could this be related?
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.
Flags: needinfo?(snorp)
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.
tracking-firefox50: --- → blocking
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
Flags: needinfo?(mihai.ninu)
mozregression should be able to figure out if it happened on inbound or fx-team, can we try to get that?
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?
Flags: needinfo?(rbarker)
Flags: needinfo?(botond)
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.
Flags: needinfo?(rbarker)
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.
(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.
Flags: needinfo?(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.
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/
Attachment #8778517 - Flags: review?(snorp)
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.
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.
Assignee: nobody → 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 on attachment 8779027 [details]
Bug 1290823 - Introduce a helper APZCCallbackHelper::GetPresContextForContent().

https://reviewboard.mozilla.org/r/70088/#review67308
Attachment #8779027 - Flags: review?(rbarker) → review+
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
Attachment #8779028 - Flags: review?(rbarker) → review+
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. :)
Fixed trailing whitespace (that's what I get for not using my usual editor).
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.)
Blocks: 1250024
No longer depends on: 1250024
Keywords: regression
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
Attachment #8778517 - Flags: review?(rbarker) → review+
Pushed by bballo@mozilla.com:
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 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
(Ignore comment 34; that user seems to be trolling / messing around, based on another recent bug comment of his. Roberto, please stop.)
See Also: → 1293494
:Ninu, could you confirm that you no longer see the issue in the latest Nightly? If so I'll request uplift to 50.
Flags: needinfo?(mihai.ninu)
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.
status-firefox51: fixed → verified
Flags: needinfo?(mihai.ninu)
tracking-fennec: ? → 50+
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
Attachment #8778517 - Flags: approval-mozilla-aurora?
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.
Attachment #8778517 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Thanks for fixing this!
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)
Status: RESOLVED → VERIFIED
status-firefox50: fixed → verified
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: