Closed Bug 806059 Opened 12 years ago Closed 12 years ago

A combination of <html style="background-attachment: fixed"> and <input type=submit> can cause page to become non-HiDPI

Categories

(Core :: Widget: Cocoa, defect)

Product:
Core
Component:
Widget: Cocoa
Platform:
All
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla19
Tracking Flags:
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: fryn, Assigned: jfkthame)

References

(
URL
)

Details

Attachments

(1 file, 2 obsolete files)

make gfxQuartzNativeDrawing aware of backing scale factor when rendering native widgets.
6.52 KB, patch
smichaud
: review+
bajaj
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
I ran into this bizarre bug on a real-world web page, where if <html/> has background-attachment: fixed with some subset of background-image: url(...) values and there is an <input type="submit"/> on the page, then the page suddenly gets rendered in what seems to be not HiDPI mode.

Here's the code for my testcase (made as minimal as I could):

<html style="background:url(https://a/) fixed">
<input type="checkbox" checked="true" onchange="s=document.documentElement.style;b='backgroundAttachment';s[b]=s[b]?'':'fixed'" />
<input type="submit" />

Click the URL link above to open this as a data URL document.
The onchange stuff is so you can click on the checkbox to toggle background-attachment: fixed.
The checkbox is there to show that having <input type="submit"/> causes other elements on the page to also not be HiDPI.

I have no idea what is going on here. o_O
Note that the problem here does _not_ reproduce with hardware acceleration disabled. I guess there may be certain situations where the accelerated layers system ends up using low-dpi buffers, or something like that.
BTW, it's interesting that the text label in the "Submit Query" button remains HiDPI, even when the button outline becomes LoDPI.

I wonder if the underlying cause will turn out to be related to bug 803383.
Also, note that this also depends on the background-image URL, oddly enough.
The bug does not appear on a data URL document when the backgroud-image does not use an HTTP protocol (didn't test other protocols though).

(In reply to Jonathan Kew (:jfkthame) from comment #2)
> I wonder if the underlying cause will turn out to be related to bug 803383.

The panel bug also causes the panel's text to be blurry until everything adjusts itself. The cause could still be related, but it's at least a different manifestation.
The problem seems to arise when gfxQuartzNativeDrawing decides that it needs to create a new surface, rather than rendering to an existing one; it creates a surface sized according to the widget's "native" size (cocoa points), which means we end up with a lo-dpi rendering. This patch makes gfxQuartzNativeDrawing accept a backing scale factor parameter, so it can be told to create a double-sized surface and scale the widget rendering to get a hi-dpi image, and then scale this appropriately when copying it to the target context.\n\nTryserver build in progress at https://tbpl.mozilla.org/?tree=Try&rev=c9fb2e85e6d4.
Slightly simplified version of the patch - let gfxQuartzNativeDrawing get the backing scale factor to use from the passed-in context, rather than requiring an extra parameter. https://tbpl.mozilla.org/?tree=Try&rev=67916771766c.
Attachment #676104 - Flags: review?(smichaud)
Attachment #676090 - Attachment is obsolete: true
Comment on attachment 676104 [details] [diff] [review]
make gfxQuartzNativeDrawing aware of backing scale factor when rendering native widgets. try: -b do -p macosx64 -u all

Jonathan, one question about your patch:

+    gfxSize scaledSize = ctx->UserToDevice(gfxSize(1.0f, 1.0f));

Are you sure that scaledSize here is always the backing scale factor, and not just the scale factor?

I've been digging through the gfx code to find out.  But what I've found so far indicates that the results of gfxContext::UserToDevice() are just the scale factor.
I was wondering about that too, but in my testing so far it remained resolutely at 2.0 (for a window on a HiDPI screen; 1.0 on non-HiDPI, obviously) regardless of any zooming I did.
I'll keep digging then, and do some of my own testing.
Comment on attachment 676104 [details] [diff] [review]
make gfxQuartzNativeDrawing aware of backing scale factor when rendering native widgets. try: -b do -p macosx64 -u all

I've now done a bunch of testing of my own, including adding logging to all calls to gfxContext::UserToDevice(gfxSize) and gfxContext::UserToDevice(gfxRect).  I still haven't figured out exactly what UserToDevice() "means".  But in all but one case it's quite clear it *doesn't* mean "scale to backing scale factor".  So I really prefer the previous version of your patch (the one that adds an aBackingScale parameter to gfxQuartzNativeDrawing::gfxQuartzNativeDrawing()).

A gfxQuartzNativeDrawing object is used in exactly three places in the tree:

http://hg.mozilla.org/mozilla-central/annotate/324985f4c4ea/widget/cocoa/nsNativeThemeCocoa.mm#l1941
http://hg.mozilla.org/mozilla-central/annotate/324985f4c4ea/layout/generic/nsObjectFrame.cpp#l1370
http://hg.mozilla.org/mozilla-central/annotate/324985f4c4ea/layout/generic/nsObjectFrame.cpp#l1784

Only the first one's scale gets changed according to the value of the current backing scale factor, here:
http://hg.mozilla.org/mozilla-central/annotate/324985f4c4ea/widget/cocoa/nsNativeThemeCocoa.mm#l1938

The others always have a scale factor of '1.0', no matter how I scale the contents -- even though plugins are a part of a page's content and can be scaled with it.  The same is true for *all* calls to gfxContext::UserToDevice() in my tests (though I haven't yet figured out how to scale the browser's chrome (as opposed to its contents), which might make a difference).
Attachment #676104 - Flags: review?(smichaud)
This bug's STR needs some clarification.

To see the bug:

1) Click on this bug's URL (above).
2) Zoom the page up as far as it will go (using Command-+).
3) Check and uncheck the checkbox.  Notice that it's low-res when checked and hi-res when unchecked.

