Closed
Bug 1067207
Opened 10 years ago
Closed 10 years ago
[HiDPI] Tabs rendered incorrectly when OS dpi > 100% (96dpi)
Categories
(Core :: Graphics: ImageLib, defect)
Tracking
()
RESOLVED
FIXED
mozilla35
Tracking | Status | |
---|---|---|
firefox34 | --- | unaffected |
firefox35 | - | affected |
People
(Reporter: streetwolf52, Assigned: seth)
References
Details
(Keywords: regression)
Attachments
(2 files, 1 obsolete file)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
Regression testing points to Bug 1057903 drawing the top portion of tabs lower on the tab.
Reporter | ||
Comment 1•10 years ago
|
||
Seems one of the inbounds is out of order. There are two possible csets causing the problem. 20140914152108 https://hg.mozilla.org/integration/mozilla-inbound/rev/606aef8e8c16 15-Sep-2014 00:30 20140914152408 https://hg.mozilla.org/integration/mozilla-inbound/rev/3bfa3ac98ba1 14-Sep-2014 23:48
Reporter | ||
Updated•10 years ago
|
No longer depends on: 1057903
Summary: Tabs rendered incorrectly after Bug 1057903. → Tabs rendered incorrectly.
Reporter | ||
Updated•10 years ago
|
Component: Untriaged → ImageLib
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → seth
Assignee | ||
Comment 2•10 years ago
|
||
This is almost certainly caused by bug 1054079, which touches SVG drawing. Bug 1057903, which doesn't, is very unlikely to be involved.
Blocks: 1054079
Assignee | ||
Comment 3•10 years ago
|
||
Gary, can give you me any more details that might help me reproduce this? Do you see this all the time or are the tabs sometimes drawn correctly and sometimes not? Could you paste the information from about:support here? I'm particularly interested in which prefs you have set.
Flags: needinfo?(garyshap)
Reporter | ||
Comment 4•10 years ago
|
||
Some more info. I changed my OS dpi from 125%(120dpi) to 100%(96dpi) and the top of the tab is placed correctly. The problem appears to be another HiDPI problem. I thought that 'layout.css.devPixelsPerPx = 1' which basically renders Fx at 96 dpi would also fix the problem but it didn't. Only setting the OS dpi seems to cause the problem.
Flags: needinfo?(garyshap)
Reporter | ||
Updated•10 years ago
|
Summary: Tabs rendered incorrectly. → Tabs rendered incorrectly when OS dpi > 100%
Reporter | ||
Updated•10 years ago
|
Summary: Tabs rendered incorrectly when OS dpi > 100% → Tabs rendered incorrectly when OS dpi > 100% (96dpi)
Assignee | ||
Comment 5•10 years ago
|
||
I can confirm that it seems like you can't use 'layout.css.devPixelsPerPx' to reproduce this problem at all. Even awkward settings like 1.3 don't cause rendering issues. I'm not sure what else changes when the OS DPI setting changes that could explain how bug 1054079 would cause this problem. I'll look into it more, though.
Reporter | ||
Comment 6•10 years ago
|
||
There a still lots of problems using a HiDPI. If you look at the tab screenshot you can see that the curves at the bottom sides of the tabs don't flow nicely into the straight border. At 96dpi it flows perfectly. I see lots of little problems using a high dpi.
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to Gary [:streetwolf] from comment #6) > There a still lots of problems using a HiDPI. If you look at the tab > screenshot you can see that the curves at the bottom sides of the tabs don't > flow nicely into the straight border. At 96dpi it flows perfectly. I see > lots of little problems using a high dpi. Yeah, I noticed that issue with the curves. All of the problems with the tab rendering look like scaling issues to me. Some code somewhere is doing scaling using a different approach than the devPixelPerPx system we normally use, and we're not handling it correctly. I'm pretty sure that code is just wrong, but I'm not sure what code it is, or why it is that bug 1054079 exposed the problem. (It's definitely not coming from code that was added in bug 1054079.)
Reporter | ||
Comment 8•10 years ago
|
||
This landed on Nightly so you're bound to get some more bug reports. How are you going to handle this Seth? I know that someone has been dealing with HiDPI issues although no patches have come through in quite awhile.
Reporter | ||
Updated•10 years ago
|
Summary: Tabs rendered incorrectly when OS dpi > 100% (96dpi) → [HiDPI] Tabs rendered incorrectly when OS dpi > 100% (96dpi)
![]() |
||
Comment 9•10 years ago
|
||
[Tracking Requested - why for this release]: Regression window(m-i) Good: https://hg.mozilla.org/integration/mozilla-inbound/rev/8e28464849fa Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0 ID:20140914145408 Bad: https://hg.mozilla.org/integration/mozilla-inbound/rev/606aef8e8c16 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0 ID:20140914152108 Pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=8e28464849fa&tochange=606aef8e8c16 Regressed by: 606aef8e8c16 Seth Fowler — Bug 1057903 - Refactor RasterImage to use DrawableFrameRef and generally clean up. r=tn
Status: UNCONFIRMED → NEW
status-firefox34:
--- → unaffected
status-firefox35:
--- → affected
tracking-firefox35:
--- → ?
Ever confirmed: true
Keywords: regression
Comment 10•10 years ago
|
||
(In reply to Seth Fowler [:seth] from comment #2) > This is almost certainly caused by bug 1054079, which touches SVG drawing. > Bug 1057903, which doesn't, is very unlikely to be involved. [responding to this, in light of comment 9: I don't think SVG is actually involved in this bug -- IIRC, tabs are still drawn from PNGs. See e.g. this search for "tab" in browser/themes/windows, which finds no SVG files, but does find various PNG files -- e.g. tab-stroke-end.png, tab-background-start.png, etc. http://mxr.mozilla.org/mozilla-central/find?string=tab&tree=mozilla-central&hint=browser|themes|windows ]
Assignee | ||
Comment 11•10 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #10) > [responding to this, in light of comment 9: I don't think SVG is actually > involved in this bug -- IIRC, tabs are still drawn from PNGs. Yup, you're right. I just noticed that this morning myself, and filed bug 1068110 about fixing *that*. Sigh. Last time I looked at the Australis theme code, it was using SVGs. That means that this should block bug 1057903 and not bug 1054079.
Assignee | ||
Comment 12•10 years ago
|
||
OK, I think I've found the problem. There's an issue in the way padding is implemented in bug 1057903; see bug 1057903, comment 14. A patch should be straightforward.
Reporter | ||
Comment 13•10 years ago
|
||
Just discovered that applying a Theme (aka Persona) seems to correct the problem.
Reporter | ||
Comment 14•10 years ago
|
||
The line comes back eventually.
Assignee | ||
Comment 15•10 years ago
|
||
So it looks to me like the problem is that padding was getting computed partially in unscaled and partially in scaled coordinates. In cases where we actually have padding (animated images), this worked correctly, because we don't scale, so scaled coordinates and unscaled coordinates are the same. Non-animated images could sometimes end up being drawn with padding, though, because in that case scaled and unscaled are different, and so we could end up getting non-zero padding values even though we didn't actually need padding. This patch both fixes this problem and adds an assertion that should help us avoid reintroducing this problem in the future.
Attachment #8490403 -
Flags: review?(tnikkel)
Assignee | ||
Comment 16•10 years ago
|
||
Try job here: https://tbpl.mozilla.org/?tree=Try&rev=d5c36b6a281e
Updated•10 years ago
|
Attachment #8490403 -
Flags: review?(tnikkel) → review+
Comment 17•10 years ago
|
||
Try build looks good, just downloaded it, and the issue is resolved.
Reporter | ||
Comment 18•10 years ago
|
||
Looks good to me too.
Assignee | ||
Comment 19•10 years ago
|
||
So the results from the try job had some oranges. The reason is that the new assertion is wrong - it actually is legal in the GIF spec for non-animated images to have padding! (Absurd, yes, but legal.) I've altered the patch to achieve the same thing in a slightly different manner: we force the padding to be (0, 0, 0, 0) unless we're using the original frame instead of a scaled frame. This should be fine, since we don't scale animated images anyway. I'm cutting a corner slightly here, because we actually will scale single frame images with padding right now. This is an ultra-rare case, and I think this should pass tests without fixing this. (Otherwise we'd get test failures now, without having this patch at all.) I'm going to fix that corner case in bug 1060200 by changing the scaling policy to forbid scaling of any padded frame. For reasons of avoiding rebase nastiness, I'd like to avoid fixing this in this bug. Bug 1060200 should land today anyway. Here's a new try job: https://tbpl.mozilla.org/?tree=Try&rev=3abdc21937ec
Assignee | ||
Updated•10 years ago
|
Attachment #8490403 -
Attachment is obsolete: true
Assignee | ||
Comment 20•10 years ago
|
||
(In reply to Gary [:streetwolf] from comment #18) > Looks good to me too. Glad to hear it! The new version I just posted should work fine too, then.
Comment 21•10 years ago
|
||
(In reply to Seth Fowler [:seth] from comment #20) > (In reply to Gary [:streetwolf] from comment #18) > > Looks good to me too. > > Glad to hear it! The new version I just posted should work fine too, then. For some reason it appears to me that the winXP Opt build did not start building.
Assignee | ||
Comment 22•10 years ago
|
||
(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #21) > For some reason it appears to me that the winXP Opt build did not start > building. I didn't request any opt builds this time, but in case you're interested in testing the newest version of the patch manually, I pushed a new try job: https://tbpl.mozilla.org/?tree=Try&rev=9c9240170887
Assignee | ||
Comment 23•10 years ago
|
||
Try looks pretty green, so I went ahead and pushed this in: https://hg.mozilla.org/integration/mozilla-inbound/rev/4746dd77b34b
Comment 24•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4746dd77b34b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Updated•10 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•