Closed Bug 1781747 Opened 2 years ago Closed 2 years ago

black border on the right side of the notification

Categories

(Core :: Widget: Win32, defect, P2)

Firefox 104
defect

Tracking

()

VERIFIED FIXED
105 Branch
Tracking Status
firefox-esr91 --- unaffected
firefox-esr102 --- verified
firefox103 --- verified
firefox104 --- verified
firefox105 --- verified

People

(Reporter: 6k64x4ma, Assigned: emilio)

References

(Regression)

Details

(Keywords: regression)

Attachments

(2 files)

Attached image screenshot.png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:104.0) Gecko/20100101 Firefox/104.0

Steps to reproduce:

Display a notification.
(e.g. take a screenshot and select "copy".)

Actual results:

There is a black border on the right side of the notification.

Expected results:

This shouldn't be there.

regression range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=bfe7d717e706a5cd43f95434f4f33bc7ee9aaf2c&tochange=fd29999c99c1fd0c246429304e7dd270946366a0

From above regression range, it seems to be regressed by Bug 1776498 .

Component: Untriaged → Widget: Win32
Keywords: regression
Product: Firefox → Core
Regressed by: 1776498

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

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

Flags: needinfo?(jteh)
Flags: needinfo?(jteh) → needinfo?(emilio)

Err, don't know what happened to my comment...

Emilio, any ideas here? This alert window doesn't seem to be using MozillaDropShadowWindowClass in either FF 102 or latest Nightly, so I don't understand why there's a change here.

The bug has a release status flag that shows some version of Firefox is affected, thus it will be considered confirmed.

Status: UNCONFIRMED → NEW
Ever confirmed: true
See Also: → 1782046
Assignee: nobody → emilio

I do get a "popup" window, initially with a non-transparent window type, which causes us to enable the shadow:

CreateWindow(type = 3, MozillaDropShadowWindowClass)
#01: nsWindow::Create (C:\moz\mozilla-unified\widget\windows\nsWindow.cpp:1027)
#02: nsIWidget::Create (C:\moz\mozilla-unified\widget\nsIWidget.h:480)
#03: mozilla::AppWindow::Initialize (C:\moz\mozilla-unified\xpfe\appshell\AppWindow.cpp:228)
#04: nsAppShellService::JustCreateTopWindow (C:\moz\mozilla-unified\xpfe\appshell\nsAppShellService.cpp:714)
#05: nsAppShellService::CreateTopLevelWindow (C:\moz\mozilla-unified\xpfe\appshell\nsAppShellService.cpp:179)
#06: nsAppStartup::CreateChromeWindow (C:\moz\mozilla-unified\toolkit\components\startup\nsAppStartup.cpp:750)
#07: nsWindowWatcher::CreateChromeWindow (C:\moz\mozilla-unified\toolkit\components\windowwatcher\nsWindowWatcher.cpp:438)
#08: nsWindowWatcher::OpenWindowInternal (C:\moz\mozilla-unified\toolkit\components\windowwatcher\nsWindowWatcher.cpp:988)
#09: nsWindowWatcher::OpenWindow (C:\moz\mozilla-unified\toolkit\components\windowwatcher\nsWindowWatcher.cpp:294)
#10: nsXULAlerts::ShowAlertWithIconURI (C:\moz\mozilla-unified\toolkit\components\alerts\nsXULAlerts.cpp:362)
#11: nsXULAlerts::ShowAlert (C:\moz\mozilla-unified\toolkit\components\alerts\nsXULAlerts.cpp:167)
#12: `anonymous namespace'::ShowWithBackend (C:\moz\mozilla-unified\toolkit\components\alerts\nsAlertsService.cpp:131)
#13: nsAlertsService::ShowPersistentNotification (C:\moz\mozilla-unified\toolkit\components\alerts\nsAlertsService.cpp:243)
#14: nsAlertsService::ShowAlert (C:\moz\mozilla-unified\toolkit\components\alerts\nsAlertsService.cpp:200)
#15: nsAlertsService::ShowAlertNotification (C:\moz\mozilla-unified\toolkit\components\alerts\nsAlertsService.cpp:195)
#16: XPTC__InvokebyIndex[C:\moz\mozilla-unified\obj-x86_64-pc-mingw32\dist\bin\xul.dll +0x95811d2]
#17: CallMethodHelper::Call (C:\moz\mozilla-unified\js\xpconnect\src\XPCWrappedNative.cpp:1179)
#18: XPCWrappedNative::CallMethod (C:\moz\mozilla-unified\js\xpconnect\src\XPCWrappedNative.cpp:1125)
#19: XPC_WN_CallMethod (C:\moz\mozilla-unified\js\xpconnect\src\XPCWrappedNativeJSOps.cpp:965)
#20: CallJSNative (C:\moz\mozilla-unified\js\src\vm\Interpreter.cpp:417)
#21: js::InternalCallOrConstruct (C:\moz\mozilla-unified\js\src\vm\Interpreter.cpp:505)
#22: Interpret (C:\moz\mozilla-unified\js\src\vm\Interpreter.cpp:3325)
#23: js::RunScript (C:\moz\mozilla-unified\js\src\vm\Interpreter.cpp:389)
#24: js::InternalCallOrConstruct (C:\moz\mozilla-unified\js\src\vm\Interpreter.cpp:537)
#25: js::jit::DoCallFallback (C:\moz\mozilla-unified\js\src\jit\BaselineIC.cpp:1583)
#26: ??? (???:???)

