287058 - Position:fixed not clipping background-images

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[philippe (part-time)]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2) Gecko/20050320 Firefox/1.0+ (PowerBook)
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2) Gecko/20050320 Firefox/1.0+ (PowerBook)

A fixed positioned block level element with a background-image whose
background-position values are [top right] or [100% 0] doesn't clip the
background-image to the width of the container.

Simplified testcase from the URL above:
http://dev.l-c-n.com/Gecko/fixed_bck.php

The problem only happens with position:fixed. On purpose the image is wider than
the container.


I could track back this to at least 
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b) Gecko/20050310;

This build
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b) Gecko/20050217
works correctly.

Reproducible: Always

Steps to Reproduce:
1.View test case
2.scroll up/down
3.

Actual Results:  
the background image, attached to the h4 tag should not overflow the container
(with lime border on the left)

Expected Results:  
Crop the image to the width of the container

Comment 1

[philippe (part-time)]

Attachment: screen shot of the testcase

Comment 2

[R.K.Aa.]

Attachment: screen shot of the testcase

WFM    Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050320

Comment 3

[philippe (part-time)]

(In reply to comment #2)
> Created an attachment (id=178113) [edit]
> screen shot of the testcase
> 
> WFM    Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050320

Did you scroll up and down ?
When the fixed div is partly out of the body tag, everything appears correct.
scroll down and the background image expands out of the container

Comment 4

[R.K.Aa.]

yes, i scrolled all i was good for. The screenshot is after scrolling a lot, all
kind of window sizes, to make sure i wasnt missing something. Still looked good.

Comment 5

[philippe (part-time)]

(In reply to comment #4)
> yes, i scrolled all i was good for. The screenshot is after scrolling a lot, all
> kind of window sizes, to make sure i wasnt missing something. Still looked good.

Ah, Ok. Mac only, then?
I can consistently reproduce on nightly builds from Firefox, Camino and
SeaMonkey, on two different machines.

Comment 6

[Martijn Wargers (dead)]

I don't see the bug, I guess this is Mac only then.
Phillipe, could you narrow the regression period further?
http://archive.mozilla.org/pub/mozilla/nightly/

Comment 7

[philippe (part-time)]

(In reply to comment #6)

> Phillipe, could you narrow the regression period further?
> http://archive.mozilla.org/pub/mozilla/nightly/

Got it down to this window:

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2) Gecko/20050219
works fine (2005-02-19-12-trunk/).
But
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2) Gecko/20050220
breaks (2005-02-20-11-trunk/)

Comment 8

[Martijn Wargers (dead)]

Thanks Philippe!
Based on that regression range:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-02-19+11%3A00%3A00&maxdate=2005-02-20+12%3A00%3A00&cvsroot=%2Fcvsroot

The fix for bug 245407 looks suspicious.

Comment 9

[jhp (no longer active)]

Yes, it does look like the Quartz image patch caused this bug.  Specifically,
there is something wrong with the clipping code.  I replaced it with a call to
|ClipCGContextToRegion()|, and it worked fine for this test case.  Of course,
that function had some other issues with it, and the documentation says it is
quite inefficient.  I'll have to do some more testing.

Comment 10

[Wevah (Nate Weaver)]

FWIW, I'm not seeing this bug with the March 21st Camino nightly.

Comment 11

[jhp (no longer active)]

This is odd.  Camino is not affected (as previously pointed out).  I created a
reduced testcase and tried it in both Camino and Mozilla.  In Camino, the
background image gets drawn twice, and both times I see the call to the
|CreatePathFromRectsProc()| function, which creates a CG path from the QD clip
region.

However, Mozilla is different.  The background image is drawn 4 times (twice as
many as Camino), and two of those times, I never see the call to
|CreatePathFromRectsProc()|;  so the clip region is getting properly parsed.  No
idea why.

So I took a look at |ClipCGContextToRegion()| again.  I got that patch working
with Mozilla, even the old issues I was seeing before.  However, it completely
breaks Camino!  Yay!

Comment 12

[jhp (no longer active)]

Attachment: patch

So the issue is that we have a QD port with an empty clip region, and we are
not handling that correctly.  |QDRegionToRects()| finds no rects to parse, so
the CGContext has no clipping path.  So I made it check for an empty clip
region and clip the CGContext to a zero by zero rect.

Of course, it seems that |ClipCGContextToRegion()| does all this for us anyway,
but there is no easy way to make this work in the Camino case (we'd have to
translate the CGContext, call |ClipCGContextToRegion()|, then translate back).

Comment 13

[Simon Fraser [no longer active]]

Can we just avoid drawing if the clip regin is empty?

Comment 14

[jhp (no longer active)]

Not from with gfx, unless we check for an empty clip region at the beginning of
each draw function.  I think the correct fix would be to change the logic in
layout;  it shouldn't be asking us to draw when there is no clip region.

Comment 15

[Simon Fraser [no longer active]]

Comment on attachment 178633 [details] [diff] [review]
patch

Sounds good (josh can't sr).

Comment 16

[Josh Aas]

Comment on attachment 178633 [details] [diff] [review]
patch

Looks good.

About sr... there was a bug open about transferring mac gfx module ownership to
me. It was marked as "fixed," but I'm not sure what that actually means. In any
case - if I own that module, I would guess that I can sr for it. Is that true?
If not, is it worth having me own it?

Comment 17

[Josh Aas]

nm - the bug I was talking about was 287115, and it seems that I now own
"Widget/Gfx : Mac" and not Mac layout.

Comment 18

[jhp (no longer active)]

Comment on attachment 178633 [details] [diff] [review]
patch

Checked in to trunk.

Comment 19

[jhp (no longer active)]

roc, might it not be good to check if the clipping region is empty before
calling |nsImage::Draw()|?  Something like this in nsCSSRendering:

 #if (!defined(XP_UNIX) && !defined(XP_BEOS)) || defined(XP_MACOSX)
   // Setup clipping so that rendering doesn't leak out of the computed
   // dirty rect
   aRenderingContext.PushState();
   aRenderingContext.SetClipRect(dirtyRect, nsClipCombine_kIntersect);
+  nsCOMPtr<nsIRegion> region;
+  aRenderingContext.GetClipRegion(getter_AddRefs(region));
+  if (region->IsEmpty()) {
+    // clip rect is empty; nothing to draw
+    aRenderingContext.PopState();
+    return;
+  }
 #endif

On my mac, this cut down the calls to |nsImageMac::Draw()| from 4 to 2.

Comment 20

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

I think it's better to do that sort of thing at the platform Gfx level.

Comment 21

[jhp (no longer active)]

-> FIXED