Closed Bug 1186521 Opened 4 years ago Closed 4 years ago

Enforce new requirements for Hamburger menu badges

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 42
Iteration:
42.3 - Aug 10
Tracking Status
firefox42 --- fixed

People

(Reporter: eoger, Assigned: eoger)

References

Details

(Whiteboard: [fxsync])

Attachments

(2 files, 2 obsolete files)

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
Blocks: 1183212
Depends on: 1180584
Attached patch bug-1186521.patch (obsolete) — Splinter Review
Assignee: nobody → edouard.oger
Status: NEW → ASSIGNED
Attachment #8638842 - Flags: feedback?(markh)
Attached file 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+
Attached patch bug-1186521.patch (obsolete) — Splinter Review
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+
Updated to use the new update-failed icon introduced in bug 1180584
Attachment #8639985 - Attachment is obsolete: true
Attachment #8643194 - Flags: review?(markh)
Attachment #8643194 - Flags: review?(markh) → review+
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)
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
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Flags: needinfo?(rfeeley)
You need to log in before you can comment on or make changes to this bug.