After setting different layout.css.devPixelsPerPx, push notifications aren't displayed at the lower right corner of the screen

RESOLVED FIXED in Firefox 55

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: 684sigma, Assigned: emk)

Tracking

({regression})

55 Branch
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 unaffected, firefox-esr52 unaffected, firefox53 unaffected, firefox54 unaffected, firefox55+ fixed)

Details

Attachments

(2 attachments)

During the testing of Bug 1352764, Bug 1352765 and Bug 1352769, I detected a problem in Nightly 55 (2017-04-01). It didn't happen in Nightly (2017-03-20).
Push notifications and notifications from Video Download Helper extension don't appear exactly at the lower right corner of the screen. Here's how to reproduce the bug:

1. Open about:config, set the pref layout.css.devPixelsPerPx to 0.9
2. Open https://nickersoft.github.io/push.js/ , click on the button "Demo" on the page
3. Open about:config, set the pref layout.css.devPixelsPerPx to 1.6
4. Open https://nickersoft.github.io/push.js/ , click on the button "Demo" on the page

Result: when the pref is set to 0.9, push notification appears further from the upper left corner of the screen. when the pref is set to 1.6, push notification appears closer to the upper left corner of the screen.

Expected: Push notification should always appear at the lower right corner of the screen.
Has STR: --- → yes
Keywords: regression
I can't reproduce it with screen resolution at 100% or 125% on 1080p display. Could you attach a screenshot of the issue.
Nevermind, I'm able to reproduce it with an old profile and Nightly. With a fresh profile, I'm hitting bug 1352772.
It regressed by bug 1194751:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9a714b38cd55604a5aff97c7e9a3fcdf41338f1e&tochange=7d3e8986a676a085a65e8bf0e060b66b8c344cd2

A fix about the notification tooltip being offset has been pushed in bug 1351329, but it's incomplete with a non-default value for layout.css.devPixelsPerPx.
Blocks: 1194751
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Ever confirmed: true
Flags: needinfo?(VYV03354)
That's strange... After bug 1351329, the defaultCSSScale calculation should be exactly the same as before bug 1194751.
Hm, defaultCSSScale is 1.0 (system scale) regardless of layout.css.devPixelsPerPx setting on the latest Nightly. DefaultScaleOverride does not work somehow...
Version: 52 Branch → 55 Branch
After bug 1351329, the screen list will be initialized very early in the startup process. So per-profile layout.css.devPixelsPerPx setting will not affect defaultCSSScaleFactor. If I set the pref globally (via defaults/pref), desktop notifications are positioned correctly.

We need to refresh the screen list when layout.css.devPixelsPerPx is changed.
(In reply to Masatoshi Kimura [:emk] from comment #6)
> After bug 1351329, the screen list will be initialized very early in the

After bug 1194751
Flags: needinfo?(VYV03354)
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Comment on attachment 8854397 [details]
Bug 1352773 - Take into account DefaultScaleOverride in Screen::GetDefaultCSSScaleFactor.

https://reviewboard.mozilla.org/r/126346/#review128882

::: layout/base/nsPresContext.cpp:694
(Diff revision 1)
>  void
>  nsPresContext::PreferenceChanged(const char* aPrefName)
>  {
>    nsDependentCString prefName(aPrefName);
>    if (prefName.EqualsLiteral("layout.css.dpi") ||
>        prefName.EqualsLiteral("layout.css.devPixelsPerPx")) {
> +    ScreenManager::GetSingleton().RefreshScreens();

I'd like to keep the ScreenManager interface minimum and restricted to widget level if possiblle.

Could you change Screen::GetDefaultCSSScaleFactor to use nsIWidget::DefaultScaleOverride() as override? I can't find where does cocoa use this pref but I assume the pref applies to all platforms.

Also I noticed that nsIWidget::DefaultScaleOverride() is used on some hot paths so it should be updated to use pref cache and probably cache the parsing result.

Thanks for fixing this!
Attachment #8854397 - Flags: review?(kchen) → review-
Tracking 55+ for this regression.
Comment on attachment 8854397 [details]
Bug 1352773 - Take into account DefaultScaleOverride in Screen::GetDefaultCSSScaleFactor.

https://reviewboard.mozilla.org/r/126346/#review128882

> I'd like to keep the ScreenManager interface minimum and restricted to widget level if possiblle.
> 
> Could you change Screen::GetDefaultCSSScaleFactor to use nsIWidget::DefaultScaleOverride() as override? I can't find where does cocoa use this pref but I assume the pref applies to all platforms.
> 
> Also I noticed that nsIWidget::DefaultScaleOverride() is used on some hot paths so it should be updated to use pref cache and probably cache the parsing result.
> 
> Thanks for fixing this!

I tried caching, but it did not work. Probably because I could not guarantee that the pref callback for "layout.css.devPixelsPerPx" is called early enough. It would not be a good idea to change Screen::GetDefaultCSSScaleFactor so that it uses nsIWidget::DefaultScaleOverride() without caching.
Attachment #8854397 - Flags: review- → review?(kchen)
Posted patch aa0b3009c855Splinter Review
Failed attempt for the record.
(In reply to Masatoshi Kimura [:emk] from comment #11)
> Comment on attachment 8854397 [details]
> Bug 1352773 - Refresh the screen list when the "layout.css.devPixelsPerPx"
> pref is changed.
> 
> https://reviewboard.mozilla.org/r/126346/#review128882
> 
> > I'd like to keep the ScreenManager interface minimum and restricted to widget level if possiblle.
> > 
> > Could you change Screen::GetDefaultCSSScaleFactor to use nsIWidget::DefaultScaleOverride() as override? I can't find where does cocoa use this pref but I assume the pref applies to all platforms.
> > 
> > Also I noticed that nsIWidget::DefaultScaleOverride() is used on some hot paths so it should be updated to use pref cache and probably cache the parsing result.
> > 
> > Thanks for fixing this!
> 
> I tried caching, but it did not work. Probably because I could not guarantee
> that the pref callback for "layout.css.devPixelsPerPx" is called early
> enough. It would not be a good idea to change
> Screen::GetDefaultCSSScaleFactor so that it uses
> nsIWidget::DefaultScaleOverride() without caching.

Did you try Preferences::AddFloatVarCache? You can use that without a callback.
Comment on attachment 8854397 [details]
Bug 1352773 - Take into account DefaultScaleOverride in Screen::GetDefaultCSSScaleFactor.

https://reviewboard.mozilla.org/r/126346/#review129024

::: layout/base/nsPresContext.cpp:694
(Diff revision 1)
>  void
>  nsPresContext::PreferenceChanged(const char* aPrefName)
>  {
>    nsDependentCString prefName(aPrefName);
>    if (prefName.EqualsLiteral("layout.css.dpi") ||
>        prefName.EqualsLiteral("layout.css.devPixelsPerPx")) {
> +    ScreenManager::GetSingleton().RefreshScreens();

Please add a comment here to explain why we need to refresh screens here.
Attachment #8854397 - Flags: review?(kchen) → review+
(In reply to Masatoshi Kimura [:emk] from comment #11)
> I tried caching, but it did not work. Probably because I could not guarantee
> that the pref callback for "layout.css.devPixelsPerPx" is called early
> enough. It would not be a good idea to change
> Screen::GetDefaultCSSScaleFactor so that it uses
> nsIWidget::DefaultScaleOverride() without caching.

Variable caching also use the observer internally so the order of cache update and observer callback is not guaranteed, which is sad.

I think we still need to fix nsIWidget::DefaultScaleOverride() to use caches to avoid read preferences in hot paths.
(In reply to Kan-Ru Chen [:kanru] (UTC-4, EDT) from comment #13)
> Did you try Preferences::AddFloatVarCache? You can use that without a
> callback.

I didn't know about Preferences::AddFloatVarCache! But it did not work either. It is using a callback internally.
Comment on attachment 8854397 [details]
Bug 1352773 - Take into account DefaultScaleOverride in Screen::GetDefaultCSSScaleFactor.

Bug 1353493 made this patch much simpler.
Attachment #8854397 - Flags: review+ → review?(kchen)
Comment on attachment 8854397 [details]
Bug 1352773 - Take into account DefaultScaleOverride in Screen::GetDefaultCSSScaleFactor.

https://reviewboard.mozilla.org/r/126346/#review129470
Attachment #8854397 - Flags: review?(kchen) → review+
Thanks again for fixing this!
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/68a13da9b6f2
Take into account DefaultScaleOverride in Screen::GetDefaultCSSScaleFactor. r=kanru
https://hg.mozilla.org/mozilla-central/rev/68a13da9b6f2
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.