Some buttons become unresponsive in gaia header

VERIFIED FIXED in Firefox 38

Status

()

defect
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: noemi, Assigned: kats)

Tracking

({regression, verifyme})

unspecified
mozilla38
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, firefox36 wontfix, firefox37 wontfix, firefox38 fixed, b2g-v2.1 unaffected, b2g-v2.2 verified, b2g-master verified)

Details

(URL)

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

4 years ago
We have seen that some buttons become unresponsive in gaia header, some examples:
1- Being in Call log (having some entries), try to access Settings (edit mode) several times. Please see https://www.youtube.com/watch?v=kywIIuckiLA, in this case, the first attempt fails, it is necessary to click again on the "Settings" button to access the edit mode window.
2- Being in Contacts app and having some Contacts, try to access Settings several times, you will see that at some point Settings or Done button become unresponsive, it is necessary to click again on Settings or Done buttons to move on.
3- Being in Messages app, try to access Settings within Messages app several times, at some point Settings button becomes unresponsive, it is necessary to click again on Settings button to move on.

Environmental variables:
Flame device
Build Id: 20150210155432
Gecko: 35d3936
Gaia: 0707fcc
Platform version: 37.0a2

We haven NOT seen this wrong behavior in master. 

qawanted to confirm this error and to see which branches are affected. Thanks!
QA Contact: bzumwalt
Issue does NOT reproduce in Flame 2.1

Gaia header buttons do not become unresponsive in Call log, Contacts, Messages, or any other app in Flame 2.1

Device: Flame 2.1
Build ID: 20150210002200
Gaia: 7dd130a312f12c89b2d41948f8557effa56afbf6
Gecko: 2de03dfa9aac
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 34.0 (2.1)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0


Issue may reproduce in 3.0

Could not reproduce any of the examples using 3.0 (in 2.2 the examples provided readily reproduce), but the back button in Settings app experienced some similar issues ONLY when the page was still populating. 

If user launches messages app, taps ellipsis icon, selects settings, then quickly taps the back button when the Messaging Settings page launches, the back button in the header is unresponsive for a short while. Unsure if this is same issue as it appears to be very specific and none of the examples from comment 0 reproduce in 3.0. Leaving 3.0 as unaffected for the time being.

Device: Flame 3.0
Build ID: 20150210010523
Gaia: 0cf517083f7eb5fc269e1236edba50534f65e3cd
Gecko: 2cb22c058add
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 38.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Keywords: qawantedregression
This is a bad end user experience. The button should work when tapped. Nominating 2.2?
blocking-b2g: --- → 2.2?
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
QA Contact: bzumwalt → ychung
We just landed a very big change in gaia-header (bug 1112131). Could QA check whether the issue happens with/without this patch?

I just merged to v2.2 so I don't think the patch is in any v2.2 nightly build yet. So maybe that's why Noemi sees the issue in v2.2 but not master.
Keywords: qawanted
Component: Gaia → Gaia::Components
(In reply to Brogan Zumwalt [:BroganZ] from comment #1)
> Issue may reproduce in 3.0
> 
> Could not reproduce any of the examples using 3.0 (in 2.2 the examples
> provided readily reproduce), but the back button in Settings app experienced
> some similar issues ONLY when the page was still populating. 
> 
> If user launches messages app, taps ellipsis icon, selects settings, then
> quickly taps the back button when the Messaging Settings page launches, the
> back button in the header is unresponsive for a short while. Unsure if this
> is same issue as it appears to be very specific and none of the examples
> from comment 0 reproduce in 3.0. Leaving 3.0 as unaffected for the time
> being.

Yeah, this is bug 1125601. So let's leave this out of this bug :)


I actually also wonder if this bug is not about the rocketbar as well. Like the "event fluffing" mechanism would choose something in the rocketbar instead of the gaia header button.

Etienne, would you remember something about this, that would be fixed in master but not in v2.2?
Flags: needinfo?(etienne)
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][MGSEI-Triage+]
mozilla-inbound regression window:

Last Working Environmental Variables:
Device: Flame
Build ID               20150129162506
Gaia Revision          6e494f1d2676d231abba7dcc2e2822d1170d2d02
Gaia Date              2015-01-29 06:34:56
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/9c8b785973bf
Gecko Version          37.0a2
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150129.202729
Firmware Date          Thu Jan 29 20:27:40 EST 2015
Bootloader             L1TC000118D0

First Broken Environmental Variables:
Device: Flame
Build ID               20150129183930
Gaia Revision          6e494f1d2676d231abba7dcc2e2822d1170d2d02
Gaia Date              2015-01-29 06:34:56
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/242eb3edf1bf
Gecko Version          37.0a2
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150129.202729
Firmware Date          Thu Jan 29 20:27:40 EST 2015
Bootloader             L1TC000118D0

Last Working gaia / First Broken gecko - Issue DOES occur
Gaia: 6e494f1d2676d231abba7dcc2e2822d1170d2d02
Gecko: 242eb3edf1bf

