Provide cross-browser Rects for box model dimensions for Elements.

Provide cross-browser Rects for box model dimensions for Elements. 

Also added Dimensions class to reduce dealing with Strings for CSS.

BUG=
R=jmesserly@google.com

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

[blois, 2013-05-22 01:18:07]

+jacob, I'm still a bit worried about adding these measurement & adjustment APIs
to our core libraries.

Thoughts?

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

https://codereview.chromium.org/15507007/diff/5001/sdk/lib/html/dart2js/html_...
sdk/lib/html/dart2js/html_dart2js.dart:7050: * Access dimensions and position of
the first Element's content in this list.
If it's just the first item's, then why have it here at all?

https://codereview.chromium.org/15507007/diff/5001/sdk/lib/html/dart2js/html_...
sdk/lib/html/dart2js/html_dart2js.dart:7142: * Access this element's content
position.
This just implements the API, right? If so, duplicating the documentation isn't
needed.

https://codereview.chromium.org/15507007/diff/5001/sdk/lib/html/dart2js/html_...
sdk/lib/html/dart2js/html_dart2js.dart:22878: * In contrast to the more general
purpose [Rect] class, this class's values are
Why not a mutable rect interface then?

https://codereview.chromium.org/15507007/diff/5001/tools/dom/src/Dimension.dart
File tools/dom/src/Dimension.dart (right):

