Closed Bug 1363056 Opened 7 years ago Closed 7 years ago

Implement new notification bar style

Categories

(Toolkit :: Themes, enhancement, P1)

Product:
Toolkit
Component:
Themes
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Iteration:
57.2 - Aug 29
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: johannh, Assigned: ntim)

References

(Blocks 1 open bug)

Details

(Whiteboard: [reserve-photon-visual][p3])

Attachments

(1 file)

Bug 1363056 - Update notification bar style for photon.
59 bytes, text/x-review-board-request
dao
: review+
Details 
We want to update the notificationbox (which is really a notification bar) for Photon.
Component: Theme → Themes
Product: Firefox → Toolkit
Summary: Implement new notificationbox style → Implement new notification bar style
Whiteboard: [photon-visual][p3] → [reserve-photon-visual][p4]
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
QA Contact: brindusa.tot
Whiteboard: [reserve-photon-visual][p4] → [reserve-photon-visual][p3]
Assignee: nobody → ntim.bugs
Status: NEW → ASSIGNED
Priority: P3 → P1
Depends on: 1385702
This patch depends on bug 1385702
Comment on attachment 8891744 [details]
Bug 1363056 - Update notification bar style for photon.

https://reviewboard.mozilla.org/r/162812/#review170360

Could you please rebase?

applying https://reviewboard-hg.mozilla.org/gecko/rev/069459a11706ecc29b61f14a4a126511c8b98737
patching file toolkit/themes/osx/global/notification.css
Hunk #1 FAILED at 0
1 out of 1 hunks FAILED -- saving rejects to file toolkit/themes/osx/global/notification.css.rej
abort: patch failed to apply
Attachment #8891744 - Flags: review?(dao+bmo)
Comment on attachment 8891744 [details]
Bug 1363056 - Update notification bar style for photon.

https://reviewboard.mozilla.org/r/162812/#review174902

::: toolkit/themes/shared/icons/help.svg:5
(Diff revision 3)
> +<!-- 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="context-fill" d="M8 1a7 7 0 1 0 7 7 7.008 7.008 0 0 0-7-7zm0 13a6 6 0 1 1 6-6 6.007 6.007 0 0 1-6 6zM8 3.125A2.7 2.7 0 0 0 5.125 6a.875.875 0 0 0 1.75 0c0-1 .6-1.125 1.125-1.125a1.105 1.105 0 0 1 1.13.744.894.894 0 0 1-.53 1.016A2.738 2.738 0 0 0 7.125 9v.337a.875.875 0 0 0 1.75 0v-.37a1.041 1.041 0 0 1 .609-.824A2.637 2.637 0 0 0 10.82 5.16 2.838 2.838 0 0 0 8 3.125zm0 7.625A1.25 1.25 0 1 0 9.25 12 1.25 1.25 0 0 0 8 10.75z"></path>

<path/>

::: toolkit/themes/shared/icons/warning.svg:5
(Diff revision 3)
> +<!-- 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="context-fill" d="M14.742 12.106L9.789 2.2a2 2 0 0 0-3.578 0l-4.953 9.91A2 2 0 0 0 3.047 15h9.905a2 2 0 0 0 1.79-2.894zM7 5a1 1 0 0 1 2 0v4a1 1 0 0 1-2 0zm1 8.25A1.25 1.25 0 1 1 9.25 12 1.25 1.25 0 0 1 8 13.25z"></path>

ditto

::: toolkit/themes/shared/notification.inc.css:11
(Diff revision 3)
> +
> +notification {
> +  padding: 2px 12px 3px;
> +  background: -moz-dialog;
> +  color: -moz-dialogText;
> +  border-color: rgba(12, 12, 13, .2);

This is going to be invisble with dark OS themes. Can you use threedshadow?

::: toolkit/themes/windows/global/notification.css
(Diff revision 3)
> -  padding: 4px 2px;
> -  border: none !important;
> -}
> -
> -.messageCloseButton > .toolbarbutton-icon {
> -  margin-inline-end: 5px;

This was supposed to make the close button clickable on the edge of the window. Can we maintain this behavior?
Attachment #8891744 - Flags: review?(dao+bmo)
Comment on attachment 8891744 [details]
Bug 1363056 - Update notification bar style for photon.

https://reviewboard.mozilla.org/r/162812/#review174938

::: toolkit/themes/shared/notification.inc.css:8
(Diff revisions 3 - 4)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
>  
>  notification {
> -  padding: 2px 12px 3px;
> +  padding: 2px 0 3px 12px;

Need to take RTL into account.

::: toolkit/themes/shared/notification.inc.css:11
(Diff revisions 3 - 4)
>  
>  notification {
> -  padding: 2px 12px 3px;
> +  padding: 2px 0 3px 12px;
>    background: -moz-dialog;
>    color: -moz-dialogText;
> -  border-color: rgba(12, 12, 13, .2);
> +  border-color: ThreeDLightShadow;

As mentioned on IRC, warning-level and critical notification bars should use an rgba border.
Attachment #8891744 - Flags: review?(dao+bmo)
Comment on attachment 8891744 [details]
Bug 1363056 - Update notification bar style for photon.

https://reviewboard.mozilla.org/r/162812/#review174944

::: toolkit/themes/shared/notification.inc.css:74
(Diff revision 6)
> +.messageCloseButton {
> +  margin: 0;
> +  padding: 0;
> +}
> +
> +.messageCloseButton > .toolbarbutton-icon {

Probably worth a comment so people touching this code know what this is doing
Comment on attachment 8891744 [details]
Bug 1363056 - Update notification bar style for photon.

https://reviewboard.mozilla.org/r/162812/#review174946

::: toolkit/themes/shared/notification.inc.css:12
(Diff revision 6)
> +notification {
> +  padding: 2px 0 3px;
> +  padding-inline-start: 12px;
> +  background: -moz-dialog;
> +  color: -moz-dialogText;
> +  border-color: ThreeDLightShadow;

Should set both top and bottom borders as it was before, in case a notificationbox doesn't have the notificationside attribute.

::: toolkit/themes/shared/notification.inc.css:13
(Diff revision 6)
> +  padding: 2px 0 3px;
> +  padding-inline-start: 12px;
> +  background: -moz-dialog;
> +  color: -moz-dialogText;
> +  border-color: ThreeDLightShadow;
> +  text-shadow: none !important;

Why did you add !important here?
Attachment #8891744 - Flags: review?(dao+bmo)
Comment on attachment 8891744 [details]
Bug 1363056 - Update notification bar style for photon.

https://reviewboard.mozilla.org/r/162812/#review175232

Thanks!

::: toolkit/themes/shared/notification.inc.css:19
(Diff revision 7)
> +  border-width: 1px 0;
> +  text-shadow: none;
> +}
> +
> +notificationbox[notificationside="top"] > notification {
> +  border-top: none;

nit: border-top-style

::: toolkit/themes/shared/notification.inc.css:23
(Diff revision 7)
> +notificationbox[notificationside="top"] > notification {
> +  border-top: none;
> +}
> +
> +notificationbox[notificationside="bottom"] > notification {
> +  border-bottom: none;

ditto
Attachment #8891744 - Flags: review?(dao+bmo) → review+
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3e233c4c7867
Update notification bar style for photon. r=dao
(In reply to Pulsebot from comment #17)
> Pushed by ntim.bugs@gmail.com:
> https://hg.mozilla.org/integration/autoland/rev/3e233c4c7867
> Update notification bar style for photon. r=dao

Backed out for packaging bustage.
https://hg.mozilla.org/integration/autoland/rev/32fe50044beace1245a6eddaf41126a6453c0e7f

https://treeherder.mozilla.org/logviewer.html#?job_id=124162990&repo=autoland&lineNumber=30315

INFO -  ERROR: The following duplicated files are not allowed:
INFO -  browser/chrome/browser/skin/classic/browser/help.svg
INFO -  chrome/toolkit/skin/classic/global/icons/help.svg
Flags: needinfo?(ntim.bugs)
Flags: needinfo?(ntim.bugs)
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/089706dfb481
Update notification bar style for photon. r=dao
https://hg.mozilla.org/mozilla-central/rev/089706dfb481
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Iteration: --- → 57.2 - Aug 29
QA Contact: brindusa.tot → ovidiu.boca
Tim, can you please provide more details about what needs to be tested here (QA)? Thank you.
Flags: needinfo?(ntim.bugs)
I think we can rely on nightly testing here without explicit verification.
Flags: qe-verify-
Flags: qe-verify+
Flags: needinfo?(ntim.bugs)
QA Contact: ovidiu.boca
Depends on: 1633235
No longer depends on: 1633235
Regressions: 1633235
Blocks: 1693979
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: