Closed Bug 1214428 Opened 9 years ago Closed 8 years ago

Dragging preview for tab is upscaled on retina mbp with e10s

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 46
Tracking Status
e10s m8+ ---
firefox42 --- unaffected
firefox43 --- wontfix
firefox44 + wontfix
firefox45 --- fixed
firefox46 --- fixed

People

(Reporter: jrmuizel, Assigned: jfkthame)

References

Details

Attachments

(3 files)

This looks bad.
This also appears to impact the thumbnails on the newtab page.
[Tracking Requested - why for this release]: UI regression.
Pretty sure this is a regression from 1197361.
Assignee: nobody → mchang
See Also: → 1215659
This should be fixed since bug 1215659. Is it fixed for you?
Flags: needinfo?(jmuizelaar)
No I can still reproduce this on a Nightly from 2015-12-03
Flags: needinfo?(jmuizelaar)
[Tracking Requested - why for this release]:
Also happening in 43, not 42. 

Bug 1197361, which was the newtab image size rewrite, landed in 44. So this seems to have happened before bug 1197361  or 1215659.
While trying to bisect this, I found out that the upscaling only happens on e10s. Otherwise, it looks fine with e10s disabled.
I couldn't bisect to an exact bug where this looked good on a retina mac with e10s. Instead, I always got an outline of a snapshot being taken. Attached is the screenshot. During bisection, I change the good/bad criteria from "has a non-upscaled /"has upscaled image" to "shows an image"/"shows an outline". This led to bug 863512.
Attached file Bisection log
In case anyone wants to reproduce. 

Gabor, can you please take a look at this? Looks like fallout from bug 863512.
Flags: needinfo?(gkrizsanits)
Unassigning from myself. This is as far as I can take it and it's unrelated to bug 1197361 or 1215659.
Assignee: mchang → nobody
Depends on: 863512
Blocks: 863512
No longer depends on: 863512
Summary: Dragging preview for tab is upscaled on retina mbp → Dragging preview for tab is upscaled on retina mbp with e10s
(In reply to Mason Chang [:mchang] from comment #10)
> Created attachment 8695974 [details]
> Bisection log
> 
> In case anyone wants to reproduce. 
> 
> Gabor, can you please take a look at this? Looks like fallout from bug
> 863512.

I have an mbp and cannot reproduce this (what the attached screenshot shows) could someone provide me an STR? Also before bug 863512 tab dnd did not display anything at all for e10s so it's hardly a regression from that bug...
Flags: needinfo?(gkrizsanits)
Assignee: nobody → gkrizsanits
STR:

1) Load a new tab with any page in it (use a retina mbp).
2) Load a second new tab with any page in it
3) Drag one tab to another position in the tab bar, but do not let go. Look at the screenshot of the page for that tab that pops up. It looks different than without e10s. It looks both bigger and grainier.
So for HiDPI display since canvas does not handle them very well we need to scale the desired canvas size [1], and then the cocoa version of nsDragService [2] scales back the image to reach pixel perfection (if I get it right...).

[1] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#5529
[2] http://mxr.mozilla.org/mozilla-central/source/widget/cocoa/nsDragService.mm#141

Now for e10s we use panel instead of dnd image, and by default in the panel we just display the up-scaled canvas which looks oversized and blurry. As a workaround what I tried to use css width+height on canvas to scale back the canvas to pixel perfection, but for some weird reason only canvas.style.height works as expected. canvas.style.width does weird things mostly, like changing the height of the canvas as well or nothing at all based on its value. I'm confused why that happens.

Jonathan, based on Bug 674373 you've worked a lot around HiDPI display issues, is there a chance you can suggest me a different workaround here? This bug is pretty urgent and important for e10s, so any help would be awesome.
Flags: needinfo?(jfkthame)
Here's an experimental patch that seems to work in my initial testing. To get the scaling to behave OK, I added a wrapper div around the canvas.
Attachment #8702081 - Flags: review?(gkrizsanits)
Comment on attachment 8702081 [details] [diff] [review]
Scale tab-preview image properly for hi-dpi displays with e10s

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

Many thanks, this looks great! I've tried a quite similar workaround but my version did not work out, but this one does.
Attachment #8702081 - Flags: review?(gkrizsanits) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/67474512da0c49ae844305eb28fdfa269e73cf12
Bug 1214428 - Scale tab-preview image properly for hi-dpi displays with e10s. r=gabor
https://hg.mozilla.org/mozilla-central/rev/67474512da0c
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Jonathan, does this need to be uplifted to Beta44 and Aurora45?
(In reply to Ritu Kothari (:ritu) from comment #19)
> Jonathan, does this need to be uplifted to Beta44 and Aurora45?

Well... it's a purely cosmetic issue, not a functional problem. So I'd question whether it's justified to uplift to Beta44, where we're only enabling e10s for an experimental minority of users, AIUI. Given that the patch seems pretty simple and safe, I guess uplifting to 45 should be fine.
Flags: needinfo?(jfkthame)
Comment on attachment 8702081 [details] [diff] [review]
Scale tab-preview image properly for hi-dpi displays with e10s

Approval Request Comment
[Feature/regressing bug #]: e10s
[User impact if declined]: inferior rendering of tab-drag image on hi-dpi (Retina) screen
[Describe test coverage new/current, TreeHerder]: tested manually (I don't know how, or if it's even possible, to test rendering of the drag-image in automation, and anyhow our tests don't run on Retina screens)
[Risks and why]: minimal risk, just wraps <canvas> to apply dpi-appropriate scaling
[String/UUID change made/needed]: n/a
Attachment #8702081 - Flags: approval-mozilla-aurora?
Attachment #8702081 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee: gkrizsanits → jfkthame
[bugday-20160323]

Status: RESOLVED,FIXED -> VERIFIED

Comments:
Test successful.

Component: 
Name 			Firefox
Version 		46.0b9
Build ID 		20160322075646
Update Channel 	        beta
User Agent 		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS                      Windows 7 SP1 x86_64

Expected Results: 
No such preview signs of screenshots

Actual Results: 
As expected
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: