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)
Core
Layout
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)
(deleted),
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
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?
Kipp, I don't whether this is you or whether it's Don for the background
rendering code
Updated•25 years ago
|
QA Contact: petersen → chrisd
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
Reporter | ||
Comment 4•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...
Updated•25 years ago
|
Summary: {css1} [BLOCK] Issues with 'background-position' on inline elements → [BLOCK] Issues with 'background-position' on inline elements
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
Reporter | ||
Comment 8•24 years ago
|
||
Lowering severity to enhancement, this can definitely wait for 5.1.
Severity: trivial → enhancement
Updated•24 years ago
|
Summary: [BLOCK] Issues with 'background-position' on inline elements → [INLINE] Issues with 'background-position' on inline elements
Reporter | ||
Updated•24 years ago
|
QA Contact: chrisd → py8ieh=bugzilla
Reporter | ||
Updated•24 years ago
|
Summary: [INLINE] Issues with 'background-position' on inline elements → [INLINE] Issues with 'background-position' on inline elements [BG]
Whiteboard: WG
Reporter | ||
Updated•24 years ago
|
Keywords: mozilla1.2
Comment 9•23 years ago
|
||
Build reassigning Buster's bugs to Marc.
Assignee: buster → attinasi
Status: ASSIGNED → NEW
Updated•23 years ago
|
Whiteboard: WG → WG [CSS1-5.3.6]
Comment 10•22 years ago
|
||
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.
Reporter | ||
Comment 11•22 years ago
|
||
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•22 years ago
|
||
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
Reporter | ||
Comment 13•22 years ago
|
||
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 → ---
Assignee | ||
Comment 14•22 years ago
|
||
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
Reporter | ||
Comment 15•22 years ago
|
||
*** Bug 165703 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 16•22 years ago
|
||
I've got this half-working, but still have some things to work out. I think I
can make 1.4a with this. Targetting.
Assignee | ||
Comment 17•22 years ago
|
||
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.
Reporter | ||
Updated•22 years ago
|
Attachment #111717 -
Flags: superreview?(roc+moz)
Reporter | ||
Comment 18•22 years ago
|
||
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"?
Assignee | ||
Comment 20•22 years ago
|
||
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().
Assignee | ||
Comment 22•22 years ago
|
||
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!)
Assignee | ||
Comment 23•22 years ago
|
||
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
Assignee | ||
Updated•22 years ago
|
Attachment #111717 -
Flags: superreview?(roc+moz)
Assignee | ||
Updated•22 years ago
|
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+
Assignee | ||
Comment 25•22 years ago
|
||
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
:-)
Hixie, you evil little man
Assignee | ||
Comment 27•22 years ago
|
||
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
Reporter | ||
Comment 28•22 years ago
|
||
looks good
Assignee | ||
Updated•21 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•