Added offset document measurement to Element.

Added offset document measurement to Element.

BUG=
R=jmesserly@google.com

Committed: https://code.google.com/p/dart/source/detail?r=25483

[Emily Fortuna, 2013-07-12 22:22:22]

Baby steps

[blois, 2013-07-12 23:10:08]

https://codereview.chromium.org/18060010/diff/3001/sdk/lib/html/dart2js/html_...
File sdk/lib/html/dart2js/html_dart2js.dart (right):

https://codereview.chromium.org/18060010/diff/3001/sdk/lib/html/dart2js/html_...
sdk/lib/html/dart2js/html_dart2js.dart:8679: Point get documentOffset() {
Seems like it would be more useful to expose something like:
Point offsetTo(Element)

Thoughts?

[Emily Fortuna, 2013-07-24 23:11:32]

now with ranges for browser variations!

[Jennifer Messerly, 2013-07-24 23:26:23]

https://codereview.chromium.org/18060010/diff/13001/tests/html/element_test.dart
File tests/html/element_test.dart (right):

https://codereview.chromium.org/18060010/diff/13001/tests/html/element_test.d...
tests/html/element_test.dart:78: expect(element.documentOffset.x, 9);
slightly confused, should this be "documentOffset" or "docOffset"?

https://codereview.chromium.org/18060010/diff/13001/tools/dom/templates/html/...
File tools/dom/templates/html/impl/impl_Element.darttemplate (right):

https://codereview.chromium.org/18060010/diff/13001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_Element.darttemplate:1101: Point get
docOffset => offsetTo(document.documentElement);
personally, I would call this "documentOffset". That name seems more consistent
with the existing DOM APIs (e.g. documentElement)

https://codereview.chromium.org/18060010/diff/13001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_Element.darttemplate:1112: * an ArgumentError
is thrown.
could use code links for [ArgumentError] and [offsetParent]

https://codereview.chromium.org/18060010/diff/13001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_Element.darttemplate:1116: parent, parent ==
this || parent.tagName == 'HTML');
can we remove this extra arg? suggestion below.

https://codereview.chromium.org/18060010/diff/13001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_Element.darttemplate:1116: parent, parent ==
this || parent.tagName == 'HTML');
use "identical(parent, this)"? one thing to watch out for with CustomElements...
once we can "extend Element", we might be able to override equals :)

https://codereview.chromium.org/18060010/diff/13001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_Element.darttemplate:1119: Point
_offsetToHelper(Element current, Element parent, bool foundAsParent) {
can this be static?

https://codereview.chromium.org/18060010/diff/13001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_Element.darttemplate:1120: if (current ==
null || current == parent) {
can we remove "foundAsParent"? it seems to duplicate what this is already
checking.

it seems like this is equivalent:

Point _offsetToHelper(Element current, Element parent) {
  bool foundAsParent = identical(current, parent) || parent.tagName == 'HTML';
  if (current == null || identical(current, parent)) {
    if (foundAsParent) return new Point(0, 0);
    throw new ArgumentError(...);
  }
  ...
}

there's probably a little more refactoring there; it seems like the
ArgumentError can only be thrown if (current == null && parent.tagName !=
'HTML')

I'm not quite clear what the special case is for tagname HTML--is it basically
saying that even if the node isn't attached to the document, we'll pretend it is
for the purpose of "offsetTo"? Might be good to add that to the comment.

https://codereview.chromium.org/18060010/diff/13001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_Element.darttemplate:1121: return
foundAsParent? new Point(0, 0) : throw new ArgumentError(
this might be cleaner as:

if (foundAsParent) return new Point(0, 0);
throw new ArgumentError( ... );

[Emily Fortuna, 2013-07-25 00:20:07]

PTAL

https://codereview.chromium.org/18060010/diff/13001/tests/html/element_test.dart
File tests/html/element_test.dart (right):

https://codereview.chromium.org/18060010/diff/13001/tests/html/element_test.d...
tests/html/element_test.dart:78: expect(element.documentOffset.x, 9);
On 2013/07/24 23:26:23, John Messerly wrote:
> slightly confused, should this be "documentOffset" or "docOffset"?

oh, whoops. It's docOffset. I had moved a variant of the test out into a
temporary test for my local testing because there are so many element_tests, but
I forgot to fix this when I moved the test back here.

https://codereview.chromium.org/18060010/diff/13001/tools/dom/templates/html/...
File tools/dom/templates/html/impl/impl_Element.darttemplate (right):

https://codereview.chromium.org/18060010/diff/13001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_Element.darttemplate:1101: Point get
docOffset => offsetTo(document.documentElement);
On 2013/07/24 23:26:23, John Messerly wrote:
> personally, I would call this "documentOffset". That name seems more
consistent
> with the existing DOM APIs (e.g. documentElement)

Done.

https://codereview.chromium.org/18060010/diff/13001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_Element.darttemplate:1112: * an ArgumentError
is thrown.
On 2013/07/24 23:26:23, John Messerly wrote:
> could use code links for [ArgumentError] and [offsetParent]

Done.

https://codereview.chromium.org/18060010/diff/13001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_Element.darttemplate:1119: Point
_offsetToHelper(Element current, Element parent, bool foundAsParent) {
On 2013/07/24 23:26:23, John Messerly wrote:
> can this be static?

Done.

https://codereview.chromium.org/18060010/diff/13001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_Element.darttemplate:1120: if (current ==
null || current == parent) {
On 2013/07/24 23:26:23, John Messerly wrote:
> can we remove "foundAsParent"? it seems to duplicate what this is already
> checking.
> 
> it seems like this is equivalent:
> 
> Point _offsetToHelper(Element current, Element parent) {
>   bool foundAsParent = identical(current, parent) || parent.tagName == 'HTML';
>   if (current == null || identical(current, parent)) {
>     if (foundAsParent) return new Point(0, 0);
>     throw new ArgumentError(...);
>   }
>   ...
> }
> 
> there's probably a little more refactoring there; it seems like the
> ArgumentError can only be thrown if (current == null && parent.tagName !=
> 'HTML')
> 
> I'm not quite clear what the special case is for tagname HTML--is it basically
> saying that even if the node isn't attached to the document, we'll pretend it
is
> for the purpose of "offsetTo"? Might be good to add that to the comment.

HTML discussed offline and added comment. and refactored!

https://codereview.chromium.org/18060010/diff/13001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_Element.darttemplate:1121: return
foundAsParent? new Point(0, 0) : throw new ArgumentError(
On 2013/07/24 23:26:23, John Messerly wrote:
> this might be cleaner as:
> 
> if (foundAsParent) return new Point(0, 0);
> throw new ArgumentError( ... );

Done.

[Jennifer Messerly, 2013-07-25 02:22:09]

couple comments, other than that LGTM!

https://codereview.chromium.org/18060010/diff/20001/tests/html/element_test.dart
File tests/html/element_test.dart (right):

https://codereview.chromium.org/18060010/diff/20001/tests/html/element_test.d...
tests/html/element_test.dart:733: <ul class="level-1">
Parse this from a mutliline string literal with `new Element.html`? That way the
code stays in sync with the comment :)

https://codereview.chromium.org/18060010/diff/20001/tools/dom/templates/html/...
File tools/dom/templates/html/impl/impl_Element.darttemplate (right):

https://codereview.chromium.org/18060010/diff/20001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_Element.darttemplate:1121: // offsetParent,
"tops out" at BODY. But peole could conceivably pass in the
people

[Emily Fortuna, 2013-07-25 17:45:47]

Committed patchset #5 manually as r25483 (presubmit successful).

[blois, 2013-07-27 00:00:56]

https://codereview.chromium.org/18060010/diff/26001/tools/dom/templates/html/...
File tools/dom/templates/html/impl/impl_Element.darttemplate (right):

https://codereview.chromium.org/18060010/diff/26001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_Element.darttemplate:1101: Point get
documentOffset => offsetTo(document.documentElement);
What's the difference between rolling the offsets up the offsetParent chain and
using getBoundingClientRect?

Looks like jQuery's offset() uses getBoundingClientRect.

https://codereview.chromium.org/18060010/diff/26001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_Element.darttemplate:1115: return
Element._offsetToHelper(this, parent);
Why not just:

return new Point(documentOffset.x - other.documentOffset.x, documentOffset.y -
other.documentOffset.y)?

Then it can work on any elements, and not just parents. Also avoids the oddity
of requiring offset parents.