1131840 - [Windows Management] - Tapping on the gray 'active alarm' banner at the top of the screen does not take the user to the active full-page alarm

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Joshua Mitchell (Inactive)]

Attachment: logcat_20150210_1523.txt

Description:
 If you set an alarm to go off through the Clock app, and then hit the homescreen button when you receive the alarm notification you can minimize the notification and return to the homescreen. The full-page alarm notification will now be represented as a gray banner at the top of the screen. If you click on this notification banner you are not taken back to the full screen alarm notification. 

Repro Steps:
1) Update a Flame to 20150209010211
2) Open the Clock app
3) Select the Timer Tab
4) Set an alarm for 1 minute, 
5) *Optional* set sound to no-sound, and turn vibrate off (for the sanity of your coworkers)
6) Hit Start
7) When you receive the notification hit the homescreen button
8) Tap the gray alarm banner at the top of the screen (if you miss the timer which goes away after a few seconds, it will return after 90 seconds or you can enlarge the alarm notification via the notification menu and re-minimize it using the homescreen button to prompt the banner again)

Actual:
Tapping on the banner does not take the user to the full screen alarm notification

Expected:
Tapping on the banner will take the user to the full screen alarm notification 

Environmental Variables:
Device: Flame 3.0
Build ID: 20150209010211
Gaia: 0d7b35f23402c4cb29bca6b98280fec48a196dec
Gecko: 3436787a82d0
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


Repro frequency: 8/8
See attached: logcat, video: http://youtu.be/BWDiA3H_mCs

-----------------------------------------------------------------------------------

This issue does NOT repro in 2.2, 2.1, or 2.0

Device: Flame 2.2 (KK - Nightly - Full Flash)
Build ID: 20150209002504
Gaia: e827781324cbde91d2434b388f5dead3303a85ee
Gecko: 0552759956d3
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

Device: Flame 2.2 (KK - Nightly - Full-Flashed)
Build ID: 20150209002504
Gaia: e827781324cbde91d2434b388f5dead3303a85ee
Gecko: 0552759956d3
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 37.0a2 (2.2)
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Device: Flame 2.1 (KK - Nightly - Full-Flashed)
Build ID: 20150209001212
Gaia: 4a14bb118d55f3d15293c2ff55b7f29f9b0bfcdb
Gecko: 6cbe28d0bb8c
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

Device: Flame 2.0 (KK - Nightly - Full-Flashed)
Build ID: 20150209000204
Gaia: 2989f2b2bd12fcc0e9c017d2db766e76a55873b8
Gecko: ad3cf982b94d
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 32.0 (2.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 1

[Peter Bylenga [:PBylenga]]

[Blocking Requested - why for this release]:
Functional regression of a core feature.

Requesting a window.

Comment 2

[Paladin]

Attachment: logcat_flame_1532.txt

Mozilla-inbound Regression Window
Last Working 
Environmental Variables:
Gaia-Rev        0db8a38f9fed18ae2abf5ef7e1b6e2a570b07e0e
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/8b881bea204a
Build-ID        20141223163030
Version         37.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150210.050217
FW-Date         Tue Feb 10 05:02:28 EST 2015
Bootloader      L1TC000118D0

First Broken 
Environmental Variables:
Gaia-Rev        cb1dad4881533bff9f06d47e34983c7b10c04a8c
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/b17e7747d3fb
Build-ID        20141223164836
Version         37.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150210.050217
FW-Date         Tue Feb 10 05:02:28 EST 2015
Bootloader      L1TC000118D0

Last Working gaia / First Broken gecko - Issue does NOT occur
Gaia: 0db8a38f9fed18ae2abf5ef7e1b6e2a570b07e0e
Gecko: https://hg.mozilla.org/mozilla-central/rev/b17e7747d3fb

First Broken gaia / Last Working gecko - Issue DOSE occur
Gaia: cb1dad4881533bff9f06d47e34983c7b10c04a8c
Gecko: https://hg.mozilla.org/mozilla-central/rev/8b881bea204a

Gecko Pushlog:
See log：logcat_flame_1532.txt

Comment 3

[Pi Wei Cheng [:piwei] (inactive)]

The above window is incorrect. The following is the correct window.

mozilla-inbound regression window:

Last Working Environmental Variables:
Device: Flame
BuildID: 20150201170935
Gaia: 740c7c2330d08eb9298597e0455f53d4619bbc1a
Gecko: 231a8c61b49f
Version: 38.0a1 (3.0 Master)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0

First Broken Environmental Variables:
Device: Flame
BuildID: 20150201174135
Gaia: 740c7c2330d08eb9298597e0455f53d4619bbc1a
Gecko: bcefc7d8d885
Version: 38.0a1 (3.0 Master)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0

Gaia is the same so it's a Gecko issue.

Gecko pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=231a8c61b49f&tochange=bcefc7d8d885

Caused by Bug 950934.

Comment 4

[KTucker [:KTucker][Inactive 3/4/2016]]

Botond, can you take a look at this please? Looks like the work done on Bug 950934 might have caused this to occur?

Comment 5

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

I can do the initial investigation here as Botond is off today.

Comment 6

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

I've been trying to catch this banner in WebIDE but I'm going around in circles and not getting anywhere. Marcus, do you know how I can (1) make this banner persistent so that it doesn't keep disappearing, and that I can debug it more easily and (2) find this banner in WebIDE? I think I found the code relating to it but the relevant .js files don't show up in the debugger view of either the system process or clock process, and when I browser around in the inspector I can't find the relevant elements either.

Comment 7

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

Ok, after digging around some more I figured out how this is handled. There's showing/hiding of the banner is controlled by the timeouts in attention_toaster.js in the system process. The banner it self is shown in a 40px-high "attentionwindow" which actually loads the clock app's onring.html file inside a mozbrowser. The click listener is at [1]. What appears to be happening though is that the click listener is registered in the system process, and the click event is actually dispatched to the onring.html child process because that's what the APZ hit test returns. I haven't confirmed this last bit yet but it seems likely and would explain why this bug is happening. This might also need to be fixed on the gaia side because in general when you have an iframe and the user taps inside the iframe the containing page should not be receiving the click event; it's only the innermost window that receives it.

[1] https://github.com/mozilla-b2g/gaia/blob/29ffd6441fef57ebe0c540f6d983ff4dc76d4ccf/apps/system/js/attention_window.js#L133

Comment 8

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

So I thought I would fix this by setting pointer-events:none on the iframe that holds the onring.html so that the APZ would exclude that from the hit test. Turns out that iframe already has pointer-events:none set on it, and in the new APZ codepaths we're not respecting it properly. We handle the in-process case fine but this is a cross-process case (where pointer-events:none in the parent process encompasses the entire child process) and we don't handle that case. Working on a fix now.

Comment 9

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

Attachment: Part 1 - convert the forceDispatchToContent flag to an enum (no functional change)

Comment 10

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

Attachment: Part 2 - Add a flag to force an empty hit region

This is to propagate the pointer-events:none from a parent process to a child process.

Comment 11

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

Attachment: Part 3 - Workaround for bug 1132895

Since this will need uplifting to 2.2, I'd rather work around that bug than fix it properly right now.

Comment 12

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

Attachment: Part 2 - Add a flag to force an empty hit region

Whoops, uploaded the wrong version last time.

Comment 13

[Botond Ballo [:botond]]

Comment on attachment 8564162 [details] [diff] [review]
Part 1 - convert the forceDispatchToContent flag to an enum (no functional change)

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

::: gfx/layers/apz/src/HitTestingTreeNode.h
@@ +81,5 @@
>  
>    void SetHitTestData(const EventRegions& aRegions,
>                        const gfx::Matrix4x4& aTransform,
>                        const Maybe<nsIntRegion>& aClipRegion,
> +                      const EventRegionsOverride& aOverride);

I generally prefer passing enums by value.

@@ +125,5 @@
>     * because we may use the composition bounds of the layer if the clip is not
>     * present. This value is in L's ParentLayerPixels. */
>    Maybe<nsIntRegion> mClipRegion;
>  
> +  /* Indicates whether or not the event regions on this node needs to be

s/needs/need

::: layout/ipc/RenderFrameParent.cpp
@@ +636,5 @@
>    int32_t appUnitsPerDevPixel = mFrame->PresContext()->AppUnitsPerDevPixel();
>    nsIntRect visibleRect = GetVisibleRect().ToNearestPixels(appUnitsPerDevPixel);
>    visibleRect += aContainerParameters.mOffset;
>    nsRefPtr<Layer> layer = mRemoteFrame->BuildLayer(aBuilder, mFrame, aManager, visibleRect, this, aContainerParameters);
> +  layer->AsContainerLayer()->SetEventRegionsOverride(mEventRegionsOverride);

RenderFrameParent::BuildLayer has some nullptr returns, so I think we should keep the null check.

Comment 14

[Botond Ballo [:botond]]

Comment on attachment 8564168 [details] [diff] [review]
Part 2 - Add a flag to force an empty hit region

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

::: gfx/layers/Layers.h
@@ +921,5 @@
> +   * as empty. Similarly, if there is a ForceDispatchToContent override then
> +   * the dispatch-to-content region must be treated as encompassing the entire
> +   * hit region, and therefore we must consult the content thread before
> +   * initiating a gesture. (If both flags are set, ForceEmptyHitRegion takes
> +   * priority.)

Given that the flags are not meant to be used in combination, does it make sense to have them bit-flags as opposed to congiguous enum values? I guess future-proofing for a time when we'll have more flags and we might want to use some in combination?

::: layout/ipc/RenderFrameParent.cpp
@@ +625,5 @@
>  {
> +  if (aBuilder->IsBuildingLayerEventRegions()) {
> +    if (aBuilder->IsInsidePointerEventsNoneDoc() ||
> +        aFrame->StyleVisibility()->GetEffectivePointerEvents(aFrame) == NS_STYLE_POINTER_EVENTS_NONE) {
> +      mEventRegionsOverride = (EventRegionsOverride)(mEventRegionsOverride | EventRegionsOverride::ForceEmptyHitRegion);

This is an aside, but operators | and |= can be overloaded for enums:

enum E { ... };

E operator|(E a, E b) {
  return (E)((int)a | (int)b);
}

E& operator|=(E& a, E b) {
  a = a | b;
  return a;
}

Comment 15

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

(In reply to Botond Ballo [:botond] from comment #14)
> Given that the flags are not meant to be used in combination, does it make
> sense to have them bit-flags as opposed to congiguous enum values? I guess
> future-proofing for a time when we'll have more flags and we might want to
> use some in combination?

I originally wrote this using sequential enums but that ended up a little messier. Plus conceptually the two flags are orthogonal and so the bitflags made more sense to me. And yes, future-proofing.. we might need more such flags for other regions that get added.

Comment 16

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

Updated with review comments and pushed to try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd86ca9a6a68

Comment 17

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

Apparently "None" is already taken on Linux (probably #define'd to something). New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff32efc02a13

Comment 18

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #17)
> Apparently "None" is already taken on Linux (probably #define'd to
> something).

By my reading of [1], "enum class", without any macros to wrap it, is fair game for 37, as long as you don't need to forward-declare it (which we don't, we just include LayersTypes.h). Any reason not to use it?

[1] https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code

Comment 19

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

Unfortunately that doesn't actually solve the "None" problem, since None is #define'd to be 0L in some header that gets included into GLContextProviderGLX.cpp.

Comment 20

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

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/901326c8a66c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/34a3bc7200c1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/23c78316747f

Comment 21

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

Also note that this will affect 2.2 now that bug 950934 has been uplifted to 2.2.

Comment 22

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #19)
> Unfortunately that doesn't actually solve the "None" problem, since None is
> #define'd to be 0L in some header that gets included into
> GLContextProviderGLX.cpp.

Ah, true. (Yuck!)

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/901326c8a66c
https://hg.mozilla.org/mozilla-central/rev/34a3bc7200c1
https://hg.mozilla.org/mozilla-central/rev/23c78316747f

Comment 24

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

Comment on attachment 8564162 [details] [diff] [review]
Part 1 - convert the forceDispatchToContent flag to an enum (no functional change)

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 950934
User impact if declined: Setting pointer-events:none on an element in the parent process doesn't carry over to any child process content contained inside it. This manifests as being unable to tap on notification banners at the top of the screen such as the one for an alarm going off.
Testing completed: locally
Risk to taking this patch (and alternatives if risky): the change looks complicated but most of it is refactoring. it is a logical extension of the work done in bug 1125422 which was already uplifted.
String or UUID changes made by this patch: none

Comment 25

[bhavana bajaj [:bajaj]]

Ktucker, can we please get thsi verified on today's 3.0 nightly ? Thanks!

Comment 26

[Pi Wei Cheng [:piwei] (inactive)]

This issue is verified fixed on Flame 3.0 master nightly user build. Tapping on the minimized alarm banner on top correctly brings the alarm screen back to full screen.

Device: Flame 3.0 (full flash, 319MB mem)
BuildID: 20150220010206
Gaia: e4f7c67378e33e83f88d38ddb4a6c2cabf1423c3
Gecko: 1b4c5daa7b7a
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

Leaving verifyme tag for 2.2 uplifting.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/6057d615cd22
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/01959a3f1d51
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/902b712b408b

Comment 28

[Paladin]

I have verified this bug successfully on Flame 2.2

Device Info:
Build ID               20150225162504
Gaia Revision          e4bf968d5a7366e7bdc58f0fdba28b32e864bdf7
Gaia Date              2015-02-25 18:39:43
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/da6ac51f5113
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150225.200419
Firmware Date          Wed Feb 25 20:04:31 EST 2015
Bootloader             L1TC000118D0

Comment 29

[Paladin]

According to comment #26,clear the "Verifyme".