We then create a child window for the contents.

These don't get styles early enough to make a shadow decision before Show().
This also matches the previous behavior (nothing would set
nsWidgetInitData::mDropShadow for these windows), so this is the less risky
fix.

It seems somewhat sketchy to use a popup window for these to begin with (Linux
for example maps them to a toplevel Window, at least on Wayland...), but let's
not change too much right now.

The hbrBackground change is a no-op because we can't have a brush before
Create() is called, we update it in SetBackgroundColor().

Flags: needinfo?(emilio)

:emilio what is the severity of this and the potential risk on your patch?
It was introduced by Bug 1776498 (S2 bug).
I was looking for an uplift request for Bug 1776498, but wondering about the regression before considering it or if it should ride the train with 104.

Flags: needinfo?(emilio)
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/52ac837db77d
Don't use shadow class for non-menupopup popup windows. r=Jamie,handyman

The patch is not too risky in my opinion, it preserves previous behavior and it's not complex. So if we need to uplift the regressing bug, I think uplifting this one should not be a concern.

Flags: needinfo?(emilio)

Thanks for the information, will follow up on this pending uplift request review on the regressor

Severity: -- → S2
Priority: -- → P2

For 104, this should be uplifted to beta either way. If an uplift request could be added when appropriate.

Comment on attachment 9287522 [details]
Bug 1781747 - Don't use shadow class for non-menupopup popup windows. r=Jamie,handyman

Beta/Release Uplift Approval Request

  • User impact if declined: comment 0
  • 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: comment 0 (take screenshot -> copy)
  • List of other uplifts needed: none
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Restores previous behavior as faithfully as possible.
  • String changes made/needed: none
  • Is Android affected?: No
Attachment #9287522 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 105 Branch

Comment on attachment 9287522 [details]
Bug 1781747 - Don't use shadow class for non-menupopup popup windows. r=Jamie,handyman

Approved for 104.0b5

Attachment #9287522 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [qa-triaged]

Reproduced the initial issue using old Nightly from 2022-07-27, verified that using latest Nightly 105.0a1 on Windows 10 64bit the push notification does not have a black border anymore on the sides.

Comment on attachment 9287522 [details]
Bug 1781747 - Don't use shadow class for non-menupopup popup windows. r=Jamie,handyman

Beta/Release Uplift Approval Request

  • User impact if declined: needed for bug 1776498
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: comment 0 etc
  • List of other uplifts needed: none
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Relatively simple patch restoring previous behavior.
  • String changes made/needed: none
  • Is Android affected?: No
Attachment #9287522 - Flags: approval-mozilla-release?

Also verified this is fixed using latest Beta build from treeherder (https://treeherder.mozilla.org/jobs?repo=mozilla-beta&selectedTaskRun=WtW7p-10SmyoG6F3RPnOjA.0&revision=d3f009907db6b7159ad7df18a09bec35c3d9070b) on Windows 10 64bit. If QA is also needed here after approval in Release please re-add the qe-verify+ flag, not sure if the verification from 1776498 will cover the change made here.

Status: RESOLVED → VERIFIED
Flags: qe-verify+

Comment on attachment 9287522 [details]
Bug 1781747 - Don't use shadow class for non-menupopup popup windows. r=Jamie,handyman

Approved for 103.0.2, thanks.

Attachment #9287522 - Flags: approval-mozilla-release? → approval-mozilla-release+

Verified this is fixed in Firefox 103.0.2 as well.

Comment on attachment 9287522 [details]
Bug 1781747 - Don't use shadow class for non-menupopup popup windows. r=Jamie,handyman

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Needed for the uplift of bug 1776498.
  • User impact if declined: see bug 1776498
  • Fix Landed on Version: 103
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This patch is simple enough / undoes a behavior change from bug 1776498 which wasn't intended.
Attachment #9287522 - Flags: approval-mozilla-esr102?

Comment on attachment 9287522 [details]
Bug 1781747 - Don't use shadow class for non-menupopup popup windows. r=Jamie,handyman

Approved for 102.2esr.

Attachment #9287522 - Flags: approval-mozilla-esr102? → approval-mozilla-esr102+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: