14777 - (inlinebg) [INLINE] Issues with 'background-position' on inline elements [BG]

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Hixie (not reading bugmail)]

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?

Comment 1

[troy]

Kipp, I don't whether this is you or whether it's Don for the background
rendering code

Comment 2

[leger]

Updating to default Layout Assignee...kipp no longer with us :-(

Comment 3

[troy]

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.

Comment 4

[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 5

[buster]

mine! mine mine mine!  all mine!  whoo-hoo!

Comment 6

[buster]

redistributing bugs across future milestones, sorry for the spam

Comment 7

[buster]

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. 

Comment 8

[Hixie (not reading bugmail)]

Lowering severity to enhancement, this can definitely wait for 5.1.

Comment 9

[Kevin McCluskey (gone)]

Build reassigning Buster's bugs to Marc.

Comment 10

[Greg K.]

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.

Comment 11

[Hixie (not reading bugmail)]

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.

Comment 12

[Greg K.]

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?

Comment 13

[Hixie (not reading bugmail)]

caillon: would you be able to look at this in the near future? Cheers.

Comment 14

[Christopher Aillon (sabbatical, not receiving bugmail)]

Man, you make one patch and all of a sudden become the resident owner... ;-)

Sure Ian, looking into it as per our discussion.

Comment 15

[Hixie (not reading bugmail)]

*** Bug 165703 has been marked as a duplicate of this bug. ***

Comment 16

[Christopher Aillon (sabbatical, not receiving bugmail)]

I've got this half-working, but still have some things to work out.  I think I
can make 1.4a with this.  Targetting.

Comment 17

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Proposed patch v1

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.

Comment 18

[Hixie (not reading bugmail)]

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!

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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"?

Comment 20

[Christopher Aillon (sabbatical, not receiving bugmail)]

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...

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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().

Comment 22

[Christopher Aillon (sabbatical, not receiving bugmail)]

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!)

Comment 23

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: v2: Roc's comments addressed

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.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 25

[Christopher Aillon (sabbatical, not receiving bugmail)]

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
 :-)

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hixie, you evil little man

Comment 27

[Christopher Aillon (sabbatical, not receiving bugmail)]

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!

Comment 28

[Hixie (not reading bugmail)]

looks good