Closed Bug 917755 Opened 6 years ago Closed 6 years ago

Implement Node.getBoxQuads

Categories

(Core :: Layout, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: roc, Assigned: roc)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-needed)

Attachments

(9 files, 15 obsolete files)

24.33 KB, patch
mats
: review+
Details | Diff | Splinter Review
1.45 KB, text/html
Details
10.34 KB, patch
mats
: review+
Details | Diff | Splinter Review
9.38 KB, patch
mats
: review+
Details | Diff | Splinter Review
1.60 KB, patch
mats
: review+
Details | Diff | Splinter Review
10.01 KB, patch
jst
: review+
Details | Diff | Splinter Review
13.56 KB, patch
jst
: review+
Details | Diff | Splinter Review
12.06 KB, patch
jst
: review+
Details | Diff | Splinter Review
7.02 KB, patch
jst
: review+
Details | Diff | Splinter Review
Node.getBoxQuads is a very powerful but fairly easy to use API for obtaining the CSS boxes for a Node relative to any other Node (or viewport).
The proposal is in this thread and I think we reached consensus:
http://lists.w3.org/Archives/Public/www-style/2013Aug/0607.html
I have an implementation of this which I will attach as soon as I've broken it up into a proper patch series. My implementation is quite complete except it doesn't handle SVG text properly yet ... I'll probably defer that to followup.

My implementation includes a barebones implementation of DOMPoint, hence this blocks 850805.
Attachment #807054 - Attachment description: Add CoordinateTransformations, containing the real implementation of GetBoxQuads → Part 3: Add CoordinateTransformations, containing the real implementation of GetBoxQuads
Comment on attachment 807054 [details] [diff] [review]
Part 3: Add CoordinateTransformations, containing the real implementation of GetBoxQuads

Regarding dom/webidl/Node.webidl: I would move the 'box' field last
since I suspect the webidl to C++ translator lays out the fields in
the same order causing a 32-bit gap on 64-bit platforms; and given
our jemalloc granularities that will cause this struct to use twice
as much memory as it would with 'box' last.

Other than that, I don't know webidl well enough to know if it could
be written in a better way or not, so I'll delegate review of that file
to Boris ... and tag along a question on webidl: does webidl mandate
an ABI?
Attachment #807054 - Flags: review?(bzbarsky)
Comment on attachment 807055 [details] [diff] [review]
Part 4: Add DOMPoint and DOMQuad implementations

There's some (simple) webidl files in this patch too, they look fine
to me, but could have a quick look on those too?
Attachment #807055 - Flags: review?(bzbarsky)
Comment on attachment 807056 [details] [diff] [review]
Part 5: Implement nsINode::GetBoxQuads

One more webidl file.
Attachment #807056 - Flags: review?(bzbarsky)
Attachment #807052 - Flags: review?(matspal) → review+
> since I suspect the webidl to C++ translator lays out the fields in
> the same order

They're laid out in lexicographic order by name, since that's the order all operations on them happen in, so the dictionary member list is just sorted in that order.

We could change that if we care, but who's ever going to be heap-allocating a BoxQuadOptions anyway?  The only consumer I see here is getBoxQuads, which takes it as an argument: that would be a stack-allocation.

As for ABI, WebIDL doesn't mandate one.  Since we generate C++ code that we then compile against Gecko headers, as long as the API is right it will work with whatever ABI the headers are using.
Fair enough for BoxQuadOptions, I was wondering more in general if there's a tradeoff
between optimal packing of fields and access speed.  It sounds like we have the freedom
to sort the fields in any order we like in the C++ translation.
<fieldset><legend>...  has getBoxQuads().length = 1, shouldn't the legend be included?
(similar to <table><caption>...)

Should outer bullet frames be included? (currently excluded)

and ::before/::after?
Comment on attachment 807054 [details] [diff] [review]
Part 3: Add CoordinateTransformations, containing the real implementation of GetBoxQuads

