Closed Bug 382049 Opened 13 years ago Closed 13 years ago

Mac native form controls draw at incorrect positions in offscreen surfaces


(Core :: Graphics, defect)

Not set





(Reporter: dbaron, Assigned: jtd)




(6 files, 2 obsolete files)

Attached file testcase
Mac native form controls seem to draw at incorrect positions in offscreen surfaces:  perhaps they are off by the difference between the window origin and the surface origin.  (See also bug 382048 for buttons.)

Steps to reproduce: load attached testcase

Expected results: the form controls are placed correctly relative to the text associated with them, and they handle events resulting from clicking on them

Actual results: the form controls are placed too far down and too far to the right, but they are activated by clicking where they should be
Flags: blocking1.9?
Assignee: joshmoz → nobody
Component: Widget: Cocoa → GFX: Thebes
Flags: blocking1.9? → blocking1.9+
QA Contact: cocoa → thebes
Assignee: nobody → jdaggett
Related issues: 382048, 382049, 382050
Note the big white box that appears, the drawing operation is shifted over and down 8 pixels.
Attached patch fix up the offsets (obsolete) — Splinter Review
This patch fixes up the offset problem but the testcase is still not drawing correctly.  For some reason, when an offscreen is used (via PushGroup / PopGroupToSource in nsDisplayOpacity::Paint) the resulting copy to the window context is not drawn correctly.  In the "trivial testcase with padding added" you see a white box where the radio button should appear.  In fact, *any* kind of drawing between the PushGroup / PopGroup appears as white, no matter what the color / alpha settings. I'm guessing the problem lies in the quartz surface code, within the code that paints the offscreen into the window context, but that's just a guess at this point.
That looks like the complete file, not a diff.

Please include the fixes to the reftest manifest to mark the relevant tests not failing/random in the patch; otherwise when it's checked in the tree will go orange.
Attached patch actual patch (obsolete) — Splinter Review
let's try that again...
Attachment #274890 - Attachment is obsolete: true
Note: the patch fixes the offset problem but not the other related drawing problems (see comment #6).  So some things will render correctly, some will still not.  See bug 372994 for a more complete list.
Finally figured out the root cause of this - we're setting up an offscreen CGBitmapContext with a default composite operation of "copy".  Setting this to "source over" looks like it fixes the problem.  Still working on what the appropriate way to fix this is, probably something like setting the default composite operation to the default operation obtained from the existing surface.

One thing that sucks is that we're using a private Quartz call, CGContextSetCompositeOperation, it would be nice if we could avoid having to use this, but that may not be possible.

So in summary, there are two subproblems here:

(1) offsets calculated incorrectly (fixed by "actual patch")
(2) default composite operation is set to "copy" not "source over"

Ah!  I didn't understand this at first, but now it makes sense.  So the fix should be to, at the end of _clone_similar, to set the composite operation back to SrcOver (Copy should still be used for the DrawImage call).  I guess OSX ops assume that the current operation is SrcOver... or rather, more likely, they just use whatever's there.

So actually, a better fix would be to explicitly set it to SrcOver at the start of the native theme drawing code (and don't bother setting it to SrcOver in clone()) -- the quartz surface doesn't care what the current composite op is, it sets it to whatever it should be for every operation.  (We'd run into this same problem if, e.g., a native theme bit was drawn at some point after a SVG file that set some non-OVER composite op.)  But if the native theme code expects it to be OVER, so it should ensure that that's the case.

Based on Vlad's suggestion, just fix this within the Mac theme code:

(1) calculate the offset correctly
(2) set the composite operation to "sourceOver" via private Quartz call

This managed to fix a reftest failure bug, bug 377118.
Attachment #275043 - Attachment is obsolete: true
Attachment #275568 - Flags: review?(vladimir)
Note: the composite operation change will have no effect outside of nsNativeThemeCocoa::DrawWidgetBackground, since it takes place within a gstate save/restore pair.
Comment on attachment 275568 [details] [diff] [review]
full patch with reftest list fixup

Looks good!  I can get this checked in for you shortly.
Attachment #275568 - Flags: review?(vladimir)
Attachment #275568 - Flags: review+
Attachment #275568 - Flags: approval1.9+
Checked in, though the unit test tinderbox is backed up.. but other mac tinderboxes are green.
Fixed in latest nightly:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a8pre) Gecko/2007080804 Minefield/3.0a8pre
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.