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

RESOLVED FIXED in Firefox 65

Status

()

enhancement
P2
normal
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: bradwerth, Assigned: bradwerth)

Tracking

unspecified
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(2 attachments, 5 obsolete attachments)

Assignee

Description

7 months ago
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]
Assignee

Comment 1

7 months ago
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'.
Summary: Flex devtools API should return anonymous box nodes when reporting flex items → Flex devtools API should return flex item anonymous box size and transform-to-parent in addition to other values
Assignee

Comment 2

7 months ago
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.
Assignee

Comment 5

7 months ago
Depends on D11783
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.)
Flags: needinfo?(bwerth)
Assignee

Comment 9

7 months ago
(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.
Flags: needinfo?(bwerth)
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.)
Assignee

Comment 11

7 months ago
(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...
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.
Assignee

Comment 14

7 months ago
(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!
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.
Assignee

Comment 16

7 months ago
(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.
Summary: Flex devtools API should return flex item anonymous box size and transform-to-parent in addition to other values → Flex devtools API should return flex item untransformed position and size in addition to other values
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.)
Attachment #9024768 - Attachment is obsolete: true
Attachment #9024765 - Attachment description: Bug 1506687 Part 2: Make FlexItemValues also provide the item's transform to its container. r?dholbert! → Bug 1506687 Part 2: Make FlexItemValues also provide the item's position and size. r?dholbert!

Comment 19

7 months ago
Pushed by bwerth@mozilla.com:
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

7 months ago
Backout by csabou@mozilla.com:
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
Assignee

Comment 22

7 months ago
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.
Attachment #9025511 - Flags: feedback?(gl)
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.
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.
(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).
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 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.)
(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.
(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.
Attachment #9025511 - Flags: feedback?(gl)
Assignee

Comment 30

7 months ago
(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.
Attachment #9024764 - Attachment is obsolete: true
Attachment #9024765 - Attachment is obsolete: true
Attachment #9024766 - Attachment is obsolete: true
Attachment #9025511 - Attachment is obsolete: true
Attachment #9025800 - Attachment description: Bug 1506687 Part 1: Make FlexItemValues also provide the item's bounding client rect. r?dholbert! → Bug 1506687 Part 1: Make FlexItemValues also provide the item's frame rect. r?dholbert!
Attachment #9025801 - Attachment description: Bug 1506687 Part 2: Add a test of FlexItemValues boundingClientRect property. r?dholbert! → Bug 1506687 Part 2: Add a test of FlexItemValues frameRect property. r?dholbert!

Comment 35

7 months ago
Pushed by bwerth@mozilla.com:
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
Depends on: 1503756
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.
Flags: needinfo?(bwerth) → needinfo?(apavel)

Comment 38

7 months ago
Pushed by apavel@mozilla.com:
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
Flags: needinfo?(apavel)

Comment 39

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/04f110532f49
https://hg.mozilla.org/mozilla-central/rev/fc6057313da6
Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.