Closed Bug 1043103 Opened 7 years ago Closed 7 years ago

[Lockscreen] Implement actionable LockScreen notifications

Categories

(Firefox OS Graveyard :: Gaia::System::Lockscreen, defect)

x86
macOS
defect
Not set
normal

Tracking

(feature-b2g:2.1)

RESOLVED FIXED
2.1 S3 (29aug)
feature-b2g 2.1

People

(Reporter: gweng, Assigned: gweng)

References

Details

User Story

As a user, I want to be able to directly action on the notifications on the lockscreen so I can quickly access the relevant apps after unlock.

Acceptance:
 - Use a gesture to access individual notifications
 - If the screen has a pin code lock then take the user to the unlock screen before entering the app

Attachments

(7 files, 1 obsolete file)

+++ 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
Assignee: nobody → gweng
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.
Attached file Patch
Depends on: 1043821
I've almost done this feature:

http://youtu.be/-Z9RA51TAEs

Will set review and UI review to see what I need to modify.
Comment on attachment 8461405 [details] [review]
Patch

Almost done. Please give opinions, thanks!
Attachment #8461405 - Flags: ui-review?(amlee)
Attachment #8461405 - Flags: review?(timdream)
Comment on attachment 8461405 [details] [review]
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!
Attachment #8461405 - Flags: ui-review?(rmacdonald)
Attachment #8461405 - Flags: ui-review?(amlee)
Attachment #8461405 - Flags: ui-review-
Comment on attachment 8461405 [details] [review]
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
Attachment #8461405 - Flags: ui-review?(rmacdonald) → ui-review-
Flags: needinfo?(fdjabri)
Blocks: 1023818
feature-b2g: --- → 2.1
No longer depends on: 1023818
Target Milestone: --- → 2.1 S2 (15aug)
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.
Flags: needinfo?(amlee)
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.
Flags: needinfo?(jlu)
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.
Flags: needinfo?(jlu)
(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.
Flags: needinfo?(amlee)
Comment on attachment 8461405 [details] [review]
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).
Attachment #8461405 - Flags: review?(timdream) → review+
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.
Flags: needinfo?(timdream)
The splitting way is similar to LockScreen notification visual updates.
Comment on attachment 8461405 [details] [review]
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.
Attachment #8461405 - Flags: ui-review?(amlee)
Attachment #8461405 - Flags: ui-review-
(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #14)
> Comment on attachment 8461405 [details] [review]
> 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.
Flags: needinfo?(timdream)
Comment on attachment 8461405 [details] [review]
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
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?
Flags: needinfo?(amlee)
Attached image notifiaction_edits.png
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!
Flags: needinfo?(amlee)
Comment on attachment 8461405 [details] [review]
Patch

See my feedback in comment 18. Thanks
Attachment #8461405 - Flags: ui-review?(amlee) → ui-review-
> 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?
Flags: needinfo?(amlee)
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.
Comment on attachment 8461405 [details] [review]
Patch

Okay, I think I fixed the issues. Please check it again, thanks.
Attachment #8461405 - Flags: ui-review- → ui-review?(amlee)
Target Milestone: 2.1 S2 (15aug) → 2.1 S3 (29aug)
Comment on attachment 8461405 [details] [review]
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.
Attachment #8461405 - Flags: ui-review?(rmacdonald)
Attachment #8461405 - Flags: ui-review?(amlee)
Attachment #8461405 - Flags: ui-review-
Flags: needinfo?(amlee) → needinfo?(rmacdonald)
Attached image Screenshot.png
Screenshot for reference for ui review (see comment 23).
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.
Flags: needinfo?(amlee)
(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!
Flags: needinfo?(amlee)
(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.
Depends on: 1055577
(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.
No longer depends on: 1055577
Depends on: 1055577
Comment on attachment 8461405 [details] [review]
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.
Attachment #8461405 - Flags: ui-review?(rmacdonald) → ui-review-
Flags: needinfo?(rmacdonald)
Flags: needinfo?(fdjabri)
Comment on attachment 8461405 [details] [review]
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.
Attachment #8461405 - Flags: ui-review?(rmacdonald)
Attachment #8461405 - Flags: ui-review?(amlee)
Attachment #8461405 - Flags: ui-review-
Comment on attachment 8461405 [details] [review]
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!
Attachment #8461405 - Flags: ui-review?(amlee) → ui-review+
Attached image screenshot_ellipsis.png
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!
Flags: needinfo?(gweng)
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.
Flags: needinfo?(gweng)
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.
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.
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: needinfo?(amlee)
Resolution: --- → FIXED
Well it looks like that I broke Gaia-Try... So I would back it out first.
Backed out

https://github.com/mozilla-b2g/gaia/commit/705c5ba3243923e1a2b0f51aa15367ebc34ad931
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Amy: sorry for the backing out. I'll land it ASAP.
(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!
Flags: needinfo?(amlee) → needinfo?(gweng)
Comment on attachment 8461405 [details] [review]
Patch

Hi Greg and Amy - Please flag me when the latest patch lands as this one is likely out of date. Thanks!
Attachment #8461405 - Flags: ui-review?(rmacdonald)
Attached image No wrapped 'y' (obsolete) —
Attached image 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.
Attachment #8477172 - Attachment is obsolete: true
Flags: needinfo?(gweng)
Attached file Patch rev2
It's now without any error:

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

Gb and Gip success after the re-runs.
master: https://github.com/mozilla-b2g/gaia/commit/cf02e96c1bb68cf480d239f15040115bb31a3ede
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
(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.
Depends on: 1061983
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.
Depends on: 1077207
You need to log in before you can comment on or make changes to this bug.