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)
Core
Layout
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)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•25 years ago
|
||
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.
Updated•25 years ago
|
Assignee: peterl → dcone
Updated•25 years ago
|
Status: NEW → ASSIGNED
Updated•25 years ago
|
QA Contact: petersen → chrisd
Reporter | ||
Updated•25 years ago
|
Summary: {css1} background-* properties clash → {css1} background-position not relative to padding edge when background-repeat is not 'no-repeat'
Reporter | ||
Comment 2•25 years ago
|
||
BTW, I can no longer see the first or third problems. I am changing the summary
line to refer to the second problem only.
Updated•25 years ago
|
Target Milestone: M14
Reporter | ||
Comment 3•25 years ago
|
||
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•25 years ago
|
||
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
Reporter | ||
Comment 5•25 years ago
|
||
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
Reporter | ||
Updated•25 years ago
|
Resolution: FIXED → ---
Updated•25 years ago
|
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'
Updated•25 years ago
|
Target Milestone: M14 → M15
Updated•25 years ago
|
Target Milestone: M15 → M17
Updated•24 years ago
|
Whiteboard: fix in tree
Updated•24 years ago
|
Status: REOPENED → RESOLVED
Closed: 25 years ago → 24 years ago
Resolution: --- → FIXED
Comment 6•24 years ago
|
||
fixed
Verified.
Windows: 2000-07-13-09-M17
Linux: 2000-07-13-08-M17
Status: RESOLVED → VERIFIED
Reporter | ||
Comment 8•24 years ago
|
||
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
Reporter | ||
Comment 9•24 years ago
|
||
Also broken in Mac commercial 6.0.17.200080304. Setting to All/All.
OS: Windows 98 → All
Hardware: PC → All
Target Milestone: M17 → M18
Comment 10•24 years ago
|
||
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!
Reporter | ||
Comment 11•24 years ago
|
||
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
Comment 12•24 years ago
|
||
OK, I recommend nsbeta3+ for this bug if at all possible. dcone, rods -- how
doomed are you looking?
Comment 13•24 years ago
|
||
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.
Updated•24 years ago
|
Whiteboard: hit during nsbeta2 standards compliance testing → [nsbeta3-]
Target Milestone: M18 → Future
Reporter | ||
Comment 14•24 years ago
|
||
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
Reporter | ||
Updated•24 years ago
|
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]
Reporter | ||
Comment 15•24 years ago
|
||
Reporter | ||
Comment 16•24 years ago
|
||
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
Reporter | ||
Comment 17•24 years ago
|
||
Hey Don, when I said there was no rush, I didn't mean it could wait until the
next millenium! ;-)
Keywords: review
Comment 18•24 years ago
|
||
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•24 years ago
|
||
+ 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?
Assignee | ||
Comment 20•24 years ago
|
||
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•24 years ago
|
||
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)|.
Assignee | ||
Comment 22•24 years ago
|
||
> 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 :-)
Reporter | ||
Comment 23•24 years ago
|
||
Wow, this bug is in keyword heaven.
Assignee: dcone → ian
Status: REOPENED → NEW
Target Milestone: Future → mozilla0.8
Comment 24•24 years ago
|
||
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.
Reporter | ||
Comment 25•24 years ago
|
||
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
Assignee | ||
Comment 26•24 years ago
|
||
Comment 27•24 years ago
|
||
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.
Reporter | ||
Updated•24 years ago
|
Assignee: ian → disttsc
Reporter | ||
Comment 28•24 years ago
|
||
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.
Reporter | ||
Comment 29•24 years ago
|
||
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 31•24 years ago
|
||
r=dcone.. make sure you run a variety of test.. make sure everything works at
least the way it used to.
Assignee | ||
Comment 32•24 years ago
|
||
Patch checked in, marking fixed.
Status: NEW → RESOLVED
Closed: 24 years ago → 24 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•