Closed Bug 1703821 Opened 8 months ago Closed 8 months ago

The separator space is missing when multiple notifications are displayed in the Hamburger menu

Categories

(Firefox :: Menus, defect)

Desktop
Unspecified
defect

Tracking

()

VERIFIED FIXED
89 Branch
Tracking Status
firefox87 --- unaffected
firefox88 --- unaffected
firefox89 --- verified

People

(Reporter: rares.doghi, Assigned: mhowell)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [proton-hamburger-menu])

Attachments

(4 files)

Attached image multiple_Banners.png

[Affected platforms]:
Platforms: Windows 10

[Preconditions]:

Reach about:config and set browser.proton.enabled = true

[Steps to reproduce]:

  1. Launch the Firefox Browser and have multiple notifications triggered.
  2. Reach the Hamburger menu.

Expected Result:
There should be a separator space between the banner notifications.

Actual Results:
There is no space between the banner notifications.

Has Regression Range: --- → no
Has STR: --- → yes
Severity: -- → S3
Duplicate of this bug: 1704004
Whiteboard: [proton-door-hangers]
Whiteboard: [proton-door-hangers] → [proton-hamburger-menu]

Rares, do you know of a spec for how we should present multiple banners like this? I haven't been able to find any.

Flags: needinfo?(rares.doghi)

I haven't seen any specs related to this, to be honest I was comparing with an older version of Firefox where you could clearly see a bit of space between notifications, maybe Meridel can help us with this ?

Flags: needinfo?(rares.doghi) → needinfo?(mwalkington)

Yes, I think space is needed between notifications but I don't think we have a spec for this. I would like to re-direct to Katie Caldwell but I don't see her handle here. I went ahead and Slacked her about it.

Attached image image-18.png

There should be 4px between items.

Flags: needinfo?(mwalkington)
Assignee: nobody → mhowell
Status: NEW → ASSIGNED

This patch takes a very rudimentary approach to solving the problem of spacing
out multiple notification banners, but I believe it should work because of the
following properties of those banners:

  • There can be any number of addon permission change notifications.
  • There can only be zero or one app update notification, and those are the only
    other type.
  • The addon notifications are always at the top of the menu.

So, this patch adds space after every addon banner, and that's enough to
guarantee there will be space between any two banners. It also means that when
the only banners are addon ones, there will be more than the normal amount of
space after them and before the accounts menu item; that's not desirable, but
it doesn't look bad, and I'm not sure how to prevent it in CSS given the logic
required to make the decision.

Pushed by mhowell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/43fa107e5536
Add some space between multiple app menu banners. r=desktop-theme-reviewers,Itiel
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → 89 Branch
Flags: qe-verify+

Hi Molly, this issue is indeed fixed for the separator space between the Addon banners but, if the user also logs in with a fresh account, the Finish Account Setup notification stretches to the Permission banner one, Should I mark this one as verified as fixed and log a new issue for the space between Finish account and the Extension request banner ?

Flags: needinfo?(mhowell)
Attached image notFixed.png

(In reply to Rares Doghi from comment #9)

Hi Molly, this issue is indeed fixed for the separator space between the Addon banners but, if the user also logs in with a fresh account, the Finish Account Setup notification stretches to the Permission banner one, Should I mark this one as verified as fixed and log a new issue for the space between Finish account and the Extension request banner ?

Yes, please file another bug. Thanks!

Flags: needinfo?(mhowell)

This issue is verified as fixed in our latest beta build 89.0b3, I added a separate issue for the other case with Bug 1707203.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
See Also: → 1709825
You need to log in before you can comment on or make changes to this bug.