Closed Bug 1267604 Opened 8 years ago Closed 8 years ago

Restyle doorhanger notifications

Categories

(Firefox :: Theme, defect, P1)

Product:
Firefox
Component:
Theme
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 53
Iteration:
51.1 - Aug 15
Tracking Flags:
Tracking Status
firefox49 --- affected
firefox53 --- fixed

People

(Reporter: Paolo, Assigned: jkt)

References

(Blocks 1 open bug,
URL
)

Details

(Whiteboard: [fxprivacy][landed-on-elm])

Attachments

(4 files, 5 obsolete files)

password popup
55.54 KB, image/png
Details
Bug 1267604 - Permission prompt doorhanger restyle
58 bytes, text/x-review-board-request
Paolo
: review+
Details
Bug 1267604 - Permission prompt doorhanger restyle.
58 bytes, text/x-review-board-request
Details
Bug 1267604 - Restyle doorhanger notifications.
58 bytes, text/x-review-board-request
Paolo
: review+
Details 
The doorhanger notifications should be restyled according to the visual design in the URL field.

This bug will affect all the doorhanger notifications at once, but no changes will be made to the placement of the buttons or functionality offered, and there will still be a drop-down submenu for implementing additional choices.

The submenu will be moved to a drop-down arrow linked to the main button in the footer of the notification. The other area of the footer will remain inactive.

Notification with custom user interface may need to keep parts of the old styling until they are individually edited.
(In reply to :Paolo Amadini from comment #0)
> The submenu will be moved to a drop-down arrow linked to the main button in
> the footer of the notification. The other area of the footer will remain
> inactive.

I don't see a design for this. Am I missing something?

Was this design reviewed for accessibility, which has been a long-standing issue with the existing notifications as far as the options dropdown is concerned? See bug 653226 for a lot of discussion about this. We don't need to fix this here necessarily, but it would be good not to make things worse - which is hard to judge when there is no design...
Flags: needinfo?(paolo.mozmail)
This is not intended as the end state, just a step towards the full implementation of the work in the design found in the URL field. We have to split the work in smaller chunks somehow and making the style changes in one bug I think makes the most sense.
Assignee: nobody → jkingston
Status: NEW → ASSIGNED
Flags: qe-verify?
Priority: -- → P2
Flags: needinfo?(paolo.mozmail)
I won't be able to change the base colour of the notification unless there is a graphics change due to OSX using a png for the arrow:
https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/arrow/panelarrow-vertical.png
The other browsers use SVG's for this should I change those?

The other changes are nearly complete.
ashgrigas: mentioned you would be able to provide resources for the above. :)
Flags: needinfo?(bbell)
(In reply to Jonathan Kingston [:kingstonTime] from comment #4)
> ashgrigas: mentioned you would be able to provide resources for the above. :)

I would love to provide what ever you need... I'm just a little confused. 

Are we looking to change this carrot at the top of the doorhanger? http://cl.ly/2Q2B1L3w3G1J
Flags: needinfo?(bbell) → needinfo?(jkingston)
> https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/
> arrow/panelarrow-vertical.png

this image is broken or missing.
Yeah bbell the carrot :D. The image is a little hard to see here is another link:
https://dxr.mozilla.org/mozilla-central/raw/toolkit/themes/osx/global/arrow/panelarrow-vertical.png

Assuming you want the same shape as the other OS and the same shadows I can probably convert it if there is an issue.
The image does use #fcfcfc which is different from the SVGs of other operating systems so I could perhaps just use that instead of making it pure white.

So I guess the question is what colour should the doorhangers be and does it matter if we use the current #fcfcfc on osx, I can change the others to match even though I used #fff.

