Closed Bug 1250835 Opened 8 years ago Closed 8 years ago

Shift-click on angles should cycle through angle units

Categories

(DevTools :: Inspector: Rules, defect, P3)

defect

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: pbro, Assigned: nchevobbe)

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 1 obsolete file)

In CSS, angles can be expressed using deg, turn, rad and grad.
In the rule-view, wherever an angle is displayed, we should allow shift-clicking it to cycle through these units.

We do this already for colors (hex -> hsl -> rgb).

Note however that the way we do this for colors is shift-click needs to happen on the color swatch next to the color.

In bug 711942 we intend to have an angle editor anyway, and therefore an angle swatch.
Status: NEW → ASSIGNED
Priority: -- → P3
I'm currently working on this and I have some questions : 

Does the swatch has to be dynamic, like the color swatch is, or should it just be an icon ( like for filter and bezier-curve swatch ) ? I have a working dynamic swatch, but I wonder if it's not too small to be really useful. Maybe the fact that there will be a proper editor with Bug 711942 allow us to just have a static icon ? 

Is there a way/ a resource to know in which CSS properties <angle> units can be used ? I'm aware of rotate() and linear-gradient() , but I can't find other properties.

Also, I tried to use cssPropertySupportsType ( https://dxr.mozilla.org/mozilla-central/source/layout/inspector/inIDOMUtils.idl#144 ) with TYPE_ANGLE ( https://dxr.mozilla.org/mozilla-central/source/layout/inspector/inIDOMUtils.idl#137 ), but it returns false when calling it with background or transform. For now I'm just checking that the property value matches \d+\.?\d*(deg|rad|grad|turn) , but this seems hacky to me.


Thanks !
Flags: needinfo?(pbrosset)
(In reply to Nicolas Chevobbe from comment #1)

> Also, I tried to use cssPropertySupportsType (
> https://dxr.mozilla.org/mozilla-central/source/layout/inspector/inIDOMUtils.
> idl#144 ) with TYPE_ANGLE (
> https://dxr.mozilla.org/mozilla-central/source/layout/inspector/inIDOMUtils.
> idl#137 ), but it returns false when calling it with background or
> transform.

This sounds like a bug to me.
It's a bit hard to be completely sure, because (IMO) the cssPropertySupportsType
contract is a bit weird.
If you'd like I can take a look at this.
See bug 1254696 for the inIDOMUtils request.
(In reply to Tom Tromey :tromey from comment #2)
> (In reply to Nicolas Chevobbe from comment #1)
> 
> > Also, I tried to use cssPropertySupportsType (
> > https://dxr.mozilla.org/mozilla-central/source/layout/inspector/inIDOMUtils.
> > idl#144 ) with TYPE_ANGLE (
> > https://dxr.mozilla.org/mozilla-central/source/layout/inspector/inIDOMUtils.
> > idl#137 ), but it returns false when calling it with background or
> > transform.
> 
> This sounds like a bug to me.
> It's a bit hard to be completely sure, because (IMO) the
> cssPropertySupportsType
> contract is a bit weird.
> If you'd like I can take a look at this.

As discussed on IRC, this doesn't really seems like a bug, as the "background" property should not be directly an angle unit, but can be a function ( linear-gradient ), which take an angle as a parameter.
For now, I'll go with an array of angle taking functions ( we already have a color taking functions array in the file ).
(In reply to Nicolas Chevobbe from comment #1)
> Does the swatch has to be dynamic, like the color swatch is, or should it
> just be an icon ( like for filter and bezier-curve swatch ) ? I have a
> working dynamic swatch, but I wonder if it's not too small to be really
> useful. Maybe the fact that there will be a proper editor with Bug 711942
> allow us to just have a static icon ? 
I wasn't sure about what static vs. dynamic meant in this context so I asked Nicolas on IRC. By dynamic, Nicolas means that the swatch icon would update with the angle, so, represent the current angle visually (just like the color swatch changes color). And by static, Nicolas means just a static image icon that looks the same all the time.
I think having a dynamic icon would be awesome, but also hard in cases like 0deg, and probably a nice to have for later (in bug 711942).
For the time being, I'm going to suggest using a static image. Unless we can come up with a design that works with all angles!
Something simple like this: http://jsbin.com/kolojotibo/edit?html,css,output would have problems if you tried to represent 0 or 360 degrees for instance.
> Is there a way/ a resource to know in which CSS properties <angle> units can
> be used ? I'm aware of rotate() and linear-gradient() , but I can't find
> other properties.
Hmm, I can't find anything in the spec that lists all properties that can be expressed with angles. I can think of rotate, skew, linear-gradient. You could argue that the hue in a hsl() color is an angle too, but it doesn't have a unit.
There's the polar-angle property but that's not implemented afaik.
Flags: needinfo?(pbrosset)
Attached image Proposal for angle swatch icon (obsolete) —
Attachment #8728815 - Flags: feedback?(pbrosset)
Comment on attachment 8728815 [details]
Proposal for angle swatch icon

Looks good to me.
And, yay, today I learned about image-orientation. I had no idea.
Attachment #8728815 - Flags: feedback?(pbrosset) → feedback+
> And, yay, today I learned about image-orientation. I had no idea.

Did not knew about it either,It will be a little challenging for the angle-editor bug to deal with it as we should provide a way to visualize how the value is rounded. ( if the user set the property to 20deg, we should show both 20deg AND 90deg value it is rounded to )
(In reply to Nicolas Chevobbe from comment #8)
> It will be a little challenging for the
> angle-editor bug to deal with it as we should provide a way to visualize how
> the value is rounded. ( if the user set the property to 20deg, we should
> show both 20deg AND 90deg value it is rounded to )
Well, knowing that this property only works on FF right now, and even there is probably not used a whole lot, I wouldn't worry about doing this for a while.
Add a swatch before angle values in the rules panel and allow cycling
through angle units with shift+click (like we already do for color units).

Review commit: https://reviewboard.mozilla.org/r/41387/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41387/
Attachment #8732819 - Flags: review?(ttromey)
Attached image angle_cycling.gif
shift + click in action
Attachment #8728815 - Attachment is obsolete: true
Attachment #8732819 - Attachment description: MozReview Request: Bug 1250835 - Display swatch for angles in the rules panel. r=tromey → MozReview Request: Bug 1250835 - Display swatch for angles in the rules panel. r=miker
Attachment #8732819 - Flags: review?(ttromey) → review?(mratcliffe)
Comment on attachment 8732819 [details]
MozReview Request: Bug 1250835 - Display swatch for angles in the rules panel. r=miker

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41387/diff/1-2/
Comment on attachment 8732819 [details]
MozReview Request: Bug 1250835 - Display swatch for angles in the rules panel. r=miker

https://reviewboard.mozilla.org/r/41387/#review38449

Really great job... I wrote the CSS color stuff so this was an easy review for me.

r+ assuming a green try.

::: devtools/client/shared/output-parser.js:195
(Diff revision 2)
>        }
>  
>        switch (token.tokenType) {
>          case "function": {
> -          if (COLOR_TAKING_FUNCTIONS.indexOf(token.text) >= 0) {
> +          if (COLOR_TAKING_FUNCTIONS.indexOf(token.text) >= 0 ||
> +              ANGLE_TAKING_FUNCTIONS.indexOf(token.text) >= 0) {

Nit: We *could* use:
```
if (COLOR_TAKING_FUNCTIONS.contains(token.text) ||
    ANGLE_TAKING_FUNCTIONS.contains(token.text)) {
```
Attachment #8732819 - Flags: review?(mratcliffe) → review+
https://reviewboard.mozilla.org/r/41387/#review38449

> Nit: We *could* use:
> ```
> if (COLOR_TAKING_FUNCTIONS.contains(token.text) ||
>     ANGLE_TAKING_FUNCTIONS.contains(token.text)) {
> ```

I meant:
if (COLOR_TAKING_FUNCTIONS.includes(token.text) ||
    ANGLE_TAKING_FUNCTIONS.includes(token.text)) {
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #14)
> https://reviewboard.mozilla.org/r/41387/#review38449
> 
> > Nit: We *could* use:
> > ```
> > if (COLOR_TAKING_FUNCTIONS.contains(token.text) ||
> >     ANGLE_TAKING_FUNCTIONS.contains(token.text)) {
> > ```
> 
> I meant:
> if (COLOR_TAKING_FUNCTIONS.includes(token.text) ||
>     ANGLE_TAKING_FUNCTIONS.includes(token.text)) {

Thanks MooTools ;)
Comment on attachment 8732819 [details]
MozReview Request: Bug 1250835 - Display swatch for angles in the rules panel. r=miker

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41387/diff/2-3/
Attachment #8732819 - Flags: review+
Comment on attachment 8732819 [details]
MozReview Request: Bug 1250835 - Display swatch for angles in the rules panel. r=miker

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41387/diff/3-4/
Attachment #8732819 - Flags: review?(mratcliffe)
There were some things I was doing wrong, and tests failed.
I fixed my patch and re-triggered TRY jobs.
So I guess you should re-review this again, right ?
Flags: needinfo?(mratcliffe)
Try jobs are over and everything seems fine ( except https://treeherder.mozilla.org/#/jobs?repo=try&revision=b33be30050db&selectedJob=18502209 , but that don't seems related to my patch ).
Comment on attachment 8732819 [details]
MozReview Request: Bug 1250835 - Display swatch for angles in the rules panel. r=miker

(In reply to Nicolas Chevobbe from comment #18)
> There were some things I was doing wrong, and tests failed.
> I fixed my patch and re-triggered TRY jobs.
> So I guess you should re-review this again, right ?

Yeah, looks better now.
Flags: needinfo?(mratcliffe)
Attachment #8732819 - Flags: review?(mratcliffe) → review+
https://hg.mozilla.org/mozilla-central/rev/7356a69124c9
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.