(inactive after 6/18) Alive Kuo [:alive] Assignee
	Description • 11 years ago

Implement AttentionWindow based on AppWindow.

	Etienne Segonzac (:etienne)
	Comment 1 • 11 years ago

When would the transition of the AttentionWindow start?

Currently we have to wait for the wallpaper to be loaded before triggering the call screen transition [1] otherwise we often miss it (the transition) completely.

Not sure how to make this transition perform properly it if lives in the system app, but happy to help!

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/js/call_screen.js#L153-160

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 2 • 11 years ago

(In reply to Etienne Segonzac (:etienne) from comment #1)
> When would the transition of the AttentionWindow start?

I think what we could do is overwrite ready function in attentionWindow.
Now in AppWindowManager of bug 907013, we call it before opening the app:
app.ready(function() {
  app.open();
});
So we could postpone the callback once the mozbrowser is loaded or try firstpaint if we want a quicker latency.

> 
> Currently we have to wait for the wallpaper to be loaded before triggering
> the call screen transition [1] otherwise we often miss it (the transition)
> completely.

Do this for caller only really sounds an anti-pattern. I hope we could do this generally.
Though mozbrowserloadend may be somewhat late than your wallpaper-ready here.

We could put ready function natively in AttentionWindow.prototype.open or have another AttentionWindowManager to control the transition.

	Etienne Segonzac (:etienne)
	Comment 3 • 11 years ago

mozbrowserloadend sounds good.

The call screen is going to need a big rework since a lot of stuff is built around the fact that we choose when the transition begins and we know when it ends...

I hope the fact that the transition happens in the system app will help with some of the performance issues.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 4 • 11 years ago

(In reply to Etienne Segonzac (:etienne) from comment #3)
> mozbrowserloadend sounds good.
> 
> The call screen is going to need a big rework since a lot of stuff is built
> around the fact that we choose when the transition begins and we know when
> it ends...
> 
> I hope the fact that the transition happens in the system app will help with
> some of the performance issues.

Let's try it!!

Could you settle down the dialer part change? 
Rex is interested in, also, I think.

	KM Lee [:rexboy] (inactivate after Sep. 22)
	Comment 5 • 11 years ago

I'm happy to help on this.
Currently the wallpaper loading actually happens when window.onload so I'm not pretty sure
if mozbrowserloadend works well, but we can try discuss and rework for it.
(for example start loading at onDOMContentReady?)

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 6 • 11 years ago

Is it using background-image or an actual image tag?
If it's background image, then mozbrowserloadend doesn't work.
If it's an Image inside the HTML, mozbrowserloadend event would be triggered once the image is loaded and we're free to do the opening transition from system app.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 7 • 11 years ago

Let's file another dialer bug and blocks this.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 8 • 10 years ago

On it right now.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 9 • 10 years ago

WIP

https://github.com/alivedise/gaia/tree/attention-window-v4
* AttentionWindow
* AttentionWindowFactroy
* AttentionWindowManager

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 10 • 10 years ago

The basic open and close transition goes smooth now,
but I found a problem: the resize event is fired to the app too lately. The dialer gets resize event nearly after the opening transition is done. But from system app side the iframe's height is already set to 100%.

Not sure how to fix this yet.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 11 • 10 years ago

Idea now,
attentionopening: show screenshot
attentionopened: remove screenshot
attentionclosing: show screenshot
attentionclosed: remove screenshot

No iframe.setVisible(false) because we cannot.

Don't know how to have clean code here yet. :)

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 12 • 10 years ago

Etienne hints me: resize , wait next paint, and do transition.
But I found next paint is not enough so I will wait until mozbrowserresize is fired after changing the size.

	Luke Chang [:lchang] (inactive)
	Comment 13 • 10 years ago

If AttentionScreen will be deprecated in this bug, the issue in Bug 921800 need to be addressed in AttentionWindowManager as well.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 14 • 10 years ago

WIP today
https://github.com/alivedise/gaia/tree/attention-window-v5

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 15 • 10 years ago

WIP today
https://github.com/alivedise/gaia/commit/3774ec274e404e59f7ebf670215ea8151b0d4b3f

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 16 • 10 years ago

My next item.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 17 • 10 years ago

Status update:
I was caught by lots blockers during the development of this one. I will switch back to this bug soon, but might not for right now.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 19 • 10 years ago

Released from blockers, so back to this one again.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 20 • 10 years ago

1. Rebased from the 6 month ago patch.
2. Not resolving callscreen issues yet.
3. Need to rework visibility manager.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 21 • 10 years ago

Hey, another big one.
I cannot test call here but I will after back to Taiwan.
It's not perfect yet, but let's start the 1st round.

* Deprecate AttentionScreen/attentionscreen* events
* Implement AttentionWindow
* Implement AttentionWindowManager: managing the opened attention windows.
* Implement CallscreenWindow
* DialerAgent plays the role as CallscreenWindow launcher and manager.
* Callscreen app change: always display the UI. I am not confident to change something else here so please advise.

To refine:
* Await the underlying window to repaint before closing the attention window.
* FakeToaster should slide from top when it's made.
* Callscreen test.
* more tests.
* Open attention window in sheet apps.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 22 • 10 years ago

Note:
The fake notification is now staying at utility tray while the attention window is created and until it's killed.
We should make app(attention) itself creates a notification on its own maybe on visibilitychange event or just on load.
After checking with UX of clock app, Juwei, they prefer to close the attention window once home button is ended. So we don't need to open a bug for them I think. What we still need to do is send the attention window to background once the fake notification is removed to let app know it's time to kill itself or create a notification inside utility tray.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 23 • 10 years ago

First trouble is resize.
The fact in order to pretend to be a notification make us need to resize the window when it's closed.
If it's already resized due to orientation or keyboard then it's a pain to do this.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 24 • 10 years ago

Fixed :)

The next one is alarm doesn't not wake the screen anymore. Guess screen_manager problem..

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 25 • 10 years ago

(In reply to Alive Kuo [:alive][NEEDINFO!][Berlin 7/14-7/18] from comment #24)
> Fixed :)
> 
> The next one is alarm doesn't not wake the screen anymore. Guess
> screen_manager problem..

Fixed by gracefully add the attentionopened handler in screen manager.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 26 • 10 years ago

Testing callscreen:
* callschanged is fired 3 times when booted. Very strange but still fine.
* The resized callscreen doesn't work for me. Still debugging.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 27 • 10 years ago

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #26)
> Testing callscreen:
> * callschanged is fired 3 times when booted. Very strange but still fine.
> * The resized callscreen doesn't work for me. Still debugging.

Fixed.
Start looking the failed tests.

Edges gesture > No reflow while swiping:
     
  AssertionError: we got 0 reflows instead of 2

Is this really a bug? :)

	Etienne Segonzac (:etienne)
	Comment 28 • 10 years ago

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #27)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #26)
> > Testing callscreen:
> > * callschanged is fired 3 times when booted. Very strange but still fine.
> > * The resized callscreen doesn't work for me. Still debugging.
> 
> Fixed.
> Start looking the failed tests.
> 
> Edges gesture > No reflow while swiping:
>      
>   AssertionError: we got 0 reflows instead of 2
> 
> Is this really a bug? :)

Definitely!
But I can help :)

	Etienne Segonzac (:etienne)
	Comment 29 • 10 years ago

Comment on attachment 8458050 [details] [review]
Patch rebased, round 1

Did a first good round of review on github, cleaning the feedback flag and waiting for the new version of the patch with what you showed me on IRC this morning :)

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 30 • 10 years ago

Note:
Now whenever screen is turned off by proximity sensor, even the callscreen is closed (folded state) we are reopen the callscreen anyway. I wonder this is a correct behavior.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 31 • 10 years ago

Another problem: callscreen on lockscreen seems strange. It's having #lock though I don't know what's wrong.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 32 • 10 years ago

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #31)
> Another problem: callscreen on lockscreen seems strange. It's having #lock
> though I don't know what's wrong.

The bottom part of callscreen#locked is covered by something makes it impossible to accept the call on lockscreen, what section I should look into?

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 33 • 10 years ago

Potential issues: we are not able to go back to opened attention window on lockscreen if home button is pressed since we cannot use utility tray on lockscreen.
Possible fix#1: Don't slide the fake toaster when system.locked is true.
Possible fix#2: Block home button on lockscreen to prevent attention window closed.

	Etienne Segonzac (:etienne)
	Comment 34 • 10 years ago

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #33)
> Potential issues: we are not able to go back to opened attention window on
> lockscreen if home button is pressed since we cannot use utility tray on
> lockscreen.
> Possible fix#1: Don't slide the fake toaster when system.locked is true.

I like this fix better.
Looking at the callscreen issue now...

	Etienne Segonzac (:etienne)
	Comment 35 • 10 years ago

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #32)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #31)
> > Another problem: callscreen on lockscreen seems strange. It's having #lock
> > though I don't know what's wrong.
> 
> The bottom part of callscreen#locked is covered by something makes it
> impossible to accept the call on lockscreen, what section I should look into?

I think the issue is the lockscreenSlider (in the callscreen app) not being initialized properly, and not being displayed.
You're not actually supposed to tap the buttons, we should have a slider here.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 36 • 10 years ago

(In reply to Etienne Segonzac (:etienne) from comment #35)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #32)
> > (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #31)
> > > Another problem: callscreen on lockscreen seems strange. It's having #lock
> > > though I don't know what's wrong.
> > 
> > The bottom part of callscreen#locked is covered by something makes it
> > impossible to accept the call on lockscreen, what section I should look into?
> 
> I think the issue is the lockscreenSlider (in the callscreen app) not being
> initialized properly, and not being displayed.
> You're not actually supposed to tap the buttons, we should have a slider
> here.

I am certain the call to new LockscreenSlide is invoked. Looking into LockscreenSlide class..

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 37 • 10 years ago

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #36)
> (In reply to Etienne Segonzac (:etienne) from comment #35)
> > (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #32)
> > > (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #31)
> > > > Another problem: callscreen on lockscreen seems strange. It's having #lock
> > > > though I don't know what's wrong.
> > > 
> > > The bottom part of callscreen#locked is covered by something makes it
> > > impossible to accept the call on lockscreen, what section I should look into?
> > 
> > I think the issue is the lockscreenSlider (in the callscreen app) not being
> > initialized properly, and not being displayed.
> > You're not actually supposed to tap the buttons, we should have a slider
> > here.
> 
> I am certain the call to new LockscreenSlide is invoked. Looking into
> LockscreenSlide class..

It's because of |ScreenLayout| throws js error :(
Fixed.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 38 • 10 years ago

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #37)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #36)
> > (In reply to Etienne Segonzac (:etienne) from comment #35)
> > > (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #32)
> > > > (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #31)
> > > > > Another problem: callscreen on lockscreen seems strange. It's having #lock
> > > > > though I don't know what's wrong.
> > > > 
> > > > The bottom part of callscreen#locked is covered by something makes it
> > > > impossible to accept the call on lockscreen, what section I should look into?
> > > 
> > > I think the issue is the lockscreenSlider (in the callscreen app) not being
> > > initialized properly, and not being displayed.
> > > You're not actually supposed to tap the buttons, we should have a slider
> > > here.
> > 
> > I am certain the call to new LockscreenSlide is invoked. Looking into
> > LockscreenSlide class..
> 
> It's because of |ScreenLayout| throws js error :(
> Fixed.

Latest: cannot reject call on lockscreen :)

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 39 • 10 years ago

Comment on attachment 8458050 [details] [review]
Patch rebased, round 1

Round 2:
* UtilityTray is tracking all attention window instances.
* AttentionWindow clip itself when utilityTray opening.
* AttentionWindow will go to background only when there is a new AttentionWindow overlay on the elder one.
* Lots of fix...

	Etienne Segonzac (:etienne)
	Comment 40 • 10 years ago

Comment on attachment 8458050 [details] [review]
Patch rebased, round 1

I think the clipping approach is worth pursuing :)
The UX is going to be so cool (and much more to-spec).

There's quite a few comments on github, once those are addressed I think we should put this patch in the hands of Rob or Francis for a spin.

Some of the details will be much easier to figure out once we agree on a precise spec.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 41 • 10 years ago

Fixed lots unit test issues, but the following are something I am not sure what to do

gaia-unit-tests TEST-UNEXPECTED-FAIL | callscreen/test/unit/call_screen_test.js | call screen toggling once the wallpaper is loaded when a callback is given should listen for transitionend | expected false to be true
gaia-unit-tests TEST-UNEXPECTED-FAIL | callscreen/test/unit/call_screen_test.js | call screen toggling once the wallpaper is loaded when a callback is given once the transition ended "before each" hook

Do you have a chance to take a look? Thanks.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 42 • 10 years ago

(In reply to Etienne Segonzac (:etienne) (PTO -> August 12) from comment #40)
> Comment on attachment 8458050 [details] [review]
> Patch rebased, round 1
> 
> I think the clipping approach is worth pursuing :)
> The UX is going to be so cool (and much more to-spec).
> 
> There's quite a few comments on github, once those are addressed I think we
> should put this patch in the hands of Rob or Francis for a spin.
> 
> Some of the details will be much easier to figure out once we agree on a
> precise spec.

Rob had a spec for me but I am not sure if he publishes that to public.
Rob, let me know if you had a chance to try out this patch. The working snapshot is here:
http://imgur.com/h0Hwo76

Basically the changes are:
* Having a LIVE attention window in the utility tray behind normal notifications - it's because the attention is not 'clear' able by the clear all button. Etienne thinks we should put attention in the same place as notifications.
* I found the notifications and its toaster are not the same size. One is 6rem and one is 5rem. We need to be consistent for attention windows. Etienne prefers use 5rem for attentions.
* The color of the attention indicator is now the same color with collapsed callscreen for all attention windows.
* On lockscreen the collapsed attention window(toaster mode) will not disappear after 5sec. It's because user cannot access utility tray on lockscreen so he need alternative to open the attention window again to full size.

	Etienne Segonzac (:etienne)
	Comment 43 • 10 years ago

I'm leaving on PTO...

This patch is in good shape, it can still change quite a bit based on UX feedback but anyway having Vivien take a thorough look at it will be beneficial.
For the dialer issue, Rik is your man (he might also have comments about the transitions ;)).

Good luck, can't wait to have it on my phone once I'm back from PTO :)

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 44 • 10 years ago

Fixed the callscreen tests. Now in the final tuning round.
The next is baking a attentionToaster sub component for attention window to install as the transition controller for the toaster mode.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 45 • 10 years ago

Comment on attachment 8458050 [details] [review]
Patch rebased, round 1

Now my turn :)
Working on the last missing tests for attention window manager but I guess you'd like the see the other parts.

	Rob MacDonald [:robmac]
	Comment 46 • 10 years ago

Hi Alive...

The Attention Window is included in the notifications spec, located here:

https://mozilla.box.com/s/l863mefh21ni92ypglwu

Also, Eric will be working on some animations for this once he's wrapped up a few other items. I've NI'd him to alert him to the bug.

- Rob

	Rob MacDonald [:robmac]
	Comment 47 • 10 years ago

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #42)

> Basically the changes are:
> * Having a LIVE attention window in the utility tray behind normal
> notifications - it's because the attention is not 'clear' able by the clear
> all button. Etienne thinks we should put attention in the same place as
> notifications.

This should be covered in the spec but let me know if I've missed something.

> * I found the notifications and its toaster are not the same size. One is
> 6rem and one is 5rem. We need to be consistent for attention windows.
> Etienne prefers use 5rem for attentions.

This would be a visual call - Eric is cc'd.

> * The color of the attention indicator is now the same color with collapsed
> callscreen for all attention windows.

I'll defer to Eric on this one as well. :)

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 48 • 10 years ago

Not sure what we could do to move this forward?

	Eric Pang [:epang] UX
	Comment 49 • 10 years ago

 
> > * I found the notifications and its toaster are not the same size. One is
> > 6rem and one is 5rem. We need to be consistent for attention windows.
> > Etienne prefers use 5rem for attentions.
> 
> 5 works for me, I'll make it 5 rem when creating the visual spec.
> 
> > * The color of the attention indicator is now the same color with collapsed
> > callscreen for all attention windows.
> 
> I'm planning to either make the line slightly lighter/darker, will need to explore this when creating the visuals.  But the colour won't be exactly the same as the callscreen. I'll be doing this soon!

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 50 • 10 years ago

Comment on attachment 8458050 [details] [review]
Patch rebased, round 1

Overall this is a very nice clean up and also the proper implementation of something that lives as an exception in our codebase for so long.

I also think it is going in the right direction. I made a comment to see if we can remove callscreen_window.js but I don't feel strong on it and it may be cleaner to keep it. I'm not sure yet.

The main thing I'm not sure about is the live view of attention window in the utility tray. I would like to see if we can get rid of them as it will make a few things funny for the moment. It will raise the priority of multiples processe when you open the utility tray, which may be painful on low end devices, as well as making it hard to keep the transition of the utility tray in sycn with the transition of those live views.

Ideally we would have some kind of permanent notifications, that let us do a UX/UI closer enough to what is wanted and then we will never need to resize attention window anymore.



diff --git a/apps/callscreen/js/call_screen.js b/apps/callscreen/js/call_screen.js
index 9b6d290..debac0d 100644
--- a/apps/callscreen/js/call_screen.js
+++ b/apps/callscreen/js/call_screen.js
@@ -264,7 +264,7 @@ var CallScreen = {
     }
 
     var screen = this.screen;
-    screen.classList.toggle('displayed');
+    // screen.classList.toggle('displayed');



Let's remove it completely.

 
diff --git a/apps/callscreen/style/oncall_status_bar.css b/apps/callscreen/style/oncall_status_bar.css
index 959085c..1298043 100644
--- a/apps/callscreen/style/oncall_status_bar.css
+++ b/apps/callscreen/style/oncall_status_bar.css
@@ -17,12 +17,12 @@
   #group-call {
     background-color: #00adad;
     background-repeat: repeat-x;
-    background-size: auto 4rem;
+    background-size: auto 6rem;
     color: white;
     font-size: 1.7rem;
     padding-left: 1rem;
     padding-right: 2rem;
-    height: 40px;
+    height: 60px;
   }

6rem ?

 
diff --git a/apps/callscreen/test/unit/call_screen_test.js b/apps/callscreen/test/unit/call_screen_test.js
index 5df2d3d..30f5b6d 100755
--- a/apps/callscreen/test/unit/call_screen_test.js
+++ b/apps/callscreen/test/unit/call_screen_test.js
@@ -11,6 +11,7 @@ require('/shared/test/unit/mocks/dialer/mock_handled_call.js');
 require('/shared/test/unit/mocks/dialer/mock_calls_handler.js');
 require('/shared/test/unit/mocks/dialer/mock_font_size_manager.js');
 require('/shared/test/unit/mocks/mock_settings_listener.js');
+require('/shared/js/lockscreen_connection_info_manager.js');
 
 var mocksHelperForCallScreen = new MocksHelper([
   'CallsHandler',
@@ -378,32 +379,14 @@ suite('call screen', function() {
 
   suite('toggling', function() {
     suiteSetup(function() {
-      CallScreen._wallpaperReady = false;
+      CallScreen._wallpaperReady = true;
     });
 
     teardown(function() {
       CallScreen._transitioning = false;
     });
 
-    test('it should wait for the wallpaper to load', function(done) {
-      var toggleSpy = this.sinon.spy(screen.classList, 'toggle');
-      CallScreen.toggle();
-      assert.isTrue(toggleSpy.notCalled);
-      MockSettingsListener.mTriggerCallback('wallpaper.image', new Blob());
-
-      setTimeout(function() {
-        assert.isTrue(toggleSpy.calledOnce);
-        done();
-      });
-    });
-
     suite('once the wallpaper is loaded', function() {
-      test('should toggle the displayed classlist', function() {
-        var toggleSpy = this.sinon.spy(screen.classList, 'toggle');
-        CallScreen.toggle();
-        assert.isTrue(toggleSpy.calledWith('displayed'));
-      });


Why does the wallpaper related code has changed ?

diff --git a/apps/system/js/app_transition_controller.js b/apps/system/js/app_transition_controller.js
index 3e95aaa..8fced19 100644
--- a/apps/system/js/app_transition_controller.js
+++ b/apps/system/js/app_transition_controller.js
@@ -204,7 +204,9 @@
       }
 
       this.resetTransition();
-      this.app.setVisible(false, true);
+      if (this.app.CLASS_NAME !== 'AttentionWindow') {
+        this.app.setVisible(false, true);
+      }
       this.app.element.classList.remove('active');
     };


Why don't we change the visibility of attention window ?

 
@@ -318,7 +321,9 @@
         'invoking', 'invoked', 'zoom-in', 'zoom-out', 'fade-in', 'fade-out',
         'transition-opening', 'transition-closing', 'immediate', 'fadeout',
         'slideleft', 'slideright', 'in-from-left', 'out-to-right',
-        'slideup', 'slidedown', 'will-become-active', 'will-become-inactive'];
+        'slide-to-top', 'slide-to-bottom', 'will-become-active',
+        'will-become-inactive',
+        'slide-from-top', 'slide-from-bottom'];

nit: Maybe we can reorder this list so the slide-to-top, slide-to-bottom, slide-from-top, slide-from-bottom may be grouped together.

 
diff --git a/apps/system/js/app_usage_metrics.js b/apps/system/js/app_usage_metrics.js
index 0370d50..5b78b6a 100644
--- a/apps/system/js/app_usage_metrics.js
+++ b/apps/system/js/app_usage_metrics.js
@@ -82,7 +82,7 @@
   AUM.UsageData = UsageData;
 
   // Set to false to silence debug output
-  AUM.DEBUG = true;
+  AUM.DEBUG = false;
 
   // This logging function is the only thing that is not exposed through
   // the AppUsageMetrics contstructor or its instance.

+1 but I feel like this should be part of a separate bug.


diff --git a/apps/system/js/app_window.js b/apps/system/js/app_window.js
@@ -426,6 +429,12 @@
       this.nextWindow = null;
     }
 
+    // Kill any attention window.
+    if (this.attentionWindow) {
+      this.attentionWindow.kill();
+      this.attentionWindow = null;
+    }
+


hum. Just want to double check. If someone kills the clock app, while there is an alarm clock running, do you really want to kill the related attention screen ?
I guess, someone can not kill the clock directly from the task manager because of 'killable', but if the main window call window.close() then the attention window will be close too ? Is it expected ?

@@ -1025,15 +1015,14 @@
   };
 
   AppWindow.prototype.show = function aw_show() {
-    if (!this.isActive()) {
-      this.element.classList.add('active');
-    }
+    this.element.classList.remove('hidden');
+    this.publish('shown');
   };
 
   AppWindow.prototype.hide = function aw_hide() {
-    if (this.isActive()) {
-      this.element.classList.remove('active');
-    }
+    this.debug('hidden the entire app window.');
+    this.element.classList.add('hidden');
+    this.publish('hidden');
   };


Don't you want to not send the event is the app window is already active/hidden as previously ? It seems a bit weird that the 'shown' events may be fired multiple times even if the app is already shown.

@@ -1477,6 +1466,11 @@
       this.calleeWindow = null;
     };
 
+  AppWindow.prototype.unsetAttentionWindow =
+    function aw_unsetCalleeWindow() {
+      this.attentionWindow = null;
+    };
+

nit: s/unsetCalleeWindow/unsetAttentionWindow

@@ -1945,5 +1941,72 @@
     win.focus();
   };
 
+
+  /**
+   * This lets task manager know if it's killable
+   * to display the close button.
+   * @return {Boolean} The app is killable or not.
+   */

nit: I think the comment should not mention task manager. This comment is all related to some specific comments of the task manager, which may change or not in the future.

+  AppWindow.prototype.killable = function() {
+    // This property is updated whenever an attentionWindow
+    // is created or destroyed.
+    if (this.attentionWindow || this.isHomescreen) {
+      return false;
+    } else {
+      return true;
+    }
+  };

if (this.attentionWindow || this.isHomescreen) {
  return false;
}

return true;

+
+  AppWindow.prototype.hasPermission =
+    function aw_hasPermission(name) {
+      if (typeof(this._hasPermission) !== 'undefined' &&
+          this._hasPermission[name]) {
+        return this._hasPermission[name];
+      }

nit: add a line break.

+      var mozPerms = navigator.mozPermissionSettings;
+      if (!mozPerms || !this.manifestURL) {
+        return false;
+      }

The manifestURL permission check here is a bit surprising. Can't we give some permission to web content ? Like geoloc for example.
Or maybe this is just that the method is intended to check permissions for app only ?


+  /**
+   * _hidewindow event handler.
+   *
+   * The event occurs when there's a higher priority window
+   * which is not an AppWindow show up.
+   * AppWindowManager will redirect the event to the current
+   * active app.
+   *
+   * If the event is because of attention window coming,
+   * evt.detail will be the instance of the attention window.
+   * If we are the opener of the attention window,
+   * we should not be sent to background due to
+   * @param  {Event} evt The hidewindow event
+   */
+  AppWindow.prototype._handle__hidewindow = function (evt) {
+    var attention = evt.detail;
+    if (attention.parentWindow &&
+        attention.parentWindow.instanceID === this.instanceID) {
+      return;
+    }

nit: add a line break

+    this.setVisible(false);
+  };


In some ways this is a bit weird that the 'iframe' can not be set to the background, while the other 'iframe' runs on the foreground. It would be nice to be able to throw away some part of the app rendering, or just run the GC on the background iframe compartment.

Does the issue is that calling setVisible(false) on one <iframe mozapp="foo">, while an other <iframe mozapp="foo"> is running lower the process priority ? If so I wonder if this is not a bug that we can fix in the platform. Does not need to be fixed in this patch, but opening a followup in and referenced the bug number here would be nice.


diff --git a/apps/system/js/app_window_manager.js b/apps/system/js/app_window_manager.js
@@ -435,23 +436,7 @@
           break;
 
         case 'hidewindow':
-          if (activeApp &&
-              activeApp.origin !== homescreenLauncher.origin) {
-            // This is coming from attention screen.
-            // If attention screen has the same origin as our active app,
-            // we cannot turn off its page visibility
-            // because they are sharing the same process and the same docShell,
-            // so turn off page visibility would overwrite the page visibility
-            // of the active attention screen.
-            if (detail && detail.origin &&
-                detail.origin === activeApp.origin) {
-              return;
-            }
-            activeApp.setVisible(false);

Ok this is an old behavior that comes from here. I'm pretty sure that while the 2 iframes share the same process, they don't same the same docShell. But maybe because of window.open one if the parent of the other. We may want to fix that in some ways.

diff --git a/apps/system/js/attention_toaster.js b/apps/system/js/attention_toaster.js
+  var AttentionToaster = function(app) {
+    this.app = app;
+    this.start();
+  };

I thought you wanted 2 states initialization all over the place ?
var toaster = new AttentionToaster(myApp);
toaster.start(); 

Nope ?

+  AttentionToaster.prototype = {
+    _currentToasterState: 'uninit',
+    _previousToasterState: null,
+    TOASTER_TIMEOUT: 5000,
+    DEBUG: false,

nit: maybe you should keep DEBUG at the top of the prototype, so it will never moves, while other properties will.

+    handleEvent: function(evt) {
+      if (this.app.isHidden() || this.app.isActive()) {
+        return;
+      }

nit: add a line break;

Also I'm not sure to understand why do we check for:
if (this.app.isHidden() || this.app.isActive())

Maybe a helper function with a good name will help, especially since you're doing the exact same check into processStateChange.

+      switch (evt.type) {
+        case '_lockscreen-appclosed':
+          if (this._currentToasterState == 'closed') {
+            return;
+          }

nit: add a line break;

+          this.processStateChange('close', evt.type);
+          break;

nit: add a line break;

+        case '_requestopen':
+          this.processStateChange('terminate', evt.type);
+          break;

nit: add a line break;

+        case 'transitionend':
+          if (evt.target !== this.app.element) {
+            return;
+          }

nit: add a line break;

+          this.processStateChange('complete', evt.type);
+          break;

nit: add a line break;

+        case '_closed':
+          this.processStateChange('init', evt.type);
+          break;

nit: add a line break;

+        case '_lockscreen-appopened':
+          this.processStateChange('open', evt.type);
+          break;
+      }
+    },

nit: add a line break and more generally, just add a empty line between methods.

+    processStateChange: function(event, reason) {
+      if (this.app.isHidden() || this.app.isActive()) {
+        return;
+      }

nit: add a line break

+      var eventIndex = toasterEvents.indexOf(event);
+      var nextState =
+        toasterStateTable[this._currentToasterState][eventIndex];
+
+      if (!nextState) {
+        return;
+      }

nit: add a line break and more generally after each return block.

+      this.app.debug(
+        this._currentToasterState + '->' + nextState +
+        ':' + event + ',' + reason);
+      this._previousToasterState = this._currentToasterState;
+      this._currentToasterState = nextState;
+      this.app.element.setAttribute('toaster-transition-state',
+        this._currentToasterState);
+      if (typeof(this['_enter_' + nextState]) == 'function') {
+        this['_enter_' + nextState](event);
+      }
+    },
+    _enter_closing: function() {
+      if (!this.app || !this.app.element || (System && System.locked)) {
+        return;
+      }

Just curious. Why do we need to check for this.app and this.app.element here, and not in the _enter_closed state if _previousToasterState was uninit ?

+      this.app.element.classList.remove('displayed');
+    },
+    _enter_closed: function() {
+      if (this._previousToasterState == 'uninit') {
+        this.becomeToaster();
+        this.app.tryWaitForFullRepaint(function() {
+          this.processStateChange('open', 'repainted');
+        }.bind(this));
+      } else if (this._previousToasterState == 'closing') {
+        this.app && this.app.setVisible(false);
+      }
+    },
+    _enter_opened: function() {
+      this.app && this.app.setVisible(true);
+      this._toasterTimer = window.setTimeout(function() {
+        if (System && System.locked) {
+          return;
+        }
+        this.processStateChange('close', 'timeout');
+      }.bind(this), this.TOASTER_TIMEOUT);
+    },
+    _enter_opening: function() {
+      this.app && this.app.setVisible(true);
+      this.app.element.classList.add('displayed');
+    },
+    _enter_uninit: function() {
+      if (this._toastertimeout) {
+        window.clearTimeout(this._toastertimeout);
+        this._toastertimeout = null;
+      }
+      this.recoverLayout();
+    },
+    recoverLayout: function() {
+      if (this.app && this.app.resized) {
+        this.app.element.classList.remove('displayed');
+        this.app.element.classList.remove('toaster-mode');
+        this.app.resized = false;
+        this.app._resize();
+      }
+    },
+    becomeToaster: function() {
+      if (!this.app || !this.app.element ||
+          this.app.isDead() || this.app.isHidden() || this.app.isActive()) {
+        return;
+      }
+      // Override the resized height anyway
+      this.app.element.style.height = this.app.closedHeight + 'px';
+      this.app.element.classList.add('toaster-mode');
+      this.app.resized = true;
+    }
+  };

Globally I think this is the part of the feature that I don't like particularly. I'm glad that we don't resize window anymore, but I wonder why don't we use a permanent notification, that can be updated by the app using the 'tag' attribute of the notification, and where the color can be set by the attention screen using the <meta name="theme-color" content="green"> meta tag on the attention window.

I feel like keeping multiple attention window around with the visibility set to true will be hard to maintain, and the UX where the minimized attention window are displayed in the notification tray while not moving in sync with it is painful to maintain.

Any reasons to not simplify the code and use a permanent notifications ? (Right we don't have that in the platform for the moment, but this can be fixed).

diff --git a/apps/system/js/attention_window.js b/apps/system/js/attention_window.js
new file mode 100644
index 0000000..3fc211e
--- /dev/null
+++ b/apps/system/js/attention_window.js
@@ -0,0 +1,304 @@
+/* global AppWindow, applications, layoutManager */
+'use strict';
+
+(function(exports) {
+  /**
+   * AttentionWindow is a special opened window with specific
+   * permission: 'attention'. It would show in front of any
+   * existing app windows to get users' attention.
+   *
+   * ##### Flow chart
+   * <a href="http://i.imgur.com/4O1Frs3.png" target="_blank">
+   * <img src="http://i.imgur.com/4O1Frs3.png"></img>
+   * </a>
+   *
+   * @example
+   * var attention = new AttentionWindow({
+   *   url: 'http://gallery.gaiamobile.org:8080/pick.html',
+   *   manifestURL: 'http://gallery.gaiamobile.org:8080/manifest.webapp',

nit: Are we not using app: for everything now ? :)

+  /**
+   * Fired when the attention window is cloing.

nit: s/cloing/closing

+  /**
+   * Fired before the attention window is rendered.
+   * @event AttentionWindow#attentionwillrender
+   */

nit: The comment seems to not be synced with the name of the event. willrender/rendered!

+  AttentionWindow.prototype.view = function attw_view() {
+    this.debug('intance id: ' + this.instanceID);
+    return '<div class="' + this.CLASS_LIST +
+            '" id="' + this.instanceID + '">' +
+            '<div class="titlebar"></div>' +
+            '<div class="browser-container"></div>' +
+            '<div class="screenshot-overlay"></div>' +
+            '</div>';

You may want to have some .chrome as well in order to display the app title.

+  AttentionWindow.prototype.render = function attw_render() {
+    this.publish('willrender');
+    this.containerElement.insertAdjacentHTML('beforeend', this.view());
+    // the iframe is provided already.
+    this.browser = {
+      element: this.config.iframe
+    };
+    this.element =
+      document.getElementById(this.instanceID);

nit: do you really need to wrap this on 2 lines ?

+    this.browserContainer = this.element.querySelector('.browser-container');
+    this.browserContainer.insertBefore(this.browser.element, null);
+    this.frame = this.element;
+    this.iframe = this.browser.element;
+    this.screenshotOverlay = this.element.querySelector('.screenshot-overlay');
+    this.fadeOverlay = this.element.querySelector('.fade-overlay');

No need for this.fadeOverlay ?

+  // XXX: We may need to wait the underlying window,
+  // which may be attention window or app window
+  // to be repainted, but we don't care it here.
+  AttentionWindow.prototype.requestOpen = function() {
+    this.element.classList.remove('fake-notification');
+    this.element.classList.remove('notification-disappearing');
+    // XXX: A hack to reset height.
+    AppWindow.prototype.requestOpen.apply(this);
+  };

If we ends up with permanent notifications, then we may never have to resize the attention screen!

+
+  AttentionWindow.prototype._handle__closed = function() {
+    if (!this.isDead()) {
+    }
+  };

Dead code ? :)

+
+  /**
+   * Make a fake notification node inside Utility tray.
+   * XXX: We should make app to create this notification on their own.
+   * XXX: The problem is app is not able to 'launch attention window'
+   * in the click callback of the notification. The workaround
+   * might be using the same name to open the attention window again.
+   * And in child window factory we need to check if current attention
+   * window has the same name as the event detail to reopen it or kill it.
+   */
+  AttentionWindow.prototype.makeNotification = function() {
+    var manifestURL = this.manifestURL;
+    if (this.notification || !manifestURL) {
+      return;
+    }

nit: add a line break

+    var manifest = applications.getByManifestURL(manifestURL).manifest;
+    this.manifest = this.config.manifest = manifest;
+
+    var iconSrc = manifestURL.replace(
+                    '/manifest.webapp',
+                    manifest.icons[Object.keys(manifest.icons)[0]]
+                  );
+
+    // Let's create the fake notification.
+    var notification = document.createElement('div');
+    notification.id = 'notification-' + this.instanceID;
+    notification.classList.add('fake-notification-toaster');
+
+    var icon = document.createElement('img');
+    icon.src = iconSrc;
+    icon.classList.add('toaster-icon');
+    notification.appendChild(icon);
+
+    var message = document.createElement('div');
+    message.appendChild(document.createTextNode(manifest.name));
+    message.classList.add('toaster-title');
+    notification.appendChild(message);
+
+    var tip = document.createElement('div');
+    var helper = window.navigator.mozL10n.get('attentionScreen-tapToShow');
+    tip.appendChild(document.createTextNode(helper));
+    tip.classList.add('toaster-detail');
+    notification.appendChild(tip);
+
+    var container =
+      document.getElementById('attention-window-notifications-container');
+    container.insertBefore(notification, null);
+
+    // Attach an event listener to the fake notification so the
+    // attention screen is shown when the user tap on it.
+    notification.addEventListener('click',
+                                  function(evt) {
+                                    this._handle_click(evt);
+                                  }.bind(this));
+    this.notification = notification;
+
+    // Hide on creating.
+    this.notification.style.display = 'none';
+  };

I feel a bit lost now. So we have the attention screen toaster, but we also create custom notifications in the utility tray ? I thought we were showing the live view of the app.

Sorry if I'm misunderstood something. I'm pretty new to this feature!

+  AttentionWindow.prototype['_handle__utility-tray-overlayopening'] =

Just curious. Why is it set differently than others ?

+    function() {
+      if (!this.isActive() &&
+          !this.isHidden() &&
+          !this.isDead()) {
+        this.setVisible(true);
+      }
+    };

And here it seems like we turn on the visibility of the live views. So I'm a bit confused.

+
+  AttentionWindow.prototype.setClip = function(offset, clientTop) {
+    if (this.isActive() ||
+        this.isHidden() ||
+        this.isDead()) {
+      return;
+    }
+    this.element.classList.add('in-utility-tray');
+    this.element.style.transform = 'translateY(' + clientTop + 'px)';
+    this.element.style.clip =
+      'rect(0, ' + layoutManager.width + 'px, ' +
+        (offset - clientTop) + 'px, 0)';
+  };
+
+  AttentionWindow.prototype.unsetClip = function() {
+    if (!this.element || this.isActive()) {
+      return;
+    }
+    this.element.classList.remove('in-utility-tray');
+    this.element.style.transform = '';
+    this.element.style.clip = '';
+  };


I would love to not have to do that. This will be hard to maintain the sync between the animation of the utility tray and those ones. I understand that you can't move those <iframe mozapp> in the DOM. 

diff --git a/apps/system/js/attention_window_manager.js b/apps/system/js/attention_window_manager.js
+    getTopMostWindow: function attwm_hasActiveWindow() {
+      return this._topMostWindow;
+    },

nit: s/hasActiveWindow/getTopMostWindow

+    /**
+     * Return current alive attention window instances in a map.
+     * @return {Map} The attention window map
+     */
+    getInstances: function() {
+      return this._instances;
+    },

The comment says {Map}, but it seems like this is an array in |start|

+
+    screen: document.getElementById('screen'),
+
+    get barHeight() {
+      return 40;
+    },

Still needed ?

+
+    start: function attwm_init() {

nit: s/init/start

+    stop: function attwm_init() {

nit: s/init/stop

+    handleEvent: function attwm_handleEvent(evt) {
+      this.debug('handling ' + evt.type);
+      var attention = evt.detail;
+      switch (evt.type) {
+        case 'attentioncreated':
+          this._instances.push(attention);
+          break;
+
+        case 'attentionopened':
+          this._openedInstances.set(attention, attention);
+          break;
+
+        case 'attentionrequestclose':
+          this._openedInstances.delete(attention);
+          if (this._topMostWindow == attention) {

if (this._topMostWindow !== attention) {
  attention.close();
  return;
}

+            var candidate = null;
+            if (this._openedInstances.size === 0) {
+              this._topMostWindow = null;
+              candidate = AppWindowManager.getActiveApp();
+            } else {
+              this._openedInstances.forEach(function(instance) {
+                candidate = instance;
+              });
+              this._topMostWindow = candidate;
+            }

nit: add a line break

+            if (candidate) {
+              candidate.ready(function() {
+                attention.close();
+              });
+            } else {
+              attention.close();
+            }
+          } else {
+            attention.close();
+          }
+          break;
+
+        case 'launchapp':
+        case 'home':
+        case 'holdhome':
+        case 'emergencyalert':
+          this._topMostWindow = null;
+          var nextApp = null;
+          if (evt.type == 'home') {
+            nextApp = homescreenLauncher.getHomescreen();
+          }
+          if (nextApp) {
+            nextApp.ready(this.closeAllAttentionWindows.bind(this));
+          } else {
+            this.closeAllAttentionWindows();
+          }
+          break;

It seems like nextApp is only really set when the event is 'home'. So I wonder if you dont want a specific case for it only, instead of wrapping it with holdhome, launchapp and emergencyalert.

+        case 'system-resize':
+          if (this._topMostWindow) {
+            this._topMostWindow.resize();
+            evt.stopImmediatePropagation();
+          }
+          break;

Any particular reasons to stop the propagation here ?


diff --git a/apps/system/js/callscreen_window.js b/apps/system/js/callscreen_window.js

My first thought when I have seen this js file was to think: "Can we get rid of it and just use attention_window.js for it ?

So I will ask you the same question! Can we ? And if not, what are the exact reasons that makes it so special ?

+++ b/apps/system/js/callscreen_window.js
@@ -0,0 +1,149 @@
+/* global AttentionWindow, System */
+'use strict';
+
+(function(exports) {
+  /**
+   * Fired when the attention window is cloing.

nit: s/cloing/closing

+   * Fired before the attention window is rendered.
+   * @event AttentionWindow#attentionwillrender
+   */

nit: willrender != rendered :)

+  CallscreenWindow.prototype.render = function cw_render() {
+    this.publish('willrender');
+    this.containerElement.insertAdjacentHTML('beforeend', this.view());
+
+    this.element =
+      document.getElementById(this.instanceID);
+    // XXX: Use BrowserFrame
+    var iframe = document.createElement('iframe');
+    iframe.setAttribute('name', 'call_screen');
+    iframe.setAttribute('mozbrowser', 'true');
+    iframe.setAttribute('remote', 'false');
+    iframe.setAttribute('mozapp', this.config.manifestURL);
+    iframe.src = this.config.url;
+    this.browser = {
+      element: iframe
+    };
+    this.browserContainer = this.element.querySelector('.browser-container');
+    this.browserContainer.insertBefore(this.browser.element, null);
+    this.frame = this.element;
+    this.iframe = this.browser.element;
+    this.screenshotOverlay = this.element.querySelector('.screenshot-overlay');
+
+    this._registerEvents();
+    this.installSubComponents();
+    this.publish('rendered');
+  };

I feel like the prebuilt iframe is the main big difference. This and the fact that it is not unloaded all the time and instead the src is resetted unless there is memory-pressure.

But the prebuilt iframe can be passed into the new AttentionWindow() call. Is it something that can be done by dialer_agent.js ?

+
+  CallscreenWindow.prototype.ensure = function() {
+    if (!this.browser || !this.browser.element) {
+      return;
+    }
+    var timestamp = new Date().getTime();
+    var src = this.config.url + '#' +
+              (System.locked ? 'locked' : '');
+    src = src + '&timestamp=' + timestamp;
+    this.browser.element.src = src;
+    this.show();
+  };

I'm pretty sure we have a settings for that and the call screen can use that instead of this hack.
Mostly saying this in order to see if we can get rid of this particular file.

diff --git a/apps/system/js/dialer_agent.js b/apps/system/js/dialer_agent.js
@@ -88,15 +94,22 @@
 
     window.addEventListener('sleep', this);
     window.addEventListener('volumedown', this);
+    window.addEventListener('mozmemorypressure', this.freeCallscreenWindow);
 
-    this._callScreen = this._createCallScreen();
-    var callScreen = this._callScreen;
-    callScreen.src = CSORIGIN + 'index.html';
-    callScreen.dataset.preloaded = true;
-    // We need the iframe in the DOM
-    AttentionScreen.attentionScreen.appendChild(callScreen);
+    this._callscreenWindow = new CallscreenWindow();
+    this._callscreenWindow.hide();
 
-    callScreen.setVisible(false);
+    if (applications && applications.ready) {
+      this.makeFakeNotification();
+    } else {
+      window.addEventListener('applicationready', this.makeFakeNotification);
+    }
+
+    return this;
+  };

It would be cool if we can just do new AttentionWindow from here. But maybe it is OK to have callscreen_window.js. I'm not going to fight for this after all.
I just feel like dialer_agent.js and callscreen_window.js share some behaviors.

diff --git a/apps/system/js/ime_menu.js b/apps/system/js/ime_menu.js
index 0eb9d2f..12f3efb 100644
--- a/apps/system/js/ime_menu.js
+++ b/apps/system/js/ime_menu.js
@@ -15,6 +15,7 @@
     this.oncancel = cancelCb || function() {};
     this.listItems = listItems;
     this.title = title;
+    window.dispatchEvent(new CustomEvent('imemenushow'));
   }


Leftover from an other patch ?

 
diff --git a/apps/system/js/screen_manager.js b/apps/system/js/screen_manager.js
@@ -272,9 +280,6 @@ var ScreenManager = {
         // If the _cpuWakeLock is already set we are in a multiple
         // call setup, the user will be notified by a tone.
         if (this._cpuWakeLock) {
-          // In case of user making an extra call, the attention screen
-          // may be hidden at top so we need to confirm it's shown again.
-          dialerAgent.showCallScreen();

I'm not sure I've see a listener for that elsewhere, so does this behavior has just been removed or is it replicated somewhere else ?
 
diff --git a/apps/system/js/utility_tray.js b/apps/system/js/utility_tray.js
@@ -280,6 +283,17 @@ var UtilityTray = {
     var notificationBottom = Math.max(0, dy - this.grippyHeight);
     this.notifications.style.clip =
       'rect(0, ' + this.screenWidth + 'px, ' + notificationBottom + 'px, 0)';
+
+    // XXX: hardcode statusbar + notficationbar height

nit: s/notfication/notification

+    var offsetTop = StatusBar.height + 30;

Maybe you want to open a followup and add the bug number here ?

+    attentionWindowManager.getInstances().forEach(function(attention) {
+      if (attention.isHidden() ||
+          attention.isDead()) {
+        return;
+      }

nit: add a line break

+      attention.setClip(StatusBar.height + notificationBottom, offsetTop);
+      offsetTop += attention.closedHeight;
+    }, this);

:/

diff --git a/apps/system/style/window_layout.css b/apps/system/style/window_layout.css
index 05b07cf..09bb6e7 100644
--- a/apps/system/style/window_layout.css
+++ b/apps/system/style/window_layout.css
@@ -23,12 +23,6 @@
 | The window position/geometry is usually static, but some System app changes
 | may affect those.
 | The list here try to cover all the possible cases:
-|   * Minimized Attention Screen
-|     When an attention screen is minimized using the 'home' button, it change
-|     from a full window to a 40px wide window covering the top of the screen.
-|     The statusbar is covered, as well as the top rendering part of the
-|     window.
-|

\o/


 /* Fullscreen windows */
 
 #screen:-moz-full-screen-ancestor .appWindow > div,
@@ -249,3 +220,37 @@
   z-index: 0;
 }
 
+/*
+ * Attention window layout.
+ */
+.attentionWindow > div.browser-container > iframe {
+  border: 0;
+}

This is probably something we want for all iframes. Is there any reasons why we need it specifically here ?

diff --git a/apps/system/style/zindex.css b/apps/system/style/zindex.css
index 367197c..a1365b2 100644
--- a/apps/system/style/zindex.css
+++ b/apps/system/style/zindex.css
@@ -72,82 +72,64 @@
   z-index: 2043;
 }
 
-/* Keyboard is shown above task manager */
-#screen.cards-view.task-manager > [data-z-index-level="keyboards"],
-#screen.cards-view.task-manager > [data-z-index-level="software-buttons"] {
-  z-index: 65538;
+#screen > [data-z-index-level="gesture-panel"] {
+  z-index: 4093;
 }

nit: Any reasons for this smaller z-index to lives closer to the top of the file. I thought the z-indexes were more or less ordered.
 
diff --git a/build/csslint/xfail.list b/build/csslint/xfail.list
@@ -55,7 +55,7 @@ apps/music/style/music.css 0 1
 apps/pdfjs/content/web/viewer.css 3 3
 apps/system/emergency-call/style/keypad.css 1 1
 apps/system/emergency-call/style/dialer.css 2 0
-apps/system/style/notifications/notifications.css 0 1
+apps/system/style/notifications/notifications.css 0 0

<3 You can remove this line completely in this case!

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 51 • 10 years ago

I am on the way addressing your comments, but the main concern you raised:
"Why not use notifications?"

It was also one of my option but I think we have some issues:
* If you mean "all attention window should fire a notification when they are closed",
  and there is an app forget to do that, the user will not be able to reopen the attention again.
* If you mean the fake notification you introduced in previous patch, it's made by system app, how could attention window communicate with the notifications to update the information?

BTW UX's opinion is they may not care the live mode is really live if "it's impossible" (from Rob.)

===

For callscreen window I think it's necessary because
* The open/close animation is necessary. Etienne insisted on that.
* The callscreen is preloaded to resolve the performance issue when call comes. We need a launcher to inject the callscreen iframe into system app. Sure we could do all the things in attention window and make dialerAgent to new AttentionWindow with specific arguments. I feel it's a little out of scope.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 52 • 10 years ago

After reading your comments again, I am still not sure what's your acceptable criteria.
Do you want me to play magic to implement the permanent live notification without platform support in this bug?

Also please read rob's spec at https://bugzilla.mozilla.org/show_bug.cgi?id=927862#c46. See page 6. If we are using a normal notification API, the toaster it created will not have the special bottom lighted toaster effect. The issue we will bring if we remove the permanent attention bar is the user won't know there is an attention now. It should have something different than normal notification/toaster.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 53 • 10 years ago

(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #50)
> Comment on attachment 8458050 [details] [review]
> Patch rebased, round 1
> 
> Overall this is a very nice clean up and also the proper implementation of
> something that lives as an exception in our codebase for so long.
> 
> I also think it is going in the right direction. I made a comment to see if
> we can remove callscreen_window.js but I don't feel strong on it and it may
> be cleaner to keep it. I'm not sure yet.

The main reason is UX requirement: for the special slidedown/slideup animation of callscreen.
I am also willing to deprecate the animation. Both iOS/android don't have special animation for callscreen. If UX could sign off: "we don't need animation for callscreen!" I could do that.

> 
> The main thing I'm not sure about is the live view of attention window in
> the utility tray. I would like to see if we can get rid of them as it will
> make a few things funny for the moment. It will raise the priority of
> multiples processe when you open the utility tray, which may be painful on
> low end devices, as well as making it hard to keep the transition of the
> utility tray in sycn with the transition of those live views.

If we totally removed the live view and we should have something like live notification support otherwise the user will lose the information about call time. But we don't have. I am not sure UX will accept this or not.

The main difference to current version is there might be more than one foreground attentions when pulling down attentions. But please note: if there is only one attention, it's "Permanently Foreground" in master now.

I tend to keep the live view but seek for another solution for low end device...

> 
> Ideally we would have some kind of permanent notifications, that let us do a
> UX/UI closer enough to what is wanted and then we will never need to resize
> attention window anymore.
> 

If we have something like live+custom notification we don't need to resize but just need to send them to background when pressing home button; but the problem is we don't have right now...

What if a (developer) settings to turn it on and turn it off later when we have the desired notifications?

> 
> 
> diff --git a/apps/callscreen/js/call_screen.js
> b/apps/callscreen/js/call_screen.js
> index 9b6d290..debac0d 100644
> --- a/apps/callscreen/js/call_screen.js
> +++ b/apps/callscreen/js/call_screen.js
> @@ -264,7 +264,7 @@ var CallScreen = {
>      }
>  
>      var screen = this.screen;
> -    screen.classList.toggle('displayed');
> +    // screen.classList.toggle('displayed');
> 
> 
> 
> Let's remove it completely.

done

> 
>  
> diff --git a/apps/callscreen/style/oncall_status_bar.css
> b/apps/callscreen/style/oncall_status_bar.css
> index 959085c..1298043 100644
> --- a/apps/callscreen/style/oncall_status_bar.css
> +++ b/apps/callscreen/style/oncall_status_bar.css
> @@ -17,12 +17,12 @@
>    #group-call {
>      background-color: #00adad;
>      background-repeat: repeat-x;
> -    background-size: auto 4rem;
> +    background-size: auto 6rem;
>      color: white;
>      font-size: 1.7rem;
>      padding-left: 1rem;
>      padding-right: 2rem;
> -    height: 40px;
> +    height: 60px;
>    }
> 
> 6rem ?

I tried to use 6rem but prefer 5rem...fixing.

> 
>  
> diff --git a/apps/callscreen/test/unit/call_screen_test.js
> b/apps/callscreen/test/unit/call_screen_test.js
> index 5df2d3d..30f5b6d 100755
> --- a/apps/callscreen/test/unit/call_screen_test.js
> +++ b/apps/callscreen/test/unit/call_screen_test.js
> @@ -11,6 +11,7 @@
> require('/shared/test/unit/mocks/dialer/mock_handled_call.js');
>  require('/shared/test/unit/mocks/dialer/mock_calls_handler.js');
>  require('/shared/test/unit/mocks/dialer/mock_font_size_manager.js');
>  require('/shared/test/unit/mocks/mock_settings_listener.js');
> +require('/shared/js/lockscreen_connection_info_manager.js');
>  
>  var mocksHelperForCallScreen = new MocksHelper([
>    'CallsHandler',
> @@ -378,32 +379,14 @@ suite('call screen', function() {
>  
>    suite('toggling', function() {
>      suiteSetup(function() {
> -      CallScreen._wallpaperReady = false;
> +      CallScreen._wallpaperReady = true;
>      });
>  
>      teardown(function() {
>        CallScreen._transitioning = false;
>      });
>  
> -    test('it should wait for the wallpaper to load', function(done) {
> -      var toggleSpy = this.sinon.spy(screen.classList, 'toggle');
> -      CallScreen.toggle();
> -      assert.isTrue(toggleSpy.notCalled);
> -      MockSettingsListener.mTriggerCallback('wallpaper.image', new Blob());
> -
> -      setTimeout(function() {
> -        assert.isTrue(toggleSpy.calledOnce);
> -        done();
> -      });
> -    });
> -
>      suite('once the wallpaper is loaded', function() {
> -      test('should toggle the displayed classlist', function() {
> -        var toggleSpy = this.sinon.spy(screen.classList, 'toggle');
> -        CallScreen.toggle();
> -        assert.isTrue(toggleSpy.calledWith('displayed'));
> -      });
> 
> 
> Why does the wallpaper related code has changed ?

The intention is the callscreen will slide itselves down by toggling displayed class on #screen on callscreen app when wallpaper is ready. But the transition is moved to system app and we are using mozbrowserloadend as the ready signal. There's no reason to keep the test now. And it fails.

> 
> diff --git a/apps/system/js/app_transition_controller.js
> b/apps/system/js/app_transition_controller.js
> index 3e95aaa..8fced19 100644
> --- a/apps/system/js/app_transition_controller.js
> +++ b/apps/system/js/app_transition_controller.js
> @@ -204,7 +204,9 @@
>        }
>  
>        this.resetTransition();
> -      this.app.setVisible(false, true);
> +      if (this.app.CLASS_NAME !== 'AttentionWindow') {
> +        this.app.setVisible(false, true);
> +      }
>        this.app.element.classList.remove('active');
>      };
> 
> 
> Why don't we change the visibility of attention window ?

We are changing in the toaster transition controller!

> 
>  
> @@ -318,7 +321,9 @@
>          'invoking', 'invoked', 'zoom-in', 'zoom-out', 'fade-in', 'fade-out',
>          'transition-opening', 'transition-closing', 'immediate', 'fadeout',
>          'slideleft', 'slideright', 'in-from-left', 'out-to-right',
> -        'slideup', 'slidedown', 'will-become-active',
> 'will-become-inactive'];
> +        'slide-to-top', 'slide-to-bottom', 'will-become-active',
> +        'will-become-inactive',
> +        'slide-from-top', 'slide-from-bottom'];
> 
> nit: Maybe we can reorder this list so the slide-to-top, slide-to-bottom,
> slide-from-top, slide-from-bottom may be grouped together.

done.

> 
>  
> diff --git a/apps/system/js/app_usage_metrics.js
> b/apps/system/js/app_usage_metrics.js
> index 0370d50..5b78b6a 100644
> --- a/apps/system/js/app_usage_metrics.js
> +++ b/apps/system/js/app_usage_metrics.js
> @@ -82,7 +82,7 @@
>    AUM.UsageData = UsageData;
>  
>    // Set to false to silence debug output
> -  AUM.DEBUG = true;
> +  AUM.DEBUG = false;
>  
>    // This logging function is the only thing that is not exposed through
>    // the AppUsageMetrics contstructor or its instance.
> 
> +1 but I feel like this should be part of a separate bug.

removed.

> 
> 
> diff --git a/apps/system/js/app_window.js b/apps/system/js/app_window.js
> @@ -426,6 +429,12 @@
>        this.nextWindow = null;
>      }
>  
> +    // Kill any attention window.
> +    if (this.attentionWindow) {
> +      this.attentionWindow.kill();
> +      this.attentionWindow = null;
> +    }
> +
> 
> 
> hum. Just want to double check. If someone kills the clock app, while there
> is an alarm clock running, do you really want to kill the related attention
> screen ?
> I guess, someone can not kill the clock directly from the task manager
> because of 'killable', but if the main window call window.close() then the
> attention window will be close too ? Is it expected ?

If we allow this there might be a big problem....
the opened window could access the opener window by window.opener,
if we don't clean it then we will have trouble if the opened window keeps touching window.opener by post message.

> 
> @@ -1025,15 +1015,14 @@
>    };
>  
>    AppWindow.prototype.show = function aw_show() {
> -    if (!this.isActive()) {
> -      this.element.classList.add('active');
> -    }
> +    this.element.classList.remove('hidden');
> +    this.publish('shown');
>    };
>  
>    AppWindow.prototype.hide = function aw_hide() {
> -    if (this.isActive()) {
> -      this.element.classList.remove('active');
> -    }
> +    this.debug('hidden the entire app window.');
> +    this.element.classList.add('hidden');
> +    this.publish('hidden');
>    };
> 
> 
> Don't you want to not send the event is the app window is already
> active/hidden as previously ? It seems a bit weird that the 'shown' events
> may be fired multiple times even if the app is already shown.

Ya. you are right!

> 
> @@ -1477,6 +1466,11 @@
>        this.calleeWindow = null;
>      };
>  
> +  AppWindow.prototype.unsetAttentionWindow =
> +    function aw_unsetCalleeWindow() {
> +      this.attentionWindow = null;
> +    };
> +
> 
> nit: s/unsetCalleeWindow/unsetAttentionWindow

done.

> 
> @@ -1945,5 +1941,72 @@
>      win.focus();
>    };
>  
> +
> +  /**
> +   * This lets task manager know if it's killable
> +   * to display the close button.
> +   * @return {Boolean} The app is killable or not.
> +   */
> 
> nit: I think the comment should not mention task manager. This comment is
> all related to some specific comments of the task manager, which may change
> or not in the future.

No better idea than "this window is killable or not"...

> 
> +  AppWindow.prototype.killable = function() {
> +    // This property is updated whenever an attentionWindow
> +    // is created or destroyed.
> +    if (this.attentionWindow || this.isHomescreen) {
> +      return false;
> +    } else {
> +      return true;
> +    }
> +  };
> 
> if (this.attentionWindow || this.isHomescreen) {
>   return false;
> }
> 
> return true;

I don't catch your point here?

> 
> +
> +  AppWindow.prototype.hasPermission =
> +    function aw_hasPermission(name) {
> +      if (typeof(this._hasPermission) !== 'undefined' &&
> +          this._hasPermission[name]) {
> +        return this._hasPermission[name];
> +      }
> 
> nit: add a line break.

done

> 
> +      var mozPerms = navigator.mozPermissionSettings;
> +      if (!mozPerms || !this.manifestURL) {
> +        return false;
> +      }
> 
> The manifestURL permission check here is a bit surprising. Can't we give
> some permission to web content ? Like geoloc for example.
> Or maybe this is just that the method is intended to check permissions for
> app only ?
> 

I think we won't use this method for non webapps page...
I am just borrow it from old attention screen.

The problem here should be: why is we doing permission check but not gecko?

> 
> +  /**
> +   * _hidewindow event handler.
> +   *
> +   * The event occurs when there's a higher priority window
> +   * which is not an AppWindow show up.
> +   * AppWindowManager will redirect the event to the current
> +   * active app.
> +   *
> +   * If the event is because of attention window coming,
> +   * evt.detail will be the instance of the attention window.
> +   * If we are the opener of the attention window,
> +   * we should not be sent to background due to
> +   * @param  {Event} evt The hidewindow event
> +   */
> +  AppWindow.prototype._handle__hidewindow = function (evt) {
> +    var attention = evt.detail;
> +    if (attention.parentWindow &&
> +        attention.parentWindow.instanceID === this.instanceID) {
> +      return;
> +    }
> 
> nit: add a line break
> 
> +    this.setVisible(false);
> +  };
> 
> 
> In some ways this is a bit weird that the 'iframe' can not be set to the
> background, while the other 'iframe' runs on the foreground. It would be
> nice to be able to throw away some part of the app rendering, or just run
> the GC on the background iframe compartment.
> 
> Does the issue is that calling setVisible(false) on one <iframe
> mozapp="foo">, while an other <iframe mozapp="foo"> is running lower the
> process priority ? If so I wonder if this is not a bug that we can fix in
> the platform. Does not need to be fixed in this patch, but opening a
> followup in and referenced the bug number here would be nice.
> 

This is also an old piece of code... I am not sure if we should remove it here.
Worth a try.

> 
> diff --git a/apps/system/js/app_window_manager.js
> b/apps/system/js/app_window_manager.js
> @@ -435,23 +436,7 @@
>            break;
>  
>          case 'hidewindow':
> -          if (activeApp &&
> -              activeApp.origin !== homescreenLauncher.origin) {
> -            // This is coming from attention screen.
> -            // If attention screen has the same origin as our active app,
> -            // we cannot turn off its page visibility
> -            // because they are sharing the same process and the same
> docShell,
> -            // so turn off page visibility would overwrite the page
> visibility
> -            // of the active attention screen.
> -            if (detail && detail.origin &&
> -                detail.origin === activeApp.origin) {
> -              return;
> -            }
> -            activeApp.setVisible(false);
> 
> Ok this is an old behavior that comes from here. I'm pretty sure that while
> the 2 iframes share the same process, they don't same the same docShell. But
> maybe because of window.open one if the parent of the other. We may want to
> fix that in some ways.

I will open follow up for platform guys.

> 
> diff --git a/apps/system/js/attention_toaster.js
> b/apps/system/js/attention_toaster.js
> +  var AttentionToaster = function(app) {
> +    this.app = app;
> +    this.start();
> +  };
> 
> I thought you wanted 2 states initialization all over the place ?
> var toaster = new AttentionToaster(myApp);
> toaster.start(); 
> 
> Nope ?

Yeah, but needs some tweak because all other subcomponents is old fashioned.

> 
> +  AttentionToaster.prototype = {
> +    _currentToasterState: 'uninit',
> +    _previousToasterState: null,
> +    TOASTER_TIMEOUT: 5000,
> +    DEBUG: false,
> 
> nit: maybe you should keep DEBUG at the top of the prototype, so it will
> never moves, while other properties will.

done

> 
> +    handleEvent: function(evt) {
> +      if (this.app.isHidden() || this.app.isActive()) {
> +        return;
> +      }
> 
> nit: add a line break;
> 
> Also I'm not sure to understand why do we check for:
> if (this.app.isHidden() || this.app.isActive())

It's because we don't want to transition the callscreen as a toaster if it's hidden.
Also we don't want to transition the opened attention window as a toaster.

> 
> Maybe a helper function with a good name will help, especially since you're
> doing the exact same check into processStateChange.

Ya, good catch, using |this.isToasterMode()| for now.

> 
> +      switch (evt.type) {
> +        case '_lockscreen-appclosed':
> +          if (this._currentToasterState == 'closed') {
> +            return;
> +          }
> 
> nit: add a line break;
> 
> +          this.processStateChange('close', evt.type);
> +          break;
> 
> nit: add a line break;
> 
> +        case '_requestopen':
> +          this.processStateChange('terminate', evt.type);
> +          break;
> 
> nit: add a line break;
> 
> +        case 'transitionend':
> +          if (evt.target !== this.app.element) {
> +            return;
> +          }
> 
> nit: add a line break;
> 
> +          this.processStateChange('complete', evt.type);
> +          break;
> 
> nit: add a line break;
> 
> +        case '_closed':
> +          this.processStateChange('init', evt.type);
> +          break;
> 
> nit: add a line break;
> 
> +        case '_lockscreen-appopened':
> +          this.processStateChange('open', evt.type);
> +          break;
> +      }
> +    },
> 
> nit: add a line break and more generally, just add a empty line between
> methods.
> 
> +    processStateChange: function(event, reason) {
> +      if (this.app.isHidden() || this.app.isActive()) {
> +        return;
> +      }
> 
> nit: add a line break
> 
> +      var eventIndex = toasterEvents.indexOf(event);
> +      var nextState =
> +        toasterStateTable[this._currentToasterState][eventIndex];
> +
> +      if (!nextState) {
> +        return;
> +      }
> 
> nit: add a line break and more generally after each return block.
> 
> +      this.app.debug(
> +        this._currentToasterState + '->' + nextState +
> +        ':' + event + ',' + reason);
> +      this._previousToasterState = this._currentToasterState;
> +      this._currentToasterState = nextState;
> +      this.app.element.setAttribute('toaster-transition-state',
> +        this._currentToasterState);
> +      if (typeof(this['_enter_' + nextState]) == 'function') {
> +        this['_enter_' + nextState](event);
> +      }
> +    },
> +    _enter_closing: function() {
> +      if (!this.app || !this.app.element || (System && System.locked)) {
> +        return;
> +      }
> 
> Just curious. Why do we need to check for this.app and this.app.element
> here, and not in the _enter_closed state if _previousToasterState was uninit
> ?

Maybe we should always check.

> 
> +      this.app.element.classList.remove('displayed');
> +    },
> +    _enter_closed: function() {
> +      if (this._previousToasterState == 'uninit') {
> +        this.becomeToaster();
> +        this.app.tryWaitForFullRepaint(function() {
> +          this.processStateChange('open', 'repainted');
> +        }.bind(this));
> +      } else if (this._previousToasterState == 'closing') {
> +        this.app && this.app.setVisible(false);
> +      }
> +    },
> +    _enter_opened: function() {
> +      this.app && this.app.setVisible(true);
> +      this._toasterTimer = window.setTimeout(function() {
> +        if (System && System.locked) {
> +          return;
> +        }
> +        this.processStateChange('close', 'timeout');
> +      }.bind(this), this.TOASTER_TIMEOUT);
> +    },
> +    _enter_opening: function() {
> +      this.app && this.app.setVisible(true);
> +      this.app.element.classList.add('displayed');
> +    },
> +    _enter_uninit: function() {
> +      if (this._toastertimeout) {
> +        window.clearTimeout(this._toastertimeout);
> +        this._toastertimeout = null;
> +      }
> +      this.recoverLayout();
> +    },
> +    recoverLayout: function() {
> +      if (this.app && this.app.resized) {
> +        this.app.element.classList.remove('displayed');
> +        this.app.element.classList.remove('toaster-mode');
> +        this.app.resized = false;
> +        this.app._resize();
> +      }
> +    },
> +    becomeToaster: function() {
> +      if (!this.app || !this.app.element ||
> +          this.app.isDead() || this.app.isHidden() || this.app.isActive()) {
> +        return;
> +      }
> +      // Override the resized height anyway
> +      this.app.element.style.height = this.app.closedHeight + 'px';
> +      this.app.element.classList.add('toaster-mode');
> +      this.app.resized = true;
> +    }
> +  };
> 
> Globally I think this is the part of the feature that I don't like
> particularly. I'm glad that we don't resize window anymore, but I wonder why
> don't we use a permanent notification, that can be updated by the app using
> the 'tag' attribute of the notification, and where the color can be set by
> the attention screen using the <meta name="theme-color" content="green">
> meta tag on the attention window.
> 
> I feel like keeping multiple attention window around with the visibility set
> to true will be hard to maintain, and the UX where the minimized attention
> window are displayed in the notification tray while not moving in sync with
> it is painful to maintain.
> 
> Any reasons to not simplify the code and use a permanent notifications ?
> (Right we don't have that in the platform for the moment, but this can be
> fixed).

It's a little out of the scope of this bug.
Note: for master now the attention screen is ALWAYS foreground. This does not regress that.

And the main reason is that I don't know how to have a permanent notification with normal notifications API unless one more hack...

> 
> diff --git a/apps/system/js/attention_window.js
> b/apps/system/js/attention_window.js
> new file mode 100644
> index 0000000..3fc211e
> --- /dev/null
> +++ b/apps/system/js/attention_window.js
> @@ -0,0 +1,304 @@
> +/* global AppWindow, applications, layoutManager */
> +'use strict';
> +
> +(function(exports) {
> +  /**
> +   * AttentionWindow is a special opened window with specific
> +   * permission: 'attention'. It would show in front of any
> +   * existing app windows to get users' attention.
> +   *
> +   * ##### Flow chart
> +   * <a href="http://i.imgur.com/4O1Frs3.png" target="_blank">
> +   * <img src="http://i.imgur.com/4O1Frs3.png"></img>
> +   * </a>
> +   *
> +   * @example
> +   * var attention = new AttentionWindow({
> +   *   url: 'http://gallery.gaiamobile.org:8080/pick.html',
> +   *   manifestURL: 'http://gallery.gaiamobile.org:8080/manifest.webapp',
> 
> nit: Are we not using app: for everything now ? :)

But we are still having http:// while running in nightly?

> 
> +  /**
> +   * Fired when the attention window is cloing.
> 
> nit: s/cloing/closing

fixed.

> 
> +  /**
> +   * Fired before the attention window is rendered.
> +   * @event AttentionWindow#attentionwillrender
> +   */
> 
> nit: The comment seems to not be synced with the name of the event.
> willrender/rendered!

fixed

> 
> +  AttentionWindow.prototype.view = function attw_view() {
> +    this.debug('intance id: ' + this.instanceID);
> +    return '<div class="' + this.CLASS_LIST +
> +            '" id="' + this.instanceID + '">' +
> +            '<div class="titlebar"></div>' +
> +            '<div class="browser-container"></div>' +
> +            '<div class="screenshot-overlay"></div>' +
> +            '</div>';
> 
> You may want to have some .chrome as well in order to display the app title.

This is a UX problem: do we really want to show "app" title for attention window?
attention window is not an app..I will feel confused if I have two "dialer" with different view.

> 
> +  AttentionWindow.prototype.render = function attw_render() {
> +    this.publish('willrender');
> +    this.containerElement.insertAdjacentHTML('beforeend', this.view());
> +    // the iframe is provided already.
> +    this.browser = {
> +      element: this.config.iframe
> +    };
> +    this.element =
> +      document.getElementById(this.instanceID);
> 
> nit: do you really need to wrap this on 2 lines ?

fixed.

> 
> +    this.browserContainer =
> this.element.querySelector('.browser-container');
> +    this.browserContainer.insertBefore(this.browser.element, null);
> +    this.frame = this.element;
> +    this.iframe = this.browser.element;
> +    this.screenshotOverlay =
> this.element.querySelector('.screenshot-overlay');
> +    this.fadeOverlay = this.element.querySelector('.fade-overlay');
> 
> No need for this.fadeOverlay ?

Removed

> 
> +  // XXX: We may need to wait the underlying window,
> +  // which may be attention window or app window
> +  // to be repainted, but we don't care it here.
> +  AttentionWindow.prototype.requestOpen = function() {
> +    this.element.classList.remove('fake-notification');
> +    this.element.classList.remove('notification-disappearing');
> +    // XXX: A hack to reset height.
> +    AppWindow.prototype.requestOpen.apply(this);
> +  };
> 
> If we ends up with permanent notifications, then we may never have to resize
> the attention screen!

Again this is to match the spec to make the toaster special.
I know resize is evil and I already removed one of the resize in this patch.
It's really difficult to figure out a nonhacking way to implement to the spec here..

> 
> +
> +  AttentionWindow.prototype._handle__closed = function() {
> +    if (!this.isDead()) {
> +    }
> +  };
> 
> Dead code ? :)

removed

> 
> +
> +  /**
> +   * Make a fake notification node inside Utility tray.
> +   * XXX: We should make app to create this notification on their own.
> +   * XXX: The problem is app is not able to 'launch attention window'
> +   * in the click callback of the notification. The workaround
> +   * might be using the same name to open the attention window again.
> +   * And in child window factory we need to check if current attention
> +   * window has the same name as the event detail to reopen it or kill it.
> +   */
> +  AttentionWindow.prototype.makeNotification = function() {
> +    var manifestURL = this.manifestURL;
> +    if (this.notification || !manifestURL) {
> +      return;
> +    }
> 
> nit: add a line break
> 
> +    var manifest = applications.getByManifestURL(manifestURL).manifest;
> +    this.manifest = this.config.manifest = manifest;
> +
> +    var iconSrc = manifestURL.replace(
> +                    '/manifest.webapp',
> +                    manifest.icons[Object.keys(manifest.icons)[0]]
> +                  );
> +
> +    // Let's create the fake notification.
> +    var notification = document.createElement('div');
> +    notification.id = 'notification-' + this.instanceID;
> +    notification.classList.add('fake-notification-toaster');
> +
> +    var icon = document.createElement('img');
> +    icon.src = iconSrc;
> +    icon.classList.add('toaster-icon');
> +    notification.appendChild(icon);
> +
> +    var message = document.createElement('div');
> +    message.appendChild(document.createTextNode(manifest.name));
> +    message.classList.add('toaster-title');
> +    notification.appendChild(message);
> +
> +    var tip = document.createElement('div');
> +    var helper = window.navigator.mozL10n.get('attentionScreen-tapToShow');
> +    tip.appendChild(document.createTextNode(helper));
> +    tip.classList.add('toaster-detail');
> +    notification.appendChild(tip);
> +
> +    var container =
> +      document.getElementById('attention-window-notifications-container');
> +    container.insertBefore(notification, null);
> +
> +    // Attach an event listener to the fake notification so the
> +    // attention screen is shown when the user tap on it.
> +    notification.addEventListener('click',
> +                                  function(evt) {
> +                                    this._handle_click(evt);
> +                                  }.bind(this));
> +    this.notification = notification;
> +
> +    // Hide on creating.
> +    this.notification.style.display = 'none';
> +  };
> 
> I feel a bit lost now. So we have the attention screen toaster, but we also
> create custom notifications in the utility tray ? I thought we were showing
> the live view of the app.
> 
> Sorry if I'm misunderstood something. I'm pretty new to this feature!

The placeoholder is:
When attention window is fully opened, the user should still be albe to pull down the utility tray.
But I feel it is strange if we have an attention but has no placeholder inside the utility tray while the closed attention window has their live view there. (I know you don't like live.)

> 
> +  AttentionWindow.prototype['_handle__utility-tray-overlayopening'] =
> 
> Just curious. Why is it set differently than others ?
> 
> +    function() {
> +      if (!this.isActive() &&
> +          !this.isHidden() &&
> +          !this.isDead()) {
> +        this.setVisible(true);
> +      }
> +    };
> 
> And here it seems like we turn on the visibility of the live views. So I'm a
> bit confused.
> 
> +
> +  AttentionWindow.prototype.setClip = function(offset, clientTop) {
> +    if (this.isActive() ||
> +        this.isHidden() ||
> +        this.isDead()) {
> +      return;
> +    }
> +    this.element.classList.add('in-utility-tray');
> +    this.element.style.transform = 'translateY(' + clientTop + 'px)';
> +    this.element.style.clip =
> +      'rect(0, ' + layoutManager.width + 'px, ' +
> +        (offset - clientTop) + 'px, 0)';
> +  };
> +
> +  AttentionWindow.prototype.unsetClip = function() {
> +    if (!this.element || this.isActive()) {
> +      return;
> +    }
> +    this.element.classList.remove('in-utility-tray');
> +    this.element.style.transform = '';
> +    this.element.style.clip = '';
> +  };
> 
> 
> I would love to not have to do that. This will be hard to maintain the sync
> between the animation of the utility tray and those ones. I understand that
> you can't move those <iframe mozapp> in the DOM. 
> 
> diff --git a/apps/system/js/attention_window_manager.js
> b/apps/system/js/attention_window_manager.js
> +    getTopMostWindow: function attwm_hasActiveWindow() {
> +      return this._topMostWindow;
> +    },
> 
> nit: s/hasActiveWindow/getTopMostWindow
> 
> +    /**
> +     * Return current alive attention window instances in a map.
> +     * @return {Map} The attention window map
> +     */
> +    getInstances: function() {
> +      return this._instances;
> +    },
> 
> The comment says {Map}, but it seems like this is an array in |start|
> 
> +
> +    screen: document.getElementById('screen'),
> +
> +    get barHeight() {
> +      return 40;
> +    },
> 
> Still needed ?
> 
> +
> +    start: function attwm_init() {
> 
> nit: s/init/start
> 
> +    stop: function attwm_init() {
> 
> nit: s/init/stop
> 
> +    handleEvent: function attwm_handleEvent(evt) {
> +      this.debug('handling ' + evt.type);
> +      var attention = evt.detail;
> +      switch (evt.type) {
> +        case 'attentioncreated':
> +          this._instances.push(attention);
> +          break;
> +
> +        case 'attentionopened':
> +          this._openedInstances.set(attention, attention);
> +          break;
> +
> +        case 'attentionrequestclose':
> +          this._openedInstances.delete(attention);
> +          if (this._topMostWindow == attention) {
> 
> if (this._topMostWindow !== attention) {
>   attention.close();
>   return;
> }
> 
> +            var candidate = null;
> +            if (this._openedInstances.size === 0) {
> +              this._topMostWindow = null;
> +              candidate = AppWindowManager.getActiveApp();
> +            } else {
> +              this._openedInstances.forEach(function(instance) {
> +                candidate = instance;
> +              });
> +              this._topMostWindow = candidate;
> +            }
> 
> nit: add a line break
> 
> +            if (candidate) {
> +              candidate.ready(function() {
> +                attention.close();
> +              });
> +            } else {
> +              attention.close();
> +            }
> +          } else {
> +            attention.close();
> +          }
> +          break;
> +
> +        case 'launchapp':
> +        case 'home':
> +        case 'holdhome':
> +        case 'emergencyalert':
> +          this._topMostWindow = null;
> +          var nextApp = null;
> +          if (evt.type == 'home') {
> +            nextApp = homescreenLauncher.getHomescreen();
> +          }
> +          if (nextApp) {
> +            nextApp.ready(this.closeAllAttentionWindows.bind(this));
> +          } else {
> +            this.closeAllAttentionWindows();
> +          }
> +          break;
> 
> It seems like nextApp is only really set when the event is 'home'. So I
> wonder if you dont want a specific case for it only, instead of wrapping it
> with holdhome, launchapp and emergencyalert.

done.

> 
> +        case 'system-resize':
> +          if (this._topMostWindow) {
> +            this._topMostWindow.resize();
> +            evt.stopImmediatePropagation();
> +          }
> +          break;
> 
> Any particular reasons to stop the propagation here ?

If keyboard opens when attention window is opened, why do we need to resize the opened app window hich is managed by AppWindowManager with system-resize event handler?

> 
> 
> diff --git a/apps/system/js/callscreen_window.js
> b/apps/system/js/callscreen_window.js
> 
> My first thought when I have seen this js file was to think: "Can we get rid
> of it and just use attention_window.js for it ?
> 
> So I will ask you the same question! Can we ? And if not, what are the exact
> reasons that makes it so special ?

As I said, we could. If you insist on doing that in this patch I could do.

> 
> +++ b/apps/system/js/callscreen_window.js
> @@ -0,0 +1,149 @@
> +/* global AttentionWindow, System */
> +'use strict';
> +
> +(function(exports) {
> +  /**
> +   * Fired when the attention window is cloing.
> 
> nit: s/cloing/closing
> 
> +   * Fired before the attention window is rendered.
> +   * @event AttentionWindow#attentionwillrender
> +   */
> 
> nit: willrender != rendered :)
> 
> +  CallscreenWindow.prototype.render = function cw_render() {
> +    this.publish('willrender');
> +    this.containerElement.insertAdjacentHTML('beforeend', this.view());
> +
> +    this.element =
> +      document.getElementById(this.instanceID);
> +    // XXX: Use BrowserFrame
> +    var iframe = document.createElement('iframe');
> +    iframe.setAttribute('name', 'call_screen');
> +    iframe.setAttribute('mozbrowser', 'true');
> +    iframe.setAttribute('remote', 'false');
> +    iframe.setAttribute('mozapp', this.config.manifestURL);
> +    iframe.src = this.config.url;
> +    this.browser = {
> +      element: iframe
> +    };
> +    this.browserContainer =
> this.element.querySelector('.browser-container');
> +    this.browserContainer.insertBefore(this.browser.element, null);
> +    this.frame = this.element;
> +    this.iframe = this.browser.element;
> +    this.screenshotOverlay =
> this.element.querySelector('.screenshot-overlay');
> +
> +    this._registerEvents();
> +    this.installSubComponents();
> +    this.publish('rendered');
> +  };
> 
> I feel like the prebuilt iframe is the main big difference. This and the
> fact that it is not unloaded all the time and instead the src is resetted
> unless there is memory-pressure.
> 
> But the prebuilt iframe can be passed into the new AttentionWindow() call.
> Is it something that can be done by dialer_agent.js ?

The reason is described below.

> 
> +
> +  CallscreenWindow.prototype.ensure = function() {
> +    if (!this.browser || !this.browser.element) {
> +      return;
> +    }
> +    var timestamp = new Date().getTime();
> +    var src = this.config.url + '#' +
> +              (System.locked ? 'locked' : '');
> +    src = src + '&timestamp=' + timestamp;
> +    this.browser.element.src = src;
> +    this.show();
> +  };
> 
> I'm pretty sure we have a settings for that and the call screen can use that
> instead of this hack.
> Mostly saying this in order to see if we can get rid of this particular file.

I am new to "callscreen app" and don't want to change it too much in this patch.
This was introduced by Etienne though. We could have followup.

> 
> diff --git a/apps/system/js/dialer_agent.js b/apps/system/js/dialer_agent.js
> @@ -88,15 +94,22 @@
>  
>      window.addEventListener('sleep', this);
>      window.addEventListener('volumedown', this);
> +    window.addEventListener('mozmemorypressure', this.freeCallscreenWindow);
>  
> -    this._callScreen = this._createCallScreen();
> -    var callScreen = this._callScreen;
> -    callScreen.src = CSORIGIN + 'index.html';
> -    callScreen.dataset.preloaded = true;
> -    // We need the iframe in the DOM
> -    AttentionScreen.attentionScreen.appendChild(callScreen);
> +    this._callscreenWindow = new CallscreenWindow();
> +    this._callscreenWindow.hide();
>  
> -    callScreen.setVisible(false);
> +    if (applications && applications.ready) {
> +      this.makeFakeNotification();
> +    } else {
> +      window.addEventListener('applicationready',
> this.makeFakeNotification);
> +    }
> +
> +    return this;
> +  };
> 
> It would be cool if we can just do new AttentionWindow from here. But maybe
> it is OK to have callscreen_window.js. I'm not going to fight for this after
> all.
> I just feel like dialer_agent.js and callscreen_window.js share some
> behaviors.

dialer_agent plays the rule as callscreen_window_launcher or callscreen_window_manager.
The reason I don't put the iframe creation code in launcher is because I think iframe should be already created in AppWindow except window.open case. But in this case it's not actual window.open()...

> 
> diff --git a/apps/system/js/ime_menu.js b/apps/system/js/ime_menu.js
> index 0eb9d2f..12f3efb 100644
> --- a/apps/system/js/ime_menu.js
> +++ b/apps/system/js/ime_menu.js
> @@ -15,6 +15,7 @@
>      this.oncancel = cancelCb || function() {};
>      this.listItems = listItems;
>      this.title = title;
> +    window.dispatchEvent(new CustomEvent('imemenushow'));

removed.

>    }
> 
> 
> Leftover from an other patch ?
> 
>  
> diff --git a/apps/system/js/screen_manager.js
> b/apps/system/js/screen_manager.js
> @@ -272,9 +280,6 @@ var ScreenManager = {
>          // If the _cpuWakeLock is already set we are in a multiple
>          // call setup, the user will be notified by a tone.
>          if (this._cpuWakeLock) {
> -          // In case of user making an extra call, the attention screen
> -          // may be hidden at top so we need to confirm it's shown again.
> -          dialerAgent.showCallScreen();
> 
> I'm not sure I've see a listener for that elsewhere, so does this behavior
> has just been removed or is it replicated somewhere else ?

IMO things happened here is already done in dialer_agent.js
They all listen to callschanged event to open callscreen window.
And it is very strange screen manager does that.

>  
> diff --git a/apps/system/js/utility_tray.js b/apps/system/js/utility_tray.js
> @@ -280,6 +283,17 @@ var UtilityTray = {
>      var notificationBottom = Math.max(0, dy - this.grippyHeight);
>      this.notifications.style.clip =
>        'rect(0, ' + this.screenWidth + 'px, ' + notificationBottom + 'px,
> 0)';
> +
> +    // XXX: hardcode statusbar + notficationbar height
> 
> nit: s/notfication/notification
> 
> +    var offsetTop = StatusBar.height + 30;
> 
> Maybe you want to open a followup and add the bug number here ?
> 
> +    attentionWindowManager.getInstances().forEach(function(attention) {
> +      if (attention.isHidden() ||
> +          attention.isDead()) {
> +        return;
> +      }
> 
> nit: add a line break
> 
> +      attention.setClip(StatusBar.height + notificationBottom, offsetTop);
> +      offsetTop += attention.closedHeight;
> +    }, this);
> 
> :/
> 
> diff --git a/apps/system/style/window_layout.css
> b/apps/system/style/window_layout.css
> index 05b07cf..09bb6e7 100644
> --- a/apps/system/style/window_layout.css
> +++ b/apps/system/style/window_layout.css
> @@ -23,12 +23,6 @@
>  | The window position/geometry is usually static, but some System app
> changes
>  | may affect those.
>  | The list here try to cover all the possible cases:
> -|   * Minimized Attention Screen
> -|     When an attention screen is minimized using the 'home' button, it
> change
> -|     from a full window to a 40px wide window covering the top of the
> screen.
> -|     The statusbar is covered, as well as the top rendering part of the
> -|     window.
> -|
> 
> \o/
> 
> 
>  /* Fullscreen windows */
>  
>  #screen:-moz-full-screen-ancestor .appWindow > div,
> @@ -249,3 +220,37 @@
>    z-index: 0;
>  }
>  
> +/*
> + * Attention window layout.
> + */
> +.attentionWindow > div.browser-container > iframe {
> +  border: 0;
> +}
> 
> This is probably something we want for all iframes. Is there any reasons why
> we need it specifically here ?

I don't know why the border of the iframe under attention window is not 0. So I added a rule to remove the border.

> 
> diff --git a/apps/system/style/zindex.css b/apps/system/style/zindex.css
> index 367197c..a1365b2 100644
> --- a/apps/system/style/zindex.css
> +++ b/apps/system/style/zindex.css
> @@ -72,82 +72,64 @@
>    z-index: 2043;
>  }
>  
> -/* Keyboard is shown above task manager */
> -#screen.cards-view.task-manager > [data-z-index-level="keyboards"],
> -#screen.cards-view.task-manager > [data-z-index-level="software-buttons"] {
> -  z-index: 65538;
> +#screen > [data-z-index-level="gesture-panel"] {
> +  z-index: 4093;
>  }
> 
> nit: Any reasons for this smaller z-index to lives closer to the top of the
> file. I thought the z-indexes were more or less ordered.
>  
> diff --git a/build/csslint/xfail.list b/build/csslint/xfail.list
> @@ -55,7 +55,7 @@ apps/music/style/music.css 0 1
>  apps/pdfjs/content/web/viewer.css 3 3
>  apps/system/emergency-call/style/keypad.css 1 1
>  apps/system/emergency-call/style/dialer.css 2 0
> -apps/system/style/notifications/notifications.css 0 1
> +apps/system/style/notifications/notifications.css 0 0
> 
> <3 You can remove this line completely in this case!

done

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 54 • 10 years ago

(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #50)
>  
> diff --git a/apps/system/js/screen_manager.js
> b/apps/system/js/screen_manager.js
> @@ -272,9 +280,6 @@ var ScreenManager = {
>          // If the _cpuWakeLock is already set we are in a multiple
>          // call setup, the user will be notified by a tone.
>          if (this._cpuWakeLock) {
> -          // In case of user making an extra call, the attention screen
> -          // may be hidden at top so we need to confirm it's shown again.
> -          dialerAgent.showCallScreen();
> 
> I'm not sure I've see a listener for that elsewhere, so does this behavior
> has just been removed or is it replicated somewhere else ?

Rex is the one adding this. Let's seek help from him if we could remove it.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 55 • 10 years ago

(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #50)
> 
> +  AttentionWindow.prototype.view = function attw_view() {
> +    this.debug('intance id: ' + this.instanceID);
> +    return '<div class="' + this.CLASS_LIST +
> +            '" id="' + this.instanceID + '">' +
> +            '<div class="titlebar"></div>' +
> +            '<div class="browser-container"></div>' +
> +            '<div class="screenshot-overlay"></div>' +
> +            '</div>';
> 
> You may want to have some .chrome as well in order to display the app title.
> 

I found that I don't have statusbar background now.. and I dislike the way we do it.
It seems the title-bar is not title bar but statusbar-overlay. What's the reason to keep it?

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 56 • 10 years ago

And another problem: since user is able to invoke rocketbar from the title area,
we have z-index issues between attention window and search window.

In normal case attention window is important than search window, no idea how we fix this case..

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 57 • 10 years ago

Just want to let you know we have issues between attention window & rocketbar, see comment 56.
I feel I cannot fix all of them in this bug.

Q1 Is the user able to accept call in rocketbar overlay and search result?
Q2 Is the user able to open rocketbar overlay and search result when callscreen is opened?
Q3 What is expected if Q1 is yes and the user press home button? Still see rocketbar overlay and search result?
Q4 What is expected if Q2 is yes and the user press close button of rocketbar overlay?
Q5 What is expected if Q2 is yes and when rocketbar overlay opens, will the callscreen become toaster or keep opened but go to background?

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 58 • 10 years ago

Also if we allow the user to invoke rocketbar on attention window that mean rocketbar overlay and search result is able to show on lockscreen. User will click the search result but nothing happens because it's locked.

I am going to disable rocketbar in this patch when device is locked.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 59 • 10 years ago

I made another branch to address your main concern.
* Clipping is removed.
* Toaster mode is STILL there, but in the long run we send the attention window to background. We won't have multiple attention window foreground.
* Fix the app titlebar part of the attention window and disable rocketbar in lockscreen
* When device is unlocked and the user clicks the title part of attention window, all of them are closed.
* CallscreenWindow is STILL there.

I feel removing clipping is just your best wish in my patch. All of others are beyond the scopes of this bug.

I also feel again rocketbar is "No Man's Land" in current system app's hierarchy. And the statusbar overlay makes me sad.

Not sure if we have a consensus here. I hope so.

	KM Lee [:rexboy] (inactivate after Sep. 22)
	Comment 60 • 10 years ago

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #54)
> (In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails,
> needinfo? please) from comment #50)
> >  
> > diff --git a/apps/system/js/screen_manager.js
> > b/apps/system/js/screen_manager.js
> > @@ -272,9 +280,6 @@ var ScreenManager = {
> >          // If the _cpuWakeLock is already set we are in a multiple
> >          // call setup, the user will be notified by a tone.
> >          if (this._cpuWakeLock) {
> > -          // In case of user making an extra call, the attention screen
> > -          // may be hidden at top so we need to confirm it's shown again.
> > -          dialerAgent.showCallScreen();
> > 
> > I'm not sure I've see a listener for that elsewhere, so does this behavior
> > has just been removed or is it replicated somewhere else ?
> 
> Rex is the one adding this. Let's seek help from him if we could remove it.

This line exists for the following use case:
1. User is having one connected call
2. User press home key to shrink the call window at top. The call is still connected.
3. User got another incoming call (So we have two calls)
Expected:
* The call window drop down again to let user decide whether to accept
  or hang up the second call.

I just tried the patch and seems erasing these line would cause the flow
above fail. (Seems the call screen just completely hided at step 2, is that intentional?)
So I guess we may need to keep it, or refactor to somewhere reasonable.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 61 • 10 years ago

I believe I fixed the second call by removing the (calls.length == 1) check in dailer_agent.js

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 62 • 10 years ago

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #51)
> I am on the way addressing your comments, but the main concern you raised:
> "Why not use notifications?"
> 
> It was also one of my option but I think we have some issues:
> * If you mean "all attention window should fire a notification when they are
> closed",
>   and there is an app forget to do that, the user will not be able to reopen
> the attention again.
> * If you mean the fake notification you introduced in previous patch, it's
> made by system app, how could attention window communicate with the
> notifications to update the information?

What about that:
 - By default, we use the fake notification mechanism.
 - If the app wants to override the notification in order to display custom information, it can trigger a permanent notification, which will replace the fake one.

 - The attention window can set the background-color of the notification banner by using the <meta name="theme-color">
 - The attention window can then customize the display of the notification, using the |tag| notification attributes.

What do you think ?

> ===
> 
> For callscreen window I think it's necessary because
> * The open/close animation is necessary. Etienne insisted on that.
> * The callscreen is preloaded to resolve the performance issue when call
> comes. We need a launcher to inject the callscreen iframe into system app.
> Sure we could do all the things in attention window and make dialerAgent to
> new AttentionWindow with specific arguments. I feel it's a little out of
> scope.

For this one I can be easily convinced. So let's go for it.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 63 • 10 years ago

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #52)
> After reading your comments again, I am still not sure what's your
> acceptable criteria.
> Do you want me to play magic to implement the permanent live notification
> without platform support in this bug?
> 

One think which can be done for now is to make all notifications from attention window, permanent's one, or use a special 'tag' name, which is used to keep the notification when the user click on it.

And then we need a followup on the platform side to implement such notifications. I don't think this is technically hard, but this is a matter of spec.
Jonas seems interested in notifications those days. Let's CC'ed him and get his opinion.

> Also please read rob's spec at
> https://bugzilla.mozilla.org/show_bug.cgi?id=927862#c46. See page 6. If we
> are using a normal notification API, the toaster it created will not have
> the special bottom lighted toaster effect. The issue we will bring if we
> remove the permanent attention bar is the user won't know there is an
> attention now. It should have something different than normal
> notification/toaster.

Until there is a platform solution which flags such notifications as appropriate, can you determine that the notification is coming from an attention screen ? Or do you only have the manifestURL/originURL and then we will need the iframe.dispachEvent hack ?

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 64 • 10 years ago

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #53)  
> > Ideally we would have some kind of permanent notifications, that let us do a
> > UX/UI closer enough to what is wanted and then we will never need to resize
> > attention window anymore.
> > 
> 
> If we have something like live+custom notification we don't need to resize
> but just need to send them to background when pressing home button; but the
> problem is we don't have right now...
> 
> What if a (developer) settings to turn it on and turn it off later when we
> have the desired notifications?
> 

I'm pretty sure live view will ask some works, which is not much bigger than implementing permanent notifications in the platform (for our platform friends). Let's wait for Jonas input.

> > diff --git a/apps/callscreen/test/unit/call_screen_test.js
> >      suite('once the wallpaper is loaded', function() {
> > -      test('should toggle the displayed classlist', function() {
> > -        var toggleSpy = this.sinon.spy(screen.classList, 'toggle');
> > -        CallScreen.toggle();
> > -        assert.isTrue(toggleSpy.calledWith('displayed'));
> > -      });
> > 
> > 
> > Why does the wallpaper related code has changed ?
> 
> The intention is the callscreen will slide itselves down by toggling
> displayed class on #screen on callscreen app when wallpaper is ready. But
> the transition is moved to system app and we are using mozbrowserloadend as
> the ready signal. There's no reason to keep the test now. And it fails.

Does it mean that we may need a followup to clean this _wallpaperReady code ?

> > 
> > diff --git a/apps/system/js/app_window.js b/apps/system/js/app_window.js
> > @@ -426,6 +429,12 @@
> >        this.nextWindow = null;
> >      }
> >  
> > +    // Kill any attention window.
> > +    if (this.attentionWindow) {
> > +      this.attentionWindow.kill();
> > +      this.attentionWindow = null;
> > +    }
> > +
> > 
> > 
> > hum. Just want to double check. If someone kills the clock app, while there
> > is an alarm clock running, do you really want to kill the related attention
> > screen ?
> > I guess, someone can not kill the clock directly from the task manager
> > because of 'killable', but if the main window call window.close() then the
> > attention window will be close too ? Is it expected ?
> 
> If we allow this there might be a big problem....
> the opened window could access the opener window by window.opener,
> if we don't clean it then we will have trouble if the opened window keeps
> touching window.opener by post message.
> 

hum. Makes sense. I wish there is a better API for attention window. Let's CC'ed Ehsan this time :)


> > +
> > +  /**
> > +   * This lets task manager know if it's killable
> > +   * to display the close button.
> > +   * @return {Boolean} The app is killable or not.
> > +   */
> > 
> > nit: I think the comment should not mention task manager. This comment is
> > all related to some specific comments of the task manager, which may change
> > or not in the future.
> 
> No better idea than "this window is killable or not"...

hm. Maybe: "This returns a boolean indicating if the current window can be killed by user interaction" ?

> 
> > 
> > +  AppWindow.prototype.killable = function() {
> > +    // This property is updated whenever an attentionWindow
> > +    // is created or destroyed.
> > +    if (this.attentionWindow || this.isHomescreen) {
> > +      return false;
> > +    } else {
> > +      return true;
> > +    }
> > +  };
> > 
> > if (this.attentionWindow || this.isHomescreen) {
> >   return false;
> > }
> > 
> > return true;
> 
> I don't catch your point here?
> 

Mostly that |else| that only does a return are better when the return is not inside the |else|. That comes from one of our old guideline for Firefox Desktop.

> > 
> > +      var mozPerms = navigator.mozPermissionSettings;
> > +      if (!mozPerms || !this.manifestURL) {
> > +        return false;
> > +      }
> > 
> > The manifestURL permission check here is a bit surprising. Can't we give
> > some permission to web content ? Like geoloc for example.
> > Or maybe this is just that the method is intended to check permissions for
> > app only ?
> > 
> 
> I think we won't use this method for non webapps page...
> I am just borrow it from old attention screen.
> 
> The problem here should be: why is we doing permission check but not gecko?
>

Yeah, you're right. I feel like this is because the attention window is not properly spec'ed for now. Let's see if discussing with Ehsan about a better API may help here. So feel free to ignore this comment for now. 
 


> > diff --git a/apps/system/js/attention_window.js
> > +   *
> > +   * @example
> > +   * var attention = new AttentionWindow({
> > +   *   url: 'http://gallery.gaiamobile.org:8080/pick.html',
> > +   *   manifestURL: 'http://gallery.gaiamobile.org:8080/manifest.webapp',
> > 
> > nit: Are we not using app: for everything now ? :)
> 
> But we are still having http:// while running in nightly?
>

Oh do we ? I thought we moved everything to app:// :(
 

> > +  AttentionWindow.prototype.view = function attw_view() {
> > +    this.debug('intance id: ' + this.instanceID);
> > +    return '<div class="' + this.CLASS_LIST +
> > +            '" id="' + this.instanceID + '">' +
> > +            '<div class="titlebar"></div>' +
> > +            '<div class="browser-container"></div>' +
> > +            '<div class="screenshot-overlay"></div>' +
> > +            '</div>';
> > 
> > You may want to have some .chrome as well in order to display the app title.
> 
> This is a UX problem: do we really want to show "app" title for attention
> window?
> attention window is not an app..I will feel confused if I have two "dialer"
> with different view.
> 

Sounds reasonable. Let's ignore it then and we will add it back if UX wants it.

> 
> > 
> > +  // XXX: We may need to wait the underlying window,
> > +  // which may be attention window or app window
> > +  // to be repainted, but we don't care it here.
> > +  AttentionWindow.prototype.requestOpen = function() {
> > +    this.element.classList.remove('fake-notification');
> > +    this.element.classList.remove('notification-disappearing');
> > +    // XXX: A hack to reset height.
> > +    AppWindow.prototype.requestOpen.apply(this);
> > +  };
> > 
> > If we ends up with permanent notifications, then we may never have to resize
> > the attention screen!
> 
> Again this is to match the spec to make the toaster special.
> I know resize is evil and I already removed one of the resize in this patch.
> It's really difficult to figure out a nonhacking way to implement to the
> spec here..
>

I agree with you :) Let's use our platform team for giving us all the things that are needed!
 
> > 
> > +        case 'system-resize':
> > +          if (this._topMostWindow) {
> > +            this._topMostWindow.resize();
> > +            evt.stopImmediatePropagation();
> > +          }
> > +          break;
> > 
> > Any particular reasons to stop the propagation here ?
> 
> If keyboard opens when attention window is opened, why do we need to resize
> the opened app window hich is managed by AppWindowManager with system-resize
> event handler?
> 

Makes sense. Maybe it worth adding such a comment there ?

> > diff --git a/apps/system/js/callscreen_window.js
> > b/apps/system/js/callscreen_window.js
> > 
> > My first thought when I have seen this js file was to think: "Can we get rid
> > of it and just use attention_window.js for it ?
> > 
> > So I will ask you the same question! Can we ? And if not, what are the exact
> > reasons that makes it so special ?
> 
> As I said, we could. If you insist on doing that in this patch I could do.
>

I don't! :)
 
> > +  CallscreenWindow.prototype.ensure = function() {
> > +    if (!this.browser || !this.browser.element) {
> > +      return;
> > +    }
> > +    var timestamp = new Date().getTime();
> > +    var src = this.config.url + '#' +
> > +              (System.locked ? 'locked' : '');
> > +    src = src + '&timestamp=' + timestamp;
> > +    this.browser.element.src = src;
> > +    this.show();
> > +  };
> > 
> > I'm pretty sure we have a settings for that and the call screen can use that
> > instead of this hack.
> > Mostly saying this in order to see if we can get rid of this particular file.
> 
> I am new to "callscreen app" and don't want to change it too much in this
> patch.
> This was introduced by Etienne though. We could have followup.
>

Makes sense. This patch is big enough already and this is unrelated to this feature.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 65 • 10 years ago

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #55)
> (In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails,
> needinfo? please) from comment #50)
> > 
> > +  AttentionWindow.prototype.view = function attw_view() {
> > +    this.debug('intance id: ' + this.instanceID);
> > +    return '<div class="' + this.CLASS_LIST +
> > +            '" id="' + this.instanceID + '">' +
> > +            '<div class="titlebar"></div>' +
> > +            '<div class="browser-container"></div>' +
> > +            '<div class="screenshot-overlay"></div>' +
> > +            '</div>';
> > 
> > You may want to have some .chrome as well in order to display the app title.
> > 
> 
> I found that I don't have statusbar background now.. and I dislike the way
> we do it.
> It seems the title-bar is not title bar but statusbar-overlay. What's the
> reason to keep it?

Yep, you're right it should probably be renamed.

We may be able to get rid of it with some efforts and move .titlebar back under the .chrome DOM. But this is a bit tricky. For example the chrome is lazy loaded, but you want to have statusbar icon as soon as you start the app...

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 66 • 10 years ago

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #57)
> Just want to let you know we have issues between attention window &
> rocketbar, see comment 56.
> I feel I cannot fix all of them in this bug.
> 
> Q1 Is the user able to accept call in rocketbar overlay and search result?

I think so.

> Q2 Is the user able to open rocketbar overlay and search result when
> callscreen is opened?

I tend to think no. But in some other bugs Jason raise that as a blocker...

> Q3 What is expected if Q1 is yes and the user press home button? Still see
> rocketbar overlay and search result?
> Q4 What is expected if Q2 is yes and the user press close button of
> rocketbar overlay?
> Q5 What is expected if Q2 is yes and when rocketbar overlay opens, will the
> callscreen become toaster or keep opened but go to background?

UX questions.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 67 • 10 years ago

Ehsan, this is a bit out of scope for this particular bug. But I think you mentioned already that the way we open attention window is unpleasant.

I think this is a good time to raise such a discussion with Alive (maybe in a separate bug).

	(no longer active)
	Comment 68 • 10 years ago

(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #67)
> Ehsan, this is a bit out of scope for this particular bug. But I think you
> mentioned already that the way we open attention window is unpleasant.
> 
> I think this is a good time to raise such a discussion with Alive (maybe in
> a separate bug).

Can you please refresh my memory here?  How do we open attention windows?  I tried reading the "v.vivien.happy" patch but it's huge and I'm kind of lost as to what it's doing...

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 69 • 10 years ago

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #68)
> (In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails,
> needinfo? please) from comment #67)
> > Ehsan, this is a bit out of scope for this particular bug. But I think you
> > mentioned already that the way we open attention window is unpleasant.
> > 
> > I think this is a good time to raise such a discussion with Alive (maybe in
> > a separate bug).
> 
> Can you please refresh my memory here?  How do we open attention windows?  I
> tried reading the "v.vivien.happy" patch but it's huge and I'm kind of lost
> as to what it's doing...

Sure. This is basically a |window.open(url, '_attention');| on the app side.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 70 • 10 years ago

(In reply to (PTO) Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #62)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #51)
> > I am on the way addressing your comments, but the main concern you raised:
> > "Why not use notifications?"
> > 
> > It was also one of my option but I think we have some issues:
> > * If you mean "all attention window should fire a notification when they are
> > closed",
> >   and there is an app forget to do that, the user will not be able to reopen
> > the attention again.
> > * If you mean the fake notification you introduced in previous patch, it's
> > made by system app, how could attention window communicate with the
> > notifications to update the information?
> 
> What about that:
>  - By default, we use the fake notification mechanism.
>  - If the app wants to override the notification in order to display custom
> information, it can trigger a permanent notification, which will replace the
> fake one.
> 
>  - The attention window can set the background-color of the notification
> banner by using the <meta name="theme-color">
>  - The attention window can then customize the display of the notification,
> using the |tag| notification attributes.
> 
> What do you think ?
> 

I will say ok but not feeling good if you want me to do it in this patch.
As you said we have difficulty to identify who is firing the notifications. Again it's "a little" out of scope of this bug.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 71 • 10 years ago

(In reply to (PTO) Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #66)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #57)
> > Just want to let you know we have issues between attention window &
> > rocketbar, see comment 56.
> > I feel I cannot fix all of them in this bug.
> > 
> > Q1 Is the user able to accept call in rocketbar overlay and search result?
> 
> I think so.
> 
> > Q2 Is the user able to open rocketbar overlay and search result when
> > callscreen is opened?
> 
> I tend to think no. But in some other bugs Jason raise that as a blocker...

What I am doing now: close(not really terminate) the attention windows once the user clicks the title area.
We still could access each other when both attention window and rocketbar are active.

	(no longer active)
	Comment 72 • 10 years ago

(In reply to (PTO) Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #69)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #68)
> > (In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails,
> > needinfo? please) from comment #67)
> > > Ehsan, this is a bit out of scope for this particular bug. But I think you
> > > mentioned already that the way we open attention window is unpleasant.
> > > 
> > > I think this is a good time to raise such a discussion with Alive (maybe in
> > > a separate bug).
> > 
> > Can you please refresh my memory here?  How do we open attention windows?  I
> > tried reading the "v.vivien.happy" patch but it's huge and I'm kind of lost
> > as to what it's doing...
> 
> Sure. This is basically a |window.open(url, '_attention');| on the app side.

Hmm, I'm not sure if this means that _attention is a special target name we're inventing or a special feature.  If the former, that's a bad idea.  If the latter, maybe less bad.  :-)  I'm not sure how similar these windows are to the other window types that window.open can open.  For example, can you close an attention screen with calling close() on the returned value?

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 73 • 10 years ago

Unfortunately Vivien disappears and what's more important, he dislike the clipping windows.
Could you follow the review here?

	Etienne Segonzac (:etienne)
	Comment 74 • 10 years ago

Comment on attachment 8469197 [details]
AttentionWindow v.vivien.happy

I'm up to date now.
Don't have an opinion yet on most of the open issues but I'll work on this :)

	Etienne Segonzac (:etienne)
	Comment 75 • 10 years ago

Comment on attachment 8469197 [details]
AttentionWindow v.vivien.happy

Given the potential for scope creep and the FL date not so far away I think we should move forward with the "vivien happy" patch and land a first stable patch ASAP.

Here's my suggestion:
* rebase the patch
* change the toaster height back to 40px
* hide |.chrome .controls .urlbar .title| for attention windows (or maybe just for the callscreen window, we want to keep the "illusion" that it's only changing the bottom part of the screen when it's displayed in locked mode)

Then we'll ask Rob for a ui-review, explaining the trade offs we have to make for the notification approach (not displaying the caller name / call duration when the toaster is hidden).

Once we have Rob's go, I'll do a quick last review round and we'll file follow ups for the Dialer cleanups (I can take it) and investigating the clipping approach.

What do you think?

	Stephany Wilkes
	Comment 76 • 10 years ago

Rob is on PTO until Monday. Eric, are you able to take Rob's ni? in the meantime?

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 77 • 10 years ago

This is the best we have at this timing.

	Eric Pang [:epang] UX
	Comment 78 • 10 years ago

Hi Alive, I've tried to answer the question to the best of my understanding.
I've also flagged NI on Rob since he's back from PTO tomorrow and will be able to give definate answers.

Also, added the ixd and visD spec links to the user story in this bug.

> Q1 Is the user able to accept call in rocketbar overlay and search result?
Yes - The call screen should take priority 

> Q2 Is the user able to open rocketbar overlay and search result when
> callscreen is opened?
Yes

> Q3 What is expected if Q1 is yes and the user press home button? Still see
> rocketbar overlay and search result?
Is this from the incoming call screen?  If so, i think a homescreen button tap should  bring you to the homescreen and the active call toast shown.

> Q4 What is expected if Q2 is yes and the user press close button of
> rocketbar overlay?
When the close buton is pressed the user should return to the call screen.

> Q5 What is expected if Q2 is yes and when rocketbar overlay opens, will the
> callscreen become toaster or keep opened but go to background?
In this situation the call screen should remain in the background.

	Etienne Segonzac (:etienne)
	Comment 79 • 10 years ago

Hey Carrie, the spec was made by the SystemFE team but since this changes the callscreen UX I'd like to get your ui-review too :)

	Etienne Segonzac (:etienne)
	Comment 80 • 10 years ago

Comment on attachment 8473514 [details] [review]
attention window final

A few comments on github, mainly testing related.
I'm tempted to put the r+ already, but since the UX review is taking some time and this is a big patch I'll take the opportunity to have one last look once those comments are addressed.

Really love the transition BTW :)

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 81 • 10 years ago

(In reply to Etienne Segonzac (:etienne) from comment #80)
> Comment on attachment 8473514 [details] [review]
> attention window final
> 
> A few comments on github, mainly testing related.
> I'm tempted to put the r+ already, but since the UX review is taking some
> time and this is a big patch I'll take the opportunity to have one last look
> once those comments are addressed.
> 
> Really love the transition BTW :)

BTW, comments addressed.
Additional change:
* I add the settings hack to notify camera to stop recording back (in attentionWindowManager). We have no way now until the setVisible API change and I don't want to cause regression.
* dialerAgent is not closing callscreen on callschanged event handler if call count is 0. Because callscreen has already called window.close().

	Stephany Wilkes
	Comment 82 • 10 years ago

Comment on attachment 8473514 [details] [review]
attention window final

This ui-review? should go to the same people who have ni? set. Assigned.

	Carrie Wang [New account: carrie75521@gmail.com]
	Comment 83 • 10 years ago

Comment on attachment 8473514 [details] [review]
attention window final

The interaction part is correct (according to the spec), but I'm not sure if the indicator is obvious enough. I'd leave this question to Rob. :)
Thanks!

	Rob MacDonald [:robmac]
	Comment 84 • 10 years ago

Comment on attachment 8473514 [details] [review]
attention window final

Hi Alive...

The functionality is in place for the most part, however, there are a number areas where it deviates from the visual spec. Although Eric would typically comment on visuals, he's on PTO until Tuesday so I'll flag him and do my best to comment here.

Here's my list:

- The ambient indicator is missing a "pulsing" animation that is posted to the bug. Without the animation, the indicator lacks visibility and may not be noticed by users.

- When using edge gestures, the ambient indicator should move with the toolbar and not be visible in the gap between sheets.

- In the implementation the attention window collapses, the notification expands, then the notification collapses. It's a bit of an awkward transition. Is it possible to have the call screen collapse to reveal the expanded notification then have the notification collapse? That way we eliminate that in between state.

- In the utility tray, the call screen should be highlighted as per the visual spec.

- Also in the utility tray, is it possible to show the text as per the spec? This includes the following "Active call" followed by the name or number of the other party.

- I'm presuming edge gestures cannot be triggered from the attention window. It would be great if they could but this has not been defined as a requirement and I suspect this isn't trivial.

- In terms of the visual specs, there are a number of discrepancies so my suggestion is to review the visual and motion specs attached to this bug against the implementation. I'm assuming a number of these issues can be resolved post feature landing. If necessary we can review these with Eric upon his return on Tuesday (GMT).

Thanks, Alive and please NI me with any questions or concerns.

- Rob

	Etienne Segonzac (:etienne)
	Comment 85 • 10 years ago

(In reply to Rob MacDonald [:robmac] from comment #84)
> - In the implementation the attention window collapses, the notification
> expands, then the notification collapses. It's a bit of an awkward
> transition. Is it possible to have the call screen collapse to reveal the
> expanded notification then have the notification collapse? That way we
> eliminate that in between state.
> 

It's possible but we'd get no transition at all. One frame the attention screen is fully open, the next one you see the "toaster".
So we're doing a magic trick where we put the attention screen outside of view while we do the ugly resize without any transition, then slide it back into view as a toaster :)

> - Also in the utility tray, is it possible to show the text as per the spec?
> This includes the following "Active call" followed by the name or number of
> the other party.

This is the main compromise we're making with the notification approach. Alive had an implementation where we could do this but it's too risky for 2.1 so we'd like to land this first simpler version first.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 86 • 10 years ago

(In reply to Rob MacDonald [:robmac] from comment #84)
> Comment on attachment 8473514 [details] [review]
> attention window final
> 
> Hi Alive...
> 
> The functionality is in place for the most part, however, there are a number
> areas where it deviates from the visual spec. Although Eric would typically
> comment on visuals, he's on PTO until Tuesday so I'll flag him and do my
> best to comment here.
> 
> Here's my list:
> 
> - The ambient indicator is missing a "pulsing" animation that is posted to
> the bug. Without the animation, the indicator lacks visibility and may not
> be noticed by users.

The pulsing animation needs some visual love from Eric but I don't see the assets..
Or you would provide me some? I am not sure what it would be, a gif?

> 
> - When using edge gestures, the ambient indicator should move with the
> toolbar and not be visible in the gap between sheets.

Do-able.

> 
> - In the implementation the attention window collapses, the notification
> expands, then the notification collapses. It's a bit of an awkward
> transition. Is it possible to have the call screen collapse to reveal the
> expanded notification then have the notification collapse? That way we
> eliminate that in between state.

As etienne said, we had no way right now. It's impossible to have two animations on the same element at the same time.

> 
> - In the utility tray, the call screen should be highlighted as per the
> visual spec.

do-able.

> 
> - Also in the utility tray, is it possible to show the text as per the spec?
> This includes the following "Active call" followed by the name or number of
> the other party.

Unable as etienne said.

> 
> - I'm presuming edge gestures cannot be triggered from the attention window.
> It would be great if they could but this has not been defined as a
> requirement and I suspect this isn't trivial.

What's the desired behavior to swipe an attention? I think we need more detail before we do. Please do in followup.

> 
> - In terms of the visual specs, there are a number of discrepancies so my
> suggestion is to review the visual and motion specs attached to this bug
> against the implementation. I'm assuming a number of these issues can be
> resolved post feature landing. If necessary we can review these with Eric
> upon his return on Tuesday (GMT).

As above, some of those could be fixed but some not.

> 
> Thanks, Alive and please NI me with any questions or concerns.
> 
> - Rob

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 87 • 10 years ago

Well I finally notice the spec is in user story area..looking at the interesting handle part.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 88 • 10 years ago

I am working on attention indicator right now, but I think we have difficult to move it with edge swipe.
* If we put it under #statusbar + #statusbar-minimized + #statusbar-maximized, current statusbar in app implementation is very strange... The minimized version of statusbar shadow is only half the width of the screen so you will see the indicator is half size.
* If we put it under #screen, it cannot move with swipe navigation.

I hope this is not a hard requirement in this bug. The statusbar change happens during this bug and we difinitely don't have time to fix the statusbar for this bug.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 89 • 10 years ago

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #88)
> I am working on attention indicator right now, but I think we have difficult
> to move it with edge swipe.
> * If we put it under #statusbar + #statusbar-minimized +
> #statusbar-maximized, current statusbar in app implementation is very
> strange... The minimized version of statusbar shadow is only half the width
> of the screen so you will see the indicator is half size.
> * If we put it under #screen, it cannot move with swipe navigation.
> 
> I hope this is not a hard requirement in this bug. The statusbar change
> happens during this bug and we difinitely don't have time to fix the
> statusbar for this bug.

Also for fullscreen app there is no statusbar by default. Do you want to show indicator in fullscreen app?

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 90 • 10 years ago

Comment on attachment 8473514 [details] [review]
attention window final

I added one more commit to implement attentionIndicator. Need feedback at first.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 91 • 10 years ago

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #90)
> Comment on attachment 8473514 [details] [review]
> attention window final
> 
> I added one more commit to implement attentionIndicator. Need feedback at
> first.

It seems sometimes attention indicator is not hidden when call ends. Will check soon, but should not block feedback.

	Rob MacDonald [:robmac]
	Comment 92 • 10 years ago

Hi Alive and Etienne...

Here are my responses to the series of questions in the last few comments.

Comment 90 (Alive)
Question - "Also for fullscreen app there is no statusbar by default. Do you want to show indicator in fullscreen app?"
Response - Yes, I would prefer that it would be shown in all cases, even if no status bar is present.

Comment 88 (Alive)
Question - (paraphrased) Is moving the ambient indicator with the sheet a hard requirement?
Response - It is preferred but not a hard requirement. If we implement an ambient indicator for notifications, though, the behaviour should be the same.

Comment 86 (Alive)
will respond below

Comment 85 (Etienne)
Comment - "It's possible but we'd get no transition at all."
Response - Yes, that sounds bad. Let's stick with what we have.

Comment - (Regarding the strings used in the notification) "This is the main compromise we're making with the notification approach. Alive had an implementation where we could do this but it's too risky for 2.1 so we'd like to land this first simpler version first."
Response - Ok. Should we track this in a separate bug?

	Rob MacDonald [:robmac]
	Comment 93 • 10 years ago

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #86)

(Removing items addressed in comment 92)

> > - The ambient indicator is missing a "pulsing" animation that is posted to
> > the bug. Without the animation, the indicator lacks visibility and may not
> > be noticed by users.
> 
> The pulsing animation needs some visual love from Eric but I don't see the
> assets..
> Or you would provide me some? I am not sure what it would be, a gif?

Typically these animations are done in code. Michael Henretty did something previously with a version of an ambient notification indicator. I'll flag him on this for comment.

> > - I'm presuming edge gestures cannot be triggered from the attention window.
> > It would be great if they could but this has not been defined as a
> > requirement and I suspect this isn't trivial.
> 
> What's the desired behavior to swipe an attention? I think we need more
> detail before we do. Please do in followup.

Now that I think about it, I would envision different behaviours depending on the type of attention. For an alarm, for example, I don't think you should be able to task away at all. Whereas for a phone call, I see it more as part of the dialer and something you could task away from. I think this requires more thought and more discussion which means we should leave it as is for 2.1. 

> > 
> > - In terms of the visual specs, there are a number of discrepancies so my
> > suggestion is to review the visual and motion specs attached to this bug
> > against the implementation. I'm assuming a number of these issues can be
> > resolved post feature landing. If necessary we can review these with Eric
> > upon his return on Tuesday (GMT).
> 
> As above, some of those could be fixed but some not.
> 

Any sense of which ones are feasible? Then we could discuss with Eric on Tuesday when he returns. I'll also flag him on the bug.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 94 • 10 years ago

(In reply to Rob MacDonald [:robmac] from comment #92)
> Hi Alive and Etienne...
> 
> Here are my responses to the series of questions in the last few comments.
> 
> Comment 90 (Alive)
> Question - "Also for fullscreen app there is no statusbar by default. Do you
> want to show indicator in fullscreen app?"
> Response - Yes, I would prefer that it would be shown in all cases, even if
> no status bar is present.
> 

This is a new request out of the spec and I prefer we have another bug.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 95 • 10 years ago

For UX/UI, what is changed in the patch right now:
* Whenever home button is pressed, attention window will perform closing animation and after that, a toaster like stuff bumps to represent some information from the attention window.
* The toaster will sustain 5 secs and slide up.
* When toaster slides ended, there might be an attention indicator on top of statusbar.
* The indicator will only be hidden when there is no closed attention window.
* The indicator will stay in app's statusbar and move with swipe navigation as requested.
* The indicator will NOT show in fullscreen app (conflict with statusbar design)
* The indicator has NO cut edge for now
* When attention window shows, you could click the left top corner to invoke rocketbar. Then all the attention window will be closed because attention window is on top of rocketbar window.
* When user opens rocketbar and a new attention comes, it will overlay on top of rocketbar.
* The user will NOT be able to swipe when attention window is opened.
* When the user pulls down the utility tray, the attention indicator will show with the attention notification.
* The context of the attention notification is fixed now. Always show app icon + 'tap to open' + app name.
* The background of attention notification is up to spec as semitransparent.
* The attention notification is always top of other notifications.
* When the attention window is closed, it will not occupy the height of current displayed app window.
* When the attention window is closed in lockscreen, it will not become indicator but will stay at toaster mode because we cannot pull down utility tray to check in lockscreen.
* When the lockscreen is unlocked and there is a attention toaster it will slide up and reveal the indicator.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 96 • 10 years ago

I had a working version now and a list above, please check and test it and tell me which part you are not able to accept definitely. Please note I don't think we have too much time to do big change before end of this week.

	Etienne Segonzac (:etienne)
	Comment 97 • 10 years ago

Comment on attachment 8473514 [details] [review]
attention window final

There was probably a misunderstanding on IRC, I thought we we're going with the simpler attention indicator, ie. one for the whole system that would not swipe with the sheets (but would easily be displayed on top of fullscreen apps, BTW).

This is adding a bunch of code last minute to a pretty huge PR and I'm really worried about the performance implications. If you turn on "flash repainted area" you'll see that for every frame of the animation the whole attention indicator of *each* app window is repainted. During an edge gesture that's 3 zones a few pixels high but as wide as the screen being repainted for every frame.

The animation itself is not an issue as you can see when we open the utility tray and display the "root" attention indicator. The issue is the moz-element on animated content.

Sorry :/

Since Rob confirmed that moving the indicator with the sheets is not a hard requirement we should go with the easier solution and just toggle a class on a #attention-indicator.

	Etienne Segonzac (:etienne)
	Comment 98 • 10 years ago

There is another extremely weird issue, that I can only reproduce with this patch but doesn't look related.
It might be a deeper gecko issue revealed by this change.

Here's the recap from the tests we did with Vivien hopefully it'll help.

STR:
- call your voicemail
- minimize the callscreen
- don't do anything else, let the phone alone

at some point your voicemail will hang up on you, a bit later the screen will turn off.

Now you're phone is in a really weird state where pressing the power button will *not* wake it up.

We've reproduced it many time, and managed to keep the remote debugger connected when this happens:
- from the system app console we added listeners to mozChromeEvent and got nothing when pressing the hardware buttons. the system app isn't receiving the mozChromeEvents
- if manually we call |ScreenManager.turnScreenOn()| the screen turns on correctly but still no mozChromeEvents and no hardware button response (including the home button that should vibrate)
- if we tap somewhere inside the screen (focusing it?) everything comes back to normal.

It _looks_ like a gecko side focus issue but it's very worrying since the only way out when this happens is to remove the battery (for somebody without adb).

Fabrice, anything rings a bell?

	Michael Henretty [:mikehenrty][:mhenretty]
	Comment 99 • 10 years ago

(In reply to Rob MacDonald [:robmac] from comment #93)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #86)
> 
> (Removing items addressed in comment 92)
> 
> > > - The ambient indicator is missing a "pulsing" animation that is posted to
> > > the bug. Without the animation, the indicator lacks visibility and may not
> > > be noticed by users.
> > 
> > The pulsing animation needs some visual love from Eric but I don't see the
> > assets..
> > Or you would provide me some? I am not sure what it would be, a gif?
> 
> Typically these animations are done in code. Michael Henretty did something
> previously with a version of an ambient notification indicator. I'll flag
> him on this for comment.

I used keyframes for a demo of the ambient notifications indicator. Although, I don't know much about the spec for the attention window, so I can't comment about how applicable that is.

https://github.com/mikehenrty/gaia/compare/notification-tray#diff-34ac4f93978f5ace27b51ad2f2229c91R5

	[:fabrice] Fabrice Desré
	Comment 100 • 10 years ago

(In reply to Etienne Segonzac (:etienne) from comment #98)

> 
> Fabrice, anything rings a bell?

No. Did you try peeking at shell.js to see what we do with the power key event there?

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 101 • 10 years ago

(In reply to Etienne Segonzac (:etienne) from comment #97)
> Comment on attachment 8473514 [details] [review]
> attention window final
> 
> There was probably a misunderstanding on IRC, I thought we we're going with
> the simpler attention indicator, ie. one for the whole system that would not
> swipe with the sheets (but would easily be displayed on top of fullscreen
> apps, BTW).
> 
> This is adding a bunch of code last minute to a pretty huge PR and I'm
> really worried about the performance implications. If you turn on "flash
> repainted area" you'll see that for every frame of the animation the whole
> attention indicator of *each* app window is repainted. During an edge
> gesture that's 3 zones a few pixels high but as wide as the screen being
> repainted for every frame.
> 
> The animation itself is not an issue as you can see when we open the utility
> tray and display the "root" attention indicator. The issue is the
> moz-element on animated content.
> 
> Sorry :/
> 
> Since Rob confirmed that moving the indicator with the sheets is not a hard
> requirement we should go with the easier solution and just toggle a class on
> a #attention-indicator.

It is easy to remove the shadow indicator if performance kills us and convinces UX.
I will look at the issue you mentioned in comment below later today but don't think I have voicemail stuff to test..

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 102 • 10 years ago

(In reply to Etienne Segonzac (:etienne) from comment #98)
> There is another extremely weird issue, that I can only reproduce with this
> patch but doesn't look related.
> It might be a deeper gecko issue revealed by this change.
> 
> Here's the recap from the tests we did with Vivien hopefully it'll help.
> 
> STR:
> - call your voicemail
> - minimize the callscreen
> - don't do anything else, let the phone alone
> 
> at some point your voicemail will hang up on you, a bit later the screen
> will turn off.
> 
> Now you're phone is in a really weird state where pressing the power button
> will *not* wake it up.
> 
> We've reproduced it many time, and managed to keep the remote debugger
> connected when this happens:
> - from the system app console we added listeners to mozChromeEvent and got
> nothing when pressing the hardware buttons. the system app isn't receiving
> the mozChromeEvents
> - if manually we call |ScreenManager.turnScreenOn()| the screen turns on
> correctly but still no mozChromeEvents and no hardware button response
> (including the home button that should vibrate)
> - if we tap somewhere inside the screen (focusing it?) everything comes back
> to normal.
> 
> It _looks_ like a gecko side focus issue but it's very worrying since the
> only way out when this happens is to remove the battery (for somebody
> without adb).
> 
> Fabrice, anything rings a bell?

The only thing I could figure out is,
I suspect this might be related to the change in screenManager about cpu wake lock stuff.

And yes I sometimes cannot wake up it too.. no Javascript Error.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 103 • 10 years ago

I had updated the pull request to remove the multiple attention indicators. NOW we have only one indicator and it will show above fullscreen app and WONT move with sheets.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 104 • 10 years ago

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #102)
> (In reply to Etienne Segonzac (:etienne) from comment #98)
> > There is another extremely weird issue, that I can only reproduce with this
> > patch but doesn't look related.
> > It might be a deeper gecko issue revealed by this change.
> > 
> > Here's the recap from the tests we did with Vivien hopefully it'll help.
> > 
> > STR:
> > - call your voicemail
> > - minimize the callscreen
> > - don't do anything else, let the phone alone
> > 
> > at some point your voicemail will hang up on you, a bit later the screen
> > will turn off.
> > 
> > Now you're phone is in a really weird state where pressing the power button
> > will *not* wake it up.
> > 
> > We've reproduced it many time, and managed to keep the remote debugger
> > connected when this happens:
> > - from the system app console we added listeners to mozChromeEvent and got
> > nothing when pressing the hardware buttons. the system app isn't receiving
> > the mozChromeEvents
> > - if manually we call |ScreenManager.turnScreenOn()| the screen turns on
> > correctly but still no mozChromeEvents and no hardware button response
> > (including the home button that should vibrate)
> > - if we tap somewhere inside the screen (focusing it?) everything comes back
> > to normal.
> > 
> > It _looks_ like a gecko side focus issue but it's very worrying since the
> > only way out when this happens is to remove the battery (for somebody
> > without adb).
> > 
> > Fabrice, anything rings a bell?
> 
> The only thing I could figure out is,
> I suspect this might be related to the change in screenManager about cpu
> wake lock stuff.
> 
> And yes I sometimes cannot wake up it too.. no Javascript Error.

I am able to repro it...

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/attention_screen.js#L248-L256
And I guess this might related. Etienne, do you know the reason to remove the frame src after phone call?

I am going to try it anyway.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 105 • 10 years ago

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #104)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #102)
> > (In reply to Etienne Segonzac (:etienne) from comment #98)
> 
> I am able to repro it...
> 
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/
> attention_screen.js#L248-L256
> And I guess this might related. Etienne, do you know the reason to remove
> the frame src after phone call?
> 
> I am going to try it anyway.

I'd fixed it by adding this in mozbrowserclose handler.

    // XXX: We are leaving the focus in the callscreen iframe
    // And the first call to blur will not succeed.
    var retryCount = 3;
    while (document.activeElement === this.browser.element && retryCount > 0) {
      document.activeElement.blur();
      retryCount--;
    }

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 106 • 10 years ago

Comment on attachment 8473514 [details] [review]
attention window final

I removed the shadow and added some more tests. Fixed the focus issue as well.

	Etienne Segonzac (:etienne)
	Comment 107 • 10 years ago

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #104)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #102)
> > (In reply to Etienne Segonzac (:etienne) from comment #98)
> > > There is another extremely weird issue, that I can only reproduce with this
> > > patch but doesn't look related.
> > > It might be a deeper gecko issue revealed by this change.
> > > 
> > > Here's the recap from the tests we did with Vivien hopefully it'll help.
> > > 
> > > STR:
> > > - call your voicemail
> > > - minimize the callscreen
> > > - don't do anything else, let the phone alone
> > > 
> > > at some point your voicemail will hang up on you, a bit later the screen
> > > will turn off.
> > > 
> > > Now you're phone is in a really weird state where pressing the power button
> > > will *not* wake it up.
> > > 
> > > We've reproduced it many time, and managed to keep the remote debugger
> > > connected when this happens:
> > > - from the system app console we added listeners to mozChromeEvent and got
> > > nothing when pressing the hardware buttons. the system app isn't receiving
> > > the mozChromeEvents
> > > - if manually we call |ScreenManager.turnScreenOn()| the screen turns on
> > > correctly but still no mozChromeEvents and no hardware button response
> > > (including the home button that should vibrate)
> > > - if we tap somewhere inside the screen (focusing it?) everything comes back
> > > to normal.
> > > 
> > > It _looks_ like a gecko side focus issue but it's very worrying since the
> > > only way out when this happens is to remove the battery (for somebody
> > > without adb).
> > > 
> > > Fabrice, anything rings a bell?
> > 
> > The only thing I could figure out is,
> > I suspect this might be related to the change in screenManager about cpu
> > wake lock stuff.
> > 
> > And yes I sometimes cannot wake up it too.. no Javascript Error.
> 
> I am able to repro it...
> 
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/
> attention_screen.js#L248-L256
> And I guess this might related. Etienne, do you know the reason to remove
> the frame src after phone call?
> 
> I am going to try it anyway.

We need to unload/reload the callscreen since the callscreen code was always developed as a window that would close.
Ie. it's gonna be state hell if we don't do it.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 108 • 10 years ago

(In reply to Etienne Segonzac (:etienne) from comment #107)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #104)
> > (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #102)
> > > (In reply to Etienne Segonzac (:etienne) from comment #98)
> > > > There is another extremely weird issue, that I can only reproduce with this
> > > > patch but doesn't look related.
> > > > It might be a deeper gecko issue revealed by this change.
> > > > 
> > > > Here's the recap from the tests we did with Vivien hopefully it'll help.
> > > > 
> > > > STR:
> > > > - call your voicemail
> > > > - minimize the callscreen
> > > > - don't do anything else, let the phone alone
> > > > 
> > > > at some point your voicemail will hang up on you, a bit later the screen
> > > > will turn off.
> > > > 
> > > > Now you're phone is in a really weird state where pressing the power button
> > > > will *not* wake it up.
> > > > 
> > > > We've reproduced it many time, and managed to keep the remote debugger
> > > > connected when this happens:
> > > > - from the system app console we added listeners to mozChromeEvent and got
> > > > nothing when pressing the hardware buttons. the system app isn't receiving
> > > > the mozChromeEvents
> > > > - if manually we call |ScreenManager.turnScreenOn()| the screen turns on
> > > > correctly but still no mozChromeEvents and no hardware button response
> > > > (including the home button that should vibrate)
> > > > - if we tap somewhere inside the screen (focusing it?) everything comes back
> > > > to normal.
> > > > 
> > > > It _looks_ like a gecko side focus issue but it's very worrying since the
> > > > only way out when this happens is to remove the battery (for somebody
> > > > without adb).
> > > > 
> > > > Fabrice, anything rings a bell?
> > > 
> > > The only thing I could figure out is,
> > > I suspect this might be related to the change in screenManager about cpu
> > > wake lock stuff.
> > > 
> > > And yes I sometimes cannot wake up it too.. no Javascript Error.
> > 
> > I am able to repro it...
> > 
> > https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/
> > attention_screen.js#L248-L256
> > And I guess this might related. Etienne, do you know the reason to remove
> > the frame src after phone call?
> > 
> > I am going to try it anyway.
> 
> We need to unload/reload the callscreen since the callscreen code was always
> developed as a window that would close.
> Ie. it's gonna be state hell if we don't do it.

Patch updated:
Implement CallscreenWindow.prototype.reloadWindow() to do that.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 109 • 10 years ago

Comment on attachment 8473514 [details] [review]
attention window final

Fix all comments in github and ask review as discussed.

	Etienne Segonzac (:etienne)
	Comment 110 • 10 years ago

Comment on attachment 8473514 [details] [review]
attention window final

Took a deep dive, many comments on github, none of them too big but not nits either we need to fix those :)

This shouldn't block the visual review BTW, Eric feel free to continue with this patch.

PS: also needs rebasing because of the recents cardview changes

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 111 • 10 years ago

The patch is already rebased to the task-manager change. I tried manually but don't something bad. BTW there is this.stack is null in task_manager.js but should be irrelevant.

Eric and Rob, we need your attention here before feature landing.
Currently what could be refined is the animation of the indicator. We may need deep color to let the user know there is an attention.

I will record a video later.

	Rob MacDonald [:robmac]
	Comment 113 • 10 years ago

Hi Alive - Thanks for the video. Eric and I actually played with the patch this morning and here's our feedback.

The one  request we had is whether we could have the pulsing animation on top of the notification toast while it appears. I'm not sure if this is feasible at this stage but please confirm. I would not consider this blocking but it will definitely help the ux.

The animation, while not exactly to spec, looks really good. So thanks for taking care of that. 

The notification toast isn't to the visual spec, but Eric will discuss this separately with Guillaume who may be able to look at this as part of his notifications work.

Although I realize we're limited on what strings we can use, one additional suggestion would be to change the wording from "Call screen / Tap to show" to "Active call / Tap to show". For alarm we could use "Alarm / Tap to show". I believe this wording would be slightly clearer.

There are other issues that are not to spec that we covered previously and, again, are not blocking. For the these include:

- The lack of edge gesture in the attention window (deferred - needs more analysis from ux)
- The ambient animation / indicator not transitioning with sheets (deferred due to complexity)
- Strings not matching the spec (deferred due to technical constraints)

One other thing I noticed, and this is way beyond the scope of this bug, is that the call window has no relationship with dialer. I would expect that, if I tapped on the dialer icon it would return me to an active call. Perhaps this is something we could revisit in 2.2.

Please NI me with any additional questions or comments.

- Rob

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 114 • 10 years ago

(In reply to Rob MacDonald [:robmac] from comment #113)
> Hi Alive - Thanks for the video. Eric and I actually played with the patch
> this morning and here's our feedback.
> 
> The one  request we had is whether we could have the pulsing animation on
> top of the notification toast while it appears. I'm not sure if this is
> feasible at this stage but please confirm. I would not consider this
> blocking but it will definitely help the ux.

When you say "on top of the toaster" you mean z-axis(z-index) top but still at the same x,y position? It's do-able for this case.

> 
> The animation, while not exactly to spec, looks really good. So thanks for
> taking care of that. 
> 
> The notification toast isn't to the visual spec, but Eric will discuss this
> separately with Guillaume who may be able to look at this as part of his
> notifications work.

Yap we don't have enough notification support right now.

> 
> Although I realize we're limited on what strings we can use, one additional
> suggestion would be to change the wording from "Call screen / Tap to show"
> to "Active call / Tap to show". For alarm we could use "Alarm / Tap to
> show". I believe this wording would be slightly clearer.

I had a question here: Do you want to show 'Incoming call' when the call is not picked up yet?
If you want to change the wording according to current call state, it's the same problem as 'dynamic notification' stuff. I don't think we could do it right now.

> 
> There are other issues that are not to spec that we covered previously and,
> again, are not blocking. For the these include:
> 
> - The lack of edge gesture in the attention window (deferred - needs more
> analysis from ux)
> - The ambient animation / indicator not transitioning with sheets (deferred
> due to complexity)
> - Strings not matching the spec (deferred due to technical constraints)
> 
> One other thing I noticed, and this is way beyond the scope of this bug, is
> that the call window has no relationship with dialer. I would expect that,
> if I tapped on the dialer icon it would return me to an active call. Perhaps
> this is something we could revisit in 2.2.

I suspect it because we should be able to open dialer to see the call history here. But yes please revisit it later.

> 
> Please NI me with any additional questions or comments.
> 
> - Rob

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 115 • 10 years ago

^^^ According to comments above I think the last change will be the z-index of the indicator to above the toaster, am I right? Or I am still missing some important things, please let me know.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 116 • 10 years ago

For the notification context part I had an idea but maybe you have some concern:

Update the application name in the fake notification when we get mozbrowsertitlechange in attention window. So alarm page could set its title to alarm and callscreen page could set its title to active call. Will you think it's too hacky or we just shouldn't do it?

	Rob MacDonald [:robmac]
	Comment 117 • 10 years ago

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #114)

> When you say "on top of the toaster" you mean z-axis(z-index) top but still
> at the same x,y position? It's do-able for this case.

That is correct - thanks for clarifying!

> I had a question here: Do you want to show 'Incoming call' when the call is
> not picked up yet?
> If you want to change the wording according to current call state, it's the
> same problem as 'dynamic notification' stuff. I don't think we could do it
> right now.

That would be ideal. Let's retain it as is and raise a follow-up bug.

Thanks!

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 118 • 10 years ago

I made the z-index change, but the task manager bug is backout.(bug 1047143)
Need to rebase again..

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 119 • 10 years ago

Comment on attachment 8473514 [details] [review]
attention window final

Might be the last round, hopefully..

Rob would you ui-review again? The only change is the z-index stuff.

	Etienne Segonzac (:etienne)
	Comment 120 • 10 years ago

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #116)
> For the notification context part I had an idea but maybe you have some
> concern:
> 
> Update the application name in the fake notification when we get
> mozbrowsertitlechange in attention window. So alarm page could set its title
> to alarm and callscreen page could set its title to active call. Will you
> think it's too hacky or we just shouldn't do it?

I love the idea! But lets keep it for a follow up ;)

	Etienne Segonzac (:etienne)
	Comment 121 • 10 years ago

Comment on attachment 8473514 [details] [review]
attention window final

TODO (on top of the attention-window-final-2):
* fix the last test-related comments (see on the PR:https://github.com/mozilla-b2g/gaia/pull/22913 )
* get the ui-review

Small enough todo list for me to put the r+ flag already, congrats!

	Rob MacDonald [:robmac]
	Comment 122 • 10 years ago

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #119)
> Comment on attachment 8473514 [details] [review]
> attention window final
> 
> Might be the last round, hopefully..
> 
> Rob would you ui-review again? The only change is the z-index stuff.

Looks good from the IxD perspective. Deferring to epang for the visual review.

	Rob MacDonald [:robmac]
	Comment 123 • 10 years ago

Also, NI'ing Alberto as an FYI in case there are any overlaps with bug 1042713. 

The pulsing attention window indicator should over-ride or be on top of any ambient indication indicator. Let me know if this is an issue.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 124 • 10 years ago

Test is all green.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 125 • 10 years ago

Due to
* Complexity of the patch
* No response from the visual for days

I proposed to land it by end of today and if visual has any opinion we could polish it later or backout.

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 126 • 10 years ago

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #125)
> I proposed to land it by end of today and if visual has any opinion we could
> polish it later or backout.

Agree.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 127 • 10 years ago

https://github.com/mozilla-b2g/gaia/pull/22913

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 128 • 10 years ago

https://github.com/mozilla-b2g/gaia/commit/6f270b9fee0c1f09863f5e1aa640937a07c7fdae I mean.

	Alberto Pastor [:albertopq]
	Comment 129 • 10 years ago

Tested bug 1042713 after rebasing with this patch, and everything looks fine. The attention window indicator is being shown on top of the ambient indicator. Thanks!

	Eric Pang [:epang] UX
	Comment 131 • 10 years ago

Comment on attachment 8473514 [details] [review]
attention window final

Hi Alive,

R+, as long as we polish following FL

It's looking great though, I didn't think we would have the animation in! :)

Some comments (some of which Rob has already commented on)

1. The Notification toast isn't to spec (for example, height), will this be fixed with https://bugzilla.mozilla.org/show_bug.cgi?id=1042713 that Alberto is working on?

2. We need to reduce the height of the ambient indicator by 1 px (it's implemented to spec, but we found it's too dominant.  I've also commented on this in the notification bug).

3, Indicator/toast doesn't move with sheets

	Etienne Segonzac (:etienne)
	Comment 132 • 10 years ago

> 3, Indicator/toast doesn't move with sheets

just want to double check that we have in the spec:
- the behavior while in the cardview
- the behavior of the ambient indicator when the app has its overlay (while edge swiping)
- the behavior for fullscreen apps.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 133 • 10 years ago

Thanks Eric, I am going to file a followup to address the visual refine.