Closed Bug 1765453 Opened 2 years ago Closed 2 years ago

Content in Update available notification misaligned (due to missing icon)

Categories

(Firefox :: Toolbars and Customization, defect, P2)

Desktop
All
defect
Points:
1

Tracking

()

VERIFIED FIXED
103 Branch
Tracking Status
firefox-esr91 --- unaffected
firefox100 --- wontfix
firefox101 --- wontfix
firefox102 --- verified
firefox103 --- verified

People

(Reporter: metasieben, Assigned: bigiri)

References

(Regression)

Details

(Keywords: regression)

Attachments

(3 files)

Attached image sceenshot

The content of the >Update available< notification-panel is misaligned.
Padding on the left seems wrong, not sure if this was a deliberate change or a regression.

The severity field is not set for this bug.
:dao, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(dao+bmo)
Status: UNCONFIRMED → NEW
Component: Theme → Toolbars and Customization
Ever confirmed: true

Based on bug 1703026 my impression is there should be an icon in the missing space, but there is not. It looks like the relevant CSS was removed in bug 1705188. I guess if they shouldn't get an icon, they shouldn't be getting leading inline padding...

Testing on a March 22 nightly build (ie before bug 1705188), I do see an icon. So I think this is just a straight-up regression and this code shouldn't have been removed. Mike, can you confirm?

Flags: needinfo?(dao+bmo) → needinfo?(mconley)
OS: macOS → All
Regressed by: 1705188
Summary: Content in Update availble dialog misaligned → Content in Update available notification misaligned (due to missing icon)
Attached image Figma screenshot

Yes, this looks like unintentional fallout - digging through the Figma documents, it does look like we wanted to keep the icons for the AppMenu update notifications.

Flags: needinfo?(mconley)

:bigiri, since you are the author of the regressor, bug 1705188, could you take a look?
For more information, please visit auto_nag documentation.

Flags: needinfo?(bigiri)
Severity: -- → S3
Priority: -- → P2

Is this the part that should have remained?
https://hg.mozilla.org/mozilla-central/rev/10fed540fa346e87fdd4a1a78fe120344c475caf#l4.210

/* UPDATE */
.popup-notification-icon[popupid="update-available"],
.popup-notification-icon[popupid="update-downloading"],
.popup-notification-icon[popupid="update-manual"],
.popup-notification-icon[popupid="update-other-instance"],
.popup-notification-icon[popupid="update-restart"] {
  background: url(chrome://browser/skin/update-badge.svg) no-repeat center;
  -moz-context-properties: fill;
  fill: var(--panel-banner-item-update-supported-bgcolor);
}

.popup-notification-icon[popupid="update-unsupported"] {
  background: url(chrome://global/skin/icons/warning.svg) no-repeat center;
  -moz-context-properties: fill;
  fill: var(--warning-icon-bgcolor);
}

Looks like it to me.

Flags: needinfo?(bigiri) → needinfo?(gijskruitbosch+bugs)

(In reply to Bernard Igiri from comment #5)

Is this the part that should have remained?
https://hg.mozilla.org/mozilla-central/rev/10fed540fa346e87fdd4a1a78fe120344c475caf#l4.210

I think so, but I haven't tested it. Please test these cases manually by invoking AppMenuNotifications.showNotification, as other tests and code do (but with dummy callbacks for the button event handlers) to check the appearance of the notification panels.

I'm a bit confused about update-downloading, as I don't see a <popupnotification> element in markup for that at all - so perhaps that one is obsolete? It's odd because all the existing ones got a hasicon attribute added (is that attribute still used for something? Worth checking that too - looking at the bug that added those attributes would probably be a good start...). Anyway, I don't know without trying it out, and I don't have time to do that myself, I just triaged the regression.

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(bigiri)
Assignee: nobody → bigiri
Status: NEW → ASSIGNED
Flags: needinfo?(bigiri)

Restoring CSS for the Update Available notification that was mistakenly removed.

Set release status flags based on info from the regressing bug 1705188

Pushed by bigiri@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d96e14f9721b
Restoring Update Notification CSS r=Gijs
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 103 Branch

The patch landed in nightly and beta is affected.
:bigiri, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Flags: needinfo?(bigiri)

Bernard, should we mark 102 as wontfix or should be uplift this patch? Thanks

Comment on attachment 9278476 [details]
Bug 1765453 - Restoring Update Notification CSS r=gijs!

Beta/Release Uplift Approval Request

  • User impact if declined: Update notification icons will be missing.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: 1. Open Browser Toolbox and go to the JS Console
  1. Disable Popup Auto-Hide
  2. In console run
AppMenuNotifications.showNotification(
	"update-manual",
	{ callback: () => console.log("update-manual called") }
);
  1. Click on the browser and verify that the update dialog appears with the update icon.
  2. Click the blue button (it should say download in this instance)
  3. In the console run
AppMenuNotifications.showNotification(
  "update-downloading",
  { callback: () => console.log("update-downloading called") },
  undefined,
  { dismissed: true }
);
  1. Click back on the browser and verify that in the hamburger menu the icon appears
  2. Click the download button in the hamburger menu
  3. In the console run
AppMenuNotifications.showNotification("update-unsupported");
  1. Verify that the unable to update icon appears.
  2. Click dismiss.
  3. Repeat steps 3-5 except with "update-available", "update-other-instance", and "update-restart" in place of "update-manual". They should all show the same icon.
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): CSS changes only. This CSS was in the browser before and is simply being restored.
  • String changes made/needed:
  • Is Android affected?: No
Flags: needinfo?(bigiri)
Attachment #9278476 - Flags: approval-mozilla-beta?
Flags: qe-verify+

Comment on attachment 9278476 [details]
Bug 1765453 - Restoring Update Notification CSS r=gijs!

Approved for 102 beta 8, thanks.

Attachment #9278476 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [qa-triaged]
Whiteboard: [fidefe-maintenance]
Points: --- → 1
Whiteboard: [fidefe-maintenance]
See Also: → 1774210
Whiteboard: [fidefe-maintenance]

Verified fixed on Firefox 102.0b8 and Nightly 103.0a1 using Win 10, Ubuntu 22.04 and MacOS 11.

Whiteboard: [fidefe-maintenance]
QA Whiteboard: [qa-triaged]
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: