Closed Bug 7039 Opened 25 years ago Closed 24 years ago

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

Categories

(Core :: Layout, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla0.9

People

(Reporter: ian, Assigned: jag+mozbugs)

References

()

Details

(Keywords: css1, regression, testcase, Whiteboard: hit during nsbeta2 standards compliance testing)

Attachments

(2 files)

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.
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.
Assignee: rickg → peterl
Assignee: peterl → dcone
Status: NEW → ASSIGNED
QA Contact: petersen → chrisd
Summary: {css1} background-* properties clash → {css1} background-position not relative to padding edge when background-repeat is not 'no-repeat'
BTW, I can no longer see the first or third problems. I am changing the summary line to refer to the second problem only.
Target Milestone: M14
Keywords: css1
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...
I think this is fixed due to the transformation fixes. I could no longer see the problems.
Status: ASSIGNED → RESOLVED
Closed: 25 years ago
Resolution: --- → FIXED
Err... look at: http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/backgroundimage.html ...the third section is still not rendered correctly. Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Summary: {css1} background-position not relative to padding edge when background-repeat is not 'no-repeat' → [BACKGROUND] background-position not relative to padding edge when background-repeat is not 'no-repeat'
Target Milestone: M14 → M15
Target Milestone: M15 → M17
Whiteboard: fix in tree
Status: REOPENED → RESOLVED
Closed: 25 years ago24 years ago
Resolution: --- → FIXED
fixed
Verified. Windows: 2000-07-13-09-M17 Linux: 2000-07-13-08-M17
Status: RESOLVED → VERIFIED
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.
Status: VERIFIED → REOPENED
QA Contact: chrisd → py8ieh=bugzilla
Resolution: FIXED → ---
Summary: [BACKGROUND] background-position not relative to padding edge when background-repeat is not 'no-repeat' → background-position not relative to padding edge when background-repeat is not 'no-repeat'
Whiteboard: fix in tree → hit during nsbeta2 standards compliance testing
Also broken in Mac commercial 6.0.17.200080304. Setting to All/All.
OS: Windows 98 → All
Hardware: PC → All
Target Milestone: M17 → M18
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!
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...
Keywords: 4xp
OK, I recommend nsbeta3+ for this bug if at all possible. dcone, rods -- how doomed are you looking?
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.
Whiteboard: hit during nsbeta2 standards compliance testing → [nsbeta3-]
Target Milestone: M18 → Future
dcone: please do not remove notes in the Status Whiteboard that were added by other people. Thanks!
Whiteboard: [nsbeta3-] → [nsbeta3-] hit during nsbeta2 standards compliance testing
Summary: background-position not relative to padding edge when background-repeat is not 'no-repeat' → background-position not relative to padding edge when background-repeat is not 'no-repeat' [BG]
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.
Keywords: patch
Hey Don, when I said there was no rush, I didn't mean it could wait until the next millenium! ;-)
Keywords: review
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
+ 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?
Keywords: nsbeta3approval
Whiteboard: [nsbeta3-] hit during nsbeta2 standards compliance testing → hit during nsbeta2 standards compliance testing
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.
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)|.
> 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 :-)
Wow, this bug is in keyword heaven.
Assignee: dcone → ian
Status: REOPENED → NEW
Target Milestone: Future → mozilla0.8
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 rpotts@netscape.com but I haven't seen him in some time so I figured he must be busy.
Fabian: Per the rules on mozilla.org, to request an sr= you mail the super reviewer and cc reviewers@mozilla.org. 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).
QA Contact: ian → dbaron
Attached patch [patch] second take (deleted) — Splinter Review
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.
Assignee: ian → disttsc
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.
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!
Targetting 0.9.
Target Milestone: mozilla0.8 → mozilla0.9
r=dcone.. make sure you run a variety of test.. make sure everything works at least the way it used to.
Patch checked in, marking fixed.
Status: NEW → RESOLVED
Closed: 24 years ago24 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: