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

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Gary [:streetwolf52]]

Attachment: Tabs rendered incorrectly.

Regression testing points to Bug 1057903 drawing the top portion of tabs lower on the tab.

Comment 1

[Gary [:streetwolf52]]

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

Comment 2

[Seth Fowler [:seth] [:s2h]]

This is almost certainly caused by bug 1054079, which touches SVG drawing. Bug 1057903, which doesn't, is very unlikely to be involved.

Comment 3

[Seth Fowler [:seth] [:s2h]]

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.

Comment 4

[Gary [:streetwolf52]]

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.

Comment 5

[Seth Fowler [:seth] [:s2h]]

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.

Comment 6

[Gary [:streetwolf52]]

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.

Comment 7

[Seth Fowler [:seth] [:s2h]]

(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.)

Comment 8

[Gary [:streetwolf52]]

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.

Comment 9

[Alice0775 White]

[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

Comment 10

[Daniel Holbert [:dholbert]]

(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
]

Comment 11

[Seth Fowler [:seth] [:s2h]]

(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.

Comment 12

[Seth Fowler [:seth] [:s2h]]

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.

Comment 13

[Gary [:streetwolf52]]

Just discovered that applying a Theme (aka Persona) seems to correct the problem.

Comment 14

[Gary [:streetwolf52]]

The line comes back eventually.

Comment 15

[Seth Fowler [:seth] [:s2h]]

Attachment: Compute imgFrame padding in unscaled coordinates

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.

Comment 16

[Seth Fowler [:seth] [:s2h]]

Try job here:

https://tbpl.mozilla.org/?tree=Try&rev=d5c36b6a281e

Comment 17

[Jim Jeffery not reading bug-mail 1/2/11]

Try build looks good, just downloaded it, and the issue is resolved.

Comment 18

[Gary [:streetwolf52]]

Looks good to me too.

Comment 19

[Seth Fowler [:seth] [:s2h]]

Attachment: Compute imgFrame padding in unscaled coordinates

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

Comment 20

[Seth Fowler [:seth] [:s2h]]

(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

[Jim Jeffery not reading bug-mail 1/2/11]

(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.

Comment 22

[Seth Fowler [:seth] [:s2h]]

(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

Comment 23

[Seth Fowler [:seth] [:s2h]]

Try looks pretty green, so I went ahead and pushed this in:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4746dd77b34b

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4746dd77b34b