Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Description • 10 years ago

+++ This bug was initially created as a clone of Bug #1023818 +++

The detail to implement:

1. Need detailed UI spec, since now it's only a draft and without any size information and possible necessary assets

2. Path: notification on LockScreen got created -> 
   add callback to fire the corresponding event ->
   put the element on LockScreen ->
   user click, the callback got triggered ->
   LockScreenWindowManager received the event with detail ->
   unlock and follow the original way the utility tray notification to launch the app

3. Possible issue: race condition to launch the app and unlock, just like the camera issue. So we need to follow the existing path of the camera issue, to extend the 'showwindow' to make this case can be solved in the same path

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 1 • 10 years ago

Update:

1. Don't know notification need to follow the same path of app window, so now it would fire event to unlock while invoking Gecko to do things (open the inline activity of the notification) at the same time. I'll ask Alive's opinions.

2. Now it would clear all notifications while unlock. If we want to keep the rest notifications after we unlock, we need some new spec.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 3 • 10 years ago

I've almost done this feature:

http://youtu.be/-Z9RA51TAEs

Will set review and UI review to see what I need to modify.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 4 • 10 years ago

Comment on attachment 8461405 [details]
Patch

Almost done. Please give opinions, thanks!

	Amy Lee [:amylee] UX
	Comment 5 • 10 years ago

Comment on attachment 8461405 [details]
Patch

Hi, 

I've added Rob for UX review. 

Here is my VsD feedback:

1. Hairline borders should disappear when user taps on the notification and it's highlighted. The highlight box height should be the height of the top and bottom hairline border.

2. The vertical hairline looks longer than what is specified in the spec. This should be 3.5 rems.

3. The length of the truncation should be the same between highlighted state and non-highlighted state (Rob to confirm).

4. When there are 4+ notifications, there is an extra hairline at the bottom above the arrow. This should be removed. 

5. There is no on-press state for the "open" button. The on-press state should be 20% opacity as specified in the spec. On release, there should be brief delay.

6. Can you add a quick fade in/fade out for the highlight state of notifications? I would start with .25 seconds.

7. I believe the notification highlight should fade out after inactivity. I will let Rob confirm the specific UX behaviour. 

Thanks!

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

Comment on attachment 8461405 [details]
Patch

