Shift-click on angles should cycle through angle units

RESOLVED FIXED in Firefox 48

Status

()

Firefox
Developer Tools: CSS Rules Inspector
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: pbro, Assigned: nchevobbe)

Tracking

({dev-doc-needed})

unspecified
Firefox 48
dev-doc-needed
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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.
(Reporter)

Updated

2 years ago
Status: NEW → ASSIGNED
Priority: -- → P3
(Assignee)

Comment 1

2 years ago
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)

Comment 2

2 years ago
(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.

Comment 3

2 years ago
See bug 1254696 for the inIDOMUtils request.
(Assignee)

Comment 4

2 years ago
(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 ).
(Reporter)

Comment 5

2 years ago
(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)
(Assignee)

Comment 6

2 years ago
Created attachment 8728815 [details]
Proposal for angle swatch icon
Attachment #8728815 - Flags: feedback?(pbrosset)
(Reporter)

Comment 7

2 years ago
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+
(Assignee)

Comment 8

2 years ago
> 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 )
(Reporter)

Comment 9

2 years ago
(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.
(Assignee)

Comment 10

2 years ago
Created attachment 8732819 [details]
MozReview Request: Bug 1250835 - Display swatch for angles in the rules panel. r=miker

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)
(Assignee)

Comment 11

2 years ago
Created attachment 8732820 [details]
angle_cycling.gif

shift + click in action
Attachment #8728815 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
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)
(Assignee)

Comment 12

2 years ago
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)) {
(Assignee)

Comment 15

2 years ago
(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 ;)
(Assignee)

Comment 16

2 years ago
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/
(Assignee)

Updated

2 years ago
Attachment #8732819 - Flags: review+
(Assignee)

Comment 17

2 years ago
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)
(Assignee)

Comment 18

2 years ago
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)
(Assignee)

Comment 19

2 years ago
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+
Keywords: checkin-needed

Comment 21

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/7356a69124c9
Keywords: checkin-needed

Comment 22

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7356a69124c9
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.