1506687 - Flex devtools API should return flex item untransformed position and size in addition to other values

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P2)

Description

[Brad Werth [:bradwerth]]

The Flex devtools API provides a way to report information on flex items to devtools. Currently the iteration of flex items will skip an anonymous box node[1] and instead return the first text node contained by it. In order to better support the devtools flexbox highlighter, this API should be changed to return the anonymous box flex items directly.

[1][https://searchfox.org/mozilla-central/rev/7f7c353e969e61a6a85201cc8ad3c3de12ac30d8/layout/generic/nsFlexContainerFrame.cpp#4827]

Comment 1

[Brad Werth [:bradwerth]]

This approach needs refinement. The anonymous boxes generated in nsFlexContainerFrame do not directly map to a DOM node -- only to their parent's or to their child's content. That's problematic because our current practice in devtools is to use secondary API like getTransformToParent to determine how to orient the item's frame to the viewport (for purposes of drawing a highlighter over it). The getTransformToParent method has to be called on a DOM node, and anonymous boxes don't have one!

So the refined solution is to report that transform directly in the FlexItemValues structure. Additionally, flex items that are anonymous boxes will have no "node" value, and their size values will be for their own frames, not for their childrens'.

Comment 2

[Brad Werth [:bradwerth]]

This testcase provided by dholbert shows how it is important for the API to report size of the anonymous box, not its child:

data:text/html,<div style="display:flex;height:400px;border:1px solid black"><i>A</i>B

As flex items, both the item for letter A and the item for letter B are stretched to 400px high and our highlighters should draw them as such. However, the second flex item is an anonymous box and our highlighters currently show it as only the unstretched frame around the letter B itself.

Comment 3

[Brad Werth [:bradwerth]]

Attachment: Bug 1506687 Part 1: Make devtools Flex API return null nodes for anonymous box flex items. r?dholbert!

Comment 4

[Brad Werth [:bradwerth]]

Attachment: Bug 1506687 Part 2: Make FlexItemValues also provide the item's position and size. r?dholbert!

Depends on D11782

Comment 5

[Brad Werth [:bradwerth]]

Attachment: Bug 1506687 Part 3: Update test expectations. r?dholbert!

Depends on D11783

Comment 6

[Brad Werth [:bradwerth]]

Attachment: Bug 1506687 Part 4: Add a test of flex item transformToContainer. r?dholbert!

Depends on D11784

Comment 7

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9a4a58729b48db496a597fa00c355bc7e468ae7

Comment 8

[Daniel Holbert [:dholbert]]

As of right now, it looks like this patch-stack gets us the position of the flex item, but not its size yet -- right?  (If so, that's OK, though we probably do need a followup bug to handle cases like comment 2 where the size is important and not easily inferred from the DOM tree.)

Comment 9

[Brad Werth [:bradwerth]]

(In reply to Daniel Holbert [:dholbert] from comment #8)
> As of right now, it looks like this patch-stack gets us the position of the
> flex item, but not its size yet -- right?

You're right, I forgot that the existing properties mainBaseSize, etc. only cover the bounds of the size, not the size itself. I'll add mainSize and crossSize members to the FlexItemValues object.

Comment 10

[Daniel Holbert [:dholbert]]

Cool! Thanks.

Though: if we're aiming to use that size in coordination with the transformed position, then things might not be entirely coherent... (i.e. it seems like we'd be reporting the *transformed* position, but the *untransformed* size...)

(I don't know offhand what the best way is to report the post-transform size, but hopefully we've got some existing utility code that we could use for that.)

Comment 11

[Brad Werth [:bradwerth]]

(In reply to Daniel Holbert [:dholbert] from comment #10)
> Cool! Thanks.
> 
> Though: if we're aiming to use that size in coordination with the
> transformed position, then things might not be entirely coherent... (i.e. it
> seems like we'd be reporting the *transformed* position, but the
> *untransformed* size...)

Well, this is not a great answer but a scaling factor could be pulled from the transform to determine the scaled size. What a foul hack...

Comment 12

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

To me it doesn't really matter how this works as long as we can get either:

1. The untransformed points of the node.
2. The transformed points of the node along with transformation matrix.

We ultimately need the untransformed points of the node relative to the viewport.

Comment 13

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b4ab6d0b98c07c5904485f44c8d388ba3bf3865

Comment 14

[Brad Werth [:bradwerth]]

(In reply to Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] from comment #12)
> To me it doesn't really matter how this works as long as we can get either:
> 
> 1. The untransformed points of the node.
> 2. The transformed points of the node along with transformation matrix.

I believe this patch provides second option. The way to go it would be...

1) On the flex container, call getAsFlexContainer, and note the mainAxisDirection and crossAxisDirection.
2) On the flex container, call getTransformToViewport to get the transformToViewport.
3) Iterate the flex items and on each note the mainSize, crossSize, and transformToContainer.
4) Compute (0, 0, mainSize, crossSize) or (0, 0, crossSize, mainSize) depending on the axis directions, transformed by inverse transformToContainer times inverse transformToViewport.
5) Profit!

Comment 15

[Daniel Holbert [:dholbert]]

Maybe we could just report the (pre-transfor) main-position, cross-position, main-size, cross-size, taken directly from the C++ FlexItem struct -- and then let devtools use whatever DOM APIs it normally uses to get transformed boxes of nodes, if necessary?

Those APIs wouldn't be available for anonymous flex items; but conveniently, anonymous flex items are guaranteed **not to be transformed/relatively-positioned**, because they can't be targeted with styles.

Comment 16

[Brad Werth [:bradwerth]]

(In reply to Daniel Holbert [:dholbert] from comment #15)
> Maybe we could just report the (pre-transfor) main-position, cross-position,
> main-size, cross-size, taken directly from the C++ FlexItem struct -- and
> then let devtools use whatever DOM APIs it normally uses to get transformed
> boxes of nodes, if necessary?

That would be cleaner. I'll add mainPosition and crossPosition to the FlexItemInfo structure, and remove transformToContainer.

Comment 17

[Daniel Holbert [:dholbert]]

Sounds good, thanks!

In your mochitest 'test_flex_items.html', you can easily test .mainPosition for the first item (should trivially be 0). And you can test it on the second item, too, if you give the first item a hardcoded width (which would then become the .mainPosition for the second item).  You'd probably be OK to leave it at that.

And then .crossPosition would be 0 in most cases; you could test one "interesting" value by giving one of the items a nonzero "margin-top" if you like.   (Probably good to be sure we're testing at least one nonzero expectation for both of these new reported values.)

Comment 18

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=12946fab7ba5663907a21c150db8ebb502c44f79

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c50af93cfc84
Part 1: Make devtools Flex API return null nodes for anonymous box flex items. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/1c3baa04d4ce
Part 2: Make FlexItemValues also provide the item's position and size. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/2788a93e179a
Part 3: Update test expectations. r=dholbert

Comment 20

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9bff4fddd787
Backed out 3 changesets for devtools failures on browser_flexbox_sizing_info_for_text_nodes. CLOSED TREE

Comment 21

[Brad Werth [:bradwerth]]

Attachment: Bug 1506687 Part 4: Update flexbox inspector to handle anonymous box flex items, which have no DOM node.

Comment 22

[Brad Werth [:bradwerth]]

Comment on attachment 9025511 [details]
Bug 1506687 Part 4: Update flexbox inspector to handle anonymous box flex items, which have no DOM node.

Gabe, I'll need help with this one. The flexbox inspector assumes that every flex item maps to a DOM node. This patch makes that no longer true. This part of the patch amends one easy case in the inspector, but there are further problems. Applying this patch and attempting to inspect text nodes generates a console error with this stack:

getRootBindingParent@resource://devtools/shared/layout/utils.js:482:9
isAnonymous@resource://devtools/shared/layout/utils.js:517:31
attachElements@resource://devtools/server/actors/inspector/walker.js:432:46
attachElement@resource://devtools/server/actors/inspector/walker.js:411:35
getNodeFromActor@resource://devtools/server/actors/inspector/walker.js:2130:12
handler@resource://devtools/shared/protocol.js:1198:21
onPacket@resource://devtools/server/main.js:1324:15
receiveMessage@resource://devtools/shared/transport/child-transport.js:66:5
MessageListener.receiveMessage*_addListener@resource://devtools/shared/transport/child-transport.js:40:5
ready@resource://devtools/shared/transport/child-transport.js:57:5
_onConnection@resource://devtools/server/main.js:888:5
connectToParent@resource://devtools/server/main.js:309:12
onConnect</<@resource://devtools/server/startup/frame.js:50:22
onConnect<@resource://devtools/server/startup/frame.js:49:7
exports.makeInfallible/<@resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14
MessageListener.receiveMessage*@resource://devtools/server/startup/frame.js:72:5
@resource://devtools/server/startup/frame.js:19:4
connectToFrame/<@resource://devtools/server/main.js:613:7
connectToFrame@resource://devtools/server/main.js:609:12
connect@resource://devtools/server/actors/targets/frame-proxy.js:54:21
async*BrowserTabList.prototype._getActorForBrowser@resource://devtools/server/actors/webbrowser.js:310:10
BrowserTabList.prototype.getTab@resource://devtools/server/actors/webbrowser.js:327:16
onGetTab@resource://devtools/server/actors/root.js:338:27
Async*onPacket@resource://devtools/server/main.js:1324:15
send/<@resource://devtools/shared/transport/local-transport.js:64:11
exports.makeInfallible/<@resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14

It looks like getNodeForActor is being called on the flex item, but I'm not sure where that call originates from or what alternative code should run, given that the node is null. I would appreciate any help you can offer me.

Comment 23

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Not sure exactly where stuff is failing but if it is the flexbox highlighter itself then the problem is that in devtools/server/actors/highlighters/flexbox.js::getFlexData() we iterate through the flexItems and save a reference to the node.

I think the simplest option is to return the text node instead of null for the node and then when we detect a text node we could use the new properties.

Comment 24

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

This is also difficult from my end e.g.

  row = horizontal-lr vertical-tb
  row-reverse + RTL = horizontal-lr vertical-tb

But using the obvious calculations:

  left = mainPosition;
  top = crossPosition;
  right = mainPosition + mainSize;
  bottom = crossPosition + crossSize;

LTR is fine but RTL mirrors things.

I am going to experiment some more on the JS side... if the worst comes to the worst I can use rotating calipers and ranges.

Comment 25

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] from comment #24)
> This is also difficult from my end e.g.
> 
>   row = horizontal-lr vertical-tb
>   row-reverse + RTL = horizontal-lr vertical-tb
> 
> But using the obvious calculations:
> 
>   left = mainPosition;
>   top = crossPosition;
>   right = mainPosition + mainSize;
>   bottom = crossPosition + crossSize;
> 
> LTR is fine but RTL mirrors things.
> 
> I am going to experiment some more on the JS side... if the worst comes to
> the worst I can use rotating calipers and ranges.

Ah, can't do that either because we can't get the transform that is applied to the text node (when it is transformed).

Comment 26

[Daniel Holbert [:dholbert]]

We could also conceivably just directly report left/top/right/bottom from the platform side, if that's all we're using these members for.  (I feel bad volunteering more reworking of these patches on Brad's end, but I suspect he'd be up for that. :))

