Closed Bug 1240085 Opened 4 years ago Closed 4 years ago

Notifications appear in middle of screen (not adjusted for 2x HiDPI)

Categories

(Toolkit :: Notifications and Alerts, defect)

46 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla47
Tracking Status
e10s - ---
firefox45 --- unaffected
firefox46 + fixed
firefox47 --- verified

People

(Reporter: jaws, Assigned: jfkthame)

References

(Depends on 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files, 5 obsolete files)

357.85 KB, image/png
Details
13.31 KB, patch
emk
: review+
Details | Diff | Splinter Review
Attached image Screenshot of bug
I updated my video drivers yesterday and after a restart now all of my desktop notifications from Firefox are appearing in the middle of my screen.

window.screen on a regular webpage shows
> Screen { availWidth: 1920, availHeight: 1040, width: 1920, height: 1080, colorDepth: 24, pixelDepth: 24, top: 0, left: 0, availTop: 0, availLeft: 0 }

window.screen for the browser window shows
> Screen { availWidth: 1920, availHeight: 1040, width: 1920, height: 1080, colorDepth: 24, pixelDepth: 24, top: 0, left: 0, availTop: 0, availLeft: 0 }

and the browser correctly paints everything in the browser chrome and content to fill the window when the window is maximized.

layout.css.devPixelsPerPx is set to -1.0.
My resolution of this display is 3840x2160 and it has display scaling set to 200%.
Attachment #8708483 - Attachment description: Screencast of bug → Screenshot of bug
I used mozregression and tracked it down to bug 890156. Jonathan, can you take this?
Blocks: 890156
Flags: needinfo?(jfkthame)
Keywords: regression
6:04.67 LOG: MainThread mozversion INFO application_buildid: 20160112231330
 6:04.67 LOG: MainThread mozversion INFO application_changeset: 98756a36223c1a2b51cd0368736b728429038caf
 6:04.67 LOG: MainThread mozversion INFO application_display_name: Nightly
 6:04.67 LOG: MainThread mozversion INFO application_id: {ec8030f7-c20a-464f-9b0e-13a3a9e97384}
 6:04.67 LOG: MainThread mozversion INFO application_name: Firefox
 6:04.67 LOG: MainThread mozversion INFO application_remotingname: firefox
 6:04.67 LOG: MainThread mozversion INFO application_repository: https://hg.mozilla.org/integration/mozilla-inbound
 6:04.67 LOG: MainThread mozversion INFO application_vendor: Mozilla
 6:04.67 LOG: MainThread mozversion INFO application_version: 46.0a1
 6:04.67 LOG: MainThread mozversion INFO platform_buildid: 20160112231330
 6:04.67 LOG: MainThread mozversion INFO platform_changeset: 98756a36223c1a2b51cd0368736b728429038caf
 6:04.67 LOG: MainThread mozversion INFO platform_repository: https://hg.mozilla.org/integration/mozilla-inbound
 6:04.67 LOG: MainThread mozversion INFO platform_version: 46.0a1
