544348 - We depend on undefined clipping semantics of cairo

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

It looks like we currently depend on the clip on a surface persisting during the use of native apis. Cairo trunk has changed the semantics here and thus breaks us.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Basically it looks like UpdateSurfaceClip() no longer works.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

I previously tried to fix UpdateSurfaceClip by drawing a 1x1 transparent pixel. This worked in many cases but was not sufficient for ensuring that the clip made it to the destination. I'm not sure what the best solution is here...

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Perhaps we should add new cairo API here. Perhaps it should be backend-specific.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #3)
> Perhaps we should add new cairo API here. Perhaps it should be
> backend-specific.

I thought about adding backend-specific API but it doesn't really fit in nice, because the clip is associated with a cairo_t which can have a many-to-one relationship with the surface. i.e. the surface/backend doesn't really have any way of knowing what the clip should be.

The best option I can think of right now is to add api get the clip out of a cairo_t and then apply that to the destination surface ourselves.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We have cairo_copy_clip_rectangle_list, but that doesn't work with general paths.

Having to write our own per-backend code to apply the cairo clip to a native context sounds pretty bad since it's duplicating code already in cairo.

How about backend-specific API that take a cairo_t? E.g.
HDC cairo_win32_get_dc_with_clip (cairo_t *cr)
where if the current target of the cr is a Win32 surface, we return the HDC and we ensure that the cairo clip has been set on that DC?

Similarly, cairo_quartz_get_cg_context_with_clip, etc.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #5)
> How about backend-specific API that take a cairo_t? E.g.
> HDC cairo_win32_get_dc_with_clip (cairo_t *cr)
> where if the current target of the cr is a Win32 surface, we return the HDC and
> we ensure that the cairo clip has been set on that DC?
> 
> Similarly, cairo_quartz_get_cg_context_with_clip, etc.

That's not a bad idea. One concern I have is that the semantics of those to functions are different. i.e. win32 only supports rectangular clips. Maybe it's not worth worrying about.

Comment 7

[Bas Schouten (:bas.schouten)]

(In reply to comment #6)
> (In reply to comment #5)
> > How about backend-specific API that take a cairo_t? E.g.
> > HDC cairo_win32_get_dc_with_clip (cairo_t *cr)
> > where if the current target of the cr is a Win32 surface, we return the HDC and
> > we ensure that the cairo clip has been set on that DC?
> > 
> > Similarly, cairo_quartz_get_cg_context_with_clip, etc.
> 
> That's not a bad idea. One concern I have is that the semantics of those to
> functions are different. i.e. win32 only supports rectangular clips. Maybe it's
> not worth worrying about.

Do not underestimate the amount of non-rectangular clips we get, for example a div with rounded borders would produce one if I'm not mistaking.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we could define cairo_win32_get_dc_with_clip to return NULL if the clip cannot be set on the HDC --- i.e. it's not a single rectangle. That will work fine for us.

Comment 9

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #7)
> Do not underestimate the amount of non-rectangular clips we get, for example a
> div with rounded borders would produce one if I'm not mistaking.

We currently rely on only clipping the clip region and not the clip path on win32. If we want to change this we can do that after the cairo update. I wasn't able to come across any cases where we actually want to clip native stuff to non-rectangular paths. For example, overflow:hidden on a div with border-radius doesn't clip to rounded rectangle (though perhaps it should as webkit does...)

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Attachment: First cut at what this could look like

Here's a copy of what this could look like. Anyone have any thoughts?

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

-cairo_quartz_surface_get_cg_context (cairo_surface_t *surface);

Probably don't want to remove this.

I'm not sure why the Win32 printing code is different from the main Win32 code.

Otherwise looks good.

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Attachment: Completed version

Comment 13

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

Comment on attachment 440570 [details] [diff] [review]
Completed version

looks fine to me, but boy does it need documentation in cairo :)