Content in Update available notification misaligned (due to missing icon)
Categories
(Firefox :: Toolbars and Customization, defect, P2)
Tracking
()
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)
56.16 KB,
image/png
|
Details | |
68.56 KB,
image/png
|
Details | |
48 bytes,
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
|
Details | Review |
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.
Comment 1•2 years ago
|
||
The severity field is not set for this bug.
:dao, could you have a look please?
For more information, please visit auto_nag documentation.
Updated•2 years ago
|
Comment 2•2 years ago
|
||
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?
Updated•2 years ago
|
Comment 3•2 years ago
|
||
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.
Comment 4•2 years ago
|
||
:bigiri, since you are the author of the regressor, bug 1705188, could you take a look?
For more information, please visit auto_nag documentation.
Updated•2 years ago
|
Assignee | ||
Comment 5•2 years ago
|
||
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.
Comment 6•2 years ago
|
||
(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.
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Comment 7•2 years ago
|
||
Restoring CSS for the Update Available notification that was mistakenly removed.
Comment 8•2 years ago
|
||
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
Comment 10•2 years ago
|
||
bugherder |
Comment 11•2 years ago
|
||
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.
Comment 12•2 years ago
|
||
Bernard, should we mark 102 as wontfix or should be uplift this patch? Thanks
Assignee | ||
Comment 13•2 years ago
|
||
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
- Disable Popup Auto-Hide
- In console run
AppMenuNotifications.showNotification(
"update-manual",
{ callback: () => console.log("update-manual called") }
);
- Click on the browser and verify that the update dialog appears with the update icon.
- Click the blue button (it should say download in this instance)
- In the console run
AppMenuNotifications.showNotification(
"update-downloading",
{ callback: () => console.log("update-downloading called") },
undefined,
{ dismissed: true }
);
- Click back on the browser and verify that in the hamburger menu the icon appears
- Click the download button in the hamburger menu
- In the console run
AppMenuNotifications.showNotification("update-unsupported");
- Verify that the unable to update icon appears.
- Click dismiss.
- 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
Assignee | ||
Updated•2 years ago
|
Comment 14•2 years ago
|
||
Comment on attachment 9278476 [details]
Bug 1765453 - Restoring Update Notification CSS r=gijs!
Approved for 102 beta 8, thanks.
Comment 15•2 years ago
|
||
bugherder uplift |
Updated•2 years ago
|
Assignee | ||
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Comment 16•2 years ago
•
|
||
Verified fixed on Firefox 102.0b8 and Nightly 103.0a1 using Win 10, Ubuntu 22.04 and MacOS 11.
Updated•2 years ago
|
Updated•2 years ago
|
Description
•