Was this inbound build good, bad, or broken? (type 'good', 'bad', 'skip', 'retry', 'back' or 'exit' and press Enter): good
 6:14.57 LOG: MainThread Bisector INFO Narrowed inbound regression window from [f58e7ca0, a9f9b36c] (3 revisions) to [98756a36, a9f9b36c] (2 revisions) (~1 steps left)
 6:14.57 LOG: MainThread main INFO Oh noes, no (more) inbound revisions :(
 6:14.57 LOG: MainThread Bisector INFO Last good revision: 98756a36223c1a2b51cd0368736b728429038caf
 6:14.57 LOG: MainThread Bisector INFO First bad revision: a9f9b36c1a2eec7626e6b749e46ab0a8bf3323e2
 6:14.57 LOG: MainThread Bisector INFO Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=98756a36223c1a2b51cd0368736b728429038caf&tochange=a9f9b36c1a2eec7626e6b749e46ab0a8bf3323e2
You're on Linux, right? I'd guess this is probably the same as bug 1239855. Begun investigating...
Flags: needinfo?(jfkthame)
No, Windows 10 (Build 11082), as the screenshot shows ;)
Flags: needinfo?(jfkthame)
Duplicate of this bug: 1240328
It also occurs on Linux (with the difference that the notification is at the middle in the top part of the screen).
OS: Unspecified → All
Hardware: Unspecified → All
Duplicate of this bug: 1241178
[Tracking Requested - why for this release]: Alert notifications on Windows and Linux will appear in the center or top-center of the screen, instead of the top-right corner.
I expect this to affect all platforms where we use toolkit/components/alerts/resources/content/alert.js to display these notifications (which doesn't seem to include OS X, afaict).

Currently investigating and experimenting; I think I have some idea what is going wrong, and hope to have a patch before too long....
Flags: needinfo?(jfkthame)
Cool; thanks, Jonathan!
Tracking for 46 since this is a recent regression. 
Since a duplicate, bug 1241178, was marked for e10s tracking I'm adding that flag as well.
Blocks: 1242449
Duplicate of this bug: 1242647
Duplicate of this bug: 1242612
These methods must operate with CSS pixels because they are DOM0 API and visible from content.
I removed a redundant comment. The return type is self-descriptive enough.

This patch will also fix this bug.
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #8715269 - Flags: review?(jfkthame)
It's correct that these methods need to revert to CSS pixels (that was an erroneous approach in bug 890156, sorry!). So this patch is a start, and fixes this bug here, but unfortunately it will also regress behavior of window placement in some multi-monitor/mixed-dpi scenarios.

I'm currently testing some additional patches that aim to help with this, and hope to have something usable soon.
Comment on attachment 8715269 [details] [diff] [review]
Fix pixel units for screenX, screenY, and moveTo()

(In reply to Jonathan Kew (:jfkthame) from comment #16)
> but unfortunately it will also regress behavior of
> window placement in some multi-monitor/mixed-dpi scenarios.

Ah, you are right. Canceling the review for now.
Attachment #8715269 - Flags: review?(jfkthame)
Assignee: VYV03354 → nobody
Status: ASSIGNED → NEW
I think what we need to do here is something along these lines. Will be testing a bit more locally before flagging for review, but so far this is working well for me.
Attachment #8715821 - Attachment is obsolete: true
Attachment #8715822 - Attachment is obsolete: true
Comment on attachment 8715961 [details] [diff] [review]
Revert to CSS-pixel units for screenX, screenY, moveTo() APIs, and adjust the origin for secondary displays with differing resolution to avoid overlapping coordinate spaces

Review of attachment 8715961 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/nsIScreen.idl
@@ +80,5 @@
> +   *
> +   * This seems poorly named (something like devicePixelsPerDesktopPixel
> +   * would be more accurate/explicit), but given that it is exposed to
> +   * content as well as used internally, it's probably not worth the
> +   * disruption of changing it.

Is this really exposed to content?
The content window.screen maps to WebIDL Screen rather than XPCOM nsIScreen. I couldn't find this property on window.screen.
Comment on attachment 8715961 [details] [diff] [review]
Revert to CSS-pixel units for screenX, screenY, moveTo() APIs, and adjust the origin for secondary displays with differing resolution to avoid overlapping coordinate spaces

Review of attachment 8715961 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsGlobalWindow.cpp
@@ +5087,5 @@
>                                              : DesktopToLayoutDeviceScale(1.0);
> +  DesktopRect screenRectDesk = screenRectDev / scale;
> +
> +  CSSPoint cssPt =
> +    LayoutDevicePoint(x - screenRectDesk.x, y - screenRectDesk.y) /

It should use screenRectDev rather than screenRectDesk. Otherwise it will not convert the coordinates correctly at least on system DPI-aware Windows (that is, Win7 or earlier or when we revert the manifest change to disable per-monitor DPI support).
I don't know why screenRectDesk works on OS X.
For all other systems, it should have no difference because screenRectDesk == screenRectDev on those systems.
(In reply to Masatoshi Kimura [:emk] from comment #21)
> Comment on attachment 8715961 [details] [diff] [review]
> Revert to CSS-pixel units for screenX, screenY, moveTo() APIs, and adjust
> the origin for secondary displays with differing resolution to avoid
> overlapping coordinate spaces
> 
> Review of attachment 8715961 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/nsIScreen.idl
> @@ +80,5 @@
> > +   *
> > +   * This seems poorly named (something like devicePixelsPerDesktopPixel
> > +   * would be more accurate/explicit), but given that it is exposed to
> > +   * content as well as used internally, it's probably not worth the
> > +   * disruption of changing it.
> 
> Is this really exposed to content?
> The content window.screen maps to WebIDL Screen rather than XPCOM nsIScreen.
> I couldn't find this property on window.screen.

Ah, right - I should have said "exposed to front-end code and add-ons" (I believe).
Attachment #8715961 - Attachment is obsolete: true
Attachment #8715961 - Flags: review?(VYV03354)
Comment on attachment 8716289 [details] [diff] [review]
Revert to CSS-pixel units for screenX, screenY, moveTo() APIs, and adjust the origin for secondary displays with differing resolution to avoid overlapping coordinate spaces

Review of attachment 8716289 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsGlobalWindow.cpp
@@ +7052,5 @@
>      aError.Throw(NS_ERROR_FAILURE);
>      return;
>    }
>  
> +  LayoutDevicePoint devPos;

nit: move this down to where this is first used.

@@ +7072,5 @@
> +    double scale;
> +    screen->GetDefaultCSSScaleFactor(&scale);
> +    devPos = cssPos * CSSToLayoutDeviceScale(scale);
> +    devPos.x += screenLeft;
> +    devPos.y += screenTop;

This should also be device pixels, no?
Attachment #8716289 - Attachment is obsolete: true
Attachment #8716289 - Flags: review?(VYV03354)
Comment on attachment 8716339 [details] [diff] [review]
Revert to CSS-pixel units for screenX, screenY, moveTo() APIs, and adjust the origin for secondary displays with differing resolution to avoid overlapping coordinate spaces

Looks good.
Attachment #8716339 - Flags: review?(VYV03354) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d6e0141de4fab594b48d54663a4cc90fc78e248
Bug 1240085 - Revert to CSS-pixel units for screenX, screenY, moveTo() APIs, and adjust the origin for secondary displays with differing resolution to avoid overlapping coordinate spaces. r=emk
https://hg.mozilla.org/mozilla-central/rev/7d6e0141de4f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Comment on attachment 8716339 [details] [diff] [review]
Revert to CSS-pixel units for screenX, screenY, moveTo() APIs, and adjust the origin for secondary displays with differing resolution to avoid overlapping coordinate spaces

>diff --git a/widget/nsIScreen.idl b/widget/nsIScreen.idl
>--- a/widget/nsIScreen.idl
>+++ b/widget/nsIScreen.idl
>@@ -69,13 +69,30 @@ interface nsIScreen : nsISupports
...
>+  readonly attribute double defaultCSSScaleFactor;
Shouldn't this change come with a new UUID?
(In reply to neil@parkwaycc.co.uk from comment #30)
> >+  readonly attribute double defaultCSSScaleFactor;
> Shouldn't this change come with a new UUID?

We have removed the UUID rev'ing requirement:
https://lists.mozilla.org/pipermail/dev-platform/2016-January/013297.html
Attachment #8715269 - Attachment is obsolete: true
Comment on attachment 8716339 [details] [diff] [review]
Revert to CSS-pixel units for screenX, screenY, moveTo() APIs, and adjust the origin for secondary displays with differing resolution to avoid overlapping coordinate spaces

Approval Request Comment
[Feature/regressing bug #]: Bug 890156 (mixed-DPI support for Windows)

[User impact if declined]: Incorrect positioning of notifications on hidpi display. Incorrect scaling of DOM API coordinates (screenX, screenY, moveTo() method) may result in other breakage for websites as well as within Firefox or addons, as this is API that is exposed to the web.

[Describe test coverage new/current, TreeHerder]: Tested manually; we don't have support for multi-screen, mixed-dpi configurations in automation.

[Risks and why]: Moderate risk; there's some complexity here, but mitigated by the fact that for non-hidpi/single-monitor systems, most of the complexity drops out.

[String/UUID change made/needed]: Adds a new attribute to nsIScreen, but we no longer need to rev the UUID for this.


(If we don't uplift this patch, I think we should back out the full set of patches from bug 890156 and its followups on Aurora. The incorrect behavior of the DOM screen APIs should not be shipped to release channels.)
Attachment #8716339 - Flags: approval-mozilla-aurora?
Unfortunately this seems to have caused a large regression on the tp5o scroll benchmark when e10s is enabled. :( See bug 1247049.
This is now fixed for me. Thanks!
Status: RESOLVED → VERIFIED
Comment on attachment 8716339 [details] [diff] [review]
Revert to CSS-pixel units for screenX, screenY, moveTo() APIs, and adjust the origin for secondary displays with differing resolution to avoid overlapping coordinate spaces

Cancelling aurora approval request as bug 890156 has now been backed out of aurora-46, making this irrelevant.
Attachment #8716339 - Flags: approval-mozilla-aurora?
This is not fixed for me, I'm now seeing notifications on the left of the screen.
Marking fixed for 46.  I'll see if it makes sense to track bug 1248427 instead.
Assignee: nobody → jfkthame
Version: unspecified → 46 Branch
You need to log in before you can comment on or make changes to this bug.