7039 - background-position not relative to padding edge when background-repeat is not 'no-repeat' [BG]

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Hixie (not reading bugmail)]

Look at the background image evil test:
   http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/backgroundimage.html

First: for some peculiar reason, the background-color is overridden if
background-repeat is anything other than "repeat". This is visible in three
out of the four tests in the second section.

Second: the exact vertical position of the background image, when the value of
background-repeat means that the image will not be vertically repeated, is
wrong by one pixel. (This will probably also be the case for the horizontal
repeat situation, but I have not tested it yet.) This is visible in the last
two tests of the second section. (You may need to take a screen shot and examine
it closely. Compare it to the reference image at the end of the page.)

Third: if you scroll the first test on that page very slowly past the top of the
viewport and back again, you will see a paint failure (the background behind
the border is sometimes not drawn completely).

This bug should probably be broken into three at some point.

Comment 1

[Hixie (not reading bugmail)]

The second problem is due to the fact that when you are repeating, you measure
from the border edge instead of the padding edge, but when you are not
repeating, you correctly measure from the padding edge.

This is clearly visible in the third section of the test page quoted above.

Comment 2

[Hixie (not reading bugmail)]

BTW, I can no longer see the first or third problems. I am changing the summary
line to refer to the second problem only.

Comment 3

[Hixie (not reading bugmail)]

Migrating from {css1} to css1 keyword. The {css1}, {css2}, {css3} and {css-moz}
radars should now be considered deprecated in favour of keywords.
I am *really* sorry about the spam...

Comment 4

[dcone (gone)]

I think this is fixed due to the transformation fixes.  I could no longer see 
the problems.

Comment 5

[Hixie (not reading bugmail)]

Err... look at:
   http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/backgroundimage.html
...the third section is still not rendered correctly.

Reopening.

Comment 6

[dcone (gone)]

fixed

Comment 7

[bsharma]

Verified.
Windows: 2000-07-13-09-M17
Linux:   2000-07-13-08-M17

Comment 8

[Hixie (not reading bugmail)]

Reopening again. 

TO REPRODUCE:
   Look at "3. More Position" in the testcase:
      http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/backgroundimage.html

EXPECTED RESULTS:
   All four big boxes should look like the first, with the two cats centered
   in the box.

ACTUAL RESULTS:
   Depending on the value of background-repeat, the cats appear/start in
   different positions, causing them to be clipped by the border.

TESTED WITH:
   It's broken in the Windows 2000 commercial build 6.0.17.2000080404.

Comment 9

[Hixie (not reading bugmail)]

Also broken in Mac commercial 6.0.17.200080304. Setting to All/All.

Comment 10

[ekrock's old account (dead)]

Hi Ian --

First favor to ask: a Golden Rule of Bugzilla is that one report should document 
one issue; this enables us to prioritize different issues independently. Could 
you please split this into a separate report for each issue that remains open? 
That way we can prioritize each one as nsbeta3+ or nsbeta3-.

Second request: to assist us in bug triaging, could you please provide the 
following severity assessment information for each bug you break out?

1) Do IE5 Windows and IE5 Mac each pass or fail the test? (If IE5 Windows fails 
too, it's questionable how soon content developers will adopt this feature to 
begin with even if we get it fixed.)

2) What is the impact of leaving the bug unfixed? 

3) As bug fixing is becoming a zero-sum game, if you think the bug must be 
fixed, what other standards-compliance bug on dcone's list would you nominate as 
less important and deserving of Futuring in order to get this one fixed?

Thanks for your help in doing the best we can with the resources and time we 
have remaining!

Comment 11

[Hixie (not reading bugmail)]

Sure.

There is only one issue remaining in this bug report. The others have either
resolved themselves or been split off separately.

1. IE5 for Windows gets this right. (IE5 for Mac gets this wrong, but that
browser has even less market share than Nav4...) (Note: WinIE5 get something
else wrong -- the meaning of 'height' and 'width' -- which means it looks like
they fail the test worse than us. But that is misleading; the actual feature in
question is correctly supported by WinIE5.)

2. The impact of leaving this bug unfixed is that people will use *wrong* CSS
to align their images, because that is a workaround for our bug. But then when
we fix it, all those pages will suddenly look awful with misaligned backgrounds.
Alternatively, if people notice that IE5 gets it right, the impact of this bug
will be to force web authors to create two stylesheets, one for IE and one for
Mozilla, which is something we are desperately trying to move away from...

3. As far as I can see, dcone has no other standards compliance bugs on his
list, so I would not future any of them! ;-)

I do think this should be solved.

Anyway, the problem is (AFAICT) simple.

In nsCSSRendering.cpp, around line 2133, an intersection of the padding area
and the dirty rect is taken. I bet that the dirty rect is relative to the
border area and not the padding area. To fix this bug, as far as I can tell
all we need to do is deflate the aDirtyRect before intersecting it with the
paddingRect -- or alternatively, intersect it before delfating the paddingRect
and then deflating both the paddingRect and the dirtyRect variables.

i.e. change:

2127    paddingArea.Deflate(border);
2128
2129    // The actual dirty rect is the intersection of the padding area and the
2130    // dirty rect we were given
2131    nsRect  dirtyRect;
2132
2133    if (!dirtyRect.IntersectRect(paddingArea, aDirtyRect)) {
2134      // Nothing to paint
2135      return;
2136    }

into:

2129    // The actual dirty rect is the intersection of the padding area and the
2130    // dirty rect we were given
2131    nsRect  dirtyRect;
2132
2133    if (!dirtyRect.IntersectRect(paddingArea, aDirtyRect)) {
2134      // Nothing to paint
2135      return;
2136    }
2137
2138 paddingArea.Deflate(border);
2139 dirtyRect.Deflate(border);

However this idea is untested...

Comment 12

[ekrock's old account (dead)]

OK, I recommend nsbeta3+ for this bug if at all possible. dcone, rods -- how 
doomed are you looking?

Comment 13

[dcone (gone)]

I have 6 nsBeta3 bugs.  I remember this bug.. its a little more involved than 
that patch.  There are other issues with the padding.  I think this regressed 
because of another tiling bug that the images did not start correctly under the 
border which me and Mark Attinasi fixed.  I think this bug needs to be fixed by 
Mark Attinasi and myself and make sure the other tiling bug (the more general 
image placement bug) does not come back.  In anycase this is a lower priority 
than my other nsBeta3 bugs.

Comment 14

[Hixie (not reading bugmail)]

dcone: please do not remove notes in the Status Whiteboard that were added by
other people. Thanks!

Comment 15

[Hixie (not reading bugmail)]

Attachment: Patch: fixes the math that moves the anchor point when repeating the background

Comment 16

[Hixie (not reading bugmail)]

During the thanksgiving weekend I had a play around with this bug and I think
I have (with the help of jag and bryner) found the root cause.

The patch I attached changes the math used when relocating the anchor point when
the background is set to repeat so that it takes into account the paddingArea. 
Before, we were working out the offset of the anchor point relative to the
dirtyRect not the paddingArea.

Having done this, the logic for the fixed and scroll attachement cases were 
identical except that every paddingArea in the scroll case was zero in the 
fixed case, so I inserted code to set paddingArea to zero in that case during
a check for this condition immediately before the math I changed, and then
merged the fixed and scroll cases. This removes two if statements and 
approximately halves the size of the code, so this should be a good thing...
However, maybe it has some unfortunate side effect that I have not detected?

I also removed an addition of tileWidth in two cases that seemed redundant to me
(these are marked with XXX in the diff). Again, this needs to be closely 
examined by someone to check that no other part of the code is relying on this.
Assuming no code uses it, this should also be a slight speed/space improvement.

dcone: I could not work out what tiling bug you were referring to in your 2000-
08-08 10:04 comment, so I could not check to see if this affected it. Could you
look at this patch and r=/a= if it is ok? Thanks!

cc'ing jag who did most of the work and bryner who said he would check that the
|+tileWidth| is not required on unix.

This patch has only been tested on Windows 2000 against the Mozilla trunk CVS 
source from this morning. It has not been tested on Unix or Mac.

Some adhoc testcases are available here:
   http://www.damowmow.com/mozilla/bugs/7039/
Note: Thoses testcases also show bug 61198, which is unaffected by this patch.

Comment 17

[Hixie (not reading bugmail)]

Hey Don, when I said there was no rush, I didn't mean it could wait until the
next millenium! ;-)

Comment 18

[dcone (gone)]

Sorry about that... the patch looks good.. passed the test cases I have.  
Keep an eye on any bugs that hit this area.  I will keep testing also.
Good Job.
r=dcone

Comment 19

[timeless]

+      if (0 != anchor.y) {
please use
+      if (anchor.y) {
(similarly for anchor.x)

do you need someone to check this in after you get an sr?

Comment 20

[Peter "jag" Annema]

Actually, I'd say use |if (anchor.y != 0) {|. Here the comparison against |0|
isn't one of "not" or "false" or "none" but part of a coordinate which just
happens to be 0. <insert type="rant about comparing a constant with variable"/>.

The code is less readable if you use |if (!anchor.y) {| because the ! isn't
logical in that context ("What, no y??") and you have to pause and realize |if
(anchor.y !=0) {| was meant.

Comment 21

[timeless]

Ok, so jag got the boolean confused. sigh

|if (!anchor.y) {|
> because the ! isn't logical in that context ("What, no y??")
Not logical and wrong. The condition is |if (anchor.y) {| if there is a y.

> and you have to pause and realize 
|if (anchor.y !=0) {| was meant.
If there is a y.  whichever.  However, we do seem to use |if (var == const)| 
over |if (const == var)|.

Comment 22

[Peter "jag" Annema]

> The condition is |if (anchor.y) {| if there is a y.

No it's not. The condition is "if y isn't 0", as in "y is ...-4, -3, -2, -1, 1,
2, 3, 4 ...".

> However, we do seem to use |if (var == const)| over |if (const == var)|.
I'm glad the former is the prevailing style, and let's not add to the latter :-)

Comment 23

[Hixie (not reading bugmail)]

Wow, this bug is in keyword heaven.

Comment 24

[Fabian Guisset]

Ian : how can anyone super-review this patch if you are the reporter, the
assignee, and the QA? CC'ing buster for sr=, I was going to CC
[e-mail] but I haven't seen him in some time so I figured he must be
busy.

Comment 25

[Hixie (not reading bugmail)]

Fabian: Per the rules on mozilla.org, to request an sr= you mail the super 
reviewer and cc [e-mail]. They can add themselves to the cc list
if they want to but that is not part of the process.

In any case I have found a potential problem with my patch. Hold off on reviews
for now; I have to investigate (namely, I think I've got an off-by-one error
calculating the dirty rect -- I'm seeing scrollbars in my debug build that don't
get repainted properly when they are slowly dragged back on-screen).

Comment 26

[Peter "jag" Annema]

Attachment: [patch] second take

Comment 27

[buster]

sr=buster.  I have *not* tested it, having an unstable local tree today.  But
the code looks fine, and I'm willing to accept Don's preliminary testing.
However, it looks like Don reviewed the original patch, not the new one.  Don,
please re-review the patch.  Maybe Ian could provide a quick description of what
changed between them to make this easy on you.

Comment 28

[Hixie (not reading bugmail)]

The new patch passes my testing (it's in my local tree). However I haven't 
examined the maths, and haven't been running with this patch for long. I ran 
with my own patch for around 2 months before noticing the regression I'd 
caused... I hope to look at it closer in the coming days.

Comment 29

[Hixie (not reading bugmail)]

r=hixie.

jag's version is my patch with the "XXX" parts actually implemented correctly.
He walked me through the "rounding up" bit, and it makes perfect sense. 
Basically, I was not calculating the exact number of tileWidths for the dirty
rect. His correct version of that part is actually more efficient than mine. :-)

dcone, do you want to moa= it again? Cheers!

Comment 30

[Peter "jag" Annema]

Targetting 0.9.

Comment 31

[dcone (gone)]

r=dcone.. make sure you run a variety of test.. make sure everything works at 
least the way it used to.

Comment 32

[Peter "jag" Annema]

Patch checked in, marking fixed.