Amy is correct regarding the time-out. In the spec (located at https://mozilla.box.com/s/gsklu2bl6ii98sbn8bsk) the notification highlight should fade out after three seconds of inactivity. The highlight should also fade out if the user taps on the lock mechanism or on an inactive area of the screen.

I'm on PTO until Aug 18 but please flag Francis Djabri in the meantime. I've NI'd him on this bug as an FYI.

Thanks!
Rob

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 7 • 10 years ago

Hi Amy,

I would implement what you mentioned in the comment. However, I notice that the formal spec on Bug 1023818 comes without the fade out/in transition. Could we merge these spec to one? To have two spec seems confused.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 8 • 10 years ago

Current status:

1: Fixed. The height on the spec is 6rem, and I'm sure it's 6rem on the device, according to the code
2: Fixed.
3: If you mean to truncate the content in the same length, from the spec it seems different. For example, the first 'Wanna go grab a quick lunch today?' extends to the end of the notification, while the highlighted one was truncated.
4: The extra hairline was already existing before this patch. I prefer no fix in this patch, and fire another follow-up to focus on the issue. NI John because he know the visual updating better.
5: Fixed.

The rest parts are related to animation, which require extra effort on animation state control. So I would update them later.

	John Lu [:mnjul][Please NI or I'll miss]
	Comment 9 • 10 years ago

Regarding extra hairline: it's also there on current master and is probably a regression some time ago, unrelated to this bug. A separate bug 1051758 has been filed and I will work on it.

	Amy Lee [:amylee] UX
	Comment 10 • 10 years ago

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #7)
> Hi Amy,
> 
> I would implement what you mentioned in the comment. However, I notice that
> the formal spec on Bug 1023818 comes without the fade out/in transition.
> Could we merge these spec to one? To have two spec seems confused.

Hi Greg, 

The documents are separate because IXD spec specifies user interaction and the VSD spec specifies visual graphics/transitions. I had left out instructions on transitions in the VSD spec because I wanted to confirm with my team on the types of transitions that we are currently using for the OS.

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

Comment on attachment 8461405 [details]
Patch

r+ the code.

Please figure out the visual detail promptly, we should target getting this landed as soon as possible and fix the visual detail in follow-up so QA can verify the behavior in detail (and in parallel).

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 12 • 10 years ago

One option is to split this bug as two, and I can land the current part (without animation, after UX review+) first, and then to fix the animation part in a proper way, rather than implement it hurriedly with too much workarounds, which may kill us in the future.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 13 • 10 years ago

The splitting way is similar to LockScreen notification visual updates.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 14 • 10 years ago

Comment on attachment 8461405 [details]
Patch

Tim said the strategy may be OK. So I want to set UI review again, and fix the further opinions ASAP, so I set the flag again.

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

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #14)
> Comment on attachment 8461405 [details]
> Patch
> 
> Tim said the strategy may be OK. So I want to set UI review again, and fix
> the further opinions ASAP, so I set the flag again.

Make sense.

	Amy Lee [:amylee] UX
	Comment 16 • 10 years ago

Comment on attachment 8461405 [details]
Patch

Hi I'm getting an error message when I tried to load this patch:

Exception: External packaged webapp `marketplace.allizom.org.gaiamobile.org  is missing an `update.webapp` file. This JSON file contains a `package_path` attribute specifying where to download the application zip package from the origin specified in `metadata.json` file.
undefined
make: *** [post-app] Error 3

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 17 • 10 years ago

Hello, I'm sure the error is irrelevant with my patch, and I just tried it again on my device, it works well:

https://www.youtube.com/watch?v=EOp_c9BcYtI

Can you try it again?

	Amy Lee [:amylee] UX
	Comment 18 • 10 years ago

Hi John, 

Here is my feedback:

1. The on press state, the text should fade to 20% opacity but the background highlight should stay the same opacity (right now the background opacity changes when you press it). On release, the highlight state should remain for a second so you can see the pressed highlight state on release.  

2. Can you please change the opacity of the on press background (the semi-transparent black bar) so it matches the same opacity as the lockscreen music player background? I think it's 30%. 

3. Can you remove the hairline right about the more notifications arrow in the on press state? See attached screen.

4. There seems to be a slight shift in the text when you first press on a notification. 

5. The “open” text should be right aligned to “just now” and vertically center to the highlight box (please move 2px up). See attached screen.

6. Vertical divider line between text and “open” should be centred (please move 3px left). See attached screen.

Thanks!

	Amy Lee [:amylee] UX
	Comment 19 • 10 years ago

Comment on attachment 8461405 [details]
Patch

See my feedback in comment 18. Thanks

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 20 • 10 years ago

> Can you please change the opacity of the on press background (the semi-transparent black bar) so it matches the same opacity as the lockscreen music player background? I think it's 30%. 

They're with not the same color code:

rgba(51, 51, 51, 0.3) for media playback widget
rgba(0, 0, 0, 0.3) for the highlighted area

So even now I set the same opacity, they're still with different color. Is this what you want?

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 21 • 10 years ago

About the highlighted background while there is music player: Bug 1023500 adjust no position while there is dual sims. So it should not be resolved in this bug.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 22 • 10 years ago

Comment on attachment 8461405 [details]
Patch

Okay, I think I fixed the issues. Please check it again, thanks.

	Amy Lee [:amylee] UX
	Comment 23 • 10 years ago

Comment on attachment 8461405 [details]
Patch

Hi John, 

Here's my feedback:

1. The notification highlight should fade out after three seconds of inactivity. The highlight should also fade out if the user taps on the lock mechanism or on an inactive area of the screen. Please see Rob's feedback in comment 6.

2. The "open" text button is still not right aligned to the time stamp. Please move the open text button 3px to the right. 

3. Notification highlight peeks through when you scroll to the top (See screenshot).

4. Notifications is selectable even when you can’t see it (See screenshot). Flagging Rob for ui review and to see if we can set some sort of minimum area that the notification needs to be showing in order for it to be selectable, especially when user is scrolling through notifications.

	Amy Lee [:amylee] UX
	Comment 24 • 10 years ago

Screenshot for reference for ui review (see comment 23).

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 25 • 10 years ago

Please read comments because I'm *not* John, apparently. And if you noticed that, I've said the animation is not as simple as it looks like (Comment 12), so we would do that in another follow-up bug after this one.

The highlighted part, if you want it to be un-highlighted when it comes close to the boundary, I can tell you it would need some relatively tricky calculation about the height and the position, which may not complete as soon as you think. Also, the model to control the triggering of calculation may bring performance impact, that I would not say it's okay to make the trade-off before I discuss this with Tim and Howie.

Also, I've NI you in Comment 20 and you have no answer for that. Please make sure the color I've mentioned is correct or not. Since we adjust even pixels to make things as correct as you want, it would be unacceptable to miss the part so obvious if it's incorrect.

	Amy Lee [:amylee] UX
	Comment 26 • 10 years ago

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #25)
> Please read comments because I'm *not* John, apparently. And if you noticed
> that, I've said the animation is not as simple as it looks like (Comment
> 12), so we would do that in another follow-up bug after this one.
> 
> The highlighted part, if you want it to be un-highlighted when it comes
> close to the boundary, I can tell you it would need some relatively tricky
> calculation about the height and the position, which may not complete as
> soon as you think. Also, the model to control the triggering of calculation
> may bring performance impact, that I would not say it's okay to make the
> trade-off before I discuss this with Tim and Howie.
> 
> Also, I've NI you in Comment 20 and you have no answer for that. Please make
> sure the color I've mentioned is correct or not. Since we adjust even pixels
> to make things as correct as you want, it would be unacceptable to miss the
> part so obvious if it's incorrect.

Hi Greg, 

My sincere apologies, I've been working with John on lockscreen for a while and I got used to referring to him in ui reviews. 

I had thought the actual transition animation (fade out) and the behavior (i.e highlight state switching off) were separate issues so that is why I pointed it out. Thanks for clarifying. 

Yes, please change the highlight state colour to rgba (51, 51, 51, 0.3) to match the lockscreen toggle.

Thanks for your patience!

	Amy Lee [:amylee] UX
	Comment 27 • 10 years ago

(In reply to Amy Lee [:amylee] from comment #26)
> (In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #25)

> > The highlighted part, if you want it to be un-highlighted when it comes
> > close to the boundary, I can tell you it would need some relatively tricky
> > calculation about the height and the position, which may not complete as
> > soon as you think. Also, the model to control the triggering of calculation
> > may bring performance impact, that I would not say it's okay to make the
> > trade-off before I discuss this with Tim and Howie.

Also, I'll wait for Rob to comment on if a bounding area is necessary for partially covered notifications now that we know the trade-offs/difficulty of creating that.

	Amy Lee [:amylee] UX
	Comment 28 • 10 years ago

(In reply to Amy Lee [:amylee] from comment #27)
> (In reply to Amy Lee [:amylee] from comment #26)
> > (In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #25)
> 
> > > The highlighted part, if you want it to be un-highlighted when it comes
> > > close to the boundary, I can tell you it would need some relatively tricky
> > > calculation about the height and the position, which may not complete as
> > > soon as you think. Also, the model to control the triggering of calculation
> > > may bring performance impact, that I would not say it's okay to make the
> > > trade-off before I discuss this with Tim and Howie.
> 
> Also, I'll wait for Rob to comment on if a bounding area is necessary for
> partially covered notifications now that we know the trade-offs/difficulty
> of creating that.

Hi, 

I've created a follow-up bug 1055577 to track lockscreen actionable notifications animation.

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

Comment on attachment 8461405 [details]
Patch

Thanks for flagging me. From an interaction perspective the main missing element is the time-out as Amy mentioned in her earlier comment and as per comment 6.

In terms of having a minimum tapable height, if you look at our other apps, we do allow active states for any height. So this so this may be more of a general pattern that we need to look at as opposed to just the lock screen.

Amy's screenshots provide examples that would be nice to avoid. For the first example, we can eliminate this issue by turning off the highlight as soon as the user starts scrolling. For the second example, where the notification can be tapped while off screen, this should be fixed as the entire notification is not visible... so selecting it is not expected.

Hopefully this makes sense but NI me if you have questions.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 30 • 10 years ago

Comment on attachment 8461405 [details]
Patch

Okay, thanks for the reply. I've fixed the static part of the patch according to the comments, which means:

1. the 'open' now move 3px right to align the timestamp above.
2. fixed the color to make the highlighted one match the media playback widget
3. fixed one bug that shows bottom hairline when new notification comes

I'll start the animation part after these fixes get verified, thanks.

	Amy Lee [:amylee] UX
	Comment 31 • 10 years ago

Comment on attachment 8461405 [details]
Patch

Hi Greg, 

Thanks for getting the fixes in. I'm going to approve the static UI visuals to keep this moving along. There is a minor inconsistency with the ellipsis not aligning when there are two long lines of text in the notification. Let me know if you want me to file a separate polish bug for this or I can add it to the animation bug. 

Thanks!

	Amy Lee [:amylee] UX
	Comment 32 • 10 years ago

This is a screenshot of the alignment issue with the ellipsis. Let me know if this should be filed as a separate polish bug or I can attach it to the lockscreen animation follow-up bug. Thanks!

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 33 • 10 years ago

I think it's fixable in this bug since it's a minor width adjustment. So I would try to patch it and NI you verify that, while I land this patch. Then, if you feel it's still not align, we can fire another polish bug for it. In this way we can land this 2.1 feature first and then do the polishing.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 34 • 10 years ago

OK. CI passed excepted irrelevant calendar tests, which failed at other PR:

https://tbpl.mozilla.org/?rev=9a5c03090d7daf9685c5feae9d9aef88ca29f7c7&tree=Gaia-Try

So I would land this.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 35 • 10 years ago

master: https://github.com/mozilla-b2g/gaia/commit/7065ddbc9c1bb08ca273362774bb142e3152cf0e

Amy: I've fixed the alignment issue in the patch. Please verify that, and if it still fails, we can fire another polish bug. Thanks.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 36 • 10 years ago

Well it looks like that I broke Gaia-Try... So I would back it out first.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 37 • 10 years ago

Backed out

https://github.com/mozilla-b2g/gaia/commit/705c5ba3243923e1a2b0f51aa15367ebc34ad931

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 38 • 10 years ago

Amy: sorry for the backing out. I'll land it ASAP.

	Amy Lee [:amylee] UX
	Comment 39 • 10 years ago

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #38)
> Amy: sorry for the backing out. I'll land it ASAP.

Hi Greg, 

Did this patch end up landing? You can just provide a screenshot of the ellipsis alignment for me to look over since it's pretty minor. Thanks!

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

Comment on attachment 8461405 [details]
Patch

Hi Greg and Amy - Please flag me when the latest patch lands as this one is likely out of date. Thanks!

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 42 • 10 years ago

Manually extend the line to make it truncated. Alignment adjusted. Note that the 'y' in original text would not be truncated because of the width, and the alignment would looks like a little too right. See the next screenshot.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 45 • 10 years ago

It's now without any error:

https://tbpl.mozilla.org/?rev=a573fd447c12f36001c4e2cfcd851fcf48e30aa9&tree=Gaia-Try

Gb and Gip success after the re-runs.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 46 • 10 years ago

master: https://github.com/mozilla-b2g/gaia/commit/cf02e96c1bb68cf480d239f15040115bb31a3ede

	Amy Lee [:amylee] UX
	Comment 47 • 10 years ago

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #42)
> Created attachment 8477173 [details]
> Truncated line
> 
> Manually extend the line to make it truncated. Alignment adjusted. Note that
> the 'y' in original text would not be truncated because of the width, and
> the alignment would looks like a little too right. See the next screenshot.

This looks great. Thanks for your patience on this Greg.

	KTucker [:KTucker][Inactive 3/4/2016]
	Comment 48 • 10 years ago

Cannot close this issue as verified because this issue depends on bug 1043821 which cannot be verified because of bug 1064630. Locking the device right after entering a passcode leaves the user in an unrecoverable state.