Bug 65324 - REGRESSION: getBoundingClientRect() method of Range incorrectly returns null for collapsed Range
Summary: REGRESSION: getBoundingClientRect() method of Range incorrectly returns null ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Sam Weinig
URL:
Keywords: EasyFix, Regression
Depends on:
Blocks:
 
Reported: 2011-07-28 10:28 PDT by Tim Down
Modified: 2011-07-31 16:19 PDT (History)
7 users (show)

See Also:


Attachments
Test case (504 bytes, text/html)
2011-07-28 10:28 PDT, Tim Down
no flags Details
Patch (8.69 KB, patch)
2011-07-31 13:17 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (8.27 KB, patch)
2011-07-31 13:18 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (8.27 KB, patch)
2011-07-31 13:23 PDT, Sam Weinig
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Down 2011-07-28 10:28:35 PDT
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().
Comment 1 Tim Down 2011-07-28 10:39:48 PDT
This looks like a regression: Safari 5.0.5 didn't have this problem, Safari 5.1 does.
Comment 2 Alexey Proskuryakov 2011-07-29 11:02:04 PDT
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?
Comment 3 Tim Down 2011-07-29 11:34:06 PDT
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
Comment 4 Evan Martin 2011-07-29 13:32:21 PDT
CC += selection master.
Comment 5 Sam Weinig 2011-07-29 15:16:13 PDT
Yeah, this clearly looks wrong, this should be an easy fix for someone.
Comment 6 Sam Weinig 2011-07-29 15:17:52 PDT
This probably regressed in http://trac.webkit.org/changeset/80408.
Comment 7 Sam Weinig 2011-07-31 13:17:30 PDT
Created attachment 102463 [details]
Patch
Comment 8 Sam Weinig 2011-07-31 13:18:35 PDT
Created attachment 102464 [details]
Patch
Comment 9 Sam Weinig 2011-07-31 13:19:52 PDT
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.
Comment 10 Sam Weinig 2011-07-31 13:23:18 PDT
Created attachment 102465 [details]
Patch
Comment 11 Ryosuke Niwa 2011-07-31 13:27:46 PDT
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?
Comment 12 Sam Weinig 2011-07-31 14:03:19 PDT
(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.
Comment 13 Sam Weinig 2011-07-31 14:05:32 PDT
Committed r92089: <http://trac.webkit.org/changeset/92089>
Comment 14 Ryosuke Niwa 2011-07-31 16:19:19 PDT
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.