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)
Firefox
Tabbed Browser
Tracking
()
RESOLVED
FIXED
Firefox 46
People
(Reporter: jrmuizel, Assigned: jfkthame)
References
Details
Attachments
(3 files)
287.68 KB,
image/jpeg
|
Details | |
1.91 KB,
text/plain
|
Details | |
3.02 KB,
patch
|
gkrizsanits
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
This looks bad.
Comment 1•9 years ago
|
||
This also appears to impact the thumbnails on the newtab page.
Reporter | ||
Updated•9 years ago
|
Keywords: regressionwindow-wanted
Reporter | ||
Comment 2•9 years ago
|
||
[Tracking Requested - why for this release]: UI regression.
tracking-firefox44:
--- → ?
Comment 3•9 years ago
|
||
Pretty sure this is a regression from 1197361.
Assignee: nobody → mchang
Keywords: regressionwindow-wanted
Tracked for FF44.
status-firefox44:
--- → ?
Comment 5•9 years ago
|
||
This should be fixed since bug 1215659. Is it fixed for you?
Flags: needinfo?(jmuizelaar)
Reporter | ||
Comment 6•8 years ago
|
||
No I can still reproduce this on a Nightly from 2015-12-03
Flags: needinfo?(jmuizelaar)
status-firefox45:
--- → affected
Comment 7•8 years ago
|
||
[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.
status-firefox42:
--- → unaffected
status-firefox43:
--- → affected
Comment 8•8 years ago
|
||
While trying to bisect this, I found out that the upscaling only happens on e10s. Otherwise, it looks fine with e10s disabled.
Updated•8 years ago
|
tracking-e10s:
--- → ?
Comment 9•8 years ago
|
||
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.
Comment 10•8 years ago
|
||
In case anyone wants to reproduce. Gabor, can you please take a look at this? Looks like fallout from bug 863512.
Flags: needinfo?(gkrizsanits)
Comment 11•8 years ago
|
||
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
Updated•8 years ago
|
Updated•8 years ago
|
Summary: Dragging preview for tab is upscaled on retina mbp → Dragging preview for tab is upscaled on retina mbp with e10s
Comment 12•8 years ago
|
||
(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)
Updated•8 years ago
|
Assignee: nobody → gkrizsanits
Comment 13•8 years ago
|
||
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.
Comment 14•8 years ago
|
||
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)
Assignee | ||
Comment 15•8 years ago
|
||
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 16•8 years ago
|
||
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+
Assignee | ||
Comment 17•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/67474512da0c49ae844305eb28fdfa269e73cf12 Bug 1214428 - Scale tab-preview image properly for hi-dpi displays with e10s. r=gabor
Comment 18•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/67474512da0c
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Jonathan, does this need to be uplifted to Beta44 and Aurora45?
Assignee | ||
Comment 20•8 years ago
|
||
(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)
Assignee | ||
Comment 21•8 years ago
|
||
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?
Updated•8 years ago
|
Updated•8 years ago
|
Attachment #8702081 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 22•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/3ac7afee9903
Assignee | ||
Updated•8 years ago
|
Assignee: gkrizsanits → jfkthame
Comment 23•8 years ago
|
||
[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.
Description
•