In my tests, the Submit Query button stays hi-res regardless.  I tested with today's mozilla-central nightly on OS X 10.7.5 (on a Retina MBP).

Jonathan's patches do fix this bug.  But (as I've said) I much prefer the first one.
(In reply to Steven Michaud from comment #10)
> 3) Check and uncheck the checkbox.  Notice that it's low-res when checked
> and hi-res when unchecked.

Sorry for the confusion. Firefox also tries to cache+restore the value of the checkbox upon revisit, so that state can be flipped sometimes too. :/

> In my tests, the Submit Query button stays hi-res regardless.

The text stays hi-res for me, but the border & styling do not.

Thanks for working on this bug, Steven and Jonathan. :)
>> In my tests, the Submit Query button stays hi-res regardless.
>
> The text stays hi-res for me, but the border & styling do not.

I just double-checked, and the Submit Query button's border and styling are always hi-res for me.

Weird, but I suspect it doesn't matter.  Jonathan's patches should fix both problems.
Attachment #676104 - Attachment is obsolete: true
OK, here's the first patch again, with some added comments in gfxQuartzNativeDrawing to clarify what the parameters are for.

Note that I've deliberately used a default parameter (with value 1.0) for aBackingScale, so that callsites that haven't been modified will maintain existing behavior. We may want to look at the other places in the tree where gfxQuartzNativeDrawing is used, to consider whether passing a hidpi-aware scale factor here would be beneficial (perhaps it could simplify other aspects of hidpi plugin support?), but I didn't want to make that a precondition for fixing this bug.
Comment on attachment 676531 [details] [diff] [review]
make gfxQuartzNativeDrawing aware of backing scale factor when rendering native widgets.

Looks good to me.
Attachment #676531 - Flags: review?(smichaud) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/8147196deeb8
Assignee: nobody → jfkthame
Target Milestone: --- → mozilla19
Comment on attachment 676531 [details] [diff] [review]
make gfxQuartzNativeDrawing aware of backing scale factor when rendering native widgets.

Yes, I think we should take this for FF18 - I don't know how prevalent the problem would be in the wild, but we know that Frank first noticed it on a real-world site (comment #0), and it'll result in a user experience with an unpredictable and disconcerting mix of "sharp" and "blurry" rendering.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 674373 (HiDPI)
User impact if declined: fail to use HiDPI rendering in certain cases
Testing completed (on m-c, etc.): confirmed locally to fix the testcase; landed on inbound without problems
Risk to taking this patch (and alternatives if risky): minimal risk - Mac-only change that only affects HiDPI mode, else maintains existing behavior
String or UUID changes made by this patch: none
Attachment #676531 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/8147196deeb8
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Attachment #676531 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: