382049 - Mac native form controls draw at incorrect positions in offscreen surfaces

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: 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

Comment 1

[John Daggett (:jtd)]

Attachment: correct rendering (no opacity)

Comment 2

[John Daggett (:jtd)]

Attachment: incorrect rendering (opacity = 0.99)

Comment 3

[John Daggett (:jtd)]

Attachment: correct (left) vs. incorrect (right) screenshot

Comment 4

[John Daggett (:jtd)]

Related issues: 382048, 382049, 382050

Comment 5

[John Daggett (:jtd)]

Attachment: trivial testcase with padding added

Note the big white box that appears, the drawing operation is shifted over and down 8 pixels.

Comment 6

[John Daggett (:jtd)]

Attachment: fix up the offsets

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.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 8

[John Daggett (:jtd)]

Attachment: actual patch

let's try that again...

Comment 9

[John Daggett (:jtd)]

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.

Comment 10

[John Daggett (:jtd)]

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"

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 12

[John Daggett (:jtd)]

Attachment: full patch with reftest list fixup

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.

Comment 13

[John Daggett (:jtd)]

Note: the composite operation change will have no effect outside of nsNativeThemeCocoa::DrawWidgetBackground, since it takes place within a gstate save/restore pair.

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 275568 [details] [diff] [review]
full patch with reftest list fixup

Looks good!  I can get this checked in for you shortly.

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Checked in, though the unit test tinderbox is backed up.. but other mac tinderboxes are green.

Comment 16

[John Daggett (:jtd)]

Fixed in latest nightly:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a8pre) Gecko/2007080804 Minefield/3.0a8pre