1275546 - Angle swatch does not appear for .2turn

Status: RESOLVED FIXED

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

Description

[Patrick Brosset <:pbro>]

STR:
- open the inspector on this page in FF>=48
- in the rule-view, enter a new declaration:
  filter: hue-rotate(.2turn);
- notice how there's no angle swatch in front of .2turn
- now change the declaration to:
  filter: hue-rotate(0.2turn);
- notice how now there's an angle swatch

I'm pretty sure this comes from devtools/client/shared/css-angle.js.
In fact, doing this:

let {angleUtils} = require("devtools/client/shared/css-angle");
let angle = new angleUtils.CssAngle("0.2turn");
angle.valid

returns true, while this:

let {angleUtils} = require("devtools/client/shared/css-angle");
let angle = new angleUtils.CssAngle(".2turn");
angle.valid

returns false.

We should accept floats without preceding 0.

Marking this as a good first bug because I'm pretty sure it is. Also marking Nicolas as a mentor since he coded that part.
Nicolas, feel free to remove yourself from the mentor field if you don't want/can't mentor for any reason. Happy to do it instead.

Comment 1

[Patrick Brosset <:pbro>]

Attachment: Capture.PNG

This shows the bug.

Comment 2

[Patrick Brosset <:pbro>]

Inspector bug triage (filter on CLIMBING SHOES).

Comment 3

[Akshat Kedia [:aks]]

Hi Patrick,

I can reproduce the bug. I am interested in taking this up. 
I have identified a regular expression which is defined to test the validity of CssAngle. 

Here is the function:
get valid() {
  return /^-?\d+\.?\d*(deg|rad|grad|turn)$/gi.test(this.authored);
}

Is tried solving the bug by updating the RegEx as:
get valid() {
  return /^-?\d*\.?\d*(deg|rad|grad|turn)$/gi.test(this.authored);
}

But no luck. Am I looking at a wrong solution ?

Comment 4

[Tom Tromey :tromey]

Note that -?\d*\.?\d* matches the empty string, which is probably not what you intend.

I think it may be better to use the CSS lexer to lex the authored text.
Then you can test that (1) there is a single returned token, (2) the token is
a "dimension", and finally (3) that the returned unit is one of the expected
angle dimensions.

The benefit of this approach is that you don't have to handle all the ways a
number can be written.  E.g., this also currently doesn't handle "1e2" notation,
or "+1".

Comment 5

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

Hi Akshat, sorry for the delay.

So, you can follow Tom advice.
CSS Lexer is defined in `devtools/shared/css-lexer.js`.
You can import the `getCSSLexer`function in the css-angle file with `const {getCSSLexer} = require("devtools/shared/css-lexer");` so you can use it.

You can have a look at `devtools/client/shared/css-color.js` to see how to use the lexer.

If you need any informations, please ask me, I'll try to reply quicker the next time.

Comment 6

[Tom Tromey :tromey]

FWIW the css lexer contract is spelled out here:

https://dxr.mozilla.org/mozilla-central/source/dom/webidl/CSSLexer.webidl

Comment 7

[Akshat Kedia [:aks]]

Thanks Tom for the help.

Hey Nicolas,

This is my first time using a lexer. I have been able to understand bits and pieces from css-color.js code on how to use the lexer but if there a way for me to log tokens to the console in something similar to this http://tools.w3clubs.com/cssex/foreplay.html it will be really helpful for me to understand what is the lexer doing.
Also is there any JavaScript debugging documentation at Mozilla, I should read through ?

Comment 8

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

Hi Akshat,

So, you can add console.log in your code, and it will displays in the jsconsole.
In order to display the jsconsole at start, you can do it with `./mach run -P dev --jsconsole` .
With the jsconsole you have a basic way to debug your code, but you can also inspect and debut the devtools, like a regular website, with the browser toolbox. https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox tells what you need to do to activate the browser toolbox and give you some tips to use it.

Good luck :)

Comment 9

[Sebastin Santy [:seban]]

Attachment: bug1275546.diff

The regex expressions in css-angles.js and output-parser.js have been changed to accommodate ".2turn" also with "0.2turn" type of expressions. Also empty digit case has been handled like "turn" expression won't showup angle-swatch. Can someone please review this.

Comment 10

[Sebastin Santy [:seban]]

Attachment: bug1275546.diff

Updated with the new patch: Also cases like "0.turn" would be avoided.

Comment 11

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

Comment on attachment 8766912 [details] [diff] [review]
bug1275546.diff

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