https://codereview.chromium.org/15507007/diff/5001/tools/dom/src/Dimension.da...
tools/dom/src/Dimension.dart:46: Dimension.css(String cssValue) {
Some edge cases for cssValue:
0
50.5%
auto
inherit

I don't like it, but would a regex be a better fit? 

In general I'm worried about getting into CSS parsing.

[Jacob, 2013-05-22 02:09:51]

https://codereview.chromium.org/15507007/diff/5001/tools/dom/src/CssRectangle...
File tools/dom/src/CssRectangle.dart (right):

https://codereview.chromium.org/15507007/diff/5001/tools/dom/src/CssRectangle...
tools/dom/src/CssRectangle.dart:7: class _ContentCssRect extends CssRect {
These should just be in their own pub package unless you plan on exposing them
directly on Element.
These sorts of helper methods are really convenient but really hard to get right
in particular in the presence of newer CSS features like 3d transforms. Also,
these should probably all be num rather than int although some of our element
methods are int where that seems dangerous at this point.

https://codereview.chromium.org/15507007/diff/5001/tools/dom/src/CssRectangle...
tools/dom/src/CssRectangle.dart:11: _addOrSubtractToBoxModel(CssRect._HEIGHT,
'content');
make 'content'  a string constant at least

https://codereview.chromium.org/15507007/diff/5001/tools/dom/src/CssRectangle...
tools/dom/src/CssRectangle.dart:91: int get height() => _element.offsetHeight;
Should we put this in the platform? For example, these don't handle fractional
sizes, transforms, etc.

https://codereview.chromium.org/15507007/diff/5001/tools/dom/src/CssRectangle...
tools/dom/src/CssRectangle.dart:138: int get height();
these should be num not int.
it is possible to have fractional pixels.

https://codereview.chromium.org/15507007/diff/5001/tools/dom/src/Rectangle.dart
File tools/dom/src/Rectangle.dart (right):

https://codereview.chromium.org/15507007/diff/5001/tools/dom/src/Rectangle.da...
tools/dom/src/Rectangle.dart:11: // TODO(efortuna): Merge with Math rect after
finalizing with Florian.
Why can't we just get the full class in Math.rect?
Having a class for Point isn't HTML specific.

[Jennifer Messerly, 2013-07-10 21:05:55]

https://codereview.chromium.org/15507007/diff/5001/tools/dom/src/CssRectangle...
File tools/dom/src/CssRectangle.dart (right):

https://codereview.chromium.org/15507007/diff/5001/tools/dom/src/CssRectangle...
tools/dom/src/CssRectangle.dart:7: class _ContentCssRect extends CssRect {
On 2013/05/22 02:09:51, Jacob wrote:
> These should just be in their own pub package unless you plan on exposing them
> directly on Element.
> These sorts of helper methods are really convenient but really hard to get
right
> in particular in the presence of newer CSS features like 3d transforms. Also,
> these should probably all be num rather than int although some of our element
> methods are int where that seems dangerous at this point. 

Isn't "hard to get right" an argument for trying to implement it once, in the
platform or a standard package everyone can use? :)

Perhaps this suggests we should we admit defeat on integrating jQuery into
dart:html, and instead introduce "$" top level function in a library, and change
our examples/conventions to use that instead?

[Jennifer Messerly, 2013-07-10 21:36:20]

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

https://codereview.chromium.org/15507007/diff/11001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_Element.darttemplate:279: CssRect get
contentEdge => new _ContentCssListRect(_elementList);
If this isn't directly on Element/ElementList, what are some of the alternative
proposed APIs?

[Emily Fortuna, 2013-07-10 21:40:16]

https://codereview.chromium.org/15507007/diff/5001/tools/dom/src/CssRectangle...
File tools/dom/src/CssRectangle.dart (right):

https://codereview.chromium.org/15507007/diff/5001/tools/dom/src/CssRectangle...
tools/dom/src/CssRectangle.dart:7: class _ContentCssRect extends CssRect {
On 2013/05/22 02:09:51, Jacob wrote:
> These should just be in their own pub package unless you plan on exposing them
> directly on Element.
> These sorts of helper methods are really convenient but really hard to get
right
> in particular in the presence of newer CSS features like 3d transforms. Also,
> these should probably all be num rather than int although some of our element
> methods are int where that seems dangerous at this point. 

They are being exposed on Element.

https://codereview.chromium.org/15507007/diff/5001/tools/dom/src/CssRectangle...
tools/dom/src/CssRectangle.dart:138: int get height();
On 2013/05/22 02:09:51, Jacob wrote:
> these should be num not int.
> it is possible to have fractional pixels.

Done.

https://codereview.chromium.org/15507007/diff/5001/tools/dom/src/Dimension.dart
File tools/dom/src/Dimension.dart (right):

https://codereview.chromium.org/15507007/diff/5001/tools/dom/src/Dimension.da...
tools/dom/src/Dimension.dart:46: Dimension.css(String cssValue) {
On 2013/05/22 01:18:07, blois wrote:
> Some edge cases for cssValue:
> 0
> 50.5%
> auto
> inherit
> 
> I don't like it, but would a regex be a better fit? 
> 
> In general I'm worried about getting into CSS parsing.

I can change this so this constructor is now private (since it never gets used
outside of the html library), and is only called after calling getComputedValue
in very specific cases, so we know for certain that it is a pixel value.

https://codereview.chromium.org/15507007/diff/5001/tools/dom/src/Rectangle.dart
File tools/dom/src/Rectangle.dart (right):

https://codereview.chromium.org/15507007/diff/5001/tools/dom/src/Rectangle.da...
tools/dom/src/Rectangle.dart:11: // TODO(efortuna): Merge with Math rect after
finalizing with Florian.
On 2013/05/22 02:09:51, Jacob wrote:
> Why can't we just get the full class in Math.rect?
> Having a class for Point isn't HTML specific.

You're right. And that's why the TODO is here. I'm working with Florian and
Gilad on making it happen, but in the meantime, I have the TODO.

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

https://codereview.chromium.org/15507007/diff/11001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_Element.darttemplate:279: CssRect get
contentEdge => new _ContentCssListRect(_elementList);
On 2013/07/10 21:36:20, John Messerly wrote:
> If this isn't directly on Element/ElementList, what are some of the
alternative
> proposed APIs?

This CL was created before that was there. This CL originally intended to add
the functionality of that and the rect/box model utility methods.

[Emily Fortuna, 2013-07-16 20:08:10]

jmesserly, PTAL.

[Jennifer Messerly, 2013-07-16 22:03:16]

Overall this is looking great! minor comments.

regardless of where this functionality ends up, I hope we can release it. Would
love to use this in the grid layout polyfill!

https://codereview.chromium.org/15507007/diff/22001/sdk/lib/html/dart2js/html...
File sdk/lib/html/dart2js/html_dart2js.dart (right):

https://codereview.chromium.org/15507007/diff/22001/sdk/lib/html/dart2js/html...
sdk/lib/html/dart2js/html_dart2js.dart:8755: * Important note: use of this
method _will_ perform CSS calculations that can
I like this note, but we might want to soften the wording a bit? IMHO, using
these properties for animation is fine, even one of the use cases. The trick is
you don't want to use them _during_ an animation frame. As long as the query is
before the animation starts (or after it finishes), it should be okay.

Perhaps link to this:
https://developers.google.com/speed/articles/reflow

https://codereview.chromium.org/15507007/diff/22001/sdk/lib/html/dart2js/html...
sdk/lib/html/dart2js/html_dart2js.dart:8769: * can be used to retrieve jQuery's
`innerHeight` value for an element. This
I wonder if we should link the jQuery docs?

 ... retrieve jQuery's [innerHeight](http://api.jquery.com/innerHeight/) value
for an element. ...

https://codereview.chromium.org/15507007/diff/22001/sdk/lib/html/dart2js/html...
sdk/lib/html/dart2js/html_dart2js.dart:26847: _ContentCssRect(element) :
super(element);
newline after this?

https://codereview.chromium.org/15507007/diff/22001/tests/html/element_dimens...
File tests/html/element_dimensions_test.dart (right):

https://codereview.chromium.org/15507007/diff/22001/tests/html/element_dimens...
tests/html/element_dimensions_test.dart:1: // Copyright (c) 2012, the Dart
project authors.  Please see the AUTHORS file
2013

https://codereview.chromium.org/15507007/diff/22001/tests/html/element_dimens...
tests/html/element_dimensions_test.dart:5: library elementDimensionsTest;
hmm, i'm not sure if we have a style guide here, but personally I try to have it
match the library name, e.g. "library element_dimensions_test" ... fwiw, just
ran this by Bob and apparently that style is in use elsewhere too. Tho the
official style guide doesn't say anything.

https://codereview.chromium.org/15507007/diff/22001/tests/html/element_dimens...
tests/html/element_dimensions_test.dart:22: void initDiv() {
maybe this should be a "setUp(() { ... })"
http://api.dartlang.org/docs/releases/latest/unittest.html#setUp?

https://codereview.chromium.org/15507007/diff/22001/tests/html/element_dimens...
tests/html/element_dimensions_test.dart:24: div.style.border = '0px solid #fff';
use cascades? :)

https://codereview.chromium.org/15507007/diff/22001/tests/html/element_dimens...
tests/html/element_dimensions_test.dart:65: initDiv();
the re-init here makes me wonder if it should be a separate test case

https://codereview.chromium.org/15507007/diff/22001/tests/html/element_dimens...
tests/html/element_dimensions_test.dart:105: });
newline after

https://codereview.chromium.org/15507007/diff/22001/tools/dom/src/CssRectangl...
File tools/dom/src/CssRectangle.dart (right):

https://codereview.chromium.org/15507007/diff/22001/tools/dom/src/CssRectangl...
tools/dom/src/CssRectangle.dart:7: class _ContentCssRect extends CssRect {
might be worth adding a short class comment here and on the others. Even though
it's internal, would still be useful to help orient a new reader

https://codereview.chromium.org/15507007/diff/22001/tools/dom/src/CssRectangl...
tools/dom/src/CssRectangle.dart:21: * function in jQuery and the calculated
`height` css value, converted to a
uppercase CSS?

https://codereview.chromium.org/15507007/diff/22001/tools/dom/src/CssRectangl...
tools/dom/src/CssRectangle.dart:24: void set height(Dimension newHeight) {
hmmm. I'm not sure about having the setter take a different type than the
getter. Maybe if this is untyped and you could take the union of Dimension and
num?

https://codereview.chromium.org/15507007/diff/22001/tools/dom/src/CssRectangl...
tools/dom/src/CssRectangle.dart:40: num get left() =>
_element.getBoundingClientRect().left -
not sure about the parens here. that looks like old-style Dart getters...
surprised it still works :)

https://codereview.chromium.org/15507007/diff/22001/tools/dom/src/CssRectangl...
tools/dom/src/CssRectangle.dart:47: List<Element> _elementList;
this could be final?

https://codereview.chromium.org/15507007/diff/22001/tools/dom/src/CssRectangl...
tools/dom/src/CssRectangle.dart:117: * SPECIAL NOTE: Do _not_ use these methods
of setting or getting dimensions
trivial nit: I think this one is "SPECIAL NOTE" and the other was "Important
note"... maybe just reuse the verbiage. Also might be worth having "important
note" it be italics.

https://codereview.chromium.org/15507007/diff/22001/tools/dom/src/CssRectangl...
tools/dom/src/CssRectangle.dart:125: num get left();
same issue here with parens

https://codereview.chromium.org/15507007/diff/22001/tools/dom/src/Dimension.dart
File tools/dom/src/Dimension.dart (right):

https://codereview.chromium.org/15507007/diff/22001/tools/dom/src/Dimension.d...
tools/dom/src/Dimension.dart:1: /** Class representing a distance measurement in
CSS. */
fyi, I think the spec terminology is "length".

either way, might be good to link something like:
https://developer.mozilla.org/en-US/docs/Web/CSS/length
?

https://codereview.chromium.org/15507007/diff/22001/tools/dom/src/Dimension.d...
tools/dom/src/Dimension.dart:51: Dimension._(String pixels = '0px') {
make this public? It seems really useful, even if it can't handle all cases
(throwing an error for invalid data would be okay IMO). This would make the very
string-based CssStyleDelcaration a lot easier to work with.

https://codereview.chromium.org/15507007/diff/22001/tools/dom/src/Dimension.d...
tools/dom/src/Dimension.dart:66: num get value => this._value;
remove this.?

https://codereview.chromium.org/15507007/diff/22001/tools/dom/src/Rectangle.dart
File tools/dom/src/Rectangle.dart (right):

https://codereview.chromium.org/15507007/diff/22001/tools/dom/src/Rectangle.d...
tools/dom/src/Rectangle.dart:121: class Rect extends RectBase {
Would be good to add a comment on why we have Rect and RectBase.

[Emily Fortuna, 2013-07-18 22:45:02]

ptal again

https://codereview.chromium.org/15507007/diff/22001/sdk/lib/html/dart2js/html...
File sdk/lib/html/dart2js/html_dart2js.dart (right):

https://codereview.chromium.org/15507007/diff/22001/sdk/lib/html/dart2js/html...
sdk/lib/html/dart2js/html_dart2js.dart:8755: * Important note: use of this
method _will_ perform CSS calculations that can
On 2013/07/16 22:03:16, John Messerly wrote:
> I like this note, but we might want to soften the wording a bit? IMHO, using
> these properties for animation is fine, even one of the use cases. The trick
is
> you don't want to use them _during_ an animation frame. As long as the query
is
> before the animation starts (or after it finishes), it should be okay.
> 
> Perhaps link to this:
> https://developers.google.com/speed/articles/reflow

Done.

https://codereview.chromium.org/15507007/diff/22001/sdk/lib/html/dart2js/html...
sdk/lib/html/dart2js/html_dart2js.dart:8769: * can be used to retrieve jQuery's
`innerHeight` value for an element. This
On 2013/07/16 22:03:16, John Messerly wrote:
> I wonder if we should link the jQuery docs?
> 
>  ... retrieve jQuery's [innerHeight](http://api.jquery.com/innerHeight/) value
> for an element. ...

Done.

https://codereview.chromium.org/15507007/diff/22001/sdk/lib/html/dart2js/html...
sdk/lib/html/dart2js/html_dart2js.dart:26847: _ContentCssRect(element) :
super(element);
On 2013/07/16 22:03:16, John Messerly wrote:
> newline after this?

Done.

https://codereview.chromium.org/15507007/diff/22001/tests/html/element_dimens...
File tests/html/element_dimensions_test.dart (right):

https://codereview.chromium.org/15507007/diff/22001/tests/html/element_dimens...
tests/html/element_dimensions_test.dart:1: // Copyright (c) 2012, the Dart
project authors.  Please see the AUTHORS file
On 2013/07/16 22:03:16, John Messerly wrote:
> 2013

Done.

https://codereview.chromium.org/15507007/diff/22001/tests/html/element_dimens...
tests/html/element_dimensions_test.dart:5: library elementDimensionsTest;
On 2013/07/16 22:03:16, John Messerly wrote:
> hmm, i'm not sure if we have a style guide here, but personally I try to have
it
> match the library name, e.g. "library element_dimensions_test" ... fwiw, just
> ran this by Bob and apparently that style is in use elsewhere too. Tho the
> official style guide doesn't say anything.

Done.

https://codereview.chromium.org/15507007/diff/22001/tests/html/element_dimens...
tests/html/element_dimensions_test.dart:22: void initDiv() {
On 2013/07/16 22:03:16, John Messerly wrote:
> maybe this should be a "setUp(() { ... })"
> http://api.dartlang.org/docs/releases/latest/unittest.html#setUp?

Done.

https://codereview.chromium.org/15507007/diff/22001/tests/html/element_dimens...
tests/html/element_dimensions_test.dart:24: div.style.border = '0px solid #fff';
On 2013/07/16 22:03:16, John Messerly wrote:
> use cascades? :)

Done. I hadn't originally because what I really wanted to do was write:
(div.style)..padding = blah
             .. border = foo
etc, but our method cascades aren't that smart. :-( Anyway, I updated it.

https://codereview.chromium.org/15507007/diff/22001/tests/html/element_dimens...
tests/html/element_dimensions_test.dart:65: initDiv();
On 2013/07/16 22:03:16, John Messerly wrote:
> the re-init here makes me wonder if it should be a separate test case

split into individual tests.

https://codereview.chromium.org/15507007/diff/22001/tests/html/element_dimens...
tests/html/element_dimensions_test.dart:105: });
On 2013/07/16 22:03:16, John Messerly wrote:
> newline after

Done.

https://codereview.chromium.org/15507007/diff/22001/tools/dom/src/CssRectangl...
File tools/dom/src/CssRectangle.dart (right):

https://codereview.chromium.org/15507007/diff/22001/tools/dom/src/CssRectangl...
tools/dom/src/CssRectangle.dart:7: class _ContentCssRect extends CssRect {
On 2013/07/16 22:03:16, John Messerly wrote:
> might be worth adding a short class comment here and on the others. Even
though
> it's internal, would still be useful to help orient a new reader

Done.

https://codereview.chromium.org/15507007/diff/22001/tools/dom/src/CssRectangl...
tools/dom/src/CssRectangle.dart:21: * function in jQuery and the calculated
`height` css value, converted to a
On 2013/07/16 22:03:16, John Messerly wrote:
> uppercase CSS?

Done.

https://codereview.chromium.org/15507007/diff/22001/tools/dom/src/CssRectangl...
tools/dom/src/CssRectangle.dart:24: void set height(Dimension newHeight) {
On 2013/07/16 22:03:16, John Messerly wrote:
> hmmm. I'm not sure about having the setter take a different type than the
> getter. Maybe if this is untyped and you could take the union of Dimension and
> num?

Done.

https://codereview.chromium.org/15507007/diff/22001/tools/dom/src/CssRectangl...
tools/dom/src/CssRectangle.dart:40: num get left() =>
_element.getBoundingClientRect().left -
On 2013/07/16 22:03:16, John Messerly wrote:
> not sure about the parens here. that looks like old-style Dart getters...
> surprised it still works :)

Oh man. Old habits die hard. Fixed.

https://codereview.chromium.org/15507007/diff/22001/tools/dom/src/CssRectangl...
tools/dom/src/CssRectangle.dart:117: * SPECIAL NOTE: Do _not_ use these methods
of setting or getting dimensions
On 2013/07/16 22:03:16, John Messerly wrote:
> trivial nit: I think this one is "SPECIAL NOTE" and the other was "Important
> note"... maybe just reuse the verbiage. Also might be worth having "important
> note" it be italics.

Done.

https://codereview.chromium.org/15507007/diff/22001/tools/dom/src/CssRectangl...
tools/dom/src/CssRectangle.dart:125: num get left();
On 2013/07/16 22:03:16, John Messerly wrote:
> same issue here with parens

Done.

https://codereview.chromium.org/15507007/diff/22001/tools/dom/src/Dimension.dart
File tools/dom/src/Dimension.dart (right):

https://codereview.chromium.org/15507007/diff/22001/tools/dom/src/Dimension.d...
tools/dom/src/Dimension.dart:1: /** Class representing a distance measurement in
CSS. */
On 2013/07/16 22:03:16, John Messerly wrote:
> fyi, I think the spec terminology is "length".
> 
> either way, might be good to link something like:
> https://developer.mozilla.org/en-US/docs/Web/CSS/length
> ?

Done.

https://codereview.chromium.org/15507007/diff/22001/tools/dom/src/Dimension.d...
tools/dom/src/Dimension.dart:51: Dimension._(String pixels = '0px') {
On 2013/07/16 22:03:16, John Messerly wrote:
> make this public? It seems really useful, even if it can't handle all cases
> (throwing an error for invalid data would be okay IMO). This would make the
very
> string-based CssStyleDelcaration a lot easier to work with.

Updated! It originally was, but I had made it private after Pete's comments. But
I do think that's what makes this class useful. Fixed!

https://codereview.chromium.org/15507007/diff/22001/tools/dom/src/Rectangle.dart
File tools/dom/src/Rectangle.dart (right):

https://codereview.chromium.org/15507007/diff/22001/tools/dom/src/Rectangle.d...
tools/dom/src/Rectangle.dart:121: class Rect extends RectBase {
On 2013/07/16 22:03:16, John Messerly wrote:
> Would be good to add a comment on why we have Rect and RectBase.

Done.

[Jennifer Messerly, 2013-07-19 00:10:37]

lgtm!

https://codereview.chromium.org/15507007/diff/41001/tests/html/element_dimens...
File tests/html/element_dimensions_test.dart (right):

https://codereview.chromium.org/15507007/diff/41001/tests/html/element_dimens...
tests/html/element_dimensions_test.dart:104: test('contentEdge.width2', () {
would be good to add a note about what this is testing, maybe:

    test('contentEdge.width box-sizing: border-box'

similar issue with a few others

[Emily Fortuna, 2013-07-19 00:41:03]

Committed patchset #3 manually as r25192 (presubmit successful).