r=me on the WebIDL bits, but I believe this patch won't compile standalone because we only codegen dictionaries that are actually used by something.  You can add a method taking BoxQuadOptions to DummyBinding.webidl to avoid that problem (and remove it later in this patch queue, I guess).
Attachment #807054 - Flags: review?(bzbarsky) → review+
Comment on attachment 807055 [details] [diff] [review]
Part 4: Add DOMPoint and DOMQuad implementations

Should the DOMPoint ctor take unrestricted double?  Specifically, should it allow infinities?

r=me modulo that question, as long as it's answered.
Attachment #807055 - Flags: review?(bzbarsky) → review+
Comment on attachment 807056 [details] [diff] [review]
Part 5: Implement nsINode::GetBoxQuads

>+  mozilla::GetBoxQuads(this, aOptions, quads, aRv);

If after this line aRv.Failed(), should probably just return immediately without uselessly touching aResult.

r=me with that and a comment added to Node.webidl, both above the partial interface and at the top of the file, pointing to the relevant spec.

That said, if we wanted to we could only expose this on Document/Element/Text instead of on Node, and maybe even make it infallible.  I guess that will get sorted out in the spec.
Attachment #807056 - Flags: review?(bzbarsky) → review+
(In reply to Mats Palmgren (:mats) from comment #12)
> <fieldset><legend>...  has getBoxQuads().length = 1, shouldn't the legend be
> included?
> (similar to <table><caption>...)

Actually, I think my suggestion might be wrong, and it's the <table><caption> that is
wrong.  I need to read up on the thread(s) discussing this feature first to understand
it better.  At the moment I'm thinking that the only boxes that should be included are
those associated with the element itself, i.e. not <legend> nor <caption> in the examples
above, but include bullets, ::before/::after etc.
(In reply to Mats Palmgren (:mats) from comment #11)
> Fair enough for BoxQuadOptions, I was wondering more in general if there's a
> tradeoff
> between optimal packing of fields and access speed.  It sounds like we have
> the freedom
> to sort the fields in any order we like in the C++ translation.

Yes. If we ever do anything to optimize this, it should be done in the bindings codegen.
(In reply to Mats Palmgren (:mats) from comment #16)
> Actually, I think my suggestion might be wrong, and it's the
> <table><caption> that is
> wrong.  I need to read up on the thread(s) discussing this feature first to
> understand
> it better.  At the moment I'm thinking that the only boxes that should be
> included are
> those associated with the element itself, i.e. not <legend> nor <caption> in
> the examples
> above, but include bullets, ::before/::after etc.

The thread doesn't really address this; we just assumed we should behave like getClientRects(). The table behavior for getClientRects() was introduced in bug 414190. It's a special case because visually a table element visually contains its caption and it would be a bit confusing to just return the inner table frame's bounds. <fieldset> <legend> is a similar sort of case. I will raise this on www-style.

::before/::after create anonymous children for the element, thus I think they should not contribute to the quad list.

I'm not sure about bullets. I think probably not but I'll mention it on the list.

(In reply to Boris Zbarsky [:bz] from comment #15)
> r=me with that and a comment added to Node.webidl, both above the partial
> interface and at the top of the file, pointing to the relevant spec.

The spec hasn't been written yet, but I think Simon Pieters is going to.

> That said, if we wanted to we could only expose this on
> Document/Element/Text instead of on Node, and maybe even make it infallible.
> I guess that will get sorted out in the spec.

We'd also have to make relativeTo a union type in that case. Could be done, but seems a little unnecessary.

(In reply to Boris Zbarsky [:bz] from comment #14)
> Should the DOMPoint ctor take unrestricted double?  Specifically, should it
> allow infinities?

I think not but I'll ask on the list.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #18)
> The thread doesn't really address this; we just assumed we should behave
> like getClientRects(). The table behavior for getClientRects() was
> introduced in bug 414190. It's a special case because visually a table
> element visually contains its caption and it would be a bit confusing to
> just return the inner table frame's bounds. <fieldset> <legend> is a similar
> sort of case.

Actually our getClientRects() behavior is explicitly mandated as a special case in the CSSOM spec:
http://dev.w3.org/csswg/cssom-view/#dom-element-getclientrects
fieldset/legend on the other hand is not. Still needs discussion on the list, though.
Probably should hold off reviewing these patches until the spec discussion settles down and I update these patches to the consensus.
Comment on attachment 807053 [details] [diff] [review]
Part 2: Add nsLayoutUtils::TransformCSSPoints and nsLayoutUtils::GetFirstNonAnonymousFrame

Clearing these review requests until the www-style discussion has settled
on how it should work, please ask again later.
Attachment #807053 - Flags: review?(matspal)
Attachment #807054 - Flags: review?(matspal)
Attachment #807055 - Flags: review?(matspal)
Attachment #807057 - Flags: review?(matspal)
Attachment #807056 - Flags: review?(matspal)
Blocks: 663778
Blocks: 726427
A problem with the current patch is that if the document is zoomed in to any extent then:

All points are:
{w: 1, x: Infinity, y: Infinity, z: 0}

All bounds are:
{bottom: NaN, height: NaN, left: Infinity, right: NaN, top: Infinity, width: NaN}
OK, I'll look into that.
Sorry about the delay here; some spec details ended up being controversial, especially around the design of DOMRect. But we have basically converged, so I'll update these patches soon.
Is it correct that the margin box co-ordinates are transformed? As far as I understand, this box should remain in it's original location because it is not affected by the transform.
If you request margin boxes, they will be transformed. I think that's the right thing. It would be weird for border-boxes to be transformed and margin-boxes for the same element not transformed.
Blocks: 944304
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #24)
> Sorry about the delay here; some spec details ended up being controversial,
> especially around the design of DOMRect. But we have basically converged, so
> I'll update these patches soon.

Any update on this?
Flags: needinfo?(roc)
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #22)
> A problem with the current patch is that if the document is zoomed in to any
> extent then:
> 
> All points are:
> {w: 1, x: Infinity, y: Infinity, z: 0}
> 
> All bounds are:
> {bottom: NaN, height: NaN, left: Infinity, right: NaN, top: Infinity, width:
> NaN}

This also happens at the default zoom level on Retina displays.
Is there anything we can do here to help you?
Flags: needinfo?(roc)
This is basically unchanged from the old "part 2", except I fixed the bug found in comments #22 and #28 (need to force floating-point division instead of integer division).
Attachment #807052 - Attachment is obsolete: true
Attachment #807053 - Attachment is obsolete: true
Attachment #807054 - Attachment is obsolete: true
Attachment #807055 - Attachment is obsolete: true
Attachment #807056 - Attachment is obsolete: true
Attachment #807057 - Attachment is obsolete: true
Attachment #8356406 - Flags: review?(matspal)
Attached patch Part 2: Implement DOMPoint (obsolete) — Splinter Review
This patch (and following patches) follows the spec in http://dev.w3.org/fxtf/geometry/.

It's simple stuff, so although I've made DOMPoint pref-controlled, I see no reason to not just let this go through to release. These geometry interfaces were quite heavily discussed and I think we just need to ship something to force them to stabilize.
Attachment #8356410 - Flags: review?(matspal)
DOMRect is not pref-controlled since stuff like getBoundingClientRect already uses it. I don't have any qualms about these changes though.
Attachment #8356411 - Flags: review?(matspal)
Attached patch Part 5: Implement getBoxQuads (obsolete) — Splinter Review
getBoxQuads IDL is given in http://dev.w3.org/csswg/cssom-view/ (designed by Simon). Unfortunately there's no description of the actual semantics there yet. There was quite a bit of discussion on www-style and as far as I can tell we reached a consensus on most edge cases; we certainly did for all the common cases. There may be some edge cases unresolved but my tests seem reasonably thorough.

I think we should land this so people can start using it. It's unlikely to create compatibility risk. I'll push on Simon to write a proper spec.
Attachment #8356415 - Flags: review?(matspal)
(In reply to Paul Rouget [:paul] from comment #29)

Sorry about the delay.
Comment on attachment 8356406 [details] [diff] [review]
Part 1: Add nsLayoutUtils::TransformCSSPoints and nsLayoutUtils::GetFirstNonAnonymousFrame

>layout/base/nsLayoutUtils.h
>+  static TransformResult TransformCSSPoints(nsIFrame* aFromFrame, nsIFrame* aToFrame,
>+                                            uint32_t aPointCount, gfxPoint* aPoints);

Do we actually gain anything by using aPoints as an in/out parameter?
If not, then "const nsPoint* aAppPoints, CSSPoint* aCSSPoints" seems better.
Add @param doc comments for those with [in] / [out] for clarity.
Please point out that the resulting aCSSPoints values are undefined
unless the return value is TRANSFORM_SUCCEEDED.
And rename the method - TransformPoints? TransformToCSSPoints?

Perhaps we should also consider using nsTArray& instead of pointer +
length?  I think explicitly sized nsAutoTArray:s have performance on par
with stack POD arrays if used correctly (and they're safer).

>layout/base/nsLayoutUtils.cpp
>+nsLayoutUtils::TransformCSSPoints(nsIFrame* aFromFrame, nsIFrame* aToFrame,

Please add spaces around / and * operators in this method.

>+  double devPixelsPerCSSPixelFromFrame =
>+  double devPixelsPerCSSPixelToFrame =

Those two should probably be gfxFloat, in case we one day define
gfxFloat to be float rather than double in some configurations.

>+    // answer [...] its reciprocal

Please end the sentence with a period.
Attachment #8356406 - Flags: review?(matspal) → review-
Comment on attachment 8356410 [details] [diff] [review]
Part 2: Implement DOMPoint

I think a DOM peer should review this part.
Attachment #8356410 - Flags: review?(matspal)
Comment on attachment 8356411 [details] [diff] [review]
Part 3: Implement DOMRect per spec

I think a DOM peer should review this part.
Attachment #8356411 - Flags: review?(matspal)
Comment on attachment 8356413 [details] [diff] [review]
Part 4: Add DOMQuad implementation

I think a DOM peer should review this part.
Attachment #8356413 - Flags: review?(matspal)
Comment on attachment 8356415 [details] [diff] [review]
Part 5: Implement getBoxQuads

I have only reviewed layout/base/GeometryUtils.*
The other files should be reviewed by a DOM peer.

>layout/base/GeometryUtils.h
>+#include "gfxPoint.h"

Can that be moved to GeometryUtils.cpp?

>layout/base/GeometryUtils.cpp
>+void GetBoxQuads(nsINode* aNode, ...
>+{
>+  nsIDocument* ownerDoc = aNode->OwnerDoc();
>+  nsIFrame* frame = GetFrameForNode(aNode);
>+  if (!frame) {
>+    // No boxes to return
>+    return;
>+  }

'ownerDoc' can be moved after the if-statement.

>+  if (!relativeToFrame) {
>+    aRv.Throw(NS_ERROR_DOM_NOT_FOUND_ERR);

The spec doesn't say we can throw here, is this really what we want?
(our GeometryUtils.webidl has [Throws] unlike the spec)  Please sort
this out with Simon and remove these aRv.Throw() calls if needed.
If you've already decided that throwing is desired and the spec is
lagging behind, then this looks OK.

r=mats, but I'd like to review AddBox again if TransformCSSPoints
changes in the earlier patch.  The rest looks good.
Attachment #8356415 - Flags: review?(matspal) → review+
Comment on attachment 8356416 [details] [diff] [review]
Part 6: Add tests for getBoxQuads

LGTM, r=mats

Perhaps add tests that checks we throw the right thing for 
document.createDocumentFragment().getBoxQuads()
and
document.body.getBoxQuads({relativeTo:document.createDocumentFragment()}
Attachment #8356416 - Flags: review?(matspal) → review+
This bug is blocking bug 663778 (drawing a box-model highlighter for nodes in the DevTools) which we'd hope to land for the up coming release.
It'd be great if you could let us know if you have an ETA for this bug?
When a page contains an iframe at 100,100 that contains a node at 10,10 getBoxQuads returns 10,10.

I would have expected the points to be relative to the viewport position and not their parent frames.
Michael, each frame has its own viewport.  So yes, the points are relative to the viewport position, for the relevant viewport.  Just like getBoundingClientRect(), client*, etc.
Attached file test.html
I am really sorry, I feel like a troll but attachment 8356406 [details] [diff] [review] did not fix the zoom / retina issue. Although it returns values they are not correct.

- The x and y co-ordinates on zoomed pages are too low at low zoom levels and too high at higher levels.
- The width and height have the same value as the non-zoomed div.

Use full zoom in this test, refresh the page, check the values and it is clear that they are not correct.
(In reply to Mats Palmgren (:mats) from comment #37)
> Do we actually gain anything by using aPoints as an in/out parameter?
> If not, then "const nsPoint* aAppPoints, CSSPoint* aCSSPoints" seems better.

We shouldn't make the inputs be nsPoints since in bug 918189 we need to convert DOM-provided CSS pixel values in double format.

Given the input and output points are the same type and the same number, it seems a little simpler to just convert them in-place than to create two separate arrays for no particular reason.

Do you think "const gfxPoint* aInputPoints, const gfxPoint* aOutputPoints" would be better? If so, why?

> Add @param doc comments for those with [in] / [out] for clarity.
> Please point out that the resulting aCSSPoints values are undefined
> unless the return value is TRANSFORM_SUCCEEDED.

Sure.

> And rename the method - TransformPoints? TransformToCSSPoints?

Sure.

> Perhaps we should also consider using nsTArray& instead of pointer +
> length?  I think explicitly sized nsAutoTArray:s have performance on par
> with stack POD arrays if used correctly (and they're safer).

Yes, but they're also a bit more verbose and these are fixed-length arrays in all callers so I don't think it will be a problem.
Flags: needinfo?(matspal)
(In reply to Mats Palmgren (:mats) from comment #41)
> >+  if (!relativeToFrame) {
> >+    aRv.Throw(NS_ERROR_DOM_NOT_FOUND_ERR);
> 
> The spec doesn't say we can throw here, is this really what we want?
> (our GeometryUtils.webidl has [Throws] unlike the spec)  Please sort
> this out with Simon and remove these aRv.Throw() calls if needed.
> If you've already decided that throwing is desired and the spec is
> lagging behind, then this looks OK. 

Yeah, I'm pretty sure the spec will catch up here.
Attached patch Part 2: Implement DOMPoint (obsolete) — Splinter Review
Attachment #8356410 - Attachment is obsolete: true
Attachment #8388504 - Flags: review?(mrbkap)
Attachment #8356413 - Attachment is obsolete: true
Attachment #8388507 - Flags: review?(mrbkap)
Comment on attachment 8388503 [details] [diff] [review]
Part 1: Add nsLayoutUtils::TransformCSSPoints and nsLayoutUtils::GetFirstNonAnonymousFrame

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #47)
> Given the input and output points are the same type and the same number, it
> seems a little simpler to just convert them in-place than to create two
> separate arrays for no particular reason.

Ah, I see that I mistakenly thought this method took app unit points
and produced CSS points which is why I suggested using separate types.

Changing CSSPoints in place is fine when it's the same type.

> > Perhaps we should also consider using nsTArray& instead of pointer +
> > length?  I think explicitly sized nsAutoTArray:s have performance on par
> > with stack POD arrays if used correctly (and they're safer).
> 
> Yes, but they're also a bit more verbose and these are fixed-length arrays
> in all callers so I don't think it will be a problem.

Yeah, I'm just a bit grumpy on adding more T* + length APIs after seeing too many
security bugs emanating from such APIs.  Fixed-length stack arrays is somewhat
easier to spot errors for though, so OK.

> layout/base/nsLayoutUtils.h
>+   * Transforms a list of points, given as gfxPoints representing CSS pixels,

Remove the part about gfxPoints since it now takes the correct type.
Maybe just shorten it to "Transforms a list of CSS points, from aFromFrame ..."
Attachment #8388503 - Flags: review?(matspal) → review+
Flags: needinfo?(matspal)
Attachment #8388509 - Flags: review?(matspal) → review+
There's a few test failures, most of which are easy to fix:
https://tbpl.mozilla.org/?tree=Try&rev=c966a1395db9

One remaining issue is due to float imprecision causing test failures. We could fix this by using gfxPoint instead of CSSPoint to get double precision. But I think for now I'll just fix it by increasing the tolerance in the tests.
Attachment #8388504 - Attachment is obsolete: true
Attachment #8388504 - Flags: review?(mrbkap)
Attachment #8389630 - Flags: review?(jst)
Attachment #8388505 - Attachment is obsolete: true
Attachment #8388505 - Flags: review?(mrbkap)
Attachment #8389631 - Flags: review?(jst)
Attachment #8388507 - Attachment is obsolete: true
Attachment #8388507 - Flags: review?(mrbkap)
Attachment #8389632 - Flags: review?(jst)
Attachment #8388510 - Attachment is obsolete: true
Attachment #8388510 - Flags: review?(mrbkap)
Attachment #8389633 - Flags: review?(jst)
Attachment #8389628 - Flags: review?(matspal) → review+
Attachment #8389630 - Flags: review?(jst) → review+
Attachment #8389631 - Flags: review?(jst) → review+
Attachment #8389632 - Flags: review?(jst) → review+
Attachment #8389633 - Flags: review?(jst) → review+
Looks all good! Given that this exposes new APIs to the web I'd appreciate an intent to ship email to dev.platform here. Thanks.
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #62)
> Looks all good! Given that this exposes new APIs to the web I'd appreciate
> an intent to ship email to dev.platform here. Thanks.

Done.
Hi,

sorry Roc had to backout this changes as part of https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=12e871cf1100 for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=36383057&tree=Mozilla-Inbound - not sure if this is a problem in the test or this checkins here
Revision 7c1d075bb7f6 landed this morning and managed to break my patches without causing a merge conflict.
Depends on: 1005588
Depends on: 1009036
So I just learned from :philor that this API isn't enabled in release builds:
http://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/init/all.js#1883

In the devtools, we have a nice box-model highlighter (bug 663778) that, before this landed, used to rely on a simple node.getBoxQuads polyfill. Now that the API is available, we removed that polyfill in bug 969306.
That bug is in 32, Aurora today, and will move to beta in less than 20 days.

Roc: what's the plan for node.getBoxQuads to be enabled in release builds?
Depending on this, we will need to re-introduce our polyfill and test for the "layout.css.getBoxQuads.enabled" flag to use the real API or the polyfill.
Flags: needinfo?(roc)
We should just enable it in release builds. Filed bug 1033276 on that.

If that doesn't get resolved in a couple of weeks, I guess you'll need to revert to your polyfill before merging to Beta.
Flags: needinfo?(roc)
Or we could enable in privileged code on all channels.
(In reply to Boris Zbarsky [:bz] from comment #73)
> Or we could enable in privileged code on all channels.

+1, bug 1033391 logged.
Blocks: 1107559

BCD PR 4828 submitted to add this to BCD.

So... getBoxQuads() is only available in privileged code at this time?

With this only available in chrome right now, I'm going to back out the addition of getBoxQuads() to Firefox's BCD record, since it's not "available available."

It's available to privileged code in release and beta; available to the web at large in nightly. Looks like bug 1107559 tracks enabling it in release builds.

You need to log in before you can comment on or make changes to this bug.