CSS values in style tools should have a transform tooltip as appropriate

RESOLVED FIXED in Firefox 29

Status

()

Firefox
Developer Tools: Style Editor
--
enhancement
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: cedricv, Assigned: pbro)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 29
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [styleeditor][computedview][ruleview])

Attachments

(4 attachments, 8 obsolete attachments)

(Reporter)

Description

6 years ago
Perhaps showing a small preview before=>after of the transform.

eg. for a scale transform :

----      --
|  |  =>  --
----

 /\
 this is a square

Comment 1

6 years ago
Not sure to understand. Can you elaborate?
(Assignee)

Comment 2

4 years ago
This is part of our work on doorhangers. 

Meta bug 711947
Roadmap here: https://gist.github.com/captainbrosset/6681978

For now, our goal is to provide a preview of the transformation with both the before and after, something like this: http://codepen.io/captainbrosset/full/lHpnK (here is a more evolved version https://github.com/captainbrosset/csstransformer but not sure it would make sense in a tooltip).
Blocks: 711947
(Assignee)

Updated

4 years ago
Depends on: 917755
(Assignee)

Comment 3

4 years ago
Making this bug depend on bug 917755 will make our lives a lot easier because we'll be able to know the real post-transformed coordinates of the transformed box and therefore draw a before/after preview easily.

Having said that, the demo http://codepen.io/captainbrosset/full/lHpnK doesn't require getQuads cause it handles transformation matrices calculation itself.
(Assignee)

Updated

4 years ago
Assignee: nobody → pbrosset
(Assignee)

Comment 4

4 years ago
Created attachment 829144 [details]
css-transform-preview.png

We already have a tooltip widget in the devtools so, mostly reusing the codepen css-transform demo I posted above, here is what I can come up with.

The code I wrote for this is still a bit rough, and there are no tests, but if we want to go ahead with this, I can post a patch pretty quickly.
(Assignee)

Comment 5

4 years ago
Cedric, I know you posted this a long time, but do you remember if this was what you had in mind?

