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.

David can you take a look - this looks to have regressed Backbase a popular web framework

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?

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

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

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.

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

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.

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

Roc should we choose an alternate reviewer?

Comment on attachment 303951 [details] [diff] [review] fix sure

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

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

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

The testcase I used to get the results in the above comment

Checked in.

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.