Credentials input screen not displayed when input box is tapped

VERIFIED FIXED in Firefox 50

Status

()

Firefox for Android
Logins, Passwords and Form Fill
P1
major
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: Ninu, Assigned: botond)

Tracking

({qablocker, regression})

Trunk
Firefox 51
ARM
Android
qablocker, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

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

Details

User Story

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

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
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
(Reporter)

Updated

2 years ago
Keywords: qablocker
(Reporter)

Updated

2 years ago
Component: General → Logins, Passwords and Form Fill
(Reporter)

Updated

2 years ago
tracking-fennec: --- → ?
User Story: (updated)
(Reporter)

Comment 1

2 years ago
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)

Updated

2 years ago
Severity: normal → major

Comment 2

2 years ago
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)

Comment 8

2 years ago
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
(Reporter)

Comment 9

2 years ago
regressionwindow
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)
(Assignee)

Comment 13

2 years ago
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.
(Assignee)

Comment 14

2 years ago
(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)
(Assignee)

Comment 15

2 years ago
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.
(Assignee)

Comment 16

2 years ago
Created attachment 8778517 [details]
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/
Attachment #8778517 - Flags: review?(snorp)
(Assignee)

Comment 17

2 years ago
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.
(Assignee)

Comment 18

2 years ago
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)

Updated

2 years ago
Assignee: nobody → botond
(Assignee)

Comment 19

2 years ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 23

2 years ago
mozreview-review
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 24

2 years ago
mozreview-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 25

2 years ago
mozreview-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. :)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 29

2 years ago
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 31

2 years ago
mozreview-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/#review67320
Attachment #8778517 - Flags: review?(rbarker) → review+

Comment 32

2 years ago
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
(Assignee)

Comment 33

2 years ago
mozreview-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/69806/#review67382
Comment hidden (spam)
Comment hidden (offtopic)
(Ignore comment 34; that user seems to be trolling / messing around, based on another recent bug comment of his. Roberto, please stop.)
(Assignee)

Updated

2 years ago
See Also: → bug 1293494
See Also: → bug 1290887
(Reporter)

Updated

2 years ago
Duplicate of this bug: 1293272
Blocks: 1290887

Comment 38

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6ee3b3be309b
https://hg.mozilla.org/mozilla-central/rev/7faf4ab858f3
https://hg.mozilla.org/mozilla-central/rev/511617938a50
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51

Updated

2 years ago
Duplicate of this bug: 1290870
(Assignee)

Comment 40

2 years ago
: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)
Duplicate of this bug: 1292002
(Reporter)

Comment 42

2 years ago
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)
Duplicate of this bug: 1290887
tracking-fennec: ? → 50+
(Assignee)

Comment 44

2 years ago
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+

Updated

2 years ago
Attachment #8779027 - Flags: approval-mozilla-aurora+

Updated

2 years ago
Attachment #8779028 - Flags: approval-mozilla-aurora+
Thanks for fixing this!

Comment 48

2 years ago
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
You need to log in before you can comment on or make changes to this bug.