1494782 - Shape editor doesn't reliably show shape, permit edits, update outline for pseudo-elements

Status: NEW

(DevTools :: Inspector, defect, P2)

Description

[Jim Blandy :jimb]

The shape editor doesn't work consistently for me. It either:
a) fails to show the shape outline on the content at all,
b) shows the shape outline, but won't let me drag the control points, or
c) lets me drag the control points, with the expected effect on the content,
   but doesn't update the shape outline.

The behavior seems quite sensitive to the exact sequence of actions taken, so
I've tried to describe the steps I've taken carefully.

To reproduce a):
- Download and unpack the attached `shape-editor-bug.zip`.
- Visit the `shapes.html` file via a `file:///` URL.
- Right-click around the center of the square with the red-blue gradient, and
  choose “Inspect element”.
- In the center panel listing the rules, expand the “Pseudo-elements” entry.
- Click on the polygon icon in the `shape-outside` declaration.

I expect the outline to show, but actually nothing happens at all. I don't see
any errors in the browser console. Perhaps this is because I haven't actually
selected the pseudo-element in the left panel, but isn't it unambiguous which
pseudo-element I'm referring to? Maybe there's some reason I'm not aware of that
doing so would be ambiguous, but it was confusing to me.

To reproduce b), having followed the steps above:
- In the left panel showing the element tree, expand the `<div class="content">`
  element. A `::before` child pseudo-element appears.
- Click on the `::before` child pseudo-element. In the center panel listing the
  rules, a `.content::before` rule appears, with a `shape-outside` declaration.
- Click on the shape icon there. An outline of the circle appears in the
  content, with handles at the center and right side.
- Drag the handle on the right side.

I expect the circle's radius to grow, and the text around it to be rearranged to
accomodate that. Instead, roughly fifty percent of the time, for reasons I
haven't been able to establish, it seems like one pixel's worth of travel
occurs, and then further dragging has no effect. The following error messages
spew to the terminal I've started Firefox from:

JavaScript error: resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/highlighters/shapes.js, line 381: TypeError: this.currentQuads[this.referenceBox][0] is undefined; can't access its "bounds" property

To reproduce c), follow the same steps as for b). On those occasions where the
handle does let me adjust the radius of the circle, the text is adjusted as I
move the mouse, but the shape outline doesn't update along with it.

Comment 1

[Jim Blandy :jimb]

Attachment: Files used in reproduction instructions.

Comment 2

[Patrick Brosset <:pbro>]

Thanks for filing Jim! I can reproduce a and c, not b for now. But I think fixing c might be enough to fix b too. They are most probably related.
The thing that caught my eye is that when reproducing c the ::before pseudo-element in the markup-view becomes unselected as soon as I start moving the points.
To me, that means that the pseudo-element got re-created. I think that might be the case, because as we rewrite the CSS rule on the server to apply the changes, we also rewrite content:"" at the same time. And doing so basically reset the pseudo-element itself.
So I think what might be happening is that we don't have a reference to this node anymore after we started moving it the first time, and that breaks the tool in weird ways.

Comment 3

[Jim Blandy :jimb]

I'm sorry I couldn't come up with a 100% reproduction recipe for b). When you get it debugged, I really want to know what the problem was.

Comment 4

[Razvan Caliman [:rcaliman]]

Situation a) happens because on inspecting the element from the context menu, the selected nodeFront isn't pointing to the pseudo-element, but to the its host node instead (div.content). The tell-tale sign is that the pseudo-element isn't selected in the Elements panel after using the context menu. There's a historical bug about this: https://bugzilla.mozilla.org/show_bug.cgi?id=1122970

I tracked back the logic to identify the element on right click to the `findCssSelector()` method which has no context about pseudo-elements (it only operates on the DOM): 
https://searchfox.org/mozilla-central/source/toolkit/modules/css-selector.js#103
via
https://searchfox.org/mozilla-central/source/browser/actors/ContextMenuChild.jsm#548

This part of the code has no context of CSS where the pseudo-elements are selected. I'm not sure how to proceed from here. Perhaps we can pass down the pointer coordinates and let the Inspector client measure them against the pseudo-element quads if any is found on the element's matching CSS rules. But then z-index and any CSS transforms complicate things.

Situation b) very likely happens due to what Patrick mentioned, the pseudo-element gets destroyed on edit and the editor is pointing at a dead end. I haven't been able to consistently reproduce it either, but it does happen.

Situation c) is perhaps a result of the tool maintaining a reference to the nodeFront of the destroyed pseudo-element, in contradiction to situation b), hence not causing the console error, but also not having an accurate boxQuad model with which to position the editor on screen.

I will focus on finding the fix for c) since, as Patrick suggests, it's likely to solve b) as well. 
a) should be fixed independently for https://bugzilla.mozilla.org/show_bug.cgi?id=1122970

Comment 5

[Razvan Caliman [:rcaliman]]

