Angle swatch does not appear for .2turn

RESOLVED FIXED in Firefox 50

Status

defect
P2
normal
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: pbro, Assigned: seban, Mentored)

Tracking

Trunk
Firefox 50

Firefox Tracking Flags

(firefox49 affected, firefox50 fixed)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(2 attachments, 7 obsolete attachments)

Reporter

Description

3 years ago
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.
Reporter

Comment 1

3 years ago
Posted image Capture.PNG
This shows the bug.
Reporter

Comment 2

3 years ago
Inspector bug triage (filter on CLIMBING SHOES).
Priority: -- → P2
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 ?
Assignee: nobody → akshat.kedia
Flags: needinfo?(chevobbe.nicolas)
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".
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.
Flags: needinfo?(chevobbe.nicolas)
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 ?
Flags: needinfo?(chevobbe.nicolas)
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 :)
Flags: needinfo?(chevobbe.nicolas)
Assignee

Comment 9

3 years ago
Posted patch bug1275546.diff (obsolete) — Splinter Review
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.
Attachment #8766904 - Flags: review?(chevobbe.nicolas)
Assignee

Comment 10

3 years ago
Posted patch bug1275546.diff (obsolete) — Splinter Review
Updated with the new patch: Also cases like "0.turn" would be avoided.
Attachment #8766912 - Flags: review?(chevobbe.nicolas)
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.
Attachment #8766912 - Flags: review?(chevobbe.nicolas) → review-
Attachment #8766904 - Flags: review?(chevobbe.nicolas) → review-
Assignee

Comment 12

3 years ago
Posted patch Angle Swatch using CSS Lexer (obsolete) — Splinter Review
Hi Nicholas, Removed regex and used lexer for angle-swatch. Hence, can support 1e2 and +1 notation.
Attachment #8767063 - Flags: review?(chevobbe.nicolas)
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.
Assignee

Comment 14

3 years ago
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.
Attachment #8767063 - Attachment is obsolete: true
Attachment #8767063 - Flags: review?(chevobbe.nicolas)
Flags: needinfo?(chevobbe.nicolas)
Attachment #8767300 - Flags: review?(chevobbe.nicolas)
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).
Assignee

Comment 16

3 years ago
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 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)
Attachment #8767300 - Flags: review?(chevobbe.nicolas) → review-
Flags: needinfo?(chevobbe.nicolas)
Assignee

Comment 18

3 years ago
Added tests and refactored code.
Attachment #8767652 - Flags: review?(chevobbe.nicolas)
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.
Attachment #8767652 - Flags: review?(chevobbe.nicolas) → review-
Assignee

Comment 20

3 years ago
Nicholas, Thanks for the review. Made the changes.
Attachment #8766904 - Attachment is obsolete: true
Attachment #8766912 - Attachment is obsolete: true
Attachment #8767300 - Attachment is obsolete: true
Attachment #8767652 - Attachment is obsolete: true
Attachment #8768490 - Flags: review?(chevobbe.nicolas)
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 :)
Attachment #8768490 - Flags: review?(chevobbe.nicolas) → review+
Assignee

Comment 22

3 years ago
Thanks for the review!
Assignee

Updated

3 years ago
Keywords: checkin-needed
will do the checkin. Sebastian can you make sure you include a complete emailadress next time in the commit message :) Thanks!
Flags: needinfo?(sebastinssanty)
Assignee

Comment 24

3 years ago
Thanks Carsten! I'll include the email in the commit message from next time. But won't that be long?
Flags: needinfo?(sebastinssanty) → needinfo?(cbook)
Assignee

Updated

3 years ago
Assignee: akshat.kedia → sebastinssanty

Comment 25

3 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/68be1552a5ee
Angle swatch does not appear for .2turn. r=nchevobbe
Keywords: checkin-needed
(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 <sebastinssanty@gmail.com>
	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).
Flags: needinfo?(cbook)
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=10376662&repo=fx-team
Flags: needinfo?(sebastinssanty)

Comment 28

3 years ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/806cd7a7b68d
Backed out changeset 68be1552a5ee for widespread failures in browser_css_angle.js
Assignee

Comment 29

3 years ago
Hi Carsten,

Very Sorry about that :/. That was because of a small thing I had overlooked. Now the tests are working perfectly fine.
Attachment #8768490 - Attachment is obsolete: true
Flags: needinfo?(sebastinssanty)
Attachment #8769102 - Flags: review?(chevobbe.nicolas)
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
Attachment #8769102 - Flags: review?(chevobbe.nicolas) → review+
Assignee

Comment 31

3 years ago
Thanks Nicholas for the review. Made the changes.
Attachment #8769102 - Attachment is obsolete: true
Attachment #8769178 - Flags: review+
Assignee

Comment 32

3 years ago
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.
Flags: needinfo?(chevobbe.nicolas)
Hi Sebastin,

I pushed your patch, you can see it here : https://treeherder.mozilla.org/#/jobs?repo=try&revision=588605d99436
Flags: needinfo?(chevobbe.nicolas)
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.
Keywords: checkin-needed
Assignee

Comment 35

3 years ago
Thanks Nicholas for this!

Comment 36

3 years ago
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/935085e3a4f0
Use CSS lexer to parse angle values in css-angle.js . r=nchevobbe
Keywords: checkin-needed

Comment 37

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/935085e3a4f0
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.