Thanks for working on this. 
I think we shouldn't try to use a regex anymore and switch to the lexer.
The main reason is that the regex is now a little hard to read, and it doesn't handle value like `5e-1`.
We could handle that one with the regex too, but it will get complex.
Using the lexer would allow us to not parse the value and let the lexer do the hard job for us.
We would only have to check what the lexer gives us like Tom said in comment 4.

Comment 12

[Sebastin Santy [:seban]]

Attachment: Angle Swatch using CSS Lexer

Hi Nicholas, Removed regex and used lexer for angle-swatch. Hence, can support 1e2 and +1 notation.

Comment 13

[Tom Tromey :tromey]

Comment on attachment 8767063 [details] [diff] [review]
Angle Swatch using CSS Lexer

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

Just a drive-by review, as I have some interest in CSS parsing.

::: devtools/client/shared/css-angle.js
@@ +66,5 @@
>    },
>  
>    get valid() {
> +     let token = getCSSLexer(this.authored).nextToken();
> +       if (token.tokenType === "dimension") {

This should probably also ensure that there is no next token.

@@ +69,5 @@
> +     let token = getCSSLexer(this.authored).nextToken();
> +       if (token.tokenType === "dimension") {
> +        return true;
> +      }
> +      else

Formatting looks off, try mach eslint.

Comment 14

[Sebastin Santy [:seban]]

Attachment: bug1275546_angleswatch_usinglexer.diff

Thanks Tom for the suggestions, I have changed the code accordingly.

Update to the previous patch: Formatted the code and added a check for nextToken. The previous code could also accept values like "0.2aaturn" which has been fixed with this patch. Please, can someone review this.

Comment 15

[Ian Moody [:Kwan] (UTC+1)]

Comment on attachment 8767300 [details] [diff] [review]
bug1275546_angleswatch_usinglexer.diff

Another drive-by review: output-parser already includes angleUtils, so it'd be easy to call that rather than duplicating the code.

Also I think tromey meant that you should check that calling nextToken() a second time returns nothing (that there is nothing after the angle).

Comment 16

[Sebastin Santy [:seban]]

Thanks Ian for the review.

(In reply to Ian Moody [:Kwan] from comment #15)
> Comment on attachment 8767300 [details] [diff] [review]
> bug1275546_angleswatch_usinglexer.diff
> 
> Another drive-by review: output-parser already includes angleUtils, so it'd
> be easy to call that rather than duplicating the code.

I'll try to do that, but I guess this would be fine, amounting to such a small code.

> Also I think tromey meant that you should check that calling nextToken() a
> second time returns nothing (that there is nothing after the angle).

I tried calling nextToken()the second time (like token.nextToken()), but it was giving some errors. I had already discussed this error with Tom.

Comment 17

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

Comment on attachment 8767300 [details] [diff] [review]
bug1275546_angleswatch_usinglexer.diff

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

Thinks look good seban, thanks :)

I have some minor comments about your patch that you should fix before I can r+ you. If you can, just amend your commit with the fixes.
Also, it would be nice to add some tests in devtools/client/shared/test/browser_css_angle.js , with both correct (.2deg, -.2turn, 1e02rad, -2e2turn) and incorrect (..2deg, ...) syntaxes.

::: devtools/client/shared/css-angle.js
@@ +72,5 @@
> +    let angleval = ["turn", "deg", "grad", "rad"];
> +    if (!token) {
> +      return false;
> +    }
> +    return (token.tokenType === "dimension" && angleval.indexOf(token.text) > -1);

we could use the CssAngle.ANGLEUNIT constant instead of duplicating it in angleval.
And we also can use array.includes instead of indexOf :


`
return (
  token.tokenType === "dimension" &&
  Object.keys(CssAngle.ANGLEUNIT).includes(token.text));
`

::: devtools/client/shared/output-parser.js
@@ +156,4 @@
>      };
>  
>      let angleOK = function (angle) {
> +      let token = getCSSLexer(angle).nextToken();

I tend to agree with Kwan here, we could simply create a new angle and check if it's valid.

return (new angleUtils.CssAngle(angle)).valid;

The code looks simple for now, but it may be more complex in the future, if a new unit is introduced for example (unlikely, but who knows)

Comment 18

[Sebastin Santy [:seban]]

Attachment: bug1275546_angleswatch_usinglexer.diff

Added tests and refactored code.

Comment 19

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

Comment on attachment 8767652 [details] [diff] [review]
bug1275546_angleswatch_usinglexer.diff

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

I made some minor comments about your patch, after you fix them we should be good to push to TRY !

::: devtools/client/shared/test/browser_css_angle.js
@@ +36,5 @@
>    }
>  }
>  
> +function testAngleValidity() {
> +  let data = getAngleData();

Maybe we can rename it to getAngleValidityData in order to be more explicit ?

@@ +39,5 @@
> +function testAngleValidity() {
> +  let data = getAngleData();
> +
> +  for (let {angle, result} of data) {
> +    let test_angle = new angleUtils.CssAngle(angle);

Please, have a look at https://wiki.mozilla.org/DevTools/CodingStandards .
On the project, we use camelCased variable name, and ESlint will fail on your code.
You can configure ESlint on your local repo by doing `mach eslint --setup`and then call it on a file with `mach eslint /path/to/file`, or you can even have a linter in your editor to see eslint errors automatically.

@@ +41,5 @@
> +
> +  for (let {angle, result} of data) {
> +    let test_angle = new angleUtils.CssAngle(angle);
> +
> +    is(test_angle.valid, result, "Testing random input values");

We could have a better test description, something like : 

`Testing that "${angle}" is ${test_angle.valid ? " a valid" : "an invalid" } angle`

@@ +60,5 @@
>    is(angle.toString(), turn, "toString() with turn type");
>  }
>  
> +function getAngleData() {
> +  return [{

nit: having cases grouped by validity (first valid ones, and then the invalid ) maybe ? it's up to you.

Comment 20

[Sebastin Santy [:seban]]

Attachment: bug1275546_angleswatch_usinglexer.diff

Nicholas, Thanks for the review. Made the changes.

Comment 21

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

Comment on attachment 8768490 [details] [diff] [review]
bug1275546_angleswatch_usinglexer.diff

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

Looks good to me, thanks !
I edited the commit message and pushed to TRY : https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6cc10e8a338
If it's green, we should be able to land this :)

Comment 22

[Sebastin Santy [:seban]]

Thanks for the review!

Comment 23

[Carsten Book [:Tomcat]]

will do the checkin. Sebastian can you make sure you include a complete emailadress next time in the commit message :) Thanks!

Comment 24

[Sebastin Santy [:seban]]

Thanks Carsten! I'll include the email in the commit message from next time. But won't that be long?

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/68be1552a5ee
Angle swatch does not appear for .2turn. r=nchevobbe

Comment 26

[Carsten Book [:Tomcat]]

(In reply to Sebastin Santy [:seban] from comment #24)
> Thanks Carsten! I'll include the email in the commit message from next time.
> But won't that be long?

no thats fine, in hg commit message and user name are separate :) looks like 

author	Sebastin Santy <[e-mail]>
	Wed, 06 Jul 2016 12:01:00 +0200 (44 hours ago)

then :) (i fixed this time in the commit message with adding your complete mail adress just in case you wonder).

Comment 27

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=10376662&repo=fx-team

Comment 28

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/806cd7a7b68d
Backed out changeset 68be1552a5ee for widespread failures in browser_css_angle.js

Comment 29

[Sebastin Santy [:seban]]

Attachment: bug1275546_angleswatch_usinglexer.diff

Hi Carsten,

Very Sorry about that :/. That was because of a small thing I had overlooked. Now the tests are working perfectly fine.

Comment 30

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

Comment on attachment 8769102 [details] [diff] [review]
bug1275546_angleswatch_usinglexer.diff

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

Looks good to me. 
There is some issue with the commit message, it should be "Bug 8769102 - Use CSS lexer to parse angle values in css-angle.js . r=nchevobbe"
Your email address should appear without you doing anything if you configured mercurial with it.

If you have TRY access can you edit the commit message and push your patch ?
If you don't, I'll push it in ~3 hours

Comment 31

[Sebastin Santy [:seban]]

Attachment: bug1275546_angleswatch_usinglexer.diff

Thanks Nicholas for the review. Made the changes.

Comment 32

[Sebastin Santy [:seban]]

Hi Nicholas,

Can you push this to the try server. It'll take me a little time to get a hang of treeherder. Thanks in advance.

Comment 33

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

Hi Sebastin,

I pushed your patch, you can see it here : https://treeherder.mozilla.org/#/jobs?repo=try&revision=588605d99436

Comment 34

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

There's some failures on TRY https://treeherder.mozilla.org/#/jobs?repo=try&revision=588605d99436&selectedJob=23814845 but none seem related to seban's patch.

Comment 35

[Sebastin Santy [:seban]]

Thanks Nicholas for this!

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/935085e3a4f0
Use CSS lexer to parse angle values in css-angle.js . r=nchevobbe

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/935085e3a4f0