Thanks!
Flags: needinfo?(jkingston)
Attachment #8751544 - Attachment is obsolete: true
(In reply to Jonathan Kingston [:kingstonTime] from comment #12)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a828318a0a9

Hey Johnathan, if you want to request screenshots of the permission prompt thanks to bug 1268619 (make sure you have rebased on top of this) you can use the following syntax:

try: -b o -p linux,linux64,macosx64,win32,win64 -u mochitest-browser-screenshots[Ubuntu,10.10,Windows XP,Windows 7,Windows 8] -t none --setenv MOZSCREENSHOTS_SETS=LightweightThemes,PermissionPrompts

(You can't just request this from TreeHerder due to the environment variable needed)

See https://developer.mozilla.org/en-US/docs/Mozilla/QA/Browser_screenshots for more details on downloading/comparing the screenshots.
Attachment #8755123 - Attachment is obsolete: true
Attachment #8755587 - Attachment is obsolete: true
(In reply to Matthew N. [:MattN] (behind on reviews) from comment #13)
> (In reply to Jonathan Kingston [:kingstonTime] from comment #12)
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a828318a0a9
> 
> Hey Johnathan, if you want to request screenshots of the permission prompt
> thanks to bug 1268619 (make sure you have rebased on top of this) you can
> use the following syntax:
> 
> try: -b o -p linux,linux64,macosx64,win32,win64 -u
> mochitest-browser-screenshots[Ubuntu,10.10,Windows XP,Windows 7,Windows 8]
> -t none --setenv MOZSCREENSHOTS_SETS=LightweightThemes,PermissionPrompts
> 
> (You can't just request this from TreeHerder due to the environment variable
> needed)
> 
> See https://developer.mozilla.org/en-US/docs/Mozilla/QA/Browser_screenshots
> for more details on downloading/comparing the screenshots.

Wow :MattN thank you!

The only change I had to make was quote the windows tests:

try -b o -p linux,linux64,macosx64,win32,win64 -u mochitest-browser-screenshots[Ubuntu,10.10,"Windows XP","Windows 7","Windows 8"] -t none --setenv MOZSCREENSHOTS_SETS=LightweightThemes,PermissionPrompts

Else I get:

Error parsing -u argument (['mochitest-browser-screenshots[Ubuntu,10.10,Windows']):
(In reply to Jonathan Kingston [:kingstonTime] from comment #16)
> (In reply to Matthew N. [:MattN] (behind on reviews) from comment #13)
> > (In reply to Jonathan Kingston [:kingstonTime] from comment #12)
> > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a828318a0a9
> > 
> > Hey Johnathan, if you want to request screenshots of the permission prompt
> > thanks to bug 1268619 (make sure you have rebased on top of this) you can
> > use the following syntax:
> > 
> > try: -b o -p linux,linux64,macosx64,win32,win64 -u
> > mochitest-browser-screenshots[Ubuntu,10.10,Windows XP,Windows 7,Windows 8]
> > -t none --setenv MOZSCREENSHOTS_SETS=LightweightThemes,PermissionPrompts
> > 
> > (You can't just request this from TreeHerder due to the environment variable
> > needed)
> > 
> > See https://developer.mozilla.org/en-US/docs/Mozilla/QA/Browser_screenshots
> > for more details on downloading/comparing the screenshots.
> 
> Wow :MattN thank you!

You can see the results on the SS jobs at https://treeherder.mozilla.org/#/jobs?repo=try&revision=fee5e8c46cb6c610b27490ed0b6db52fbee7a289&exclusion_profile=false&filter-tier=1&filter-tier=2&filter-tier=3&selectedJob=21610453

They comparison doesn't work properly on https://screenshots.mattn.ca/compare/?oldProject=try&oldRev=dd72e0596d6bbf0c0c55fe1b50cb7464ec216a93&newProject=try&newRev=fee5e8c46cb6c610b27490ed0b6db52fbee7a289 yet due to a change I made yesterday which treats try jobs differently. I'll hopefully fix that this weekend.

> The only change I had to make was quote the windows tests:
> 
> try -b o -p linux,linux64,macosx64,win32,win64 -u
> mochitest-browser-screenshots[Ubuntu,10.10,"Windows XP","Windows 7","Windows
> 8"] -t none --setenv MOZSCREENSHOTS_SETS=LightweightThemes,PermissionPrompts
> 
> Else I get:
> 
> Error parsing -u argument
> (['mochitest-browser-screenshots[Ubuntu,10.10,Windows']):

I assume you got this when using `mach try`? I don't use that so I normally quote the whole string with `hg trychooser -m "…"`. I guess this is also a bug in the trychooser website since it doesn't have quotes there: http://trychooser.pub.build.mozilla.org/
Comment on attachment 8757433 [details]
Bug 1267604 - Permission prompt doorhanger restyle

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55868/diff/1-2/
Attachment #8757433 - Attachment description: MozReview Request: Bug 1267604 - Permission prompt doorhanger restyle → Bug 1267604 - Permission prompt doorhanger restyle
Iteration: --- → 49.3 - Jun 6
Priority: P2 → P1
Comment on attachment 8757433 [details]
Bug 1267604 - Permission prompt doorhanger restyle

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55868/diff/2-3/
Comment on attachment 8757433 [details]
Bug 1267604 - Permission prompt doorhanger restyle

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55868/diff/3-4/
Comment on attachment 8757433 [details]
Bug 1267604 - Permission prompt doorhanger restyle

Hey Paolo,

I appreciate this is a big patch and in important places, however some initial feedback would be great on this or pass it onto someone else that could.


Thanks
Jonathan
Attachment #8757433 - Flags: review?(paolo.mozmail)
Comment on attachment 8757433 [details]
Bug 1267604 - Permission prompt doorhanger restyle

(In reply to Jonathan Kingston [:kingstonTime] from comment #23)
> I appreciate this is a big patch and in important places, however some
> initial feedback would be great on this or pass it onto someone else that
> could.

Adding Jared because I don't know when I'll be able to get to this review in detail.

I've looked at the patch briefly and the general approach looks good to me. There might be general CSS considerations that Jared might catch and I might miss though.
Attachment #8757433 - Flags: review?(jaws)
Attachment #8757433 - Flags: feedback+
Iteration: 49.3 - Jun 6 → ---
Priority: P1 → P2
Comment on attachment 8757433 [details]
Bug 1267604 - Permission prompt doorhanger restyle

Redirecting to adw per IRC.
Attachment #8757433 - Flags: review?(jaws) → review?(adw)
Attachment #8757433 - Flags: review?(paolo.mozmail)
Attached image password popup — 
Sorry it took so long for me to look at this.  The code looks OK to me, but there are a couple of problems I noticed when I tested the patch, so I don't think I can r+ it yet.

Buttons are way too tall.  It doesn't look like the height matches the spec.

When a popup contains more than one paragraph of text or other widgets, all the ones after the first are shifted right, as if they're being indented.  I saw this with the password popup and the pointer lock popup, and I didn't test beyond those.

This attached screenshot shows both problems.
Attachment #8757433 - Flags: review?(adw)
Comment on attachment 8757433 [details]
Bug 1267604 - Permission prompt doorhanger restyle

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55868/diff/4-5/
Attachment #8757433 - Flags: feedback+
Comment on attachment 8757433 [details]
Bug 1267604 - Permission prompt doorhanger restyle

This should address your comments :adw. Let me know if there is anything else you need.

The screenshots of the latest changes can be seen here: https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=3e8ee3599a67edd971770af4982ad4b0fe77f073&newProject=try&newRev=9a57c612657f40841e4b2c193485f6fab74c0e89


Thanks :)
Attachment #8757433 - Flags: review?(adw)
Comment on attachment 8757433 [details]
Bug 1267604 - Permission prompt doorhanger restyle

https://reviewboard.mozilla.org/r/55868/#review57350

The button height is still taller than it should be according to the spec linked to in the bug's URL field.  The spec doesn't give an exact height, so you should probably ask whoever made it about that, but just looking at it, it doesn't look right.

And this screenshot here, for example, shows margins that aren't quite right either:

http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/672033832f25056ac9247f1431b3b5d4d54eed878d3b3af324b5c301260dc34b42c1342cccf3efec61d23e13670821f249451fce21563c338a49ef9729522a46

The left edges of the two textboxes and checkbox aren't aligned with the left edge of the two labels above them, and the right edges of the textboxes are too close to the panel edge.  It looks unbalanced.

It'd be nice to have those fixed before landing, but I'll r+ this with or without it fixed as long as it gets fixed in a follow-up bug.

More examples of unbalanced margins:

http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/729d028d02bf3641316cd2775c9ca91902f41427370ce7072a35f32dc6c23601bd2ee8b05c55bd3eb83a9983dfd11e952a71c2d20e9617d1da057e322895d9df

http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/592c61ff4ab5ece0698c40fc91ac3e6197931e79aa84018ed0fa75c68f46a27759bfcf8673ba93c4c89dd7044508d888e3c958b90fc34633f9cd5b6a1311cd2f

http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/58f1dcfb706636ceeff3d3dea60a10f3b3e5df251bba06bdda712f53651812df31ab985de43a5786e76ed4db94b5cc2da4528199b41cd6912a34c22fee5bc7a1
Attachment #8757433 - Flags: review?(adw) → review+
Blocks: 1282768
Whiteboard: [fxprivacy] → [fxprivacy][a][b][c]
Hey Jonathan, thanks for the great progress you've been making on this patch, but in case you feel overloaded with your other commitments, feel free to ask someone else from the team to pick it up and make the final tweaks. I am eager to see this land as it is blocking some other work, but of course if you feel confident that you have the bandwidth to finish it soon, then that's even better!
Comment on attachment 8765951 [details]
Bug 1267604 - Permission prompt doorhanger restyle

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61060/diff/1-2/
This should be landable now, sorry I was working on other things. The review I pushed yesterday should fix the comments. Do you know if Paolo should review?
Flags: needinfo?(past)
Flags: needinfo?(paolo.mozmail)
Comment on attachment 8765951 [details]
Bug 1267604 - Permission prompt doorhanger restyle

It's an important change and I'd like to take a closer look. For instance, some of the SVG files still need to be optimized.
Flags: needinfo?(past)
Flags: needinfo?(paolo.mozmail)
Attachment #8765951 - Flags: review?(paolo.mozmail)
Whiteboard: [fxprivacy][a][b][c] → [fxprivacy]
Blocks: 1284563
There's a style guide somewhere, with things like the colors to be used for the blue buttons for the hover and active states. Stephen, can you help me find it?
Flags: needinfo?(shorlander)
Jonathan, I've found some color definitions as variables for the in-content buttons:

https://dxr.mozilla.org/mozilla-central/search?q=in-content-primary-button-background&redirect=false

I used them in combination with other colors for the previous Private Browsing in-content page:

https://hg.mozilla.org/mozilla-central/file/3cb68e7454d7/browser/components/privatebrowsing/content/aboutPrivateBrowsing.css#l167

Not sure if we have a chrome version of the same variables, though.
And the colors seem different than those specified in the mockup, so I'm confused.
Comment on attachment 8765951 [details]
Bug 1267604 - Permission prompt doorhanger restyle

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61060/diff/2-3/
Attachment #8765951 - Flags: review?(paolo.mozmail)
(In reply to :Paolo Amadini from comment #39)
> There's a style guide somewhere, with things like the colors to be used for
> the blue buttons for the hover and active states. Stephen, can you help me
> find it?

We have been working on defining all of this in one place, that is still a WIP. In the meantime this is the panel Styleguide: https://mozilla.invisionapp.com/d/main#/console/4656956/111461103/preview (let me know if you can view that)

Colors are here: http://firefoxux.github.io/StyleGuide/#/visualDesign/colours

They won't match what's in the product now because we need to start the long process of unifying all of these things.
Flags: needinfo?(shorlander)
Comment on attachment 8765951 [details]
Bug 1267604 - Permission prompt doorhanger restyle

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61060/diff/3-4/
It would be nice to put all this styling in a shared CSS file instead of duplicating it on every platform.
Iteration: --- → 50.4 - Aug 1
Priority: P2 → P1
Comment on attachment 8765951 [details]
Bug 1267604 - Permission prompt doorhanger restyle

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61060/diff/4-5/
Attachment #8765951 - Flags: review?(paolo.mozmail)
Comment on attachment 8765951 [details]
Bug 1267604 - Permission prompt doorhanger restyle

https://reviewboard.mozilla.org/r/61060/#review62916

The new screenshot look really nice!

I've done just a quick review pass on this version, but there are probably enough items to work on a new version.

::: browser/base/content/popup-notifications.inc:11
(Diff revision 5)
>      <popupnotification id="webRTC-shareDevices-notification" hidden="true">
> -      <popupnotificationcontent id="webRTC-selectCamera" orient="vertical">
> +      <popupnotificationcontent id="webRTC-selectCamera">
> -        <label value="&getUserMedia.selectCamera.label;"
> -               accesskey="&getUserMedia.selectCamera.accesskey;"
> -               control="webRTC-selectCamera-menulist"/>

The dropdown does not expand to the full length, I wonder if it's related to orient=vertical being removed? Or a XUL "align" attribute that should be removed elsewhere to return to the default align=stretch.

If we remove the labels, they should also be removed from the DTD files if they are not used elsewhere. Maybe move this change to a separate changeset, and ask Florian for review.

::: toolkit/themes/linux/global/icons/panelarrow-down-inverted.svg:17
(Diff revision 5)
> +  <metadata
> +     id="metadata10">
> +    <rdf:RDF>
> +      <cc:Work
> +         rdf:about="">
> +        <dc:format>image/svg+xml</dc:format>

All these SVG files should be simplified by removing the extra markup, like the other SVG files we have in the tree.

We should have one SVG in the "shared" folder.

This is also not a panelarrow, but a dropmarker. A file name like "menubutton-dropmarker-white.svg" could be better.

That said, we might be able to style this icon using a glyph and a "filter" property, which would give us flexibility on the color. I'll try and see if this is feasible with menu-buttons.

::: toolkit/themes/linux/global/notification.css:82
(Diff revision 5)
> +  /* icon margin left (spacing size) + icon width (*2 spacing size) + icon margin right */
> +  margin-inline-start: calc(calc(var(--panel-arrowcontent-spacing-size) * 3) + 10px) !important;

There's probably an easier way to handle some of these rules, will take a look later.

::: toolkit/themes/linux/global/popup.css:9
(Diff revision 5)
>  
>  @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
>  
>  /* ::::: Variables ::::: */
>  .panel-arrowcontent {
> -  --panel-arrowcontent-padding: 10px;
> +  --panel-arrowcontent-padding: 16px;

So this changes the spacing for all arrow panels on Linux. I'm fine with that, but I'm not sure why it was different in the first place, I guess someone with more context should look at this. Also test with other panels like the bookmarks one.

::: toolkit/themes/linux/global/popup.css:15
(Diff revision 5)
> +#notification-popup .panel-arrowcontent {
> +  --panel-arrowcontent-background: white;
> +}

We should keep the default color here. If we change the background not to use system colors, we also have to change the text color not to use system colors, that might be tricky.

::: toolkit/themes/linux/global/popup.css:65
(Diff revision 5)
> +#notification-popup .panel-arrow[side="top"],
> +#notification-popup .panel-arrow[side="bottom"] {
> +  list-style-image: url("chrome://global/skin/icons/panelarrow-vertical-notification.svg");
> +}
> +

So we can remove this as well.

::: toolkit/themes/osx/global/notification.css.orig:1
(Diff revision 5)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Reminder to remove this .orig file.

::: toolkit/themes/shared/notification.css:1
(Diff revision 5)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

I guess we still need the separate platform styling for notification bars. If they are very similar across platforms, we can use "#ifdef" lines in the shared files, otherwise we can just leave that part in the individual platform files.

Also, choose a file from one of the platforms and "hg cp" it to the new file in the "shared" folder, so we can follow the history of the newly created file and see what hasn't changed.
Depends on: 1288747
Comment on attachment 8765951 [details]
Bug 1267604 - Permission prompt doorhanger restyle

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61060/diff/5-6/
Comment on attachment 8765951 [details]
Bug 1267604 - Permission prompt doorhanger restyle

This should have addressed your comments :).
Let me know if you need anything else.
Attachment #8765951 - Flags: review?(paolo.mozmail)
Comment on attachment 8765951 [details]
Bug 1267604 - Permission prompt doorhanger restyle

I strongly suspect that if the styles for notification bars are very different across platforms, we want to keep them inside the original "notification.css" files, and name the shared file "popupnotification.css". The latter would look just like the shared file in the current patch (including the move under version control) except that it wouldn't have the initial part with the notification bar styles.

In my previous comment I was referring to having a short #ifdef inside individual rules if the rest of the styles are similar. See for example how they are used in "notification-icons.inc.css".

Jared, do you agree? If you have other general style suggestions on the patch, they're welcome.
Attachment #8765951 - Flags: feedback?(jaws)
For the record, the latest patch does not build with "./mach build faster". As a workaround, incremental builds with "./mach build browser toolkit" work, although they are a bit slower.
(In reply to :Paolo Amadini from comment #57)
> Comment on attachment 8765951 [details]
> Bug 1267604 - Permission prompt doorhanger restyle
> 
> I strongly suspect that if the styles for notification bars are very
> different across platforms, we want to keep them inside the original
> "notification.css" files, and name the shared file "popupnotification.css".
> The latter would look just like the shared file in the current patch
> (including the move under version control) except that it wouldn't have the
> initial part with the notification bar styles.
> 
> In my previous comment I was referring to having a short #ifdef inside
> individual rules if the rest of the styles are similar. See for example how
> they are used in "notification-icons.inc.css".
> 
> Jared, do you agree? If you have other general style suggestions on the
> patch, they're welcome.

Having very large chunks of platform specific CSS at the top of notification.css seems counter intuitive when we already have a good system for keeping platform specific styling in separate directories. If there are large amounts of shared CSS that is specific to popup notifications, then please create a shared popup-notifications.inc.css file.
Comment on attachment 8765951 [details]
Bug 1267604 - Permission prompt doorhanger restyle

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61060/diff/6-7/
Attachment #8765951 - Flags: review?(jaws)
Comment on attachment 8765951 [details]
Bug 1267604 - Permission prompt doorhanger restyle

https://reviewboard.mozilla.org/r/61060/#review65368

I would prefer that Paolo review this as I've got a heavy backlog already and Paolo has looked at this longer than I have.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #61)
> I would prefer that Paolo review this as I've got a heavy backlog already
> and Paolo has looked at this longer than I have.

Sure, I might still ask you specific questions later when we're closer to final.

I'll try to review the code in detail tomorrow.
Attachment #8765951 - Flags: review?(paolo.mozmail)
Comment on attachment 8765951 [details]
Bug 1267604 - Permission prompt doorhanger restyle

https://reviewboard.mozilla.org/r/61060/#review65666

Just dumping another round of comments, I have still more work to do to finish the review. Thanks for the patience!

::: browser/base/content/popup-notifications.inc
(Diff revision 7)
> -        <label value="&getUserMedia.selectCamera.label;"
> -               accesskey="&getUserMedia.selectCamera.accesskey;"
> -               control="webRTC-selectCamera-menulist"/>

Strings used by the removed code should also be removed from the localization files:

https://dxr.mozilla.org/mozilla-central/search?q=getUserMedia.selectCamera.label

::: toolkit/themes/linux/global/notification.css:69
(Diff revision 7)
>  .messageCloseButton {
>    padding-left: 11px;
>    padding-right: 11px;
>  }
>  
> -/* Popup notification */
> +%include ../../shared/notification.inc

I guess this should be "popupnotification.inc.css".

::: toolkit/themes/linux/global/popup.css:9
(Diff revision 7)
>  
>  @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
>  
>  /* ::::: Variables ::::: */
>  .panel-arrowcontent {
> -  --panel-arrowcontent-padding: 10px;
> +  --panel-arrowcontent-padding: 16px;

Reminder that we should check what else this change affects other than the notification panel. Can you take a few screenshots?

::: toolkit/themes/osx/global/arrow/panelarrow-down-inverted.svg:1
(Diff revision 7)
> +<?xml version="1.0" encoding="UTF-8" standalone="no"?>
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->

Looks like there are two leftover files?

::: toolkit/themes/shared/notification.inc:53
(Diff revision 7)
> -  margin-top: 17px;
> +  border-top: 1px solid #eee;
> +  background-color: #ddd;
>  }
>  
>  .popup-notification-menubutton {
> +  --main-color: #5295e5;

I'm looking into how to share this color with other controls, and also how to handle the border and the separator.

::: toolkit/themes/windows/global/popup.css:9
(Diff revision 7)
>  
>  @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
>  
>  /* ::::: Variables ::::: */
>  .panel-arrowcontent {
> +  --panel-arrowcontent-spacing-size: 22px;

Leftover.
So, Jonathan, as you have noticed, with the current menu-button based implementation there is some trouble in handling the hover behavior for the button's dropmarker. Additionally, we need a visible separator between the main button and the dropmarker, and currently it would need to be styled with different rules than the separators in the main menu, even if the final behavior will be the same.

So, maybe the correct way to implement this is to treat the dropmarker as a separate <button type="menu"> or <dropmarker> altogether, with a <toolbarseparator> before it. The only difference may be in how the choices are presented to keyboard users, but it's not a substantial one in my opinion.

I'll try and see if this approach works for the Downloads Panel and we may then copy it here.
This bug is a bit messy, so I'm not sure what the current state of these patches is, but note that arrow panels in general and also popup notifications specifically must not hardcode white as their background on Linux and non-default Windows themes. You need to use platform colors there (such as -moz-field which is also white in default themes). This has always been the rule but sometimes we fail to enforce it (or deliberately make an exception in certain cases, but I don't think this is one of them). I happen to be dealing with these kind of issues in bug 1016556 right now.

Some other random things I noticed:

- You use the descendant selector a lot. Please avoid as much as you can. Use the child selector.

- notification-specific popup styling belongs entirely in notification.css, not popup.css

- notification.inc should be notification.inc.css

- @focusRingShadow@ is specific to OS X, you can't use it in shared/
Oh, and another random thing I noticed just as I wanted to close that tab: Your use of the 'rem' unit for the menu button styling doesn't seem to make sense. I think you want 'em' there.
Depends on: 1293018
Component: General → Theme
(In reply to Dão Gottwald [:dao] from comment #65)
> This bug is a bit messy, so I'm not sure what the current state of these
> patches is, but note that arrow panels in general and also popup
> notifications specifically must not hardcode white as their background on
> Linux and non-default Windows themes.

Thanks, we had these changes in the first few versions but we reverted them exactly for these concerns.

> - notification-specific popup styling belongs entirely in notification.css,
> not popup.css

One change to popup.css is in fact a leftover, the other one changes "--panel-arrowcontent-padding" for all panels to 16px on Linux. This is a separate change from the rest of the patch.

Are you aware of any reason why the spacing is different on Linux? I've already suggested in comment 53 that someone with more context should look at this change in particular, so maybe Jonathan can ask you for review on a separate bug?
Attachment #8757433 - Attachment is obsolete: true
Attachment #8770953 - Attachment is obsolete: true
FYI :Paolo, just pushed the code and to test on try screenshot, however it hasn't been tested on Mac or Windows yet. However all feedback should now be incorporated etc. I'll get some screenshots tomorrow as requested :)
Comment on attachment 8765951 [details]
Bug 1267604 - Permission prompt doorhanger restyle

https://reviewboard.mozilla.org/r/61060/#review67558

::: browser/base/content/popup-notifications.inc
(Diff revision 8)
> -        <label id="webRTC-selectWindow-label"
> -               control="webRTC-selectWindow-menulist"/>

I've just noticed this would break the webrtcUI.jsm code that looks for this label:

https://dxr.mozilla.org/mozilla-central/rev/720b5d2c84d5b253d4dfde4897e13384dc97a46a/browser/modules/webrtcUI.jsm#448

I hadn't noticed before that this part involved a code module. We definitely need a separate review from Florian, and it's better to get that review in a separate bug. Let's revert all the changes to popup-notifications.inc and browser.dtd in this patch.

::: toolkit/themes/linux/global/popup.css:9
(Diff revision 8)
>  
>  @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
>  
>  /* ::::: Variables ::::: */
>  .panel-arrowcontent {
> -  --panel-arrowcontent-padding: 10px;
> +  --panel-arrowcontent-padding: 16px;

Let's file a separate bug for this change as well.

::: toolkit/themes/osx/global/notification.css:109
(Diff revision 8)
>  .popup-notification-menubutton:not([type="menu-button"]):-moz-focusring,
>  .popup-notification-menubutton:-moz-focusring > .button-menubutton-dropmarker,
>  .popup-notification-menubutton > .button-menubutton-button:-moz-focusring {
>    box-shadow: @focusRingShadow@;
>    position: relative;
>  }

These rules must be updated to match the new class in the XUL file. I also think that the focus ring style for the modern buttons should be changed to match the one you can see in the Downloads Panel. It's still different for each platform.

::: toolkit/themes/shared/popupnotification.inc.css:47
(Diff revision 8)
>  .popup-notification-button-container {
> -  margin-top: 17px;
> +  border-top: 1px solid #eee;
> +  background-color: #ddd;
> +  --primary-button-color: #5295e5;
>  }

The button styles should be copied more literally from the Downloads Panel. They should replace the entirety of the old button styles. In this case we don't use CSS variables to share the colors.

https://dxr.mozilla.org/mozilla-central/rev/720b5d2c84d5b253d4dfde4897e13384dc97a46a/browser/themes/shared/downloads/downloads.inc.css#39-83

Note that "color: black;" should have been "color: inherit;" there, and the height of 40px might have changed by the time we land this bug on mozilla-central, as noted in bug 1293023. We can keep this height for the moment as the patch will initially land on the elm branch.

The "downloadsPanelFooter" class should be "popup-notification-button-container" here, and you should apply the same class "popup-notification-button" to each button individually in the XUL file, to match "downloadsPanelFooterButton". The blue "\[default\]" styling should be the only one to use here for now, without the "\[default\]" specification, you don't need to copy the other gray styling. In the end it turns out you can incorporate the "color: white;" in the base rule.

You should look at the individual platform stylesheets to copy the focus ring styles.

::: toolkit/themes/shared/popupnotification.inc.css:85
(Diff revision 8)
> +  min-width: 0;
> +  padding: 0 1em;
> +}
> +
> +.popup-notification-menubutton-dropmarker > hbox > dropmarker > image {
> +  list-style-image: url("chrome://global/skin/icons/menubutton-dropmarker-white.svg");

This image doesn't appear for me on OS X for some reason. Regardless, you can probably use a simpler rule, just look at the styling used for the icon buttons in the footer of the application menu.

You can look for example to the #PanelUI-help rules in panelUI.inc.css, they are scattered around the file but you should be able to consolidate the relevant parts into one or two rules.

::: toolkit/themes/shared/popupnotification.inc.css:94
(Diff revision 8)
> +.popup-notification-menubutton-dropmarker[open="true"],
> +.popup-notification-menubutton-button[buttondown="true"] {

I'll note that the selectors \[open=true\] and \[buttondown=true\] can be replaced by the same ":active" styles from the Downloads Panel since we have normal buttons now.

::: toolkit/themes/shared/icons/menubutton-dropmarker-white.svg:1
(Diff revision 8)
> +<?xml version="1.0" encoding="UTF-8" standalone="no"?>
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg
> +   xmlns="http://www.w3.org/2000/svg"
> +   width="16"
> +   height="16"
> +   viewBox="0 0 16 16">
> +  <polygon
> +     points="10.5,14 12,12.5 7.625,8 12,3.5 10.5,2 4.625,8 "
> +     transform="matrix(0,-1,1,0,0.3125,16.3125)"
> +     style="fill:#ffffff" />
> +</svg>

This SVG is off-center, and the transform must be optimized. Use this one instead:

<?xml version="1.0"?>
<!-- This Source Code Form is subject to the terms of the Mozilla Public
   - License, v. 2.0. If a copy of the MPL was not distributed with this
   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
<svg xmlns="http://www.w3.org/2000/svg"
     width="16" height="16" viewBox="0 0 16 16">
  <path fill="#fff" d="m 2,6 6,6 6,-6 -1.5,-1.5 -4.5,4.5 -4.5,-4.5 z" />
</svg>

::: toolkit/themes/windows/global/notification.css:69
(Diff revision 8)
> -/* Popup notification */
> -
> +#notification-popup .panel-arrowcontent {
> +  --panel-arrowcontent-background: #fff;

Ah, yes this definitely shouldn't be here.

::: toolkit/themes/windows/global/notification.css:73
(Diff revision 8)
> -  .popup-notification-menubutton:not([type="menu-button"]),
> -  .popup-notification-menubutton > .button-menubutton-button,
> +#notification-popup .panel-notification-button-container,
> +#notification-popup .panel-arrowcontent popupnotification {
> -  .popup-notification-menubutton > .button-menubutton-dropmarker {
> -    -moz-appearance: none;
> -    margin: 0;
> +  margin: 0;

Is this Windows-specific rule necessary?
Attachment #8765951 - Flags: review?(paolo.mozmail)
Comment on attachment 8765951 [details]
Bug 1267604 - Permission prompt doorhanger restyle

https://reviewboard.mozilla.org/r/61060/#review67558

> Is this Windows-specific rule necessary?

Not sure, was going to test. Will drop and test instead.
Comment on attachment 8765951 [details]
Bug 1267604 - Permission prompt doorhanger restyle

>-popupnotificationcontent {
>-  margin-top: .5em;
>-}

Why did you remove this / what did you replace this with?

> .popup-notification-button-container {
>-  margin-top: 17px;
>+  background-color: hsla(210,4%,10%,.07);
>+  border-top: 1px solid hsla(210,4%,10%,.14);
> }

Please use var(--panel-separator-color) for the border.

>+.popup-notification-button-container > toolbarseparator {
>+  margin: 0;
>+  border: 0;
>+  min-width: 0;
>+  border-left: 1px solid hsla(210,4%,10%,.14);
>+  -moz-appearance: none !important;

Why !important?
(In reply to Dão Gottwald [:dao] from comment #74)
> >+.popup-notification-button-container > toolbarseparator {
> >+  margin: 0;
> >+  border: 0;
> >+  min-width: 0;
> >+  border-left: 1px solid hsla(210,4%,10%,.14);
> >+  -moz-appearance: none !important;
> 
> Why !important?

That is still required until the platform styling for Linux is fixed to avoid using "!important":

https://dxr.mozilla.org/mozilla-central/rev/6cf0089510fad8deb866136f5b92bbced9498447/toolkit/themes/linux/global/toolbar.css#48

Fixing the platform styling to avoid "!important" is out of scope for this bug because it may cause regressions, thus it's best tracked in a separate bug. We can also remove the "!important" in other places in the front-end CSS when we fix that bug.

We can add a comment to the code for now.
Whiteboard: [fxprivacy] → [fxprivacy][will-land-on-elm]
(In reply to :Paolo Amadini from comment #77)
> (In reply to Dão Gottwald [:dao] from comment #74)
> > >+.popup-notification-button-container > toolbarseparator {
> > >+  margin: 0;
> > >+  border: 0;
> > >+  min-width: 0;
> > >+  border-left: 1px solid hsla(210,4%,10%,.14);
> > >+  -moz-appearance: none !important;
> > 
> > Why !important?
> 
> That is still required until the platform styling for Linux is fixed to
> avoid using "!important":
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 6cf0089510fad8deb866136f5b92bbced9498447/toolkit/themes/linux/global/toolbar.
> css#48
> 
> Fixing the platform styling to avoid "!important" is out of scope for this
> bug because it may cause regressions, thus it's best tracked in a separate
> bug. We can also remove the "!important" in other places in the front-end
> CSS when we fix that bug.
> 
> We can add a comment to the code for now.

Please don't hesitate to file Toolkit::Theme bugs for these kind of issues. Fixing this one should be trivial, I can also look into it.
(In reply to Dão Gottwald [:dao] from comment #79)
> Please don't hesitate to file Toolkit::Theme bugs for these kind of issues.
> Fixing this one should be trivial, I can also look into it.

I just filed bug 1294136 for this. Thanks!
Comment on attachment 8765951 [details]
Bug 1267604 - Permission prompt doorhanger restyle

https://reviewboard.mozilla.org/r/61060/#review68060

Thanks Jonathan for the infinite patience in getting this first version right! With only the change below, the styling is ready to land on the "elm" branch.

So, "r+" for landing on elm!

Since we made changes to the structure of the button, the next task is to check that all regression tests still pass, if not it means there might be something to update in the PopupNotifications.jsm module. I've tested manually for some permission notifications and they seem to work as expected.

We need to run mozscreenshots, and also add manual screenshots to the bug for the hover state on the various platforms, then send the relevant screenshots to the designers to get feedback.

I'd like to have more eyes on this patch later, when it's time to merge it to mozilla-central.

::: toolkit/themes/shared/popupnotification.inc.css:49
(Diff revision 13)
>    margin-top: .5em !important;
>  }
>  
>  .popup-notification-button-container {
> -  margin-top: 17px;
> +  background-color: hsla(210,4%,10%,.07);
> +  border-top: 1px solid hsla(210,4%,10%,.14);

I think this can be already updated to:

border-top: 1px solid var(--panel-separator-color);
Attachment #8765951 - Flags: review?(paolo.mozmail) → review+
Landed on elm: https://hg.mozilla.org/projects/elm/rev/3c2a12362ace
Whiteboard: [fxprivacy][will-land-on-elm] → [fxprivacy][landed-on-elm]
Jonathan I see quite a few broken tests, can you investigate?

https://treeherder.mozilla.org/#/jobs?repo=elm&revision=3c2a12362acebd7c438afb6458056eaf78163a95

I won't backout as this gives me a better foundation for my work in bug 1282768 and we use elm as a test bed anyway.
Hi Panos, after initially looking I can see lots are intermittent so retriggering those. Lots of these I don't see from my initial try based off central.

There are a few that look like a product of other changes on elm like mainButton being undefined in: https://treeherder.mozilla.org/#/jobs?repo=elm&revision=3c2a12362acebd7c438afb6458056eaf78163a95&selectedJob=40373

The code here [1] suggests that the button was removed or changed in the password prompt.

I will keep looking through them though :).

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/test/browser/browser_notifications_2.js?q=path%3Acomponents%2Fpasswordmgr%2Ftest%2Fbrowser%2Fbrowser_notifications_2.js&redirect_type=single#41
Any screenshots or anything we can see? I know shorlander is looking forward to the UX review.
Iteration: 50.4 - Aug 1 → 51.1 - Aug 15
Blocks: 1279586
Depends on: 1296252
Do we need to uplift this?
Flags: needinfo?(past)
Flags: needinfo?(jhofmann)
Nope, it's all only on elm for now :)
Flags: needinfo?(past)
Flags: needinfo?(jhofmann)
Blocks: 1308310
No longer depends on: 1288747
Posted a rebased version of the patch on top of fx-team tip and bug 1004061.
Comment on attachment 8812583 [details]
Bug 1267604 - Restyle doorhanger notifications.

https://reviewboard.mozilla.org/r/94260/#review94386
Attachment #8812583 - Flags: review?(paolo.mozmail) → review+
https://hg.mozilla.org/mozilla-central/rev/d04cc86f4313
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Depends on: 1320314
Depends on: 1320317
Depends on: 1320318
Blocks: 1323494
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: