1248675 - Dragging window to secondary screen with hwa disabled creates twin window with different scaling

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[u279076]

1. Start Firefox with a new profile
2. Disable hardware acceleration and restart Firefox
3. Drag window to a secondary screen

Result: https://youtu.be/KP2jyxwrY0Y

mozilla-central
===============
Last good revision: 9ddf0da90fb3bc1ae29966dc596013fc54a44bd2 (2015-12-29)
First bad revision: c690c50b2b543b420803e8192d6e08e06b20e0a3 (2015-12-30)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9ddf0da90fb3bc1ae29966dc596013fc54a44bd2&tochange=c690c50b2b543b420803e8192d6e08e06b20e0a3

mozilla-inbound
===============
Last good revision: fbe047a4fbf493a602d30ff0fb903b59ab384d09
First bad revision: c0b3b26d05cfdcb53d9877a6f9388f7d3d2e20d6
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=fbe047a4fbf493a602d30ff0fb903b59ab384d09&tochange=c0b3b26d05cfdcb53d9877a6f9388f7d3d2e20d6

Blame:
Markus Stange — Bug 1187322 - Don't require accelerated OpenGL contexts for BasicCompositor on OS X. r=jrmuizel

Comment 1

[u279076]

[Tracking Requested - why for this release]: regression causes inability to use Firefox on a secondary screen when hardware acceleration is disabled on Mac OS X.

Comment 2

[Markus Stange [:mstange]]

Does resizing the window fix it?

Comment 3

[u279076]

(In reply to Markus Stange [:mstange] from comment #2)
> Does resizing the window fix it?

Yes.

Comment 4

[Jonathan Kew [:jfkthame]]

Does the tryserver build from https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e49e0b8585f (see bug 1240533 comment 16) fix this?

Comment 5

[u279076]

(In reply to Jonathan Kew (:jfkthame) from comment #4)
> Does the tryserver build from
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e49e0b8585f

No, it still reproduces.

Comment 6

[Markus Stange [:mstange]]

One problem is that the mBounds field of the nsChildView stays at the old value until the window is resized. mBounds is in LayoutDeviceIntPixels, so it definitely needs to be updated at some point during the resolution change.

Comment 7

[Markus Stange [:mstange]]

Jonathan, is this something you could look into?

Comment 8

[Jonathan Kew [:jfkthame]]

Yes, I'm trying to work through various DPI-related issues; hope to dig into this next week. (Though contributions are very welcome if anyone else has time to debug it in the meantime!)

Comment 9

[Markus Stange [:mstange]]

Great, thanks!

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Too late for 45...

Comment 11

[Doug Turner (:dougt)]

any update?

Comment 12

[Jonathan Kew [:jfkthame]]

I experimented a bit with this, in particular by addressing the nsChildView's mBounds issue Markus noted in comment 6; this fixes the behavior nicely in some cases, but unfortunately not all; there's still a scenario where the window ends up badly broken.

Comment 13

[Jonathan Kew [:jfkthame]]

Attachment: Update the cached mBounds in nsChildView when its backing scale factor (display DPI) changes

OK, this does fix the problem with the nsChildView bounds. The remaining issue I was seeing here turns out to be bug 1256576, for which I have just posted a separate patch.

Comment 14

[Jonathan Kew [:jfkthame]]

Try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=62ae6747aa1d

Comment 15

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e4c92961007c882bca44d563db3afb0fb2b2b82
Bug 1248675 - Update the cached mBounds in nsChildView when its backing scale factor (display DPI) changes. r=mstange

Comment 16

[Jonathan Kew [:jfkthame]]

Comment on attachment 8733792 [details] [diff] [review]
Update the cached mBounds in nsChildView when its backing scale factor (display DPI) changes

Approval Request Comment
[Feature/regressing bug #]: bug 1187322 (I think the bug here predates that, but the change in 1187322 exposed it as a user-visible issue)

[User impact if declined]: incorrectly drawn window after dragging between hi- and lo-dpi screens when HWA disabled on OS X

[Describe test coverage new/current, TreeHerder]: tested manually (involves multiple monitors & mixed DPI)

[Risks and why]: minimal; trivial fix to update widget bounds

[String/UUID change made/needed]: none

Note that on aurora (47), this is dependent on the fix for bug 1256576 (also nominated for uplift); on beta (46), that issue is not present so the patch here should be sufficient.

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8733792 [details] [diff] [review]
Update the cached mBounds in nsChildView when its backing scale factor (display DPI) changes

Support for mixed DPI screens for OS X.
For aurora, please uplift 1256576 first.
For beta this should be OK to uplift without anything special

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0e4c92961007

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/809b0f59b556

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e685b8ec0483

Comment 21

[Kent James (:rkent)]

It has been suggested to uplift this to the Thunderbird branch of m-esr45 to resolve bug Thunderbird bug 1266354.

Comment 22

[Kent James (:rkent)]

Pushed to THUNDERBIRD_45_VERBRANCH https://hg.mozilla.org/releases/mozilla-esr45/rev/18fab019eb66

Comment 23

[Nomis101]

It seems this does not build for esr45
nsChildView.mm:987:11: error: no viable overloaded '='

Comment 24

[Kent James (:rkent)]

Right, backed out http://hg.mozilla.org/releases/mozilla-esr45/rev/999f56e779ad

Jonathan, we would still like to land this on the Thunderbird branch of mozilla-esr45 so we can fix this issue. I have no experience with the Objective-C code here, but this is only a two line change. Is there an obvious way to port this to the mozilla-esr45?

Comment 25

[Jonathan Kew [:jfkthame]]

Attachment: Update the cached mBounds in nsChildView when its backing scale factor (display DPI) changes

Here's the patch ported to esr45. The widget's mBounds field wasn't using strongly-typed units in mozilla-45, so we need to convert from LayoutDevice pixels to "unknown" units for the assignment.

Comment 26

[Kent James (:rkent)]

Thanks, Jonathan!

Comment 27

[Kent James (:rkent)]

Comment on attachment 8749985 [details] [diff] [review]
Update the cached mBounds in nsChildView when its backing scale factor (display DPI) changes

Pushed to THUNDERBIRD_45_VERBRANCH https://hg.mozilla.org/releases/mozilla-esr45/rev/04ca47a6ad05

Comment 28

[Bogdan Maris, Desktop Test Engineering]

Reproduced the initial issue using old Nightly 2016-02-16 on a Macbook Pro + second monitor. Verified that the issue is fixed using Firefox 46.0.1 RC, 47beta9, latest Developer Edition and latest Nightly on the same hardware.