Closed Bug 1067207 Opened 5 years ago Closed 5 years ago

[HiDPI] Tabs rendered incorrectly when OS dpi > 100% (96dpi)

Categories

(Core :: ImageLib, defect)

35 Branch
x86_64
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox34 --- unaffected
firefox35 - affected

People

(Reporter: streetwolf, Assigned: seth)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

Regression testing points to Bug 1057903 drawing the top portion of tabs lower on the tab.
Depends on: 1057903
Product: Firefox → Core
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
No longer depends on: 1057903
Summary: Tabs rendered incorrectly after Bug 1057903. → Tabs rendered incorrectly.
Component: Untriaged → ImageLib
Assignee: nobody → seth
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
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)
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)
Summary: Tabs rendered incorrectly. → Tabs rendered incorrectly when OS dpi > 100%
Summary: Tabs rendered incorrectly when OS dpi > 100% → Tabs rendered incorrectly when OS dpi > 100% (96dpi)
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.
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.
(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.)
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.
Summary: Tabs rendered incorrectly when OS dpi > 100% (96dpi) → [HiDPI] Tabs rendered incorrectly when OS dpi > 100% (96dpi)
[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
Blocks: 1057903
No longer blocks: 1054079
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
(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
]
(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.
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.
Just discovered that applying a Theme (aka Persona) seems to correct the problem.
The line comes back eventually.
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)
Attachment #8490403 - Flags: review?(tnikkel) → review+
Try build looks good, just downloaded it, and the issue is resolved.
Looks good to me too.
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
Attachment #8490403 - Attachment is obsolete: true
(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.
(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.
(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
Try looks pretty green, so I went ahead and pushed this in:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4746dd77b34b
https://hg.mozilla.org/mozilla-central/rev/4746dd77b34b
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Duplicate of this bug: 1068694
Blocks: 1068881
No longer blocks: 1068881
Blocks: 849590
You need to log in before you can comment on or make changes to this bug.