To be clear, I think the highlighter would solve this a lot better, but I still think a simple preview tooltip would help too. Especially if you want to preview overridden transforms (which wouldn't be shown at the highlighter level).
Flags: needinfo?(cedricv)
(Assignee)

Comment 6

4 years ago
Created attachment 8334517 [details] [diff] [review]
bug726427-transform-tooltip.patch

Here is a patch for this bug.

It basically introduces the following things:

- A new shared widget that can create <canvas> elements representing transformed nodes. It is based on a pen I did a couple of months ago: http://codepen.io/captainbrosset/pen/lHpnK
For now, this widget multiplies matrices itself to get transformed coordinates but that may change when we have access to the getQuads API iirc.

- A new `setContent`-type function in Tooltip.js that uses this widget to create a canvas and put it in the tooltip content.
This function accepts a transform value and a transform-origin value, but if omitted will get it via the pageStyle actor using the currently selected node.
It will also get the computed width and height of the currently selected node to give a more accurate preview.

- Finally, the patch modifies the rule-view and computed-view to show the  preview tooltip on transform css properties mouseover.

Ongoing try build: https://tbpl.mozilla.org/?tree=Try&rev=f448a18c7ea1
Attachment #8334517 - Flags: review?(bgrinstead)
(Assignee)

Comment 7

4 years ago
Try failed. Will fix and attach the new patch soon,
(Assignee)

Comment 8

4 years ago
Created attachment 8334841 [details] [diff] [review]
bug726427-transform-tooltip V2.patch

Sorry Brian, last minute change. I had test failures in another test that I unfortunately hadn't run locally before pushing to try.
The patch should be fine now.
New try build: https://tbpl.mozilla.org/?tree=Try&rev=6f47a12240a6
Attachment #8334517 - Attachment is obsolete: true
Attachment #8334517 - Flags: review?(bgrinstead)
Attachment #8334841 - Flags: review?(bgrinstead)
Comment on attachment 8334841 [details] [diff] [review]
bug726427-transform-tooltip V2.patch

Review of attachment 8334841 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking great!  A few UI thoughts:

Sometimes it can be hard to distinguish between certain types of transforms (I will attach a screenshot demonstrating this).  One solution would be to add some text on the transformed element, or maybe some universally recognizable picture or symbol (maybe a clock face?).  I'm not sure how possible this would be inside the canvas though - and it could definitely be done later as an improvement, but something to think about.

Another UI thing I noticed - sometimes the thumbnail opens as a quite large and jumps shows/hides a bunch.  I believe this is only if the rule wraps multiple lines in the ruleview, but I'm not positive.  Will attach a screencast demonstrating.

I've also reviewed much of the code and left a few notes.

::: browser/devtools/shared/widgets/CSSTransformPreviewer.js
@@ +13,5 @@
> + * help debug tricky transformations. It is used today in a tooltip, and this
> + * tooltip is shown when hovering over a css transform declaration in the rule
> + * and computed view panels.
> + *
> + * TODO: For now, it multiplies matrices itself to calculate the coordinates of

Is there a function that we will be able to swap out when getQuads becomes available?

@@ +62,5 @@
> +  /**
> +   * Destroy removes the canvas from the parentelement passed in the constructor
> +   */
> +  destroy: function() {
> +    if (this.canvas) this.parentEl.removeChild(this.canvas);

Nits: please use prevailing style for brackets and spacing

::: browser/devtools/shared/widgets/Tooltip.js
@@ +335,5 @@
>    _showOnHover: function(target) {
> +    let res = this._targetNodeCb(target, this);
> +
> +    // The cb can also return an element to change the target of the tooltip
> +    target = res && res.nodeType ? res : target;

Is this option being used in this patch?  The ability to return an element from targetNodeCb, I mean.  I don't see anywhere where this actually returning (_buildTooltipContent still just returns true).

::: browser/devtools/styleinspector/rule-view.js
@@ +1130,5 @@
> +    // Test for css transform
> +    if (property && property.name === "transform") {
> +      // Look into the rule see if there is the origin
> +      let origin;
> +      for (let prop of property.rule.textProps) {

This looks like it will use a transform origin even if it is disabled (unchecked in the rule view).  More generally, I wonder what the case is when we want to grab the origin from the textProps instead of just using the computed style. Using the computed style will guarantee that the preview they are seeing looks like what is on the page - is there a time when we don't want this?
Attachment #8334841 - Flags: review?(bgrinstead)
Created attachment 8335598 [details]
csstransforms-similar.png

Demonstration of similar transforms from Comment 9
Created attachment 8335600 [details]
csstransform-jump.mp4

Demonstration of tooltip height changing and jumping from Comment 9
(Assignee)

Comment 12

4 years ago
Cool! Thanks Brian for the review.

> Sometimes it can be hard to distinguish between certain types of transforms
> (I will attach a screenshot demonstrating this).  One solution would be to
> add some text on the transformed element, or maybe some universally
> recognizable picture or symbol (maybe a clock face?).  I'm not sure how
> possible this would be inside the canvas though - and it could definitely be
> done later as an improvement, but something to think about.

I think using text would be a good idea, but it would be hard to transform the text in order to position it correctly on the transformed shape.
The thing is I can only convert an x/y coordinate into a transformed x/y coordinate, meaning I need to know all coordinates of the border of a shape to be able to draw it transformed, so for text, that wouldn't be very easy. See what I did here: http://captainbrosset.github.io/csstransformer/
We can do that here too, but I'm not too sure

> Another UI thing I noticed - sometimes the thumbnail opens as a quite large
> and jumps shows/hides a bunch.  I believe this is only if the rule wraps
> multiple lines in the ruleview, but I'm not positive.  Will attach a
> screencast demonstrating.

Good catch, thanks, I will investigate.

> ::: browser/devtools/shared/widgets/CSSTransformPreviewer.js
> @@ +13,5 @@
> > + * help debug tricky transformations. It is used today in a tooltip, and this
> > + * tooltip is shown when hovering over a css transform declaration in the rule
> > + * and computed view panels.
> > + *
> > + * TODO: For now, it multiplies matrices itself to calculate the coordinates of
> 
> Is there a function that we will be able to swap out when getQuads becomes
> available?

Not really. It's true it would make the refactoring simpler when we do have getQuads. I'll work on that.

> @@ +62,5 @@
> > +  /**
> > +   * Destroy removes the canvas from the parentelement passed in the constructor
> > +   */
> > +  destroy: function() {
> > +    if (this.canvas) this.parentEl.removeChild(this.canvas);
> 
> Nits: please use prevailing style for brackets and spacing

Will do.

> ::: browser/devtools/shared/widgets/Tooltip.js
> @@ +335,5 @@
> >    _showOnHover: function(target) {
> > +    let res = this._targetNodeCb(target, this);
> > +
> > +    // The cb can also return an element to change the target of the tooltip
> > +    target = res && res.nodeType ? res : target;
> 
> Is this option being used in this patch?  The ability to return an element
> from targetNodeCb, I mean.  I don't see anywhere where this actually
> returning (_buildTooltipContent still just returns true).

Good catch too. I actually introduced it at some stage because I needed it, but finally changed the code and didn't need it after all, but forgot to remove the option.

> ::: browser/devtools/styleinspector/rule-view.js
> @@ +1130,5 @@
> > +    // Test for css transform
> > +    if (property && property.name === "transform") {
> > +      // Look into the rule see if there is the origin
> > +      let origin;
> > +      for (let prop of property.rule.textProps) {
> 
> This looks like it will use a transform origin even if it is disabled
> (unchecked in the rule view).  More generally, I wonder what the case is
> when we want to grab the origin from the textProps instead of just using the
> computed style. Using the computed style will guarantee that the preview
> they are seeing looks like what is on the page - is there a time when we
> don't want this?

The idea was that, if you have both a transform and transform-origin in a rule and both of them are overridden by a higher priority rule, you'd still want to preview the transform on this rule as if it was applied. At least, that made more sense to me.
True though, if the origin is unchecked, it would then make more sense to use the computed one.
I don't know, perhaps always relying on the computed origin would be easier to understand for the user? 
I can't decide here, what's your point of view?
(Assignee)

Comment 13

4 years ago
Created attachment 8336796 [details] [diff] [review]
bug726427-transform-tooltip V3.patch

This new patch changes the following:

- Refactoring of the CSSTransformPreviewer class to make it easier to maintain and easier to migrate to getQuads when it'll be ready. Still there'll be quite some changes to do when it is (removing the hidden element used to get the matrix, ...)

- Added a setSize function to the tooltip API to make sure the size doesn't get weird in some strange cases. On a related note, I also made it so that the _buildTooltipContent can now return a promise, so that the tooltip content is really ready before it gets shown.

- As for the transform origin, the logic is the following: if the rule has a transform-origin and it is not disabled, this one is used. Otherwise, the computed origin is used. I think this is what makes most sense.

- I have added a triangle in the original and transformed shapes top right corners so that it's immediately visible which way the transform shape is facing. Text would have been a good idea too, but too hard to transform, so would have a picture been.
I've also tried with HTML elements rather than canvas, but have given up because the whole preview gets resized to some MAX_DIM, so the transformation matrix doesn't apply correctly anymore.
Another way would have been to use HTML elements, with the same size as the actual elements in the page, and then only resize everything with a scale(.8) or something, but that ended up making things look not very nice (very thin arrows, ...)
So all in all, the canvas solution was the best compromise. And I think the corner triangle does help.

Ongoing try build: https://tbpl.mozilla.org/?tree=Try&rev=29b14b35b56e
Attachment #8334841 - Attachment is obsolete: true
Attachment #8336796 - Flags: review?(bgrinstead)
(Assignee)

Comment 14

4 years ago
Green try build
(Assignee)

Comment 15

4 years ago
Note: 3d transforms within perspective-applied parents are not represented correctly. The parent perspective isn't applied at all.
This would be solved with getQuads
Comment on attachment 8336796 [details] [diff] [review]
bug726427-transform-tooltip V3.patch

Review of attachment 8336796 [details] [diff] [review]:
-----------------------------------------------------------------

Again, looking good.  Main notes have to do with always using computed style on origin.

As far as 3D transforms with perspective applied parents not previewing properly, it probably isn't worth figuring out how to properly handle this when getQuads will handle it automatically.  I think with this tooltip previewing properly in all other cases, and the general improvements here to tooltip widget it would make sense to land it and deal with the perspective issue once getQuads is ready.  If this perspective thing is a bigger deal, we could wait until getQuads is ready to land anything - I'm just guessing that most cases would be correct already with what is here.

::: browser/devtools/shared/test/browser_csstransformpreview.js
@@ +82,5 @@
> +  is(canvas.width, 200, "width is correct");
> +  is(canvas.height, 100, "height is half of the width");
> +
> +  // Rotate on the top right corner
> +  p.preview("rotate(-90deg)", "top right", 200, 200);

Maybe also test skew() and scale() here.

::: browser/devtools/shared/widgets/CSSTransformPreviewer.js
@@ +69,5 @@
> +    }
> +    if (this._hiddenDiv) {
> +      this.parentEl.removeChild(this._hiddenDiv);
> +    }
> +    this.parentEl = this.canvas = this.ctx = null;

Do we also need to set this.doc = null?

@@ +250,5 @@
> +
> +  /**
> +   * Compute the largest width and height of all the given shapes and map all
> +   * points that they fit into the configured MAX_DIM - 2*PAD area.
> +   * Also returns the X/Y mapping functions

Comment is out of date - the function doesn't return anything.  May want to update this comment to indicate that it is updating the arguments by reference.

@@ +274,5 @@
> +      this.MAX_DIM * Math.min(spanX, spanY) / Math.max(spanX, spanY);
> +    let ch = !isWide ? this.MAX_DIM :
> +      this.MAX_DIM * Math.min(spanX, spanY) / Math.max(spanX, spanY);
> +
> +    this.canvas.setAttribute("width", cw);

Small refactor: return an object with the { cw, ch } variables and set canvas attributes from calling function to limit side effects of this function (wasn't expecting this function to do DOM stuff).

Alternatively, make this function also call drawShapes / drawArrows / drawOrigin and rename it to indicate it is drawing to the canvas as well as fitting the shapes into the bounding box.

::: browser/devtools/shared/widgets/Tooltip.js
@@ +522,5 @@
> +   *        The NodeActor for the currently selected node
> +   * @return A promise that resolves when the tooltip content is ready, or
> +   *         rejects if no transform is provided or is invalid
> +   */
> +  setCssTransformContent: function(transform, origin, pageStyle, node) {

As discussed, let's drop the option of passing in an origin, and just always use computed.  This way the preview will always match what the user is seeing on the page, and it will simplify the API we are exposing.

::: browser/devtools/styleinspector/rule-view.js
@@ +1129,5 @@
> +
> +    // Test for css transform
> +    if (property && property.name === "transform") {
> +      // Look into the rule see if there is an enabled transform-origin prop
> +      let origin;

Won't need to set or pass in this origin variable anymore.
Attachment #8336796 - Flags: review?(bgrinstead)
(Assignee)

Comment 17

4 years ago
Created attachment 8339829 [details] [diff] [review]
bug726427-transform-tooltip V4.patch

New patch containing changes as per review.
Ongoing try build: https://tbpl.mozilla.org/?tree=Try&rev=fa9e712b8e17
Attachment #8336796 - Attachment is obsolete: true
Attachment #8339829 - Flags: review?(bgrinstead)
(Assignee)

Comment 18

4 years ago
Created attachment 8339831 [details] [diff] [review]
bug726427-transform-tooltip V5.patch

Last minute change to simply remove an outdated comment
Attachment #8339829 - Attachment is obsolete: true
Attachment #8339829 - Flags: review?(bgrinstead)
Attachment #8339831 - Flags: review?(bgrinstead)
Comment on attachment 8339831 [details] [diff] [review]
bug726427-transform-tooltip V5.patch

Review of attachment 8339831 [details] [diff] [review]:
-----------------------------------------------------------------

Would you mind rebasing this patch?  It's failing to apply on the latest fx-team.
(Assignee)

Comment 20

4 years ago
Created attachment 8341083 [details] [diff] [review]
bug726427-transform-tooltip V6.patch

Rebased
Attachment #8339831 - Attachment is obsolete: true
Attachment #8339831 - Flags: review?(bgrinstead)
Attachment #8341083 - Flags: review?(bgrinstead)
Comment on attachment 8341083 [details] [diff] [review]
bug726427-transform-tooltip V6.patch

Review of attachment 8341083 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with a new try push.  One thing I noticed, which could be done as a separate bug if you want: If the tooltip is open and you press the down or up arrow to switch nodes in the inspector, the tooltip doesn't disappear until you move the mouse.  It should disappear when the selected node is changed.

::: browser/devtools/shared/widgets/CSSTransformPreviewer.js
@@ +170,5 @@
> +
> +  _getTransformedPoints: function(matrix, rect, origin) {
> +    let transformedRect = [];
> +
> +    for (let i = 0; i < rect.length; i ++) {

Extra whitespace on i ++.  Could also just use rect.map() here.
Attachment #8341083 - Flags: review?(bgrinstead) → review+
(Assignee)

Comment 22

4 years ago
Thanks Brian for the review.

I've filed follow up bug 946331 to fix the issue whereby the tooltip remains visible when you select a new node.

I'll take care of the whitespace and rect.map and attach a new patch (and push to try).
Blocks: 946331
No longer blocks: 946331
(Assignee)

Comment 23

4 years ago
Created attachment 8342937 [details] [diff] [review]
bug726427-transform-tooltip V7.patch

- Updated the code as per Brian's review
- Marking as R+
- Ongoing try build: https://tbpl.mozilla.org/?tree=Try&rev=79dd0a40634d
Attachment #8341083 - Attachment is obsolete: true
Attachment #8342937 - Flags: review+
(Assignee)

Comment 24

4 years ago
Try build is green, R+, asking for check-in!
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/57c170af6a23
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [styleeditor][computedview][ruleview] → [styleeditor][computedview][ruleview][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/57c170af6a23
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [styleeditor][computedview][ruleview][fixed-in-fx-team] → [styleeditor][computedview][ruleview]
Target Milestone: --- → Firefox 28
backed this changeout in https://hg.mozilla.org/mozilla-central/rev/ae2c044c6418 because of introducing the test failure in bug 947126
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 28

4 years ago
Created attachment 8345784 [details] [diff] [review]
bug726427-transform-tooltip V8.patch

Brian: this new patch only changes one of the bc tests a little bit to avoid an intermittent failure on osx 10.6 (see bug 947126).

This will make the test stable, but there is still some kind of problem related to inplace-editor and auto-complete underneath, for which I'll file a separate bug just now.

Ongoing try build : https://tbpl.mozilla.org/?tree=Try&rev=919aed9ef67f
(I'll relaunch many bc test runs as soon as platform builds are done to make sure this is stable)
Attachment #8342937 - Attachment is obsolete: true
Attachment #8345784 - Flags: review?(bgrinstead)
(Assignee)

Comment 29

4 years ago
Actually I don't need to open a new bug for the inplace/autocomplete problem, let's use bug 947126!
Comment on attachment 8345784 [details] [diff] [review]
bug726427-transform-tooltip V8.patch

Review of attachment 8345784 [details] [diff] [review]:
-----------------------------------------------------------------

I've compared the two patches using http://benjamin.smedbergs.us/interdiff/interdiff.php?patch1url=https%3A%2F%2Fbug726427.bugzilla.mozilla.org%2Fattachment.cgi%3Fid%3D8342937&patch2url=https%3A%2F%2Fbug726427.bugzilla.mozilla.org%2Fattachment.cgi%3Fid%3D8345784 and this seems like a better way to add a property from a test (where you aren't wanting to test the adding itself and you just want it to be added).  There is an extra function that should to be removed.  Besides that, let's wait for the try to finish a few more green tests and I'll r+.

::: browser/devtools/styleinspector/test/browser_bug726427_csstransform_tooltip.js
@@ +159,5 @@
> +    cb();
> +  }, tooltip.defaultShowDelay + 100);
> +}
> +
> +function typeKeySequence(sequence, cb, index=0) {

This function can be removed since it isn't used anymore
Attachment #8345784 - Flags: review?(bgrinstead)
(Assignee)

Comment 31

4 years ago
Created attachment 8345892 [details] [diff] [review]
bug726427-transform-tooltip V9.patch

Removed extra function.
Attachment #8345784 - Attachment is obsolete: true
Attachment #8345892 - Flags: review?(bgrinstead)
Comment on attachment 8345892 [details] [diff] [review]
bug726427-transform-tooltip V9.patch

Review of attachment 8345892 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, seems that 10.6 Debug is passing now, though a couple more are queue up so we may wait for those to finish just to be sure.
Attachment #8345892 - Flags: review?(bgrinstead) → review+
(Assignee)

Comment 33

4 years ago
Try build is green enough on osx 10.6 :)
Let's check this in!
(Assignee)

Comment 34

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/cf00b1552a04
Whiteboard: [styleeditor][computedview][ruleview] → [styleeditor][computedview][ruleview][fixed-in-fx-team]
(Assignee)

Updated

4 years ago
Flags: needinfo?(cedricv)
https://hg.mozilla.org/mozilla-central/rev/cf00b1552a04
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Whiteboard: [styleeditor][computedview][ruleview][fixed-in-fx-team] → [styleeditor][computedview][ruleview]
Target Milestone: Firefox 28 → Firefox 29
You need to log in before you can comment on or make changes to this bug.