Closed Bug 14777 (inlinebg) Opened 25 years ago Closed 22 years ago

[INLINE] Issues with 'background-position' on inline elements [BG]

Categories

(Core :: Layout, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla1.3beta

People

(Reporter: ian, Assigned: caillon)

References

()

Details

(Keywords: css1, css3, Whiteboard: [CSS1-5.3.][Hixie-P1])

Attachments

(1 file, 1 obsolete file)

Technically speaking, 'background-position' isn't allowed on inline elements. However, that is, IMHO, an oversight on the spec's behalf. Anyhow, we currently do implement 'background-position' on inline elements. Currently, we render the background of inline elements for each part of the inline box individually, if the inline box splits into many lines. This means that an image specified to be "background-repeat: no-repeat" will be repeated on each line, probably not what the author intended. This can get easily ugly -- see the last paragraph of: http://www.bath.ac.uk/%7Epy8ieh/internet/projects/mozilla/background/bg-pos- 1.html ...where padding has been provided for the image, but since the text wraps onto more than one line, the image appears under the text on subsequent lines. Our current implementation, while logical, is therefore inadequate. IE5 has an alternative approach: it finds the union of all the inline boxes created by the inline element, and then places the image relative to that. This creates a nice effect with big images -- see the second section of that test page -- but causes problems with small images positioned in corners. For example, if the inline box is split like this: This is some [NICE TEXT] isn't it. ...then an image positioned at the bottom right would end up invisible, as the bottom right of the union of those two parts of the inline box is not actually in either box. Hence, IE5's implementation is also inadequate, although just as logical. Opera 3.60 doesn't apply background-image to inline elements at all. (Hmm?!) I therefore suggest that we position the background image as if the inline box was not split. That is, treat the inline box as one long box, position the background relative to that, and then split the box up. Hence, the bottom right of the [NICE TEXT] example above would be at the bottom right of TEXT, and the position "50% 50%" would be roughly between the two words. So an image roughly 1em square, position there, would be half on the first line and half on the second line. This would mean that "center left" and "center right" would _always_ position at the start and end of the line respectively. It would also mean that big images would not line up as they do in IE5, unfortunately. In fact, an image bigger than the font-size would only ever have one part of it shown (this is already the case with the current implementation, BTW). Comments? Peter: What do the WG think of this issue?
Assignee: troy → kipp
Kipp, I don't whether this is you or whether it's Don for the background rendering code
QA Contact: petersen → chrisd
Severity: minor → trivial
Status: NEW → ASSIGNED
Target Milestone: M19
Updating to default Layout Assignee...kipp no longer with us :-(
Why are you re-reassing layout bugs? Do NOT touch layout bugs. The bugs are assigned to Kipp so they can stay neatly organized until we have a new owner for the block/inline code.
Summary: {css1} Issues with 'background-position' on inline elements → {css1} [BLOCK] Issues with 'background-position' on inline elements
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...
Summary: {css1} [BLOCK] Issues with 'background-position' on inline elements → [BLOCK] Issues with 'background-position' on inline elements
mine! mine mine mine! all mine! whoo-hoo!
Assignee: kipp → buster
redistributing bugs across future milestones, sorry for the spam
Target Milestone: M19 → M21
This bug has been marked "future" because the original netscape engineer working on this is over-burdened. If you feel this is an error, that you or another known resource will be working on this bug,or if it blocks your work in some way -- please attach your concern to the bug for reconsideration.
Status: NEW → ASSIGNED
Target Milestone: M21 → Future
Lowering severity to enhancement, this can definitely wait for 5.1.
Severity: trivial → enhancement
Summary: [BLOCK] Issues with 'background-position' on inline elements → [INLINE] Issues with 'background-position' on inline elements
QA Contact: chrisd → py8ieh=bugzilla
Summary: [INLINE] Issues with 'background-position' on inline elements → [INLINE] Issues with 'background-position' on inline elements [BG]
Whiteboard: WG
Keywords: mozilla1.2
Build reassigning Buster's bugs to Marc.
Assignee: buster → attinasi
Status: ASSIGNED → NEW
Whiteboard: WG → WG [CSS1-5.3.6]
Ian, do you still observe the problem with your last paragraph? Using FizzillaCFM/2002070913, I only see the blue diamond once in each of the four SPANs, and in the expected, identical position in each.
The problem is still visible, but now in the third paragraph instead of the last one. Basically, we now only do the first slice of the inline box, instead of doing all of them -- but what we should IMHO be doing is positioning the image before the inline box is split, and then using that position when they are split.
Then I guess I can reconfirm this using FizzillaCFM/2002070913. Setting All/All. Soes anyone else see the fourth SPAN in the last paragraph get two vertically-aligned diamonds when scrolling to the bottom of page using the scroll arrows or scroll wheel, but not when the scroll elevator is manipulated directly?
OS: Windows 98 → All
Hardware: PC → All
caillon: would you be able to look at this in the near future? Cheers.
Alias: inlinebg
Assignee: attinasi → caillon
Severity: enhancement → normal
Keywords: css3
Whiteboard: WG [CSS1-5.3.6] → [CSS1-5.3.][Hixie-P1]
Target Milestone: Future → ---
Man, you make one patch and all of a sudden become the resident owner... ;-) Sure Ian, looking into it as per our discussion.
Status: NEW → ASSIGNED
*** Bug 165703 has been marked as a duplicate of this bug. ***
I've got this half-working, but still have some things to work out. I think I can make 1.4a with this. Targetting.
Keywords: mozilla1.2
Priority: P3 → P2
Target Milestone: --- → mozilla1.4alpha
Attached patch Proposed patch v1 (obsolete) (deleted) — Splinter Review
It would be great to sanely render inline backgrounds for 1.3beta. See http://www.hixie.ch/tests/adhoc/css/background/inline/policy/ for a description of what I am implementing. I pass these tests (from what I can tell, Hixie will need to verify these if this patch lands). One note, that this patch will essentially back out the fix for bug 38764. However, that was just an evil hack and should be removed anyway in favor of these policies. This _does not_ regress editor though, since they are using no-repeat rules, and the background will not wrap on to subsequent lines with that and the (default) continuous policy.
Attachment #111717 - Flags: superreview?(roc+moz)
Looks good to me, although I'm not 100% sure that the ...-origin property will work right (?). I'll test this further once it's checked in! :-) roc: could you grant caillon a review? Cheers!
Maximal comment: This whole approach of the InlineBackgroundData global seems a bit dodgy to me. First of all mLastFrame could be deleted and later another frame recreated at the same address, which would kill you. Also, you could easily get a reflow in between background paints which would cause you to use stale data. I think at least you should have an nsCSSRendering method which invalidates cached painting data, called at the end of a paint by nsPresContext. This method would (at least) Reset() your InlineBackgroundData. Major comments: Get rid of DoBoundingBox and use nsRect::UnionRect instead. SetNextFrame is a poor name since the frame is really the *current* frame. I'd just change it to "SetFrame". + // Are we positioned? This value is only meaningful to us if we are. I don't understand why you don't apply the full unbroken width for all inlines. I think having InitBoundingBox and InitContinuationPoint is an unnecessary optimization. The extra cost of computing the full bounding box is minimal. Just do that always and simplify the code. You should add a comment explaining why you're using the InlineBackgroundData cache (I presume it's to make painting of a long paragraph with N lines and M visible lines O(N + M) instead of O(N*M).) Minor comments: + NS_ASSERTION(NS_STYLE_BG_INLINE_POLICY_CONTINUOUS == Color.mBackgroundInlinePolicy, + "unknown background-inline-policy!"); Why don't you just move the default: to before the CONTINUOUS case and put an ASSERTION(PR_FALSE) between it and "case CONTINUOUS"?
Yes, I'll admit it is a bit dodgy. I'm trying to not have to bloat nsInlineFrame (by as much as 24 bytes), and it would be a good thing to keep painting these at a linear time rather than exponential...
Not exponential, just quadratic, but that's bad enough, I agree. Can you flush the data after the prescontext has finished painting, as I suggested? That would be good enough for performance, I think. You could actually call the nsCSSRendering method from PresShellViewEventListener::DidRefreshRect() and DidRefreshRegion().
Right, I meant quadratic. Yep I'm looking to add that method right now. I'll post an updated patch with that and your other comments addressed soon. (UnionRect is a beautiful thing that I can't believe I overlooked!)
Attached patch v2: Roc's comments addressed (deleted) — Splinter Review
As I was in the middle of a full tree re-compile, I have not run with this to verify it still works as planned. Nothing in the logic has changed though (aside from the changes roc requested), so everything should still be kosher.
Attachment #111717 - Attachment is obsolete: true
Attachment #111717 - Flags: superreview?(roc+moz)
Attachment #111759 - Flags: superreview?(roc+moz)
Comment on attachment 111759 [details] [diff] [review] v2: Roc's comments addressed Nice. Please test it before you check in to make sure it does work :-). r+sr=roc+moz
Attachment #111759 - Flags: superreview?(roc+moz)
Attachment #111759 - Flags: superreview+
Attachment #111759 - Flags: review+
Just ran the tests with my freshly compiled copy and the tests on Hixie's site still pass. With flying colors, I might add: http://www.hixie.ch/tests/adhoc/css/background/inline/policy/continuous/008.html :-)
Fix checked in 01/17/2003 01:33 PDT. Ian, could you please verify this in tomorrow's builds and let me know of any issues ASAP? Thanks!
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.4alpha → mozilla1.3beta
looks good
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: