Implement AttentionWindow based on AppWindow.

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

(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.

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.

(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.

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?)

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.

Let's file another dialer bug and blocks this.

On it right now.

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

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.

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. :)

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.

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

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

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

My next item.

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.

Released from blockers, so back to this one again.

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

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.

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.

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.

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

(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.

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

(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? :)

(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 :)

Comment on attachment 8458050 [details] 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 :)

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.

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

(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?

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.

(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...

(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.

(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..

(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.

(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 :)

Comment on attachment 8458050 [details] 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...

Comment on attachment 8458050 [details] 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.

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.

(In reply to Etienne Segonzac (:etienne) (PTO -> August 12) from comment #40) > Comment on attachment 8458050 [details] > 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.

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 :)

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.

Comment on attachment 8458050 [details] 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.

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

(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. :)

Not sure what we could do to move this forward?

> > * 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!

Comment on attachment 8458050 [details] 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!

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.

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.

(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #50) > Comment on attachment 8458050 [details] > 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

(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.

(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?

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..

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?

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.

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.

(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.

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

(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.

(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 ?

(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.

(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...

(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.

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).

(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...

(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.

(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.

(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.

(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?

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

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 :)

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?

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

This is the best we have at this timing.

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.

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 :)

Comment on attachment 8473514 [details] 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 :)

(In reply to Etienne Segonzac (:etienne) from comment #80) > Comment on attachment 8473514 [details] > 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().

Comment on attachment 8473514 [details] attention window final This ui-review? should go to the same people who have ni? set. Assigned.

Comment on attachment 8473514 [details] 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!

Comment on attachment 8473514 [details] 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

(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.

(In reply to Rob MacDonald [:robmac] from comment #84) > Comment on attachment 8473514 [details] > 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

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

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.

(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?

Comment on attachment 8473514 [details] attention window final I added one more commit to implement attentionIndicator. Need feedback at first.

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #90) > Comment on attachment 8473514 [details] > 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.

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?

(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.

(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.

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.

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.

Comment on attachment 8473514 [details] 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.

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?

(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

(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?

(In reply to Etienne Segonzac (:etienne) from comment #97) > Comment on attachment 8473514 [details] > 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..

(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 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.

(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.

(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--; }

Comment on attachment 8473514 [details] attention window final I removed the shadow and added some more tests. Fixed the focus issue as well.

(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.

(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.

Comment on attachment 8473514 [details] attention window final Fix all comments in github and ask review as discussed.

Comment on attachment 8473514 [details] 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

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.

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

(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

^^^ 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.

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?

(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!

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

Comment on attachment 8473514 [details] attention window final Might be the last round, hopefully.. Rob would you ui-review again? The only change is the z-index stuff.

(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 ;)

Comment on attachment 8473514 [details] 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!

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #119) > Comment on attachment 8473514 [details] > 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.

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.

Test is all green.

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.

(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.

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

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!

Comment on attachment 8473514 [details] 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

> 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.

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