First Broken gaia / Last Working gecko - Issue does NOT occur
Gaia: 6e494f1d2676d231abba7dcc2e2822d1170d2d02
Gecko: 9c8b785973bf
Given the regression window, this is not rocketbar related.
Flags: needinfo?(etienne)
Given the pushlog, this could be bug 1116588, given that maybe we trigger the edge gestures trigger when we tap on the button.

Kats, what do you think?
Component: Gaia::Components → Layout
Flags: needinfo?(bugmail.mozilla)
Product: Firefox OS → Core
That's quite possible. If that is case then it might be fixed by bug 1128672 in master. I will request uplift on that bug shortly, but it needs to get uplifted with a bunch of other stuff. Marking it as a dependency for now and parking this bug with me.
Assignee: nobody → bugmail.mozilla
Depends on: 1128672
Flags: needinfo?(bugmail.mozilla)
Blocks: 1116588
So I'm able to reproduce this problem on a local 2.2 build, and the patch from bug 1128672 doesn't entirely fix it. The problem appears to be that when the touch_forwarder.js sends the mouse event using iframe.sendMouseEvent to the child process, that goes through [1] and that never accounts for the 30-pixel offset to the child process root layer. So all the mouseevents sent by sendMouseEvent (and sendTouchEvent) are going to be off by 30 pixels, always. In fact I'm amazed that we haven't run into this until now!

[1] http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementParent.js?rev=bccb6cedee61#548
Oh actually sendTouchEvent doesn't have this problem if APZ is enabled on the child process, because then it uses TabParent.injectTouchEvent. So that probably explains why it's been obscured. Still, I would expect this bug to manifest in 2.1 as well and I'm not really sure why it supposedly doesn't.
Posted patch WIP (obsolete) — Splinter Review
For example, this fixes the problem (when combined with bug 1128672) but obviously isn't something we can land. If necessary we can extract that 30 from the tabparent, but I have a different idea that I would like to explore first.
The window for this issue may have been found incorrectly.  The builds used seem to be working with gaia and gecko from different branches which might have caused false positives when finding last working and first broken builds.  Rechecking this window might be the correct course of action.
QA Whiteboard: [QAnalyst-Triage+][MGSEI-Triage+] → [QAnalyst-Triage?][MGSEI-Triage+]
Flags: needinfo?(ktucker)
QA Contact: ychung
Flags: needinfo?(ktucker)
QA Contact: ktucker
Julien appears to be correct here.

bug 1116588 seems to be the cause of this issue.

Mozilla-inbound regression window:

Last Working Environmental Variables:
Environmental Variables:
Device: Flame 3.0
BuildID: 20150126062631
Gaia: 793773bb2944b42a85dd160049e605cbd880c4da
Gecko: 0bec74187553
Version: 38.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0

First Broken Environmental Variables:
Environmental Variables:
Device: Flame 3.0
BuildID: 20150126065334
Gaia: 793773bb2944b42a85dd160049e605cbd880c4da
Gecko: babd56077826
Version: 38.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0

Last Working Gecko & First Broken Gaia - issue DOES occur
Gaia: 793773bb2944b42a85dd160049e605cbd880c4da
Gecko: babd56077826

Last Working Gaia & First Broken Gecko - issue does NOT occur
Gaia: 793773bb2944b42a85dd160049e605cbd880c4da
Gecko: 0bec74187553

Gaia pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=0bec74187553&tochange=babd56077826
Issue still occurs in latest Flame 2.2

Clearing QAWanted as issue still occurs and bug 1112131 is a gaia fix while this bug is a gecko issue.

Confirming that comment 14 is calling this is a gecko issue that occurs on first broken gecko and the pushlog provided is a gecko, and not a gaia pushlog.

Device: Flame 2.2
Build ID: 20150212002504
Gaia: 791e53728cd8018f1d7cf7efe06bbeb1179f0370
Gecko: 5dec207fcbeb
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 37.0a2 (2.2)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0
Flags: needinfo?(ktucker)
Keywords: qawanted
QA Whiteboard: [QAnalyst-Triage?][MGSEI-Triage+] → [QAnalyst-Triage+][MGSEI-Triage+]
Flags: needinfo?(ktucker)
I'm able to reproduce this on master as well, fwiw. That's expected given the root cause.
Component: Layout → DOM: Content Processes
Comment on attachment 8563411 [details] [diff] [review]
Part 1 - Refactor to get the CSS -> device pixel scale

Review of attachment 8563411 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the question resolved.

::: dom/ipc/TabParent.cpp
@@ +1168,5 @@
> +  nsIPresShell* shell = (doc ? doc->GetShell() : nullptr);
> +  nsPresContext* ctx = (shell ? shell->GetPresContext() : nullptr);
> +  return LayoutDeviceToCSSScale(ctx
> +    ? (float)ctx->AppUnitsPerDevPixel() / nsPresContext::AppUnitsPerCSSPixel()
> +    : 0.0f);

Are you sure you want 0.0 here, and not 1.0 ?
Attachment #8563411 - Flags: review?(fabrice) → review+
Comment on attachment 8563412 [details] [diff] [review]
Part 2 - Ensure the child process offset is accounted for in BrowserElementParent.js

Review of attachment 8563412 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not a big fan of the code duplication in BrowserElementParent.js
What about doing something like that instead:

let offsets;
let tabParent = this._frameLoader.tabParent;
if (tabParent) {
      let offsetX = {};
      let offsetY = {};
      tabParent.getChildProcessOffset(offsetX, offsetY);
      offsets = { x: offsetX.value, y: offsetY.value };
      x += offsets.x;
      y += offsets.y;
}

.....

if (offsets) {
  for (let i = 0; i < touchesX.length; i++) {
    touchesX[i] += offsets.x;
  }
  for (let i = 0; i < touchesY.length; i++) {
    touchesY[i] += offsets.y;
  }
}
Attachment #8563412 - Flags: review?(fabrice) → feedback+
Duplicate of this bug: 1131837
(In reply to Fabrice Desré [:fabrice] from comment #19)
> 
> Are you sure you want 0.0 here, and not 1.0 ?

Yes, that way on failure the offset is dropped entirely. That's what the code did before and I didn't want to change the error-condition behaviour.

I'll refactor the second patch to reduce duplication.
Attachment #8564182 - Flags: review?(fabrice) → review+
(Assignee)

Comment 26

4 years ago
failuretry
The test is wrong; it sends coordinates at (10,10) in the top-level window's coordinate space which end up being negative in the child iframe coordinate space. For some reason mouse events don't check for that but touch events do, and so the touch events get discarded in the child process. Adjusting the coordinates in the test so that they fall inside the iframe fixes it for me locally.

Try push to verify: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6739e695a60f
Attachment #8564182 - Attachment is obsolete: true
Flags: needinfo?(bugmail.mozilla)
Attachment #8564544 - Flags: review?(fabrice)
Attachment #8564544 - Flags: review?(fabrice) → review+
The updated try push failed also. I did another push with extra logging and it looks like on the tryserver machines the child process offset is -189, -373 and the touch coordinates are at 276,108. So the y-coord still ends up negative which will cause the event to get dropped and the test to fail.
(Assignee)

Comment 28

4 years ago
greentry
I ended up having to call the tabParent method in the test as well to make this work.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c01b3b816443
sorry had to back this out for perma test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=6670856&repo=mozilla-inbound
Flags: needinfo?(bugmail.mozilla)
(Assignee)

Comment 31

4 years ago
greentry
Third time's the charm. Let's see how this try run does:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9e96f3235a2
Flags: needinfo?(bugmail.mozilla)
https://hg.mozilla.org/mozilla-central/rev/9aeb417a0af8
https://hg.mozilla.org/mozilla-central/rev/b41c6cb9d022
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
blocking-b2g: 2.2? → 2.2+
Kats, we'll need an approval here :)
Thanks !
Flags: needinfo?(bugmail.mozilla)
(Assignee)

Comment 35

4 years ago
see-comment36
Comment on attachment 8563411 [details] [diff] [review]
Part 1 - Refactor to get the CSS -> device pixel scale

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 1116588
User impact if declined: when clicking on items near the left or right edge of the screen, the click is off by the height of the status bar
Testing completed: locally, on m-c
Risk to taking this patch (and alternatives if risky): low risk as this code only run in a small set of cases.
String or UUID changes made by this patch: none
Flags: needinfo?(bugmail.mozilla)
Attachment #8563411 - Flags: approval-mozilla-b2g37?
Attachment #8564544 - Flags: approval-mozilla-b2g37?
Ah, sorry - the part 2 patch does bump the UUID in dom/interfaces/base/nsITabParent.idl!
Keywords: verifyme
Attachment #8563411 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8564544 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+

Comment 39

4 years ago
This issue has been verified successfully on Flame2.2&3.0,
Verify video: "verify.mp4"

Flame 2.2 build:
Build ID               20150302002504
Gaia Revision          77609916ca5ab721150fab2b7bc5c37f43ee3a5a
Gaia Date              2015-02-27 16:35:06
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/27ab8aa34201
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150302.042723
Firmware Date          Mon Mar  2 04:27:34 EST 2015
Bootloader             L1TC000118D0

Flame 3.0 build:
Build ID               20150302010223
Gaia Revision          f34ce82a840ad3c0aed3bfff18517b3f6a0eb37f
Gaia Date              2015-02-27 15:48:31
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/eea6188b9b05
Gecko Version          39.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150302.043726
Firmware Date          Mon Mar  2 04:37:37 EST 2015
Bootloader             L1TC000118D0

Updated

4 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.