Closed Bug 726427 Opened 12 years ago Closed 10 years ago

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

Categories

(DevTools :: Style Editor, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 29

People

(Reporter: cedricv, Assigned: pbro)

References

(Blocks 1 open bug)

Details

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

Attachments

(4 files, 8 obsolete files)

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

eg. for a scale transform :

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

 /\
 this is a square
Not sure to understand. Can you elaborate?
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
Depends on: 917755
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: nobody → pbrosset
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.
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)
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)
Try failed. Will fix and attach the new patch soon,
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)
Demonstration of similar transforms from Comment 9
Attached video csstransform-jump.mp4
Demonstration of tooltip height changing and jumping from Comment 9
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?
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)
Green try build
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)
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)
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.
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+
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
- 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+
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
Closed: 11 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 → ---
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)
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)
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+
Try build is green enough on osx 10.6 :)
Let's check this in!
https://hg.mozilla.org/integration/fx-team/rev/cf00b1552a04
Whiteboard: [styleeditor][computedview][ruleview] → [styleeditor][computedview][ruleview][fixed-in-fx-team]
Flags: needinfo?(cedricv)
https://hg.mozilla.org/mozilla-central/rev/cf00b1552a04
Status: REOPENED → RESOLVED
Closed: 11 years ago10 years ago
Resolution: --- → FIXED
Whiteboard: [styleeditor][computedview][ruleview][fixed-in-fx-team] → [styleeditor][computedview][ruleview]
Target Milestone: Firefox 28 → Firefox 29
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: