Closed Bug 433640 Opened 16 years ago Closed 16 years ago

When the "top center" value of the "background-position" CSS property is used for a background image of an element that is an odd number of pixels wide, Firefox 3 blurs the background image (No other browsers, including Firefox 2 behave in such a manner)

Categories

(Core Graveyard :: GFX, defect)

Product:
Core Graveyard
Component:
GFX
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: Zhay07, Assigned: dbaron)

References

(
URL
)

Details

(Keywords: regression, testcase, verified1.9.0.1, Whiteboard: [RC2-])

Attachments

(9 files, 5 obsolete files)

test case
20.14 KB, text/html
Details
screenshot
46.48 KB, image/png
Details
test case with fractional-pixel fixed widths
4.33 KB, text/html
Details
easier-to-see test case
2.83 KB, text/html
Details
test case for comment 23
2.89 KB, text/html
Details
pre-patch: don't do unnecessary refcounting of gfxContexts
8.40 KB, patch
roc
: review+
roc
: superreview+
Details | Diff | Splinter Review
patch
36.97 KB, patch
roc
: review+
roc
: superreview+
beltzner
: approval1.9.0.1+
Details | Diff | Splinter Review
Verified fixed on 1.9.0.1
23.70 KB, image/png
Details
How it looked before fix.
23.08 KB, image/png
Details 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5) Gecko/2008032620 Firefox/3.0b5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5) Gecko/2008032620 Firefox/3.0b5

The summary says everything.  Simply visit the example page to see it in action.

Reproducible: Always

Steps to Reproduce:
1. Type the follow in a text editor and save it as "test.html":

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en">
	<head>
		<title>Firefox Bug</title>
		<meta http-equiv="content-type" content="text/html; charset=ISO-8859-1" />
		<style type="text/css">
		#container
		{
			/* Setting the width to 198 doesn't produce a problem */
			width: 199px;
		}

		#oopsie
		{
			height: 200px;
			background-image: url('blur-test.png');
			background-position: top center; /* This is the culprit! */
		}

		#whoopsie
		{
			height: 200px;
			background-image: url('blur-test.png');
			background-position: 0px 0px;
		}

		h1
		{
			text-align: center;
			font-size: 10pt;
		}
		</style>
	</head>
	<body>
		<div id="container">
			<h1>BUG: This image should not be blurred</h1>
			<div id="oopsie"></div>
			<br /><br />
			<h1>DEFAULT: This is what the above image should look like</h1>
			<div id="whoopsie"></div>
		</div>
	</body>
</html>

2.  Make an image called "blur-test.png" that is 200px by 200px wide (or a different size if you reflect the change in the above CSS code).

3. Open "test.html" in Firefox 3 Beta 5
Actual Results:  
The image in the container with the background-position of "top center" became blurred.

Expected Results:  
The background image should not blur.

See the example page at the given URL.
Product: Firefox → Core
QA Contact: general → general
Confirming, as I was running into this earlier today.

That regressed from the checking for bug 403181:
Gecko/2008030904 Minefield/3.0b5pre ok
Gecko/2008030921 Minefield/3.0b5pre fails

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=2008-03-09+04%3A00%3A00&maxdate=2008-03-09+21%3A00%3A00&cvsroot=%2Fcvsroot

bug 430465 comment 10 is possibly related
Status: UNCONFIRMED → NEW
Component: General → GFX
Ever confirmed: true
Flags: blocking1.9?
OS: Windows XP → All
QA Contact: general → general
Hardware: PC → All
Version: unspecified → Trunk
Attached file test case — 
Resize the window to see the first block (with percentage width) alternate between crisp and blurred display.
It's not great, but it doesn't look that bad to me.
Keywords: regression, testcase
It may not look that bad, but when you're coding a website for someone, it is certainly a BAD thing.
(In reply to comment #4)
> It's not great, but it doesn't look that bad to me.

It is very visible when you have one set of pages that shows the bug, and another set of pages that doesn't.
The thing I have under development right now has two templates. Set one has no problems, set two does show the issue. Users of that site will inevitably go from set one to set two.
The background image used contains one largish Kanji set in a mincho font.


David/Roc - does this a) force an RC2 or b) something we'd fix in whichever RC2 or 3.0.1 came first?
I don't think this should force RC2. I'll work up a fix and then we can assess the risk.
nsCSSRendering::PaintBackgroundWithSC is calling nsLayoutUtils::DrawImage with a 31x32 destination rect and a source rect at 0.5,0 (also 31x32). That makes some kind of sense. nsLayoutUtils::DrawImage passes that more or less straight down to nsThebesImage::Draw.

Not sure what the ideal fix is here. Perhaps we should just round the edges of pxSrc in nsLayoutUtils::DrawImage, preserving pixel centers.
Attached patch fix? (obsolete) — — Splinter Review 
I think this patch is reasonable.

We have to decide what the desired behaviour actually is. We only have 199px to draw into and of course we don't want to scale the image. So which row of pixels is dropped? This patch will drop the leftmost row because the logic seems simpler that way. If that's acceptable then I think this patch is probably the way to go. Using Round() and preserving pixel centers ensures that the source rect width doesn't change (which would be bad, of course).
Assignee: nobody → roc
Status: NEW → ASSIGNED
(In reply to comment #10)
> Created an attachment (id=321031) [details]
> fix?

It is certainly much better. For the case of fixed-width containers, the image remains crisp all the time. In the case if a percentage-width container, I still see some blurring, but it is much, much less noticeable than it was (the first block in the attached testcase).
Given comment 8 taking this off the nomination list.  If you think you have a patch ready for RC2 set [RC2?] in the whiteboard or wanted1.9.0.x for 3.0.1.
Flags: blocking1.9? → blocking1.9-
philippe: can you make a testcase that reproduces the bluriness with a particular fixed size?
FWIW, this affects heights too (bug 432369, which I guess should probably be duped to this, as it looks like the patch will address that case as well).  Not sure if it's worth changing the bug title to reflect that...
(In reply to comment #13)
> philippe: can you make a testcase that reproduces the bluriness with a
> particular fixed size?

With your patch applied,  I don't see any bluriness at all when the container has a fixed width in px, em, etc.

Only when the container has a width declared in percentage (width:20%), there remains a faint blurriness, but as I said, it is _much_ less noticeable. In the test case (attachment.cgi?id=320863), you can see the glyphs jump slightly when slowly resizing the window. I'll attach a screenshot next.

I also noticed that this is even less pronounced when _not_ using percentage margins (the attached testcase uses body {margin:1em 5%;}).
Attached image screenshot — 
Screenshot illustrating comment 11 and 15. Compare the rendering of the image in the top box (width:20%) with those below (width in px).
Did you try different fractional-pixel fixed widths (say, in increments of 0.1px, then 0.02px if that fails)?
With fractional-pixel fixed widths, and reference at the top.
The image appears blurred for widths with .1px ~ .5px
Note  also that the blur appears more significant in the centre of the block than on the edges (left and right)
Here's another test case that's a bit easier to see.  For the sizes closest to an even pixel (such as 32.01px), the blurring is not visible to the naked eye, but still can be detected (copy rendered image into an image editing program that has a "count colors" feature, and see if it comes out with anything other than 2).
Attached patch fix (obsolete) — — Splinter Review 
That's a great testcase, thanks. I've made it into a reftest here.

I'm pretty sure this is the right thing to do. I think this restructuring to focus on preserving scale factors is a good idea --- I thought I had a patch elsewhere that did it for another bug, but I can't find it.
Attachment #321031 - Attachment is obsolete: true
Attachment #321343 - Flags: superreview?(dbaron)
Attachment #321343 - Flags: review?(dbaron)
Attached patch fix v2 (obsolete) — — Splinter Review 

        Attachment #321343 -
        Attachment is obsolete: true

        Attachment #321353 -
        Flags: superreview?(dbaron)

        Attachment #321353 -
        Flags: review?(dbaron)

        Attachment #321343 -
        Flags: superreview?(dbaron)

        Attachment #321343 -
        Flags: review?(dbaron)

    
    

    
  
Oops, forgot to mention that this new patch has the same code but the test is improved so it actually passes.

    
    

    
  
I found a problem with this patch.
A box with {background-position:right;}; the background sometimes is clipped by 1px on the right when the parent element has horizontal margins expressed in percentage(s). This does _not_ happen with an unpatched build.

The patch does work fine for the issues with background-position:center.

    
    

    
  


  
Same as previous test case by Kai Liu, but with
body {margin:1em 5%}
div {background-position:right;}

One vertical line on the right is sometimes missing.

    
    

    
  
That doesn't surprise me, can we live with that behaviour? The alternative requires background-position knowing about pixel alignment, which would suck.

    
    

    
  
I don't think that behavior is acceptable.

    
    

    
  
I don't see why the alternative requires background-position knowing about pixel alignment.  I think this ought to just work if we're consistent about snapping things to pixel boundaries in the same way, and if we don't do any intermediate pixel-snapping.

(That said, I don't actually understand the patch yet.)

    
    

    
  
Here are the requirements on nsLayoutUtils::DrawImage:
1) The scaling of the image must not be changed at all. In particular, if the incoming source and dest rects are the same size, then the source and dest rects we pass down to nsThebesImage must be the same size.
2) The edges of the outgoing source rect must be pixel-aligned.
3) The edges of the outgoing dest rect must be pixel-aligned.

