1355020 - Include update and other 'notification' UI on hamburger panel in new Photon hamburger panel

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, enhancement, P1)

Description

[:Gijs (he/him)]

We currently show update notifications in the hamburger panel, including badges on the panel button. Sideloaded add-on notifications also appear there, and bug 893505 simplified some of the logic there.

When swapping out the panel menu for one with a different layout, these notifications will continue to need a place to go. The badges can stay the same, but the design at https://mozilla.invisionapp.com/share/5ZAEYEW8M#/screens/218570316 will need to be updated to include such notifications.

Comment 1

[:Gijs (he/him)]

Attachment: Bug 1355020 - part 1: disentangle update notifications from 'PanelUI' and hardcoded IDs,

Review commit: https://reviewboard.mozilla.org/r/135842/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/135842/

Comment 2

[:Gijs (he/him)]

Attachment: Bug 1355020 - part 2: disentangle add-on notifications from 'PanelUI' and their specific spot,

Review commit: https://reviewboard.mozilla.org/r/135844/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/135844/

Comment 3

[:Gijs (he/him)]

Attachment: Bug 1355020 - part 3: add notification container and styling for photon hamburger panel,

Review commit: https://reviewboard.mozilla.org/r/135864/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/135864/

Comment 4

[:Gijs (he/him)]

Because of the mix of background-image and list-style-image (for the correctly-positioned image on the left), switching to 1 icon wasn't entirely trivial. There'd also be the question, for the add-on icons, whether we show the add-on icon or the warning icon we show now. So just keeping the status quo seemed simplest. Per discussion on slack, that's what I've done for now.

Comment 5

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8864187 [details]
Bug 1355020 - part 1: disentangle update notifications from 'PanelUI' and hardcoded IDs,

https://reviewboard.mozilla.org/r/135842/#review138912

Comment 6

[:Gijs (he/him)]

Comment on attachment 8864187 [details]
Bug 1355020 - part 1: disentangle update notifications from 'PanelUI' and hardcoded IDs,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135842/diff/1-2/

Comment 7

[:Gijs (he/him)]

Comment on attachment 8864188 [details]
Bug 1355020 - part 2: disentangle add-on notifications from 'PanelUI' and their specific spot,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135844/diff/1-2/

Comment 8

[:Gijs (he/him)]

Comment on attachment 8864200 [details]
Bug 1355020 - part 3: add notification container and styling for photon hamburger panel,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135864/diff/1-2/

Comment 9

[Mike de Boer [:mikedeboer]]

Comment on attachment 8864200 [details]
Bug 1355020 - part 3: add notification container and styling for photon hamburger panel,

https://reviewboard.mozilla.org/r/135864/#review139596

These changes look a-ok to me! I think it might be handy to explain to Kris the _why_ behind the browser-addons.js changes, which might make it easier for him to dive in and review.

::: browser/themes/shared/customizableui/panelUI.inc.css:678
(Diff revision 2)
>    border-top: 1px solid var(--panel-separator-color);
>    border-bottom: 1px solid transparent;
>    margin-bottom: -1px;
>  }
>  
> +/* in Photon, we have a bottom border as well. Reconcile with the above rule

Yeah, I'm also using 'photon' labeled sections in places... makes them easy to find & merge before we ship 57 - because I think that's when this needs to happen, not after shipping; then we can cleanup things nicely during the 57 cycle.

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8864188 [details]
Bug 1355020 - part 2: disentangle add-on notifications from 'PanelUI' and their specific spot,

https://reviewboard.mozilla.org/r/135844/#review139786

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d7331d0d4252
part 1: disentangle update notifications from 'PanelUI' and hardcoded IDs, r=dthayer
https://hg.mozilla.org/integration/autoland/rev/43aad7ae71a6
part 2: disentangle add-on notifications from 'PanelUI' and their specific spot, r=kmag
https://hg.mozilla.org/integration/autoland/rev/b265fdf79862
part 3: add notification container and styling for photon hamburger panel, r=mikedeboer

Comment 12

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/d7331d0d4252
https://hg.mozilla.org/mozilla-central/rev/43aad7ae71a6
https://hg.mozilla.org/mozilla-central/rev/b265fdf79862

Comment 13

[u554753]

I was able to verify this in Windows and Ubuntu/Linux, but I cannot seem to get any visuals regarding updating Firefox in the hamburger window in the Mac OSX build. Is this intended due to the difference in interface of Mac OSX?

Comment 14

[:Gijs (he/him)]

(In reply to Grover Wimberly IV [:Grover-QA] from comment #13)
> I was able to verify this in Windows and Ubuntu/Linux, but I cannot seem to
> get any visuals regarding updating Firefox in the hamburger window in the
> Mac OSX build. Is this intended due to the difference in interface of Mac
> OSX?

Nope, it should work the same way on OS X I think... I don't know off-hand why it wouldn't work on OS X... I've heard my colleagues say they managed to use the new UI, I *think* that was on OS X... Sorry I'm not more help! Not sure how to debug this. I guess you can look, in the build where you're trying to get this to appear, (a) if it works without the photon pref flipped, and (b) if you open view-source:chrome://browser/content/browser.xul, and find in page for "panel-banner-item", does that get a hit? Maybe the nightly for OS X was spun on a build before this bug merged, or something?

Comment 15

[u554753]

Got it, I will try the suggestions from Comment 14 and follow-up.

The way I verified Windows and Ubuntu was from downloading yesterday's Nightly build and flipping the pref and simply waiting for the icon.

Comment 16

[u554753]

Ok, I was able to get a different notification UI element to appear on OSX from testing another feature in conjunction with this one. Looks like everything is good.