I think we could just return the (converted-to-css-pixels) members of FlexItem::mFrame->GetRect(), basically.

Comment 27

[Daniel Holbert [:dholbert]]

(if we go that route, nsRect [GetRect()'s return type] has accessors for  .X(), .Y(), .XMost() and .YMost() -- or equivalently, .Width() and .Height() if that ends up easier to consume on the devtools end.)

Comment 28

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Daniel Holbert [:dholbert] (PTO Fri Nov 16) from comment #26)
> We could also conceivably just directly report left/top/right/bottom from
> the platform side, if that's all we're using these members for.  (I feel bad
> volunteering more reworking of these patches on Brad's end, but I suspect
> he'd be up for that. :))
> 
> I think we could just return the (converted-to-css-pixels) members of
> FlexItem::mFrame->GetRect(), basically.

If that would give us the untransformed positions then that would be perfect.

Comment 29

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Brad Werth [:bradwerth] from comment #22)
> Comment on attachment 9025511 [details]
> Bug 1506687 Part 4: Update flexbox inspector to handle anonymous box flex
> items, which have no DOM node.
> 
> Gabe, I'll need help with this one. The flexbox inspector assumes that every
> flex item maps to a DOM node. This patch makes that no longer true. This
> part of the patch amends one easy case in the inspector, but there are
> further problems. Applying this patch and attempting to inspect text nodes
> generates a console error with this stack:
> 
> getRootBindingParent@resource://devtools/shared/layout/utils.js:482:9
> isAnonymous@resource://devtools/shared/layout/utils.js:517:31
> attachElements@resource://devtools/server/actors/inspector/walker.js:432:46
> attachElement@resource://devtools/server/actors/inspector/walker.js:411:35
> getNodeFromActor@resource://devtools/server/actors/inspector/walker.js:2130:
> 12
> handler@resource://devtools/shared/protocol.js:1198:21
> onPacket@resource://devtools/server/main.js:1324:15
> receiveMessage@resource://devtools/shared/transport/child-transport.js:66:5
> MessageListener.receiveMessage*_addListener@resource://devtools/shared/
> transport/child-transport.js:40:5
> ready@resource://devtools/shared/transport/child-transport.js:57:5
> _onConnection@resource://devtools/server/main.js:888:5
> connectToParent@resource://devtools/server/main.js:309:12
> onConnect</<@resource://devtools/server/startup/frame.js:50:22
> onConnect<@resource://devtools/server/startup/frame.js:49:7
> exports.makeInfallible/<@resource://devtools/shared/ThreadSafeDevToolsUtils.
> js:109:14
> MessageListener.receiveMessage*@resource://devtools/server/startup/frame.js:
> 72:5
> @resource://devtools/server/startup/frame.js:19:4
> connectToFrame/<@resource://devtools/server/main.js:613:7
> connectToFrame@resource://devtools/server/main.js:609:12
> connect@resource://devtools/server/actors/targets/frame-proxy.js:54:21
> async*BrowserTabList.prototype._getActorForBrowser@resource://devtools/
> server/actors/webbrowser.js:310:10
> BrowserTabList.prototype.getTab@resource://devtools/server/actors/webbrowser.
> js:327:16
> onGetTab@resource://devtools/server/actors/root.js:338:27
> Async*onPacket@resource://devtools/server/main.js:1324:15
> send/<@resource://devtools/shared/transport/local-transport.js:64:11
> exports.makeInfallible/<@resource://devtools/shared/ThreadSafeDevToolsUtils.
> js:109:14
> 
> It looks like getNodeForActor is being called on the flex item, but I'm not
> sure where that call originates from or what alternative code should run,
> given that the node is null. I would appreciate any help you can offer me.

So, this is probably being called from https://searchfox.org/mozilla-central/source/devtools/client/inspector/flexbox/flexbox.js#189. We are getting all the flex items here and getting their associated NodeFront, which is then used to generate an ElementRep (label) for the flex item list in the UI. So, we will need some way to know in the client side that this is a text node and have an ideal representation of the text node in the flex item UI without having a nodeFront to use to generate the Rep.

Comment 30

[Brad Werth [:bradwerth]]

(In reply to Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] from comment #28)
> (In reply to Daniel Holbert [:dholbert] (PTO Fri Nov 16) from comment #26)
> > We could also conceivably just directly report left/top/right/bottom from
> > the platform side, if that's all we're using these members for.  (I feel bad
> > volunteering more reworking of these patches on Brad's end, but I suspect
> > he'd be up for that. :))
> > 
> > I think we could just return the (converted-to-css-pixels) members of
> > FlexItem::mFrame->GetRect(), basically.
> 
> If that would give us the untransformed positions then that would be perfect.

That sounds fine. We're aiming for simple.

(In reply to Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] from comment #23)
> I think the simplest option is to return the text node instead of null for
> the node and then when we detect a text node we could use the new properties.

Also sensible. I'll keep it returning the first text node of an anonymous box (current behavior) which will also avoid the problems noted in comment 22. Node == TEXT_NODE means ignore the node frame and use the flex item frame instead, for purposes of drawing a highlighter.

Together, these are significant reworks of the code, particularly of the tests. I'm going to scrap the whole patch stack and submit a new set of patches.

Comment 31

[Brad Werth [:bradwerth]]

Attachment: Bug 1506687 Part 1: Make FlexItemValues also provide the item's frame rect. r?dholbert!

Comment 32

[Brad Werth [:bradwerth]]

Attachment: Bug 1506687 Part 2: Add a test of FlexItemValues frameRect property. r?dholbert!

Depends on D12182

Comment 33

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b48bc6a4c7c448c18c7f8227d73745cf3d6f01b

Comment 34

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=be264a77b24b2ae227045a120c5735dda2d864cf

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d8230f3fdd2c
Part 1: Make FlexItemValues also provide the item's frame rect. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/f93f179fb3b4
Part 2: Add a test of FlexItemValues frameRect property. r=dholbert

Comment 36

[Cosmin Sabou [:CosminS]]

Backed out for causing mochitest mass failures.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception&fromchange=2b51cc7acba5fdac3642d3972e489d297bc608c2&tochange=b3ca607f2595dba7b94124868581c62a9fce668d&searchStr=mochitest&group_state=expanded&selectedJob=212362445

Failure log:
https://treeherder.mozilla.org/logviewer.html#?job_id=212362445&repo=autoland&lineNumber=7706
https://treeherder.mozilla.org/logviewer.html#?job_id=212362277&repo=autoland&lineNumber=1420

Backout link: https://hg.mozilla.org/integration/autoland/rev/b3ca607f2595dba7b94124868581c62a9fce668d

Please take a look over https://bugzilla.mozilla.org/show_bug.cgi?id=1503756

Comment 37

[Cosmin Sabou [:CosminS]]

This should be relanded when the trees are reopened. The failures here were caused by Bug 1508016. ni'ed Andreea to take care of this when possible. Thanks and sorry for the trouble.

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/04f110532f49
Part 1: Make FlexItemValues also provide the item's frame rect. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/fc6057313da6
Part 2: Add a test of FlexItemValues frameRect property. r=dholbert

Comment 39

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/04f110532f49
https://hg.mozilla.org/mozilla-central/rev/fc6057313da6