Some context: the pseudo-element's nodeFront does get re-created. It happens in response to a mutation from MutationObserver whenever the `shape-outside` property is changed: https://searchfox.org/mozilla-central/source/devtools/client/inspector/markup/markup.js#1104,1112

Other properties, like `float` or `content`, trigger a mutation with the same signature suggesting it could be related to layout. However, that's not accurate. Changes to properties like `margin`, `padding`, `border` for the pseudo-element *do not* trigger a mutation. Likewise, no mutation for `clip-path`, thus explaining why the CSS Shape Path Editor works as expected in that editing mode.

See also: https://bugzilla.mozilla.org/show_bug.cgi?id=1034110

I'm exploring the option to prevent that mutation from being fired. This requires input from platform devs familiar with layout. I followed @bgrins' advice to ask for assistance in #layout in IRC.

PS: pseudo-elements are hard.

Comment 6

[Sean Voisen (:svoisen)]

(In reply to Razvan Caliman [:rcaliman] from comment #5)

> I'm exploring the option to prevent that mutation from being fired. This
> requires input from platform devs familiar with layout. I followed @bgrins'
> advice to ask for assistance in #layout in IRC.

ni'ing TYLin and bradwerth in case one of them might be able to answer your questions.

Comment 7

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: shape-changes-after-timeout.html

I made a simplified test case that will change the 'shape-outside' value in the .content::before after 3 seconds.

Re comment 5:
> the pseudo-element's nodeFront does get re-created. It happens in response
> to a mutation from MutationObserver whenever the `shape-outside` property
> is changed

I'm not sure what the nodeFront is ..., but in my test case, the entire frame tree of <div class="content"> is reconstructed because we change the "shape-outside" value in its .content::before style.

The reason of the above is:
Due to "shape-outside" value changes, the style difference returns a hint nsChangeHint_ScrollbarChange [1] from nsStyleDisplay::CalcDifference(). Later in RestyleManager(), we *upgrade* the hint to nsChangeHint_ReconstructFrame [2]! Hence the reconstruction.

I don't have a solution on top of my head. Loop more people.

[1] https://searchfox.org/mozilla-central/rev/a11c128b90ea85d7bb68f00c5de52c0794e0b215/layout/style/nsStyleStruct.cpp#3759
[2] https://searchfox.org/mozilla-central/rev/a11c128b90ea85d7bb68f00c5de52c0794e0b215/layout/base/RestyleManager.cpp#1361-1415

Comment 8

[Emilio Cobos Álvarez (:emilio)]

We should ideally not reframe the whole host when a pseudo-element changes, that's bug 1476281. But generally assuming things about the life of a pseudo-element is wrong.

We could try to preserve the nodes around and make a pseudo-element's lifetime tied to the host instead of the frame, maybe just creating and destroying them on RestyleManager, but that's far from trivial.

That being said, it looks to me like that ScrollbarChange is very wrong, why is it needed? It seems to go back to bug 1359834, which says:

  Force changes to shape-outside to trigger reflow and overflow recalculation

(Back in the day ScrollbarChange was called CSSOverflowChange).

But that CSSOverflowChange doesn't mean 'recompute overflow', it means 'the overflow property changed', and thus causes the reframe.

Looks to me that instead of ScrollbarChange that should be UpdateOverflow.

Comment 9

[Razvan Caliman [:rcaliman]]

Thank you for your input, everyone!

> I'm not sure what the nodeFront is ..., 

It's a representation of DOM nodes (and pseudo-elements) which allow the DevTools to interact with them across the client / server divide. 

> but in my test case, the entire frame tree of <div class="content"> is reconstructed because we change the "shape-outside" value in its .content::before style.

Yes, that seems to be the issue. The representation of the old pseudo-element is lost due to the tree reconstruction.

> Looks to me that instead of ScrollbarChange that should be UpdateOverflow.

Is this likely to help with avoiding the tree reconstruction mentioned above?

Comment 10

[Emilio Cobos Álvarez (:emilio)]

(In reply to Razvan Caliman [:rcaliman] from comment #9)
> Is this likely to help with avoiding the tree reconstruction mentioned above?

Yes, I'd expect it to fix this bug. Though I haven't checked.

Comment 11

[Brad Werth [:bradwerth]]

I agree the fix in Bug 1496014 should resolve this.

Comment 12

[Razvan Caliman [:rcaliman]]

Thank you, both! Looking forward to trying it when it lands. (I'm unable to do a full build now to test)

Comment 13

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

I think b) and c) should be fixed by Bug 1496014 (tested on 2018-10-10 Nightly)

Comment 14

[Sean Voisen (:svoisen)]

Hi Razvan, did you get a chance to verify that this is fixed in 64 (at least b and c in the testcase)?

Comment 15

[Razvan Caliman [:rcaliman]]

Hi Sean! Sorry for the silence on this one. 

Indeed, cases b) and c) are solved by the fix for Bug 1496014. Thank you for your work, Emilio and Ting-Yu! 

The leftover case a) is a duplicate of Bug 1122970 which I haven't started work on yet.