Closed Bug 14777 (inlinebg) Opened 20 years ago Closed 17 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) — 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!)
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: 17 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.