REGRESSION: getBoundingClientRect() method of Range incorrectly returns null for collapsed Range

• Component: DOM
• Status: RESOLVED FIXED

[Tim Down]

Created attachment 102269 [details]
Test case

This contradicts the CSSOM spec (http://www.w3.org/TR/cssom-view/#extensions-to-the-range-interface) which mandates that:

- in all cases a ClientRect must be returned;
- the ClientRect returned must include the first ClientRect returned by getClientRects().

[Tim Down]

This looks like a regression: Safari 5.0.5 didn't have this problem, Safari 5.1 does.

[Alexey Proskuryakov]

Confirmed on Mac OS X as a regression. CC'ing some people who changed getBoundingClientRect in the last year.

Do you know if this affects any actual Web sites?

[Tim Down]

Not to my knowledge. I discovered it while answering a question on Stack Overflow asking how to find the coordinates of the start of the selection: http://stackoverflow.com/questions/6846230/javascript-text-selection-page-coordinates

[Evan Martin]

CC += selection master.

[Sam Weinig]

Yeah, this clearly looks wrong, this should be an easy fix for someone.

[Sam Weinig]

This probably regressed in http://trac.webkit.org/changeset/80408.

[Sam Weinig]

Created attachment 102463 [details]
Patch

[Sam Weinig]

Created attachment 102464 [details]
Patch

[Sam Weinig]

This patch definitely gets us going in the right direction, the only open question in my mind is if the collapsed range being at an x/y of 0,0 is correct.  It seems to match Firefox, but also seems a bit wonky.

[Sam Weinig]

Created attachment 102465 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 102465 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102465&action=review

The change makes sense to me.

> LayoutTests/fast/dom/Range/getBoundingClientRect.html:114
> +        /*4*/  { left: 0, top: 0, width: 0, height: 0 },
> +        /*5*/  { left: -14.574, top: 1761.947, width: 504.009, height: 535.849 },
> +        /*6*/  { left: 0, top: 0, width: 0, height: 0 },

It's not great that this test depends on particular font metrics and is failing on non-Mac ports :(  Can we instead wrap each text with a span and compare span's offset* and bounding rect?

[Sam Weinig]

(In reply to comment #11)
> (From update of attachment 102465 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102465&action=review
> 
> The change makes sense to me.
> 
> > LayoutTests/fast/dom/Range/getBoundingClientRect.html:114
> > +        /*4*/  { left: 0, top: 0, width: 0, height: 0 },
> > +        /*5*/  { left: -14.574, top: 1761.947, width: 504.009, height: 535.849 },
> > +        /*6*/  { left: 0, top: 0, width: 0, height: 0 },
> 
> It's not great that this test depends on particular font metrics and is failing on non-Mac ports :(  Can we instead wrap each text with a span and compare span's offset* and bounding rect?

I think it would probably make more sense to use Ahem if possible, since part of what we want to test with this kind of test is that it works when the range is just selecting a subset of an element. That said, the two subtests I added do not depend on metrics, only the existing ones do.

[Sam Weinig]

Committed r92089: <http://trac.webkit.org/changeset/92089>

[Ryosuke Niwa]

It seems like you've accidentally added some debugging code to the test when you landed the patch.  Removed it in http://trac.webkit.org/changeset/92091.