REGRESSION: span.offsetWidth is width of offsetParent and not width of self

RESOLVED FIXED

Status

()

P1
normal
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: sjoerdmulder, Assigned: roc)

Tracking

Trunk
x86
Windows XP
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Reporter)

Description

11 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.12) Gecko/20080201 Firefox/2.0.0.12
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9b3) Gecko/2008020514 Firefox/3.0b3

When you have a SPAN, with inside a DIV the span's offsetWidth should just be the text defined in there, but since the latests beta (regression is from Beta 2 to Beta 3) the offsetWidth property is now the width of the full offsetParent!

Reproducible: Always

Steps to Reproduce:
1.See attachted testcase
2.Alert should be 'true' (it is in Fx2, IE, Op, Sf3)
3.
(Reporter)

Comment 1

11 years ago
Created attachment 303039 [details]
Testcase

Comment 2

11 years ago
David can you take a look - this looks to have regressed Backbase a popular web framework
Assignee: nobody → dbaron
Flags: blocking1.9+
Priority: -- → P1
Almost certainly a regression from bug 410229.

It's actually not clear to me which behavior is more correct; the CSS spec tends to imply that the wrapper block we create isn't really a box for the element:

# When an inline box contains a block box, the inline box (and its inline
# ancestors within the same line box) are broken around the block. The line
# boxes before the break and after the break are enclosed in anonymous boxes,
# and the block box becomes a sibling of those anonymous boxes. When such an
# inline box is affected by relative positioning, the relative positioning
# also affects the block box. 
 -- http://www.w3.org/TR/CSS21/visuren.html#anonymous-block-level

... and then makes an exception for relative positioning (that wouldn't be needed if the wrapper were considered part of the element).


I'm open to arguments either way on which behavior is more sensible... but then there's the issue of compatibility.

roc, are you interested in taking this?
Blocks: 410229
Status: UNCONFIRMED → NEW
Ever confirmed: true
... and it's worth checking how the more compatible behavior differs across these various APIs.

I'd also note that the old code (before bug 410229) was definitely *incorrect*, so I don't think the question is whether to revert it or not ... but rather a question of whether we should count the middle block in the IB split, or just the two inlines on either side (although one or the other might be missing, I think).

(But testing of compatibility could show that that's not actually the question -- but it's definitely worth being aware that there are 3 possible behaviors here.)
Yes, I'll take this.
Assignee: dbaron → roc
Fortunately the fix should be easy as soon as we figure out what the right behaviour is.

CCing Anne so once we've figured out the right behaviour he can update the CSSOM spec...
Version: unspecified → Trunk
Created attachment 303696 [details]
testcase

Here's a more general testcase so we can see what's going on.

In Safari 3 and Firefox 2, offsetWidth and offsetHeight just give us the width and height of the part of the span before the block. (When there is nothing before the block (cases 2 and 3), Safari and IE7 create no box while Firefox 2 and 3 create an empty span box, but that is a separate bug/issue.)

In IE7 and Firefox 3, offsetWidth and offsetHeight give the same dimensions as getBoundingClientRect (except in FF3 offsetWidth and offsetHeight are rounded while getBoundingClientRect is not). Here are the results of getClientRects for IE:

offsetWidth=500
offsetHeight=138
getBoundingClientRect.width=500
getBoundingClientRect.height=138
getClientRects()[0].width=106
getClientRects()[0].height=23
getClientRects()[1].width=500
getClientRects()[1].height=104
getClientRects()[2].width=202
getClientRects()[2].height=19
 
offsetWidth=500
offsetHeight=119
getBoundingClientRect.width=500
getBoundingClientRect.height=119
getClientRects()[0].width=0
getClientRects()[0].height=0
getClientRects()[1].width=500
getClientRects()[1].height=104
getClientRects()[2].width=202
getClientRects()[2].height=19
 
offsetWidth=202
offsetHeight=119
getBoundingClientRect.width=202
getBoundingClientRect.height=119
getClientRects()[0].width=0
getClientRects()[0].height=0
getClientRects()[1].width=150
getClientRects()[1].height=104
getClientRects()[2].width=202
getClientRects()[2].height=19

And for Firefox 3:

offsetWidth=800
offsetHeight=141
getBoundingClientRect.width=800
getBoundingClientRect.height=140.59999084472656
getClientRects()[0].width=104
getClientRects()[0].height=20
getClientRects()[1].width=800
getClientRects()[1].height=100
getClientRects()[2].width=204
getClientRects()[2].height=20

offsetWidth=800
offsetHeight=141
getBoundingClientRect.width=800
getBoundingClientRect.height=140.60000610351562
getClientRects()[0].width=4
getClientRects()[0].height=20
getClientRects()[1].width=800
getClientRects()[1].height=100
getClientRects()[2].width=204
getClientRects()[2].height=20

offsetWidth=800
offsetHeight=141
getBoundingClientRect.width=800
getBoundingClientRect.height=140.60003662109375
getClientRects()[0].width=4
getClientRects()[0].height=20
getClientRects()[1].width=800
getClientRects()[1].height=100
getClientRects()[2].width=204
getClientRects()[2].height=20

There's a rounding bug in getBoundingClientRect that I should look at ... but in the meantime, the key difference here is that in FF3 the second rect is the border-box of our anonymous wrapper block, whereas in IE7 the second rect is the border-box of the actual block.

I think IE's behaviour is reasonable so we should just emulate it.
Created attachment 303951 [details] [diff] [review]
fix

This should do it. I've refactored the logic so we can share more code between getBoxObject().offset*, element.offset*, element.getBoundingClientRect() and element.getClientRects().
Attachment #303951 - Flags: superreview?(mats.palmgren)
Attachment #303951 - Flags: review?(mats.palmgren)
Whiteboard: [needs review]
Oh, the "rounding bug" there is not actually a bug. You can't represent e.g. 140.6 exactly in machine floating point, so some rounding error is inevitable. The important thing is that (thanks to the somewhat-freaky scaling code in SetTextRectangle) the widths and heights are all integers even though they have to be computed by doing bottom - top and right - left.

Comment 10

11 years ago
Mats - can we get a review here?  We need this for b4 which is coming up next tues.

Comment 11

11 years ago
Roc should we choose an alternate reviewer?
Comment on attachment 303951 [details] [diff] [review]
fix

sure
Attachment #303951 - Flags: superreview?(mats.palmgren)
Attachment #303951 - Flags: superreview?(bzbarsky)
Attachment #303951 - Flags: review?(mats.palmgren)
Attachment #303951 - Flags: review?(bzbarsky)
I'm out of town with very limited internet access and even less free time.  I very much doubt I can get to this before freeze.
Comment on attachment 303951 [details] [diff] [review]
fix

Oh well, only one option then...
Attachment #303951 - Flags: superreview?(dbaron)
Attachment #303951 - Flags: superreview?(bzbarsky)
Attachment #303951 - Flags: review?(dbaron)
Attachment #303951 - Flags: review?(bzbarsky)
Comment on attachment 303951 [details] [diff] [review]
fix

Maybe call GetClientRectContainingBlock GetContainingBlockForClientRect
instead?  The function name confused me the first time.  (My initial parse had rect as the subject of containing rather than as (part of) an adjective modifying block.)

In nsGenericHTMLElement::GetOffsetRect, it seems like there might be
some value (e.g., for table+caption, or for block in inline splits,
particularly in RTL cases) to using the x and y results of
GetAllInFlowRectsUnion, since it seems like there are probably some
cases (particularly for RTL table/caption cases or RTL block-in-inline
splits) where GetAllInFlowRectsUnion(frame,frame) might not have an
(x,y) of (0,0).  In that case, the optimal thing to do seems like
getting rid of origin and using the x and y resulting from your current
call, although I'm not sure if that's really identical.  Maybe file a
followup bug?

(Same applies to nsBoxObject::GetOffsetRect.)

Seems like AddRectsForFrame shouldn't be null-safe, but instead you
should just null-check the result of
GetFirstChild(nsGkAtoms::captionList).  Or were there other cases where
you needed the null-safety?

I'm not sure whether it's worth keeping TryGetSVGBoundingRect as a
separate function; it might be better inline at its one remaining
caller.

In RectAccumulator:
+    mResultRect.UnionRect(mResultRect, aRect);

Do you really want to ignore 0-width or 0-height rects?  Or should you
use mSeenFirstRect instead, and use your new union function?  (Was that
old code really the right fix?)

r+sr=dbaron
Attachment #303951 - Flags: superreview?(dbaron)
Attachment #303951 - Flags: superreview+
Attachment #303951 - Flags: review?(dbaron)
Attachment #303951 - Flags: review+
(In reply to comment #15)
> (From update of attachment 303951 [details] [diff] [review])
> Maybe call GetClientRectContainingBlock GetContainingBlockForClientRect
> instead?  The function name confused me the first time.  (My initial parse had
> rect as the subject of containing rather than as (part of) an adjective
> modifying block.)

OK

> In nsGenericHTMLElement::GetOffsetRect, it seems like there might be
> some value (e.g., for table+caption, or for block in inline splits,
> particularly in RTL cases) to using the x and y results of
> GetAllInFlowRectsUnion, since it seems like there are probably some
> cases (particularly for RTL table/caption cases or RTL block-in-inline
> splits) where GetAllInFlowRectsUnion(frame,frame) might not have an
> (x,y) of (0,0).  In that case, the optimal thing to do seems like
> getting rid of origin and using the x and y resulting from your current
> call, although I'm not sure if that's really identical.  Maybe file a
> followup bug?

In IE at least, offsetLeft and offsetTop are the top left of the first box, not the bounding box, so that would be a compatibility problem. In general offset* is super-fragile and I don't intend to change it without a really good reason :-). (Although I agree the code could be refactored so we don't maintain origin specially.)

> Seems like AddRectsForFrame shouldn't be null-safe, but instead you
> should just null-check the result of
> GetFirstChild(nsGkAtoms::captionList).  Or were there other cases where
> you needed the null-safety?

No, you're right.

> I'm not sure whether it's worth keeping TryGetSVGBoundingRect as a
> separate function; it might be better inline at its one remaining
> caller.

OK.

> In RectAccumulator:
> +    mResultRect.UnionRect(mResultRect, aRect);
> 
> Do you really want to ignore 0-width or 0-height rects?  Or should you
> use mSeenFirstRect instead, and use your new union function?  (Was that
> old code really the right fix?)

Looks like IE ignores rects that are 0x0 but includes rects that are 0 in just one dimension. That feels a bit quirky, so I think we should do whatever is most useful to authors. My guess is that our current behaviour of ignoring empty rects is best, e.g. if the author wanted to highlight or scroll to an element on the page. (And since we return the original rect when there is only one rect, this only affects multiple-box elements.) Authors (or JS libraries) that want the other behaviour can easily get it by walking getClientRects() themselves.
Created attachment 305883 [details]
testcase

The testcase I used to get the results in the above comment
Checked in.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs review]
I had to adjust this test to take into account the fix for bug 480452.
Is it an important aspect of Web compat that the <span><div> is in <p> in the test case?

HTML5 makes <body><p><span><div> cause the div to close the p, but with <body><span><div> the div stays in the span.
No, I should have used <div> instead of <p> in that testcase.
You need to log in before you can comment on or make changes to this bug.