1201397 - New layout of XUL Notifications

Status: RESOLVED FIXED

(Toolkit Graveyard :: Notifications and Alerts, defect)

Description

[Bill Maggs (bmaggs)]

Attachment: This is just to show the cog graphic. The message design is being redone.

The FF Windows user needs an actionable anchor to access an interface to turn off or turn down Push notifications for the sites the user has granted permission to send them. To do this currently the user must click on the message, then know where to go on the page to access Control Center, which exposes the permission to receive push notifications, as well as show notifications. It only works for one site, and if the user finds herself making a company presentation after allowing several website pages in FF to receive push notifications, it becomes a huge problem to manage them site by site. She needs an immediate way to get to the notification permissions in one place. 

A simple anchor would be something like a settings cog in the corner of every message on FF Windows (see attachment for concept, NOT finished mock). 

In FF 43 this cog would link to a centralized interface to manage Push notifications, described in Bug  

For FF 42, this cog graphic involves no strings and could possibly be uplifted  A more simple management interface than the one described in this bug would probably be necessary when the cog is clicked on.

Comment 1

[Chris Karlof [:ckarlof]]

Bill, based on what we saw from phlsa today in the product meeting, is this bug going to morph to visual refresh of our Web Notifications for Windows?

Comment 2

[Bill Maggs (bmaggs)]

This bug is now a refresh of what Web Notifications based on Push Notifications will look like on FF Windows (See Message format attachment).  It includes ability to change permission for the site, a Do Not Disturb button, and navigate to a centralized interface.

Comment 3

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Here's the link to the mocks/specs: https://invis.io/3A4A895XZ
It should update automatically.

The only thing that's still tbd. is a better close icon and the hover states for the active elements.
I'll attach assets to this bug once those are fixed.

Comment 4

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Animation proposal for notifications: http://phlsa.github.io/fx-notification-animation/index.html

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch showing new layout and animations, missing settings menu

Comment 6

[Bill Maggs (bmaggs)]

(In reply to Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from comment #3)
> Here's the link to the mocks/specs: https://invis.io/3A4A895XZ
> It should update automatically.
> 
> The only thing that's still tbd. is a better close icon and the hover states
> for the active elements.
> I'll attach assets to this bug once those are fixed.

Because favicons are out of scope, the bottom three design in phlsa's doc are what users will see in 44. 

For the close icon, what it shipping now has a nice circle that shows up on focus. Is that going to stay? I think it a big help. Phlsa's design is really nice and deatailed, but I think the close button needs a big target. 



> Animation proposal for notifications:
> http://phlsa.github.io/fx-notification-animation/index.html

the bounce is nice.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

This will need to be rebased when bug 1202933 lands, but the rebasing will have minor effects on the rest of the patch.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Note that with this new animation, the opaque window behind the notification is now very visible (it was always there just a bit harder to see). I will file a follow-up bug to request help from platform people in getting the window transparent. Some investigations there have shown that we may need something special for us to have a topmost window (dialog=yes) while also getting the window to be transparent. Code within nsWindow.cpp may be stopping transparent windows for dialogs and popups (bug 450322).

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8669178 [details] [diff] [review]
Patch

Review of attachment 8669178 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/alerts/resources/content/alert.js
@@ +8,5 @@
>  const NS_ALERT_HORIZONTAL = 1;
>  const NS_ALERT_LEFT = 2;
>  const NS_ALERT_TOP = 4;
>  
> +const WINDOW_MARGIN = 0;

Was this just for testing the menu behaviour?

@@ +63,5 @@
>      case 2:
>        document.getElementById('alertTitleLabel').setAttribute('value', window.arguments[1]);
>      case 1:
>        if (window.arguments[0]) {
> +        document.getElementById('alertImageBox').setAttribute('hasImage', true);

I thought I was reminded last week that the @src attribute mirrors the property so can't we check if the @src attribute exists in CSS instead?

@@ +194,5 @@
>  }
>  
> +function onAlertClick(event) {
> +  if (event.target &&
> +      event.target.id == "alertSettings") {

For consistency with the close button, couldn't you add @onclick="event.stopPropagation();" to the gear <button>?

@@ +216,5 @@
> +  let brandShortName = "";
> +  try {
> +    const BRAND_BUNDLE = Services.strings.createBundle(
> +      "chrome://branding/locale/brand.properties");
> +    brandShortName = BRAND_BUNDLE.GetStringFromName("brandShortName");

Is this actually something we need to support? I thought this came up on another bug recently with you or Gijs and I don't recall the outcome.

::: toolkit/components/alerts/resources/content/alert.xul
@@ +44,5 @@
> +        <box>
> +          <label id="alertViaOriginPre" class="plain" value="&viaOriginPre.label;"/>
> +          <label id="alertViaOrigin" value="example.com" class="plain"/>
> +          <label id="alertViaOriginPost" class="plain" value="&viaOriginPost.label;"/>
> +          <button type="menu" id="alertSettings" aria-label="&settings.label;"

This gear should only appear for web origin alerts. See the helper that kit added in bug 1202933. It should probably be exposed to JS via IDL. I guess you can use the added `case 11`. Can you remove the origin additions in your next pass and build on top of bug 1202933 or just leave this out? There is enough going on in this patch already.

Then we need to handle what a title-only alert looks like without an origin as we probably don't want to have a line under the title with empty space below it.

@@ +45,5 @@
> +          <label id="alertViaOriginPre" class="plain" value="&viaOriginPre.label;"/>
> +          <label id="alertViaOrigin" value="example.com" class="plain"/>
> +          <label id="alertViaOriginPost" class="plain" value="&viaOriginPost.label;"/>
> +          <button type="menu" id="alertSettings" aria-label="&settings.label;"
> +                  tooltiptext="&settings.label;" onpopupshowing="onSettingsMenuShowing();">

Is aria-label necessary if we use @tooltiptext? It seems redundant.

@@ +50,5 @@
> +            <menupopup position="after_end">
> +              <menuitem id="doNotDisturbMenuItem"/>
> +              <menuseparator/>
> +              <menuitem id="disableForOriginMenuItem"/>
> +              <menuitem label="&moreOptions.label;"/>

Oh, we had separate bugs for the gear menu (items) (bug 1209602 and bug 1205172) which would have made this easier to review therefore allow it to land faster IMO. I think since I see enough issues/questions with this patch that I think it makes sense to move the gear stuff to it's own patch or bug (one of the ones mentioned above).

::: toolkit/locales/en-US/chrome/alerts/alert.dtd
@@ +10,5 @@
> +     between viaOriginPre.label and viaOriginPost.label. Please include
> +     a space either at the end of viaOriginPre.label or at the beginning
> +     of viaOriginPost.label. -->
> +<!ENTITY     viaOriginPre.label          "via ">
> +<!ENTITY     viaOriginPost.label         "">

This is handled by bug 1202933 now.

::: toolkit/themes/shared/alert-common.css
@@ +38,5 @@
>  
>  #alertImage {
>    max-width: 48px;
>    max-height: 48px;
>    list-style-image: url(chrome://global/skin/alerts/notification-48.png);

Isn't this list-style-image now unused?

::: toolkit/themes/windows/global/alerts/alert.css
@@ +12,5 @@
>  
> +#alertBox {
> +  border: 1px solid rgba(107,107,107,.4);
> +  border-radius: 1px;
> +  background-color: rgba(255,255,255,.9);

This may not work with high-contrast mode or customized Windows colors since you're not setting the foreground colour with the background. I think we should use the old values unless the user has the default windows theme and when we specify the background we should be specifying a contrasting foreground color too.

FWIW I personally think we should stick with the system colours anyways.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Matthew N. [:MattN] (behind on mail) from comment #9)
> Comment on attachment 8669178 [details] [diff] [review]
> Patch
> 
> Review of attachment 8669178 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/alerts/resources/content/alert.js
> @@ +8,5 @@
> >  const NS_ALERT_HORIZONTAL = 1;
> >  const NS_ALERT_LEFT = 2;
> >  const NS_ALERT_TOP = 4;
> >  
> > +const WINDOW_MARGIN = 0;
> 
> Was this just for testing the menu behaviour?

No, we need to have a 0 margin here because we want the animation to come in from the edge of the display. The previous margin would cause the animation to start 10 pixels from the edge (right and bottom).

> @@ +63,5 @@
> >      case 2:
> >        document.getElementById('alertTitleLabel').setAttribute('value', window.arguments[1]);
> >      case 1:
> >        if (window.arguments[0]) {
> > +        document.getElementById('alertImageBox').setAttribute('hasImage', true);
> 
> I thought I was reminded last week that the @src attribute mirrors the
> property so can't we check if the @src attribute exists in CSS instead?

OK I'll look in to that.

> @@ +194,5 @@
> >  }
> >  
> > +function onAlertClick(event) {
> > +  if (event.target &&
> > +      event.target.id == "alertSettings") {
> 
> For consistency with the close button, couldn't you add
> @onclick="event.stopPropagation();" to the gear <button>?

Good idea.

> @@ +216,5 @@
> > +  let brandShortName = "";
> > +  try {
> > +    const BRAND_BUNDLE = Services.strings.createBundle(
> > +      "chrome://branding/locale/brand.properties");
> > +    brandShortName = BRAND_BUNDLE.GetStringFromName("brandShortName");
> 
> Is this actually something we need to support? I thought this came up on
> another bug recently with you or Gijs and I don't recall the outcome.

Yes, we need to support this. The question came up and Philipp replied that we should keep "Firefox" in the string so people aren't confused and think that they may need to restart their computer to reset the pref.

> ::: toolkit/components/alerts/resources/content/alert.xul
> @@ +44,5 @@
> > +        <box>
> > +          <label id="alertViaOriginPre" class="plain" value="&viaOriginPre.label;"/>
> > +          <label id="alertViaOrigin" value="example.com" class="plain"/>
> > +          <label id="alertViaOriginPost" class="plain" value="&viaOriginPost.label;"/>
> > +          <button type="menu" id="alertSettings" aria-label="&settings.label;"
> 
> This gear should only appear for web origin alerts. See the helper that kit
> added in bug 1202933. It should probably be exposed to JS via IDL. I guess
> you can use the added `case 11`. Can you remove the origin additions in your
> next pass and build on top of bug 1202933 or just leave this out? There is
> enough going on in this patch already.
> 
> Then we need to handle what a title-only alert looks like without an origin
> as we probably don't want to have a line under the title with empty space
> below it.

OK.

> @@ +45,5 @@
> > +          <label id="alertViaOriginPre" class="plain" value="&viaOriginPre.label;"/>
> > +          <label id="alertViaOrigin" value="example.com" class="plain"/>
> > +          <label id="alertViaOriginPost" class="plain" value="&viaOriginPost.label;"/>
> > +          <button type="menu" id="alertSettings" aria-label="&settings.label;"
> > +                  tooltiptext="&settings.label;" onpopupshowing="onSettingsMenuShowing();">
> 
> Is aria-label necessary if we use @tooltiptext? It seems redundant.

Well, to be honest, Gijs has told me that the a11y of these notifications is really bad, in that they aren't announced to screen readers. I think he was going to verify that when he was at his a11y workweek. Gijs, did you get that confirmed?

> @@ +50,5 @@
> > +            <menupopup position="after_end">
> > +              <menuitem id="doNotDisturbMenuItem"/>
> > +              <menuseparator/>
> > +              <menuitem id="disableForOriginMenuItem"/>
> > +              <menuitem label="&moreOptions.label;"/>
> 
> Oh, we had separate bugs for the gear menu (items) (bug 1209602 and bug
> 1205172) which would have made this easier to review therefore allow it to
> land faster IMO. I think since I see enough issues/questions with this patch
> that I think it makes sense to move the gear stuff to it's own patch or bug
> (one of the ones mentioned above).

OK, this was confusing since the summary for this bug included "with a dropdown". I would like to leave the gear as part of this patch since it affects the styling, but I can move the menupopup code out if you still desire. To me the popup code is pretty simple and it hasn't been fleshed out (just the menuitems and they do nothing at this point).

> ::: toolkit/locales/en-US/chrome/alerts/alert.dtd
> @@ +10,5 @@
> > +     between viaOriginPre.label and viaOriginPost.label. Please include
> > +     a space either at the end of viaOriginPre.label or at the beginning
> > +     of viaOriginPost.label. -->
> > +<!ENTITY     viaOriginPre.label          "via ">
> > +<!ENTITY     viaOriginPost.label         "">
> 
> This is handled by bug 1202933 now.

That is unfortunate. We may want a separate bug then, since the spec called for "via" to be italicized but the origin to have normal styling.

> ::: toolkit/themes/shared/alert-common.css
> @@ +38,5 @@
> >  
> >  #alertImage {
> >    max-width: 48px;
> >    max-height: 48px;
> >    list-style-image: url(chrome://global/skin/alerts/notification-48.png);
> 
> Isn't this list-style-image now unused?

It appears so. I can remove it.

> ::: toolkit/themes/windows/global/alerts/alert.css
> @@ +12,5 @@
> >  
> > +#alertBox {
> > +  border: 1px solid rgba(107,107,107,.4);
> > +  border-radius: 1px;
> > +  background-color: rgba(255,255,255,.9);
> 
> This may not work with high-contrast mode or customized Windows colors since
> you're not setting the foreground colour with the background. I think we
> should use the old values unless the user has the default windows theme and
> when we specify the background we should be specifying a contrasting
> foreground color too.
> 
> FWIW I personally think we should stick with the system colours anyways.

OK, I'll switch this back to platform colors by default but use the spec'd colors if the default theme is being used.

Comment 11

[Lina Butler [:lina]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #10)
> That is unfortunate. We may want a separate bug then, since the spec called
> for "via" to be italicized but the origin to have normal styling.

Oops, sorry about that. Should I take care of that now, or will it cause even more churn for your patch?

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

We can leave it as-is for now and file a follow-up to tweak it if necessary. As it was designed, it is one word that is in italics and then a hostname that is not, which may looks awkward since the italicized word is such a small percentage of the sentence fragment that it's balance may look like a graphical glitch.

Comment 13

[Matthew N. [:MattN]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #10)
> (In reply to Matthew N. [:MattN] (behind on mail) from comment #9)
> > Review of attachment 8669178 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: toolkit/components/alerts/resources/content/alert.js
> > @@ +8,5 @@
> > >  const NS_ALERT_HORIZONTAL = 1;
> > >  const NS_ALERT_LEFT = 2;
> > >  const NS_ALERT_TOP = 4;
> > >  
> > > +const WINDOW_MARGIN = 0;
> > 
> > Was this just for testing the menu behaviour?
> 
> No, we need to have a 0 margin here because we want the animation to come in
> from the edge of the display. The previous margin would cause the animation
> to start 10 pixels from the edge (right and bottom).

Oh, ok, I was mostly thinking about the vertical offset since IIRC we had talked about how the menupopup would handle being close to the bottom of the screen.

> > @@ +216,5 @@
> > > +  let brandShortName = "";
> > > +  try {
> > > +    const BRAND_BUNDLE = Services.strings.createBundle(
> > > +      "chrome://branding/locale/brand.properties");
> > > +    brandShortName = BRAND_BUNDLE.GetStringFromName("brandShortName");
> > 
> > Is this actually something we need to support? I thought this came up on
> > another bug recently with you or Gijs and I don't recall the outcome.
> 
> Yes, we need to support this. The question came up and Philipp replied that
> we should keep "Firefox" in the string so people aren't confused and think
> that they may need to restart their computer to reset the pref.

To clarify, I was wondering if we need to handle an exception being thrown in the code to get the bundle and get the brandShortName string?

> > @@ +45,5 @@
> > > +          <label id="alertViaOriginPre" class="plain" value="&viaOriginPre.label;"/>
> > > +          <label id="alertViaOrigin" value="example.com" class="plain"/>
> > > +          <label id="alertViaOriginPost" class="plain" value="&viaOriginPost.label;"/>
> > > +          <button type="menu" id="alertSettings" aria-label="&settings.label;"
> > > +                  tooltiptext="&settings.label;" onpopupshowing="onSettingsMenuShowing();">
> > 
> > Is aria-label necessary if we use @tooltiptext? It seems redundant.
> 
> Well, to be honest, Gijs has told me that the a11y of these notifications is
> really bad, in that they aren't announced to screen readers. I think he was
> going to verify that when he was at his a11y workweek. Gijs, did you get
> that confirmed?

Yeah, we talked about this before and during last week. Gijs already landed a fix in bug 1052776. I have a list of other bugs to file to make them more accessible.

> > @@ +50,5 @@
> > > +            <menupopup position="after_end">
> > > +              <menuitem id="doNotDisturbMenuItem"/>
> > > +              <menuseparator/>
> > > +              <menuitem id="disableForOriginMenuItem"/>
> > > +              <menuitem label="&moreOptions.label;"/>
> > 
> > Oh, we had separate bugs for the gear menu (items) (bug 1209602 and bug
> > 1205172) which would have made this easier to review therefore allow it to
> > land faster IMO. I think since I see enough issues/questions with this patch
> > that I think it makes sense to move the gear stuff to it's own patch or bug
> > (one of the ones mentioned above).
> 
> OK, this was confusing since the summary for this bug included "with a
> dropdown".

Sorry about that. I updated the title in the spreadsheet but it didn't get back to the bug and the spreadsheet isn't using my script to fetch bug metadata from BMO so we don't have manual duplication.

> I would like to leave the gear as part of this patch since it
> affects the styling, but I can move the menupopup code out if you still
> desire. To me the popup code is pretty simple and it hasn't been fleshed out
> (just the menuitems and they do nothing at this point).

OK, removing the menupopup would help. Could you land the gear with visibility:hidden though so it doesn't land with no functionality? Otherwise we could have it call alertsettingscallback oncommand but we shouldn't put too much work into that since it will change to a popup.

> > ::: toolkit/locales/en-US/chrome/alerts/alert.dtd
> > @@ +10,5 @@
> > > +     between viaOriginPre.label and viaOriginPost.label. Please include
> > > +     a space either at the end of viaOriginPre.label or at the beginning
> > > +     of viaOriginPost.label. -->
> > > +<!ENTITY     viaOriginPre.label          "via ">
> > > +<!ENTITY     viaOriginPost.label         "">
> > 
> > This is handled by bug 1202933 now.
> 
> That is unfortunate. We may want a separate bug then, since the spec called
> for "via" to be italicized but the origin to have normal styling.

I see a warning was recently added to https://mozilla.invisionapp.com/share/3A4A895XZ#/screens/104465680 "…visual style in this comp. is outdated." and that page had both in italics. We can fix this in a follow-up for bug 1202933 or you can build atop bug 1202933 here (ideally in a separate patch or bug though).

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch (rebased on top of bug 1202933 and minus the settings popup code)

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2.01

Comment 16

[Matthew N. [:MattN]]

I'll take a look at this tomorrow since I didn't get to it today.

Comment 17

[Matthew N. [:MattN]]

Comment on attachment 8670537 [details] [diff] [review]
Patch v2.01

Review of attachment 8670537 [details] [diff] [review]:
-----------------------------------------------------------------

The patch itself looks fine so I was going to give r+ until I tested the patch and found a few issues:

The image doesn't seem to appear (on OS X) with this patch.

The other main issue is that the panel doesn't grow vertically with the alert body so things like the origin and gear would be missing. You can test with multi-line input on https://people.mozilla.org/~mnoorenberghe/w3c_notifications.htm

Also, the gear isn't right-aligned with a long title: http://grab.by/L2ly (though there is an existing bug to make that wrap at some length).

::: toolkit/locales/en-US/chrome/alerts/alert.properties
@@ -15,1 @@
>  source.label=via %1$S

Is deleting the localization note intentional?

::: toolkit/themes/osx/global/alerts/alert.css
@@ +27,5 @@
>    text-shadow: 0 1px white;
>  }
>  
> +#alertBox[animate] {
> +  animation-duration: 5.2s;

The duration is already defined in the content stylesheet. Why override it here?

@@ +69,5 @@
> +}
> +
> +@keyframes alert-fadeout-animation {
> +  to {
> +    transform: translate(0, 60px) rotate(15deg);

At least on OS X, the rotatation gets clipped on the bottom by a line parallel to the bottom of the screen. The weird things is that the zoom animation doesn't get clipped in this same way so maybe it's a graphics/widget bug?

Also, there is a flicker on OS X 10.10 but that's probably the same known issue as doorhangers flickering on 10.10 (not earlier versions) and XUL notifications should only be used on OS X 10.6-10.8.

::: toolkit/themes/shared/alert-common.css
@@ +15,5 @@
> +  padding-inline-start: 90px;
> +}
> +
> +#alertBox[hasOrigin] > #alertTitleBox {
> +  border-bottom: 1px solid ThreeDShadow;

We don't want a border for non-web alerts? Don't we want do do this only if the body text is empty?

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v3

Sorry, I didn't mean to delete the localization note. Tested with and without images, and fixed the line wrapping of the title text, as well as the width of the body text.

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v3.1

Comment 20

[Matthew N. [:MattN]]

Comment on attachment 8672092 [details] [diff] [review]
Patch v3.1

Review of attachment 8672092 [details] [diff] [review]:
-----------------------------------------------------------------

This is really close but the clipping issue might need a few changes which I'd like to look at.

(In reply to Matthew N. [:MattN] (PTO Oct. 8-12) from comment #17)
> The other main issue is that the panel doesn't grow vertically with the
> alert body so things like the origin and gear would be missing. You can test
> with multi-line input on
> https://people.mozilla.org/~mnoorenberghe/w3c_notifications.htm

There are still possibly related problems (at least on OS X). See http://grab.by/LaPs / http://grab.by/LaPW / http://grab.by/LaQ2

> @@ +69,5 @@
> > +}
> > +
> > +@keyframes alert-fadeout-animation {
> > +  to {
> > +    transform: translate(0, 60px) rotate(15deg);
> 
> At least on OS X, the rotation gets clipped on the bottom by a line
> parallel to the bottom of the screen. The weird things is that the zoom
> animation doesn't get clipped in this same way so maybe it's a
> graphics/widget bug?

Does this also happen on Windows? If so, we should probably get this filed and/or change the animation.

::: toolkit/components/alerts/resources/content/alert.js
@@ +40,3 @@
>        if (window.arguments[10]) {
> +        document.getElementById('alertBox').setAttribute('hasOrigin', true);
> +        document.getElementById('alertSourceLabel').setAttribute('value', window.arguments[10]);

Reminder to switch the additions in this file to double-quotes

::: toolkit/themes/shared/alert-common.css
@@ +27,5 @@
>  
>  .alertTextBox {
> +  padding-top: 8px;
> +  padding-inline-start: 8px;
> +  width: 319px;

This number seems quite specific (e.g. why not 320?). Does it deserve a comment?

Comment 21

[Matthew N. [:MattN]]

Design feedback: Title-only chrome notifications look really bare. I like the old megaphone default icon and wish we would keep it… See http://grab.by/LaVa

Comment 22

[Matthew N. [:MattN]]

Or at least have some min-height or something

Comment 23

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Matthew N. [:MattN] (PTO Oct. 8-12) from comment #21)
> Design feedback: Title-only chrome notifications look really bare. I like
> the old megaphone default icon and wish we would keep it… See
> http://grab.by/LaVa

Well, in that screenshot the entire section with the origin and the options button is missing, which would make the whole thing twice as high. See here: https://mozilla.invisionapp.com/static-signed/live-embed/21067263/104465680/4/latest/N1kCDQGGgwOxudaWEYlEuEXpylEez1LHDsJtAnlhnBO1qAPWoNjJreCWmKz7zsgQ2dQpcnlEGA0a2leNK0JTEglE/Notification-variations.png

Comment 24

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from comment #23)
> (In reply to Matthew N. [:MattN] (PTO Oct. 8-12) from comment #21)
> > Design feedback: Title-only chrome notifications look really bare. I like
> > the old megaphone default icon and wish we would keep it… See
> > http://grab.by/LaVa
> 
> Well, in that screenshot the entire section with the origin and the options
> button is missing, which would make the whole thing twice as high. See here:
> https://mozilla.invisionapp.com/static-signed/live-embed/21067263/104465680/
> 4/latest/
> N1kCDQGGgwOxudaWEYlEuEXpylEez1LHDsJtAnlhnBO1qAPWoNjJreCWmKz7zsgQ2dQpcnlEGA0a2
> leNK0JTEglE/Notification-variations.png

That's correct, but there is no origin or relevant options button for non-web-based notifications such as a Firefox update.

Comment 25

[Matthew N. [:MattN]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #24)
> (In reply to Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from
> comment #23)
> > (In reply to Matthew N. [:MattN] (PTO Oct. 8-12) from comment #21)
> > > Design feedback: Title-only chrome notifications look really bare. I like
> > > the old megaphone default icon and wish we would keep it… See
> > > http://grab.by/LaVa
> > 
> > Well, in that screenshot the entire section with the origin and the options
> > button is missing, which would make the whole thing twice as high. See here:
> > https://mozilla.invisionapp.com/static-signed/live-embed/21067263/104465680/
> > 4/latest/
> > N1kCDQGGgwOxudaWEYlEuEXpylEez1LHDsJtAnlhnBO1qAPWoNjJreCWmKz7zsgQ2dQpcnlEGA0a2
> > leNK0JTEglE/Notification-variations.png
> 
> That's correct, but there is no origin or relevant options button for
> non-web-based notifications such as a Firefox update.

No, this is a "chrome" notifications meaning it's not from a web origin and so the gear and origin aren't relevant.

Comment 26

[Matthew N. [:MattN]]

(In reply to Matthew N. [:MattN] (PTO Oct. 8-12) from comment #25)
> No, this is a "chrome" notifications meaning it's not from a web origin and
> so the gear and origin aren't relevant.

Oops, this was meant as a reply to comment 23

Comment 27

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v3.2

I fixed the overflow problem on OSX. There were some style changes that were made for Windows that didn't make their way over to OSX. I used a diff tool to compare the two CSS files.

The fade-out animation bottom will still clip on OSX. This is due to the size of the window that the alert appears in. We could make the window larger, but that will further decrease the number of alerts that we show on the screen at the same time. We may want to tweak the animation here. I would prefer to land this as-is and then iterate on the animation in a separate bug.

The zoom animation is unaffected because it fades out fast enough and the padding around the box is enough that it is faded out before it goes past the boundaries of the window.

The change from single- to double-quotes will happen in the bug that is set aside for that (bug 1214029) and applies on top of this patch.

MattN and I discussed over IRC that the question of what to do with chrome notifications that lack body text should not hold up this bug.

Comment 29

[Matthew N. [:MattN]]

Comment on attachment 8674358 [details] [diff] [review]
Patch v3.2

Review of attachment 8674358 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #27)
> I fixed the overflow problem on OSX. There were some style changes that were
> made for Windows that didn't make their way over to OSX. I used a diff tool
> to compare the two CSS files.

Oh, ok, thanks.

> The fade-out animation bottom will still clip on OSX. This is due to the
> size of the window that the alert appears in. We could make the window
> larger, but that will further decrease the number of alerts that we show on
> the screen at the same time. We may want to tweak the animation here. I
> would prefer to land this as-is and then iterate on the animation in a
> separate bug.

So this doesn't happen on Windows or Linux?

> MattN and I discussed over IRC that the question of what to do with chrome
> notifications that lack body text should not hold up this bug.

We should make sure the bug gets in the backlog spreadsheet once filed.

::: toolkit/themes/shared/alert-common.css
@@ +28,5 @@
>  .alertTextBox {
> +  padding-top: 8px;
> +  padding-inline-start: 8px;
> +  /* 319px is 255px (the text box width when a picture is present [255px] + the
> +     width of the picture [64px]). The text box width is increased to make up

This first sentence needs some cleanup with the braces and to not mention 255px twice.

Comment 30

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Matthew N. [:MattN] (PTO Oct. 8-12) from comment #29)
> > The fade-out animation bottom will still clip on OSX. This is due to the
> > size of the window that the alert appears in. We could make the window
> > larger, but that will further decrease the number of alerts that we show on
> > the screen at the same time. We may want to tweak the animation here. I
> > would prefer to land this as-is and then iterate on the animation in a
> > separate bug.
> 
> So this doesn't happen on Windows or Linux?

It is more obvious on OSX since the first notification appears at the top of the screen as compared to Windows where it appears at the bottom of the screen and could be assumed to go behind the taskbar.

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/6ecebbea7718

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6ecebbea7718

Comment 33

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Matthew N. [:MattN] from comment #25)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #24)
> > (In reply to Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from
> > comment #23)
> > > (In reply to Matthew N. [:MattN] (PTO Oct. 8-12) from comment #21)
> > > > Design feedback: Title-only chrome notifications look really bare. I like
> > > > the old megaphone default icon and wish we would keep it… See
> > > > http://grab.by/LaVa
> > > 
> > > Well, in that screenshot the entire section with the origin and the options
> > > button is missing, which would make the whole thing twice as high. See here:
> > > https://mozilla.invisionapp.com/static-signed/live-embed/21067263/104465680/
> > > 4/latest/
> > > N1kCDQGGgwOxudaWEYlEuEXpylEez1LHDsJtAnlhnBO1qAPWoNjJreCWmKz7zsgQ2dQpcnlEGA0a2
> > > leNK0JTEglE/Notification-variations.png
> > 
> > That's correct, but there is no origin or relevant options button for
> > non-web-based notifications such as a Firefox update.
> 
> No, this is a "chrome" notifications meaning it's not from a web origin and
> so the gear and origin aren't relevant.

Sorry, I've been on PTO…
The notification in your screen shot doesn't seem to be very useful to me. If we were to make it useful (by, say, adding a body text that said »Click to update Firefox to the latest version«), it would look a lot less weird. It would also make sense to use the Firefox icon as the image for this one.
Generally, non-web notifications are an exception that we fully control, so I'd prefer to go on a case by case basis here.

Comment 34

[Matthew N. [:MattN]]

(In reply to Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from comment #33)
> Generally, non-web notifications are an exception that we fully control, so
> I'd prefer to go on a case by case basis here.

Unfortunately we don't control extension developers…

Comment 35

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Matthew N. [:MattN] from comment #34)
> (In reply to Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from
> comment #33)
> > Generally, non-web notifications are an exception that we fully control, so
> > I'd prefer to go on a case by case basis here.
> 
> Unfortunately we don't control extension developers…

Right, I didn't think of those…

Let's do this then: We show the footer even for non-push notifications, but without the origin. In the gear menu, we hide the »Block notifications from <site name>« item (toggling DND and going to prefs can still be useful).
That way these notifications would also have a decent size.