Amy Lee [:amylee] UX Reporter
	Description • 10 years ago

The position of the notifications are not to spec. Please see screenshots and spec for reference. I am aware that we can't implement the scrolling of the date/time right now but can we still have some of the design elements in place? i.e the "more notifications" arrow when there is additional notifications.

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

Hi Amy,

Thanks for pointing out the issues. I understand I may not have been conveying what/why some things are (relatively) hard to carry out, so let me try to clarify things a bit more here.

1. I'd like to confirm that the space between the top of notifications and date is 39px.

2. The small vertical arrow to indicate "more notifications": To add the arrow and make it show statically would be simple, however, to make it [(a) show when there are more notifications and (b) hide automatically when the user scrolls to the bottom of the list] would be quite more difficult. This requires the lockscreen logics to detect scrolling of the notifications container, which may also bring performance impact.
Please note that the current "automatically activating content scrolling" mechanism is directly carried in gecko and not much gaia is involved.

3. The cutoff of notifications and showing "less and only intact notifications": Yes, I can (read: without much efforts) make notifications container show only "full/intact" notifications and not cut notifications off due to container overflow, but I guess it doesn't make much sense if the "scroll to see more" small vertical arrow can't be properly implemented and working as expected, functionally and visually, by UX and end-users?

4. Again, the cutoff of notifications and "fade out of notifications":
a) This would need to detect scrolling, again (only when the contents are being scrolled can we show the fade-out of the top side of the notifications container)
b) I don't really think it's easily feasible to implement fade out of any overflowing containers when the background is something non-trivial (in this case, a <div> wallpaper backgound lying several z-indices under) without resorting to dirty canvas operations. I need to NI greg for confirmation of this.

I hope these explain things a bit more. Anyway, Aside from issue 4, things are implementable but may come with quite some debug-regression-debug-optimize cycles and I might want to consult my EPM if we need a schedule and/or a defined criticalness for this.

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

greg, NI'ing you for issue 4b) in comment 4, thanks.

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

Hi John, 

Thanks for the explanation. To answer question 1. The spacing between the date/time and notifications is fluid since depending on the device, the height of the screen varies. I would like notifications to be centered between the date and unlock toggle when we are showing the maximum number of notifications on screen (which is 3). So whatever the height is between the date and top of the first notification when there are 3 notifications centered on the screen would be correct. Let me know if that's unclear.

The most important aspect of notifications that I would like to see fixed is providing some sort of indicator when there are more than 3 notifications on screen. In the current OS, it is unclear to users that they can scroll to see more notifications which I think is fairly important from a UX perspective. The fade from the top and bottom is to prevent the text from looking like it's being cut off abruptly which looks like a mistake.

Keep me updated if these fixes are possible. Thanks!

 

 


So in my spec I had measured a fixed height of 32px between the unlock toggle and notifications.

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

Well understood, Amy. Let me see what I can do.

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

Hi Amy, please confirm my guess that "when media player widget is shown with one notification, the whole thing should be vertically centered (instead of showing more than one notifications to crowd the screen assets)". Much appreciated!

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

Hi John, 

In the spec, I had used a 320x480 screen (our base size). In that scenario, only one notification could comfortably fit while music player is on. On the Flame, it looks like 2 notifications can fit comfortably with the music player on and an arrow appears if there are more than 2. 

My question is, is it possible to have the number of notifications shown on screen with the music player be based on the height of the screen? So if the height is 854px or higher it would show 2 notifications while on shorter screens it would show 1? If not, then I would stick to one notification if music player is on.

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

Hi John, 

Also one more thing. There was a bug for fixing the fuzzy graphics in the music player. I just want to make sure that gets fixed in the updated lockscreen. https://bugzilla.mozilla.org/show_bug.cgi?id=1011710 Thanks

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

Can we only detect the first and last scrolling event to add and remove a mask between the green and purple areas (see the attachment) to make the fade-out effect? The mask would has the same color with the background when it's in notification mode, but has some different opacity and the gradient gray/white shape, just like the old mask in LockScreen. The first event may not be a problem, and then we can do nothing while the scrolling position is not the end, or we got a last scrolling event.

However, if we still hit a performance issue, I would strongly suggest to invite UX to join the next follow up performance bug, to see if we can find out a compromised solution.

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

Amy,

The fuzzy music player widget icons (and actually they're wrong sizes -- being super large -- when on Flame, right?) have just now got their own bug 1025845.

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

Regarding: comment 11's "mask": Here's a mock. So we still see some cut-off effects because the gradient on the solid-color side doesn't blend well with the background image. This might not be a huge problem for the top side if we are eventually to push everything up to top edge of the screen (and the gradient will be there instead of the top edge of notifications container), but it will still be a problem for the bottom side since it will be what it looks like in my mock.

So shall we now look into SVG-based masks? I remember seeing them in (old, horizontal) homescreen style.

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

SVG would cause performance impact and we usually use it carefully. So if you want to test it, you must take care about the issue. If the issue really occurs, we may need more offline discussions.

	Stephany Wilkes
	Comment 15 • 10 years ago

Candice, flagging you on this one as a potential risk. This is going to take some time to get right and has a late target milestone marked.

	Candice Serran (:cserran)
	Comment 16 • 10 years ago

Okay thanks Stephany...but this is specific to lockscreen (owned by tim's team) :)

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

I test-added the wanted gradient opacity mask on the notification container and its performance impact looks somewhat discouraging. See https://www.youtube.com/watch?v=h4Xj14VfoUI for demo video (remember to use bigger player window & 1080p quality to read the FPS numbers).

In the demo video you can read the FPS numbers on the top-left corner of the screen (read the left-most number of the three). On Flame, Without the gradient opacity mask (and the notifications are cut-off visually), the FPS is about 45~48fps while scrolling repeatedly. With the mask applied, the FPS drops to about 15~20fps while scrolling.

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

(In reply to John Lu [:mnjul] @MoCoTaipei from comment #13)
> Created attachment 8441920 [details]
> 2014-06-18-15-29-05.png

This does look right. The scrollable content fades to a solid color, not to transparent (so we could see background).

(In reply to John Lu [:mnjul] @MoCoTaipei from comment #17)
> I test-added the wanted gradient opacity mask on the notification container
> and its performance impact looks somewhat discouraging. See
> https://www.youtube.com/watch?v=h4Xj14VfoUI for demo video (remember to use
> bigger player window & 1080p quality to read the FPS numbers).
> 
> In the demo video you can read the FPS numbers on the top-left corner of the
> screen (read the left-most number of the three). On Flame, Without the
> gradient opacity mask (and the notifications are cut-off visually), the FPS
> is about 45~48fps while scrolling repeatedly. With the mask applied, the FPS
> drops to about 15~20fps while scrolling.

I however don't see that on this video. John, please confirm we are not fade to a solid color.

I don't know if the scroll performance regression is acceptable here, nor who could give the final word on that. Flagging UX on this.

I also have question since last week on why this should block. Howie, any update on this?

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

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #18)
> (In reply to John Lu [:mnjul] @MoCoTaipei from comment #13)
> > Created attachment 8441920 [details]
> > 2014-06-18-15-29-05.png
> 
> This does look right. 

s/This does look right/This does NOT look right/

Ouch.

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

Comment on attachment 8441920 [details]
2014-06-18-15-29-05.png

mujul conform with me offline we are indeed fade out to transparent with SVG mask. The approach depicted in this screenshot is obsolete.

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

Hi everyone,

I gather that issues/TODOs in this bug have been a bit confusing and mixed together so here is a quick review from an engineer's prospective.

To achieve the desired results in the spec (excluding the "scrolling time/date/carrier/... upwards out of screen"), these issues/requirements have to be addressed:

1. We need to show exactly 3 notifications in lockscreen, and 1 (or 2) notifications in lockscreen with media player widget. No cutoff artifacts. The notifications container should be vertically centered (comment 6 and comment 9)

2. We need to show an arrow when there are more than 3, and 1 (or 2) notifications (without and with widget). Ideally, the arrow should disappear when the user has scrolled to the bottom of the notifications container.

3. We need to apply a gradient opacity mask which fades out the top and bottom edge of the notifications container, as illustrated in comment 17, when the user is scrolling the container; this is to prevent the cutoff artifact during scrolling.

Current status:
1. I'm doing this right now.

2. This will be done after 1 and hopefully it doesn't require excessively listening into scroll event...

3. A demo of SVG mask is at https://github.com/mnjul/gaia/commit/6539769c5192b109b3b607f88fb97163b0e99030. Additionally, I will need to dynamically apply the mask after I finish Issue 2, so that 1) the mask shows correctly when user scrolls the notifications container, and 2) hides when there are (<=3, and 1 (or <=2) (without and with widget)) notifications.

Please comment if I'm missing anything.

	howie [:howie]
	Comment 22 • 10 years ago

The blocking of this bug is based on per discussion between release management and Stephany. We can flag Stephany in if not happy with the blocking flag.

Let's do it this way:
If fade out performance is acceptable, then there's no big obstacle solving this bug, we remain the blocking tag since the patch will request uplift anyway.

If not, I suggest to complete the rest of items except fade out, and open fade out as another followup bug but no blocking, since I see no reason fade out alone should block the whole release.

Also, the demo video seems like a corner case, the chance of user repeatedly swiping notifications back and forward in a extreme speed might be rare. I suggest to consider the fade out performance under normal usage.

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

(In reply to howie [:howie] from comment #22)
> Let's do it this way:
> If fade out performance is acceptable, then there's no big obstacle solving
> this bug, we remain the blocking tag since the patch will request uplift
> anyway.
> 
> If not, I suggest to complete the rest of items except fade out, and open
> fade out as another followup bug but no blocking, since I see no reason fade
> out alone should block the whole release.

Make sense, thanks.

> Also, the demo video seems like a corner case, the chance of user repeatedly
> swiping notifications back and forward in a extreme speed might be rare. I
> suggest to consider the fade out performance under normal usage.

OK.

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

I've finished the issues in comment 21.

Demo video will be posted shortly.

Currently the fade-out feature is affected by bug 1030604.

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

Here is the demo video. Note that the disappearance of notifications during 0:45 is due to bug 1030604

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

Hi John, 

This is looking really good! 

Just a few adjustments:

1. Is it possible to have the fade start on the 4th notification? So there will always be 3 notifications that can be read clearly with no fade. The exception would be on a 320x480 screen where the screen is too short, I would leave as is.

2. Is it possible to have the fade start on the 3rd notification when music player is on lockscreen? The arrow can be move down lower (6px above music player background) to provide more space so there should be 2 notifications that can be read clearly and the fade appears on the 3rd notification when you scroll. The exception would be on a 320x480 screen where the screen is too short (also I tested it and the scroll area is really small, making it difficult to actually scroll through notifications). 

3. There should be a hairline above the arrow (See attached). 

4. The arrow should fade out (not just disappear). This should happen as soon as you start scrolling. The arrow should only reappear when there is another new notification.

Flagging UX to take a look as well.

Thanks

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

Hi John, 

Is it possible to just show one notification and remove the scrolling when the music player is on lockscreen for a 320x480 device? The scroll area is so small and it feels awkward trying to scroll in such a small space.

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

^^

	Stephany Wilkes
	Comment 34 • 10 years ago

This is noted as blocking because the Lockscreen Visual Refresh is one of the things required to make this release a 2.0.

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

> 2. Is it possible to have the fade start on the 3rd notification when music
> player is on lockscreen? The arrow can be move down lower (6px above music
> player background) to provide more space so there should be 2 notifications
> that can be read clearly and the fade appears on the 3rd notification when
> you scroll.

I did a mock-up and it appears that things are a bit crowded (from an engineer's eyes, of course, haha). Shall I proceed this item with this mock-up?

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

Amy:

(In reply to Amy Lee [:amylee] from comment #31)
> The exception would be on a 320x480 screen where the screen is
> too short (also I tested it and the scroll area is really small, making it
> difficult to actually scroll through notifications). 
(In reply to Amy Lee [:amylee] from comment #32)
> Is it possible to just show one notification and remove the scrolling when
> the music player is on lockscreen for a 320x480 device? The scroll area is
> so small and it feels awkward trying to scroll in such a small space.

I'm not sure I understand what you meant with comment 32 -- could you elaborate on the "remove the scrolling" bit? Thanks.

By the way, I adjusted the codes a bit, and it did felt a little bit easier to scroll with 320x480 device and music player widget displayed. (you may try out the adjusted codes at my github branch. Make sure the tip commit is e6a6c1627e6c085bcc1188a54acebdab452a026e or you'd be using the old codes)

And to double check: it's already the case that on a 320x480 device, with music player widget displayed, only one notification is shown, right? This is being verified on Buri.

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

(In reply to Amy Lee [:amylee] from comment #31)
> 1. Is it possible to have the fade start on the 4th notification? So there
> will always be 3 notifications that can be read clearly with no fade. The
> exception would be on a 320x480 screen where the screen is too short, I
> would leave as is.

 OK, will be done.

> 
> 3. There should be a hairline above the arrow (See attached). 

 Sorry for having forgotten about it. Will be added.

> 4. The arrow should fade out (not just disappear). This should happen as
> soon as you start scrolling. The arrow should only reappear when there is
> another new notification.

Actually it is fading out right now, but the animation time is quite fast (0.1sec). I will adjust the animation time.

And to confirm:
(When the user starts scrolling, the arrow/hairline disappears immediately.) Suppose there is no new notification, if the user scrolls back to the top of the list, the arrow/hairline should *not* reappear, right? Thanks.

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

(In reply to John Lu [:mnjul] @MoCoTaipei from comment #35)
> Created attachment 8447008 [details]
> 3x_notification_ls_music_widget.png
> 
> > 2. Is it possible to have the fade start on the 3rd notification when music
> > player is on lockscreen? The arrow can be move down lower (6px above music
> > player background) to provide more space so there should be 2 notifications
> > that can be read clearly and the fade appears on the 3rd notification when
> > you scroll.
> 
> I did a mock-up and it appears that things are a bit crowded (from an
> engineer's eyes, of course, haha). Shall I proceed this item with this
> mock-up?

Yes, can you proceed with the 3x notification option? I know it's a bit more cluttered but from a practical standpoint it's more useful to be able to see more than 1 notification at a time. Thanks!

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

(In reply to John Lu [:mnjul] @MoCoTaipei from comment #37)
> (In reply to Amy Lee [:amylee] from comment #31)
> > 1. Is it possible to have the fade start on the 4th notification? So there
> > will always be 3 notifications that can be read clearly with no fade. The
> > exception would be on a 320x480 screen where the screen is too short, I
> > would leave as is.
> 
>  OK, will be done.
> 
> > 
> > 3. There should be a hairline above the arrow (See attached). 
> 
>  Sorry for having forgotten about it. Will be added.
> 
> > 4. The arrow should fade out (not just disappear). This should happen as
> > soon as you start scrolling. The arrow should only reappear when there is
> > another new notification.
> 
> Actually it is fading out right now, but the animation time is quite fast
> (0.1sec). I will adjust the animation time.
> 
> And to confirm:
> (When the user starts scrolling, the arrow/hairline disappears immediately.)
> Suppose there is no new notification, if the user scrolls back to the top of
> the list, the arrow/hairline should *not* reappear, right? Thanks.

This is a UX question. I'll get Rob to see what he thinks.

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

^^

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

Here we go with a newer patch. Amy, could you check it out? Thanks.
 
I also need some clarification regarding comment 32, as I had said in comment 36. Thanks.

Changes:
- "More notifications" arrow now fading out more visibly
- Hairline above the arrow
- Arrow disappearance/appearance mechanism as outlined in comment 31
- 4x notifications for Flame 'normal', 3x for Flame 'with music player widget'
- 3x notifications for small-screen devices 'normal', 1x for those 'with music player widget'

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

(In reply to Amy Lee [:amylee] from comment #39)
> (In reply to John Lu [:mnjul] @MoCoTaipei from comment #37)
> > And to confirm:
> > (When the user starts scrolling, the arrow/hairline disappears immediately.)
> > Suppose there is no new notification, if the user scrolls back to the top of
> > the list, the arrow/hairline should *not* reappear, right? Thanks.
> 
> This is a UX question. I'll get Rob to see what he thinks.

Hi John and thanks for confirming. I would actually display the arrow/hairline in this case to maintain the consistency of the behaviour. I would consider this lower priority than the other issues Amy has identified, though.

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

(In reply to Stephany Wilkes from comment #34)
> This is noted as blocking because the Lockscreen Visual Refresh is one of
> the things required to make this release a 2.0.

Stephany, 

I'd like to know how the UX team thinks about the performance/FPS impact with the gradient opacity mask on the lockscreen, especially if scrolling through the notifications list is still perceived as smooth and/or responsive. You may try the current implementation at attachment 8447851 [details] and compare it with what we have on the master branch. Thanks!

	Stephany Wilkes
	Comment 44 • 10 years ago

Flagging Rob again on notifications.

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

(In reply to John Lu [:mnjul] @MoCoTaipei from comment #41)
> Created attachment 8447851 [details]
> Patch again (PR @ Github)
> 
> Here we go with a newer patch. Amy, could you check it out? Thanks.
>  
> I also need some clarification regarding comment 32, as I had said in
> comment 36. Thanks.
> 
> Changes:
> - "More notifications" arrow now fading out more visibly
> - Hairline above the arrow
> - Arrow disappearance/appearance mechanism as outlined in comment 31
> - 4x notifications for Flame 'normal', 3x for Flame 'with music player
> widget'
> - 3x notifications for small-screen devices 'normal', 1x for those 'with
> music player widget'

Hi John, 

I flashed the patch and I don't see the updated changes that you outlined.

Also to clarify for comment 32. What i mean is that for small screens (320x480) where we only have one notification show up when the music player is on. I would remove the option to scroll to see more notifications because area is so small and it feels awkward to scroll to see more notifications. If a new notification comes up, it would replace the current one on the screen. Does that make sense?

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

(In reply to Amy Lee [:amylee] from comment #45)
> 
> Hi John, 
> 
> I flashed the patch and I don't see the updated changes that you outlined.

Yep, I think I made some mistake in the patch URL...sorry about that. Please try the one attached in this comment.

> Also to clarify for comment 32. What i mean is that for small screens
> (320x480) where we only have one notification show up when the music player
> is on. I would remove the option to scroll to see more notifications because
> area is so small and it feels awkward to scroll to see more notifications.
> If a new notification comes up, it would replace the current one on the
> screen. Does that make sense?

Yep, that's well understood and is included in the patch in the attachment of this comment. Please check & see if you have any concerns about the implementation.

If the patch still doesn't work for you I can record a demo video. Just tell me what you want to see in this case (and especially with which screen sizes). Thanks!

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

Hi John,

Here’s my feedback:

1. [Flame] When you have a 4th notification all the notifications disappear and there is only a hairline and arrow remaining (try taking a screenshot 4 times to make the notifications appear and you’ll see what I mean). When you take a 5th screenshot all the notifications come back (See attached).

2. [Flame] The arrow looks for more notifications looks blurry. Let me know if you need a new graphic for this. (See attached)

3. Is it possible to have the fade appear below the arrow or where the arrow is once it disappears? So you only see the fade when you start scrolling. Right now it doesn’t seem to make sense that the fade sits on top of the 4th notification and only when there is 5+ notifications. (See attached)

4. [Flame] When the music player is on, I was hoping 3 notifications could fit in the screen. Right now the 3rd one is overlapping with the date/time. Please keep it to 2 notifications and add an arrow when there is more. The fade show happen on the 3rd notification when you’re scrolling so you can see 2 notifications clearly at all times. (See attached)

We're getting close! Thanks

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

(In reply to Amy Lee [:amylee] from comment #47)
> Created attachment 8451173 [details]
> screen.png
> 
> Hi John,
> 
> Here’s my feedback:
> 
> 1. [Flame] When you have a 4th notification all the notifications disappear
> and there is only a hairline and arrow remaining (try taking a screenshot 4
> times to make the notifications appear and you’ll see what I mean). When you
> take a 5th screenshot all the notifications come back (See attached).

Yes, this is bug 1030604 and is a Gecko bug, which I have no control over.

> 2. [Flame] The arrow looks for more notifications looks blurry. Let me know
> if you need a new graphic for this. (See attached)

I think this is because "GAIA_DEV_PIXELS_PER_PX=1.5" wasn't supplied when you used "make reset-gaia" to flash my patch (note that the slider arrows are proportionally smaller, and statusbar icons are also blurred).

> 3. Is it possible to have the fade appear below the arrow or where the arrow
> is once it disappears? So you only see the fade when you start scrolling.
> Right now it doesn’t seem to make sense that the fade sits on top of the 4th
> notification and only when there is 5+ notifications. (See attached)
> 
> 4. [Flame] When the music player is on, I was hoping 3 notifications could
> fit in the screen. Right now the 3rd one is overlapping with the date/time.
> Please keep it to 2 notifications and add an arrow when there is more. The
> fade show happen on the 3rd notification when you’re scrolling so you can
> see 2 notifications clearly at all times. (See attached)

Issues 3 and 4 have been addressed in the patch attached in this comment. Please check it out.

> We're getting close! Thanks

As long as the issues have been sorted out, I'd like to seek ui-review+. The ui-review should probably not be inhibited by issue 1, since it is a separate technical bug 1030604 in a separate FxOS component (with the dependency of this bug and that bug having been marked), and whether it should stand in 2.0's way is to be taken care of by the release management and/or product management process. Thanks!

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

Forgot to NI Amy ^^^

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

Side note: applying "will-change: scroll-position" CSS property onto the notifications container does not introduce observable performance improvement during scrolling.

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

Hi John, 

Here is my feedback:

1. There is still a fade on the 4th notification. The fade should start below the 4th notification once you start scrolling and the arrow disappears. See screenshot

2. [music player on lockscreen] There is a fade on the 2nd notification. The fade should start below the 2nd notification once you start scrolling and the arrow disappears. See screenshot

3. [music player on lockscreen] Please move the notifications 23px down so the two notifications are centered between the date/time and music player. See screenshot

4. When there are 4+ notifications and I scroll to the last notification at the bottom, the top should not have a fade. 4 notifications should be seen clearly with no fade (same as when there are only 4 notifications on screen). See screenshot

5. When you scroll midway or to the bottom of notifications and leave it in that state and there is a new notification, it should automatically scroll back to the top to show the new notification.

6. The hit area for the unlock toggle currently extends past the top of the toggle switch. Sometimes when I try to scroll up notifications, I accidentally activate the unlock toggle. Can you reduce the height of the hit area of the toggle switch?

Thanks

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

(In reply to Amy Lee [:amylee] from comment #51)
> Created attachment 8451771 [details]
> screensho_v2.png
> 
> Hi John, 
> 
> Here is my feedback:
>
> 3. [music player on lockscreen] Please move the notifications 23px down so
> the two notifications are centered between the date/time and music player.
> See screenshot

Hi Amy,

Please have a look at my screenshot: https://bug1023500.bugzilla.mozilla.org/attachment.cgi?id=8452164
Your lockscreen header (carrier info + date + time) is vertically larger than mine, as the carrier information of yours has two lines (extra "no SIM"), and that of mine has only one line since I have a valid SIM card. The notifications are vertically centered in the space determined by the lockscreen header with one-line carrier information, so it would look a bit misaligned when carrier information have two lines (additionally, the space between the descent of the first line and the baseline of the second line is right about 23px).

Since the normal everyday use is having a valid SIM card, I would think the current implementation is good.

Aside from issue 3, other issues have been addressed. Please check the attached patch. Thanks!

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

Hi John, 

Thanks for the explanation for the alignment. I'm good with that.

Here are my edits (hopefully this will be the last once these are resolved)

1. [Flame] When there are 4+ notifications and you scroll to the last notification, a hairline appears at the top. Please remove since we don’t have a hairline at the top when we have 4 or less notifications. See screenshot

2. [Flame] Lockscreen with music player and 2+ notifications. Same as above. Please remove top hairline. See screenshot

3. The arrow and hairline doesn’t disappear when you have music player and notifications on and the pin code comes up. This is also the same case for notifications with no music player. See screenshot. 

4. [Inari] There is an odd artifact that appears when there are 4+ notifications and you scroll to the bottom

5. The “w” in “just now” is cut off

I'm adding Rob for UI review since we are pretty close to final. Thanks for all your patience!

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

(In reply to Amy Lee [:amylee] from comment #54)
> Created attachment 8452544 [details]
> screenshots_3.png
> 
> Hi John, 
> 
> Thanks for the explanation for the alignment. I'm good with that.
> 
> Here are my edits (hopefully this will be the last once these are resolved)
> 
> 1. [Flame] When there are 4+ notifications and you scroll to the last
> notification, a hairline appears at the top. Please remove since we don’t
> have a hairline at the top when we have 4 or less notifications. See
> screenshot
> 
> 2. [Flame] Lockscreen with music player and 2+ notifications. Same as above.
> Please remove top hairline. See screenshot
> 
> 4. [Inari] There is an odd artifact that appears when there are 4+
> notifications and you scroll to the bottom

Issues 1, 2, and 4 have been resolved.

> 3. The arrow and hairline doesn’t disappear when you have music player and
> notifications on and the pin code comes up. This is also the same case for
> notifications with no music player. See screenshot. 

This has been resolved too.

> 5. The “w” in “just now” is cut off

Actually, I had observed this issue before too (on lower-resolution phones). But it seemed to have been resolved for a week or two, probably either by some newer Gecko build, or the newer font assets in bug 987872. If possible, I would recommend you flash the new font assets and latest Gecko build, but I'm not sure how UX team does that. For your information, here is what things look like on my lower-resolution phone: https://bug1023500.bugzilla.mozilla.org/attachment.cgi?id=8452969

> I'm adding Rob for UI review since we are pretty close to final. Thanks for
> all your patience!

Here we go for the ui-review request ;) Thanks everyone!

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

Hi Greg, here is the PR for v2.0 branch. Please review it for me.

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

Comment on attachment 8452973 [details]
Patch yet{4} again (PR @ GitHub)

Hi John, 

Just noticed a new inconsistent behavior with the updated patch. When there are 4+ notifications and you scroll to the last notification at the bottom, when a new notification pops up, all the notifications shifts up when the hairline arrow appears but when there are 2+ notifications when the music player is on, the notifications don't shift and just a hairline and arrow appears below. 

The behavior should be consistent in both scenarios. The notifications shouldn't shift up when the hairline and arrow appears. The hairline/arrow should just appear at the bottom and the notifications shouldn't shift up.

I will + the patch after this fix. Thanks!

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

Comment on attachment 8452978 [details]
v2.0 Patch (PR @ GitHub)

Something need to be adjusted. I think the most important is the patch make these two components coupling too strong. Unfortunately since John is OOO, I may need to take the patch and modify it on my own.

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

(In reply to Amy Lee [:amylee] from comment #58)
> Just noticed a new inconsistent behavior with the updated patch. When there
> are 4+ notifications and you scroll to the last notification at the bottom,
> when a new notification pops up, all the notifications shifts up when the
> hairline arrow appears but when there are 2+ notifications when the music
> player is on, the notifications don't shift and just a hairline and arrow
> appears below. 

Hi Amy,

I think what you're observing is actually this:

> 5. When you scroll midway or to the bottom of notifications and leave it in
> that state and there is a new notification, it should automatically scroll
> back to the top to show the new notification.

I think what you're seeing as some "shift-up" is because the "scroll back to top" mechanism scrolled a relatively short space such that you felt that things were erroneously shifting up.
If I'm mistaken about this, could you record a demo video for me? Thanks a lot.

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

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #59)
> Comment on attachment 8452978 [details]
> v2.0 Patch (PR @ GitHub)
> 
> Something need to be adjusted. I think the most important is the patch make
> these two components coupling too strong. Unfortunately since John is OOO, I
> may need to take the patch and modify it on my own.

I've done the decoupling. Please check the patch for v2.0, thanks.

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

(In reply to John Lu [:mnjul] [Out-of-Office 7/11~7/20] [MoCoTPE] from comment #60)
> (In reply to Amy Lee [:amylee] from comment #58)
> > Just noticed a new inconsistent behavior with the updated patch. When there
> > are 4+ notifications and you scroll to the last notification at the bottom,
> > when a new notification pops up, all the notifications shifts up when the
> > hairline arrow appears but when there are 2+ notifications when the music
> > player is on, the notifications don't shift and just a hairline and arrow
> > appears below. 
> 
> Hi Amy,
> 
> I think what you're observing is actually this:
> 
> > 5. When you scroll midway or to the bottom of notifications and leave it in
> > that state and there is a new notification, it should automatically scroll
> > back to the top to show the new notification.
> 
> I think what you're seeing as some "shift-up" is because the "scroll back to
> top" mechanism scrolled a relatively short space such that you felt that
> things were erroneously shifting up.
> If I'm mistaken about this, could you record a demo video for me? Thanks a
> lot.

Hi John, 

I made a video of what I'm talking about. It looks like the positioning of 4+ notifications (when you scroll to the bottom) sits lower than when there are only 4 notifications and that's what is causing the shift. 

https://www.youtube.com/watch?v=_VI1dzkaCew&feature=youtu.be

Also, I think you already mentioned that this issue will be resolved in another bug correct? When the notifications just disappear and reappear. 

https://www.youtube.com/watch?v=hZzvkitPjXw&feature=youtu.be

Thanks

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

Comment on attachment 8452973 [details]
Patch yet{4} again (PR @ GitHub)

Based on Amy's comments

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

Trying the latest patch but I'm having some problems downloading. I'll speak to Amy on Monday.

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

Since John now is OOO, it's clear that I need to take the patch and to land the v2.0 part ASAP. So I'll do:

1. I'll compare master and v2.0 patch to see if they can be the same patch, which can reduces the work I need to do, and the adjustments after UI review

2. Modify the patch according the incoming UI review, which Rob would post it later

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

Since the 2.0 patch is the refactored one, but the master patch is for ui-review. I may deprecate the master version and set the next UI review on the 2.0 patch. I would wait Rob's opinions to update the patch.

	howie [:howie]
	Comment 67 • 10 years ago

Hi Amy, after the initial discussion with Greg, the shifting behavior you mentioned in Comment 62 might related to gradient effect also the view port / position calculation, it might also differs on each devices with different screen resolution, it needs more time to investigate and it's not a trivial fix. The user impact of this should be very tiny, I would consider the improvement we did so far is very good enough to meet the unblock criteria.

Let's land the patch Greg currently have and close this bug. We'll look into the shifting thing as a followup bug 1038502 Greg had opened. Thank you!

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

Since we shouldn't review ourselves' code, I set review to Tim.
This one is based on the original patch, what I do is to resolve the conflicts.

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

The PR only failed at notification integration tests which already broken the test on the newest master:

https://travis-ci.org/mozilla-b2g/gaia/jobs/30045370

for example:

https://tbpl.mozilla.org/php/getParsedLog.php?id=43905360&tree=Gaia-Try
https://tbpl.mozilla.org/php/getParsedLog.php?id=43905039&tree=Gaia-Try
https://tbpl.mozilla.org/php/getParsedLog.php?id=43903623&tree=Gaia-Try

So I'll land it into master, and then adjust the v2.0 patch to land it, too.

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

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

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

Waiting for CI results.

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

2.0: https://github.com/mozilla-b2g/gaia/commit/ce33d6f07000cc43abd21e310ba93d8d04c81391

Let's see if this can keep in the tree. I'll continue to fix the shift notifications problem with other blockers.

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

As a memo about the notification: the Comment 54 make no change about the different alignment of the last line in the LockScreen header, which would make trouble for Bug 1043103 (the highlighted background would be under the date line).