Now suppose the incoming source rect and dest rect (xywh) are (0.5,0,99,100). What should we pass to nsThebesImage?

I think we have to make the dest rect (1,0,99,100). The source rect could be either (1,0,99,100) or (0,0,99,100), but the first cuts off the first column of pixels of the image, which I assume you'd find unacceptable if background-position is 'left', and the second cuts off the last column of pixels of the image, and I assume you'd find that unacceptable if background-position is 'right'.

    
    

    
  
(This is all only for the case where the background tiling code detects that the tiling can be done using a single image, right?  Shouldn't we also be ensuring that the single-image codepath is producing the same results as the tiling codepath?)

Perhaps the background tiling code should be pixel-snapping the source rect (i.e., the background-position calculations?).  I tend to think that when the element moves from a position of 0.4px to 0.5px and its edges shift over a pixel, that's probably also when its background should shift a pixel.

    
    

    
  
(In reply to comment #25)
> That doesn't surprise me, can we live with that behaviour? The alternative
> requires background-position knowing about pixel alignment, which would suck.

Given that authors frequently use background images to simulate rounded/cutted corners (with borders included), I think it will be quite visible.

Like this:
http://animate-e.com/articles/2005/0327.html



    
    

    
  
(In reply to comment #29)
> (This is all only for the case where the background tiling code detects that
> the tiling can be done using a single image, right?

Yes.

> Shouldn't we also be
> ensuring that the single-image codepath is producing the same results as the
> tiling codepath?)

I don't think so. See https://bugzilla.mozilla.org/show_bug.cgi?id=421438#c1, option 3.

OTOH option 2 there might be acceptable --- make background painting always use the tiling path.

> Perhaps the background tiling code should be pixel-snapping the source rect
> (i.e., the background-position calculations?).

That is an option, but I'm not sure what the desired behaviour is. Elaborating on the previous example, suppose we have an element at (0.5,0,99,100). When background-position-x is 0%, we want the source rect to be (0,0,99,100) and when background-position-x is 100%, we want the source rect to be (1,0,99,100). I suppose we can get that by pixel-snapping aOriginBounds and tileWidth/tileHeight in ComputeBackgroundAnchorPoint before computing the result. How does that sound?

    
    

    
  
(In reply to comment #31)
> > Shouldn't we also be
> > ensuring that the single-image codepath is producing the same results as the
> > tiling codepath?)
> 
> I don't think so. See https://bugzilla.mozilla.org/show_bug.cgi?id=421438#c1,
> option 3.

I'm not so sure the example in option (3) has been relevant in the past; authors can't tile images that aren't an integer number of CSS pixels wide.  It is possible with 'border-image', though, and one of the things I think we need to be careful to get right there (bug 378217).

I'm really not sure whether we want 9 or 10 tiles each way in that example.

> OTOH option 2 there might be acceptable --- make background painting always use
> the tiling path.

Though that bug makes it sound like the tiling path is even worse right now, though, since it changes the rasterization of unscaled images.

> > Perhaps the background tiling code should be pixel-snapping the source rect
> > (i.e., the background-position calculations?).
> 
> That is an option, but I'm not sure what the desired behaviour is. Elaborating
> on the previous example, suppose we have an element at (0.5,0,99,100). When
> background-position-x is 0%, we want the source rect to be (0,0,99,100) and
> when background-position-x is 100%, we want the source rect to be (1,0,99,100).

Assuming the background image is 100*100, yeah.

> I suppose we can get that by pixel-snapping aOriginBounds and
> tileWidth/tileHeight in ComputeBackgroundAnchorPoint before computing the
> result. How does that sound?

It'll take me a bit of time to understand what ComputeBackgroundAnchorPoint does.  Though it looks like aTileWidth and aTileHeight always come from an image size, so should already be pixel-snapped.

I guess what I was thinking was pixel-snapping bgOriginArea )and maybe also bgClipArea) in PaintBackgroundWithSC, though -- it looks like bgOriginArea is what's passed as aOriginBounds.

    
    

    
  
If you want me to, sure.

    
    

    
  
Seems like you're already working out what you want to do :-)
Assignee: roc → dbaron
Status: ASSIGNED → NEW

    
    

    
  

      
      
      
      

        Attached patch
          
          
        possible patch (obsolete)
        — 
          — Splinter Review
      

    


  
This is my current thinking; I still need to write tests.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  
This patch:
 * adds an additional reftest in reftests/pixel-rounding.  This test is passed by Fx2 but both your patch (attachment 321353 [details] [diff] [review]) and my original patch (attachment 322208 [details] [diff] [review]) fail it
 * incorporates your reftest and modifies it by:
   + using float instead of inline-block to make it easier to test in other browsers
   + incrementing all the background-position offsets in the reference by +15 so they make sense conceptually
   + changing the reference for the 31.1px case so that it matches 31px rather than 32px

This means that in your reftest, the behavior this patch implements:
 * matches Fx2 except for the 31.1px case
 * matches IE7 on the {31,32}.{0,1}px cases and not on the {31,32}.{5,8}px cases

        Attachment #321353 -
        Attachment is obsolete: true

        Attachment #322208 -
        Attachment is obsolete: true

        Attachment #322307 -
        Flags: superreview?(roc)

        Attachment #322307 -
        Flags: review?(roc)

        Attachment #321353 -
        Flags: superreview?(dbaron)

        Attachment #321353 -
        Flags: review?(dbaron)

    
    

    
  
(and also changes the code, obviously)

    
    

    
  
+  nsRefPtr<gfxContext> ctx = aRenderingContext.ThebesContext();

This can be a regular pointer.

+    aRect.x = aDC->GfxUnitsToAppUnits(tmpRect.pos.x);
+    aRect.y = aDC->GfxUnitsToAppUnits(tmpRect.pos.y);
+    aRect.width = aDC->GfxUnitsToAppUnits(tmpRect.size.width);
+    aRect.height = aDC->GfxUnitsToAppUnits(tmpRect.size.height);

This rounds. It doesn't matter much since we're only rounding to the nearest appunit, but probably it should round the top-left and bottom-right points instead of rounding width/height directly.

Otherwise looks good.

    
    

    
  


  
Your comment about the nsRefPtr<gfxContext> was actually on code I was moving.  In any case, this patch fixes unnecessary nsRefPtr<gfxContext>s that I found using grep in layout/base and layout/generic.  (Must be applied before the patch I'm about to attach.)

        Attachment #322413 -
        Flags: superreview?(roc)

        Attachment #322413 -
        Flags: review?(roc)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch
        — 
          — Splinter Review
      

    


  
And I think this should address the remaining comment from comment 39.

        Attachment #322307 -
        Attachment is obsolete: true

        Attachment #322414 -
        Flags: superreview?(roc)

        Attachment #322414 -
        Flags: review?(roc)

        Attachment #322307 -
        Flags: superreview?(roc)

        Attachment #322307 -
        Flags: review?(roc)

    
    

    
  
Comment on attachment 322413 [details] [diff] [review]
pre-patch: don't do unnecessary refcounting of gfxContexts

I just noticed one of the diffs here is actually patching the patch from bug 378217, which is lower in my queue, so I redirected that change to bug 378217 comment 50.

        Attachment #322413 -
        Flags: superreview?(roc)

        Attachment #322413 -
        Flags: superreview+

        Attachment #322413 -
        Flags: review?(roc)

        Attachment #322413 -
        Flags: review+

        Attachment #322414 -
        Flags: superreview?(roc)

        Attachment #322414 -
        Flags: superreview+

        Attachment #322414 -
        Flags: review?(roc)

        Attachment #322414 -
        Flags: review+
Flags: blocking1.9.0.1?

    
    

    
  
There is *some* risk to this patch, although I'm pretty comfortable with it.

I was initially going to try to push to get this into RC2 if we do an RC2, but I've now changed my mind.  The reason I was initially thinking I wanted that was because I was thinking this bug would affect techniques like http://www.alistapart.com/articles/slidingdoors/ + http://www.alistapart.com/articles/slidingdoors2/ -- but then I realized that they're always going to use top/left/right/bottom/0%/100%/<length-in-pixels> alignment (to line an edge or position within the image up with an edge of the box) rather than center or some percentage that's not 0% or 100% (which is what triggers this bug).

Because of that, I don't think it's the type of thing that's going to make authors redesign their pages (from flexible layouts to fixed layouts).  They'll just complain the pages are ugly in Fx3.0 (which will hopefully be fixed in Fx3.0.1)

But I'd be open to a strong case that we should try to fix this in an Fx3.0 RC2 (if we do an RC2).
Flags: blocking1.9.0.1? → blocking1.9.0.1+
Whiteboard: [RC2?] → [RC2-]

    
    

    
  
It's a big assumption David.

Unfortunately it breaks in a lot of areas on the new BBC iPlayer designs as we move towards sprites and background center aligning techniques involved when attaching to text elements.

We can't change the design to accommodate and use fix positioning - as this doesn't produce a good cross browser result, neither is it flexible for increased text sizes.

The iPlayer site receives over 500,000 visitors a day. I urge you to reconsider and get the fix in. It really looks unprofessional to see even one bug in the Firefox 3 release that a huge audience of FF3 users will see.

    
    

    
  
http://hg.mozilla.org/mozilla-central/index.cgi/rev/fee5c84460d3
http://hg.mozilla.org/mozilla-central/index.cgi/rev/b9fd38b8f1e0
plus an extra change to mark the reftest as known to fail on Mac due to a known bug that affects our rendering of the reference:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/1a969abc8556

-> fixed for 1.9.1

It's too late for Firefox 3 at this point; we'll try to get it in for 3.0.1.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Blocks: 431993

    
    

    
  
Is it linked with the very slow scrolling with fixed backgrounds or elements? (https://bugzilla.mozilla.org/show_bug.cgi?id=124150 and https://bugzilla.mozilla.org/show_bug.cgi?id=201307)

    
    

    
  
(In reply to comment #48)
> No.
> 

See bug 431993 comment #13 and #14 (and the forums posts linked from there) for why this is likely to be related.  The most serious problems with scrolling speed are not present in the 3.1 nightlies (where this bug is fixed) but are still there in the 3.0 RCs.

    
    

    
  
I can believe that it's related to a performance regression that happened in the past few months, since the drawing would be more expensive.  It's not related to a bug that's been around for 5-6 years.

    
    

    
  
re: these don't appear to be the same. I did read 433640 beforehand. The difference is that 1. today's release did not fix the problem, and 2. My bug reported is not based on "an element that is an odd number of pixels wide".(In reply to comment #51)
> *** Bug 438903 has been marked as a duplicate of this bug. ***
> these don't appear to be the same. I did read 433640 beforehand. The difference is that 1. today's release did not fix the problem, and 2. My bug reported is not based on "an element that is an odd number of pixels wide".



    
    

    
  
433640 says it is resolved, but it is not, according to this thread. Percentage width containers still blur BG images. This rendering issue will not be unacceptable to many, and is not present in any other browser, including older FF.


    
    

    
  
(In reply to comment #52)
> 1. today's release did not fix the problem
>

See comment #45.  This fix is not in RC3, but it is in the latest trunk nightly (3.1a1pre).

> is not based on "an element that is an odd number of pixels wide".
>

The element being an odd # of pixels wide is only one of the ways to trigger this bug; what you reported in your bug is just another way to triggering this same bug.


    
    

    
  
Adding another test case for you here:

As you can see in this test case, it causes a very bad result and not merely 'blurring'
http://www.multiblah.com/exps/css/bg_problem/bg_problem2.html

Also, this problem is very severe since it applies to ANY containing block level element. That includes the BODY element.

So, while you might have some control over you DIV sizes, you can't control the size of the browser window. See this link for how it gets effected if you're windows size is 701px wide. This will affect a lot of existing designs, as having a large background image centered via CSS is highly common.
http://www.multiblah.com/exps/css/bg_problem/bg_problem.html

You can also see a problem in the header of msnbc.com which uses this technique too.

    
    

    
  
i have the same problem:
http://loomarea.com/bgbug.htm

    
    

    
  
I've run into this problem on a site that I helped design, except I have varying results.
Site:
http://www.shopvirtualshops.com

The images in the "top navigation bar" have the following background image:
http://shopvirtualshops.com/media/themeimages/css-theshop-bg.gif

I use the image and move it up or down to simulate mouse over effects.  It seems to blur the image on some of the menu items.  It's very evident because I have text in the image...

Hope this gets resolved soon.

    
    

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

a=beltzner, this has been on mozilla-central for about a month, so confidence is high.

        Attachment #322414 -
        Flags: approval1.9.0.1? → approval1.9.0.1+

    
    

    
  
Not blocking, though approved, so if it gets landed before we freeze for builds (which depends on the orange clearing) that's fine, otherwise it'll wait for 1.9.0.2.
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.1-
Flags: blocking1.9.0.1+

    
    

    
  
Landed on CVS head for 1.9.0.1.
Keywords: fixed1.9.0.1

    
    

    
  


  
Verified based on test case in comment 58. 1.9.0.1 checks out as you can see from this screen shot.

    
    

    
  


  
BTW, this is how it looked in FF 3.

    
    

    
  
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.1) Gecko/2008070206 Firefox/3.0.1 <== Verified on this build.
Keywords: fixed1.9.0.1 → verified1.9.0.1

    
  
Depends on: 446100

    
  
Depends on: 446284

    
    

    
  
This patch seems to have caused a regression on bug 230101 which in 1.9.0 appeared to be fixed although the bug was never closed.

    
    

    
  
How is this a duplicate of that bug if that bug was reported at a later date?

    
    

    
  
That bug is a duplicate of this one.
Product: Core → Core Graveyard

    
  
Depends on: 230101

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: