The default bug view has changed. See this FAQ.

Enforce new requirements for Hamburger menu badges

RESOLVED FIXED in Firefox 42

Status

()

Firefox
Toolbars and Customization
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: eoger, Assigned: eoger)

Tracking

unspecified
Firefox 42
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox42 fixed)

Details

(Whiteboard: [fxsync])

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

2 years ago
Follow up on these comments : https://bugzilla.mozilla.org/show_bug.cgi?id=1183212#c1.

Concrete actions to be taken:

- Add update badge inside the Hamburger menu popup update button.
- Drop all badges on click on the Hamburger menu button.

Did I forget anything? CC :shorlander :markh
(Assignee)

Updated

2 years ago
Blocks: 1183212
(Assignee)

Updated

2 years ago
Depends on: 1180584
(Assignee)

Comment 1

2 years ago
Created attachment 8638842 [details] [diff] [review]
bug-1186521.patch
Assignee: nobody → edouard.oger
Status: NEW → ASSIGNED
Attachment #8638842 - Flags: feedback?(markh)
(Assignee)

Comment 2

2 years ago
Created attachment 8638844 [details]
hamb.zip

How it looks now
Flags: needinfo?(rfeeley)
Comment on attachment 8638842 [details] [diff] [review]
bug-1186521.patch

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

Looking good, and the screen-shots are what I was expecting to see!

::: browser/base/content/browser.js
@@ +2593,5 @@
>        PanelUI.menuButton.setAttribute("badge-status", badgeToShow);
>        if (badgeToShow == "update-failed") {
>          PanelUI.menuButton.setAttribute("badge", "!");
>        }
> +      PanelUI.panel.addEventListener("popupshowing", this, true);

I'm not sure the event listener state is being tracked correctly - eg, multiple addBadge calls, or many removeBadge calls with no corresponding addBadge calls seems likely to screw it up. I don't see a problem with always having the event listener exist (which might even mean you can just have the handler be inline and avoid the handleEvent function and the check it does for the correct event)

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +542,5 @@
>    position: relative;
>    top: 25%;
>  }
>  
> +#PanelUI-update-status[update-status="succeeded"]::after {

it would be awesome if we could do something clever to avoid duplicating these rules, but I haven't thought too much about how we might go about that. Worth considering though.
Attachment #8638842 - Flags: feedback?(markh) → feedback+
(Assignee)

Comment 4

2 years ago
Created attachment 8639985 [details] [diff] [review]
bug-1186521.patch

Comments addressed!
Attachment #8638842 - Attachment is obsolete: true
Attachment #8639985 - Flags: review?(markh)
Comment on attachment 8639985 [details] [diff] [review]
bug-1186521.patch

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

This looks fine to me pending shorlander's ui-review in bug 1180584.
Attachment #8639985 - Flags: review?(markh) → review+
(Assignee)

Comment 6

2 years ago
Created attachment 8643194 [details] [diff] [review]
bug-1186521.patch

Updated to use the new update-failed icon introduced in bug 1180584
Attachment #8639985 - Attachment is obsolete: true
Attachment #8643194 - Flags: review?(markh)

Updated

2 years ago
Attachment #8643194 - Flags: review?(markh) → review+

Comment 7

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/4df929a2069c

Comment 8

2 years ago
Comment on attachment 8643194 [details] [diff] [review]
bug-1186521.patch

>+#PanelUI-update-status[update-status]::after {
>+  content: "";
>+  width: 14px;
>+  height: 14px;
>+  margin-right: 16.5px;
>+  box-shadow: 0px 1px 0px rgba(255, 255, 255, 0.2) inset, 0px -1px 0px rgba(0, 0, 0, 0.1) inset, 0px 1px 0px rgba(12, 27, 38, 0.2);
>+  border-radius: 2px;
>+  background-size: contain;
>+  display: -moz-box;
>+}
>+
>+#PanelUI-update-status[update-status="succeeded"]::after {
>+  background-image: url(chrome://browser/skin/update-badge.svg);
>+  background-color: #74BF43;
>+}
>+
>+#PanelUI-update-status[update-status="failed"]::after {
>+  background-image: url(chrome://browser/skin/update-badge-failed.svg);
>+  background-color: #D90000;
>+}
Out of interest, what is this trying to achieve? I ask because I had to remove some uses of ::after in bug 1029937 and I wouldn't want you to trip over the same bug.
Edouard, can you please address comment 8 and see how applicable that bug is here?
Flags: needinfo?(edouard.oger)
(Assignee)

Comment 10

2 years ago
I played with the window size and the inspector, wasn't able to reproduce this kind of bug.
Flags: needinfo?(edouard.oger)
Iteration: --- → 42.3 - Aug 10
Whiteboard: [fxsync]
Flags: firefox-backlog+
https://hg.mozilla.org/mozilla-central/rev/4df929a2069c
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
(Assignee)

Updated

2 years ago
Flags: needinfo?(rfeeley)
You need to log in before you can comment on or make changes to this bug.