Fix issue 325 by adding algo for equals() #412

nainar · 2017-06-05T23:15:47Z

@shans could you take a look, took a stab at writing up the algo for .equals()

shans · 2017-06-06T16:11:11Z

css-typed-om/Overview.bs

+    when called on a {{CSSNumericValue}} |this|,
+    must perform the following steps:
+
+    1. If the |value| item is the same type as the {{CSSNumericValue}} the method is being called on continue to step 2, else return false.


You don't need to continue on to step 2 as this happens automatically. Maybe rewrite as 'If |value| is of different type to |this|, return false.'

Also we need to define what 'different type' means.

shans · 2017-06-06T16:12:00Z

css-typed-om/Overview.bs

+    The <dfn for=CSSNumericValue method>equals(...|value|)</dfn> method,
+    when called on a {{CSSNumericValue}} |this|,
+    must perform the following steps:
+


Will need to either explicitly iterate over |value| (change to values?) or just restrict this to take a single value.

And by "or" Shane means "do the first thing", because all the other methods take multiple values too.

Had done the first thing. Commit 3 should make equals accept an array.

shans · 2017-06-06T16:13:04Z

css-typed-om/Overview.bs

+
+    1. If the |value| item is the same type as the {{CSSNumericValue}} the method is being called on continue to step 2, else return false.
+
+    2. If the |value| item has the same type as the {{CSSNumericValue}} continue to the next step, else return false.


Isn't this the same as step 1?

This was supposed to be class.

shans · 2017-06-06T16:13:18Z

css-typed-om/Overview.bs

+    2. If the |value| item has the same type as the {{CSSNumericValue}} continue to the next step, else return false.
+
+    3. If the |value| item has the same unit as the {{CSSNumericValue}} return true, else return false.
+


Also need to handle calc values, etc.

Done in commit 3

tabatkins · 2017-06-06T17:04:23Z

css-typed-om/Overview.bs

-<div class=issue>
-    Define {{CSSNumericValue/equals()}}.
+<div algorithm="CSSNumericValue.equals(...|value|)">
+    The <dfn for=CSSNumericValue method>equals(...|value|)</dfn> method,


Note that .equals() is variadic; you have to check that all the values are equal to each other/this.

tabatkins · 2017-06-06T17:07:32Z

css-typed-om/Overview.bs

+    when called on a {{CSSNumericValue}} |this|,
+    must perform the following steps:
+
+    1. If the |value| item is the same type as the {{CSSNumericValue}} the method is being called on continue to step 2, else return false.


Just reverse this conditional and return false. No need to explicitly say to go to the next step; that's implied by the list.

Be clear what you mean by "type" - you use the exact same word in the second step, but clearly mean something different. I suspect here you're talking about the interface type.

"the CSSNumericValue the method is being called on" is already stored in the |this| variable, so you just refer to it that way.

Done in commit 3

tabatkins · 2017-06-06T17:07:43Z

css-typed-om/Overview.bs

+
+    1. If the |value| item is the same type as the {{CSSNumericValue}} the method is being called on continue to step 2, else return false.
+
+    2. If the |value| item has the same type as the {{CSSNumericValue}} continue to the next step, else return false.


Same concerns as about the structure; reverse the conditional and just return false.

I think you mean "value" here? Refer to |this|’s {{CSSUnitValue/value}} internal slot then (and same for |value|).

Done in commit 3

tabatkins · 2017-06-06T17:10:08Z

css-typed-om/Overview.bs

+
+    2. If the |value| item has the same type as the {{CSSNumericValue}} continue to the next step, else return false.
+
+    3. If the |value| item has the same unit as the {{CSSNumericValue}} return true, else return false.


Use {{CSSUnitValue/unit}} internal slot.

Done in commit 3

tabatkins · 2017-06-06T17:18:58Z

css-typed-om/Overview.bs

+<div algorithm="CSSNumericValue.equals(...|value|)">
+    The <dfn for=CSSNumericValue method>equals(...|value|)</dfn> method,
+    when called on a {{CSSNumericValue}} |this|,
+    must perform the following steps:


The algo claims to be defined for CSSNumericValue, but the steps assume that you're looking at a CSSUnitValue. It needs to work for all the CSSMathValue subtypes too.

Done in commit 3

tabatkins · 2017-06-06T17:22:46Z

Whoops, I still had this page up from earlier, and it hadn't updated with Shane's comments. Sorry for doubling up. ^_^

nainar · 2017-06-06T18:45:51Z

I suck at Github but I have added a new commit. Hopefully that addresses both @shans and @tabatkins comments.

nainar

Commit 3 makes equals accept an array of values instead of just one value. Take a look?

nainar · 2017-06-06T19:05:13Z

css-typed-om/Overview.bs

+<div algorithm="CSSNumericValue.equals(...|value|)">
+    The <dfn for=CSSNumericValue method>equals(...|value|)</dfn> method,
+    when called on a {{CSSNumericValue}} |this|,
+    must perform the following steps:


Done in commit 3

nainar · 2017-06-06T19:05:37Z

css-typed-om/Overview.bs

+    The <dfn for=CSSNumericValue method>equals(...|value|)</dfn> method,
+    when called on a {{CSSNumericValue}} |this|,
+    must perform the following steps:
+


Had done the first thing. Commit 3 should make equals accept an array.

nainar · 2017-06-06T19:05:43Z

css-typed-om/Overview.bs

+    2. If the |value| item has the same type as the {{CSSNumericValue}} continue to the next step, else return false.
+
+    3. If the |value| item has the same unit as the {{CSSNumericValue}} return true, else return false.
+


Done in commit 3

nainar · 2017-06-06T19:06:08Z

css-typed-om/Overview.bs

+    when called on a {{CSSNumericValue}} |this|,
+    must perform the following steps:
+
+    1. If the |value| item is the same type as the {{CSSNumericValue}} the method is being called on continue to step 2, else return false.


Done in commit 3

nainar · 2017-06-06T19:06:11Z

css-typed-om/Overview.bs

+
+    1. If the |value| item is the same type as the {{CSSNumericValue}} the method is being called on continue to step 2, else return false.
+
+    2. If the |value| item has the same type as the {{CSSNumericValue}} continue to the next step, else return false.


Done in commit 3

nainar · 2017-06-06T19:06:21Z

css-typed-om/Overview.bs

+
+    2. If the |value| item has the same type as the {{CSSNumericValue}} continue to the next step, else return false.
+
+    3. If the |value| item has the same unit as the {{CSSNumericValue}} return true, else return false.


Done in commit 3

tabatkins

Looking much better overall, just some nits left.

tabatkins · 2017-06-06T19:56:37Z

css-typed-om/Overview.bs

+
+    1. [=list/If all=] |item|s of |thisAndValues| do not belong to the same class return false. Example: all items in |thisAndValues| must be {{CSSUnitValue}}.
+
+    2. [=list/If all=] |item|s in |thisAndValues| are {{CSSUnitValue}s and have the same {{CSSUnitValue/value}} return true.


unit and value ^_^

tabatkins · 2017-06-06T19:56:45Z

css-typed-om/Overview.bs

+
+    2. [=list/If all=] |item|s in |thisAndValues| are {{CSSUnitValue}s and have the same {{CSSUnitValue/value}} return true.
+
+    4. =list/If all=] |item|s in |thisAndValues| are {{CSSMathValue}s :


Missing [

tabatkins · 2017-06-06T19:57:16Z

css-typed-om/Overview.bs

+    2. [=list/If all=] |item|s in |thisAndValues| are {{CSSUnitValue}s and have the same {{CSSUnitValue/value}} return true.
+
+    4. =list/If all=] |item|s in |thisAndValues| are {{CSSMathValue}s :
+        1. If the {{CSSMathValue/operator}} are not equal for all {{CSSMathValues}} in |thisAndValues| return false.


This is already covered by "same class" earlier, so you can drop this.

tabatkins · 2017-06-06T19:58:18Z

css-typed-om/Overview.bs

+        1. If the {{CSSMathValue/operator}} are not equal for all {{CSSMathValues}} in |thisAndValues| return false.
+        2. If |thisAndValues| are all either {{CSSMathSum}}, {{CSSMathProduct}}, {{CSSMathMin}} or {{CSSMathMax}}, iterate over the {{CSSNumericArray}}s on |thisAndValues|,
+        	1. For each {{CSSUnitValue}} in each of the {{CSSNumericArray}}s go back to step 1 to check that the {{CSSUnitValue}}s on all items in |thisAndValue| are equal.
+    	3. If |this| and |value| are both either {{CSSMathNegate}} or {{CSSMathInvert}}:


Need to handle multiples.

Do you mean CSSMathProduct? Cause that is dealt with in the above step. Or do you mean the case where values in |thisAndValues| are not all of the same type? If the latter in that case we should return a false right?

nainar · 2017-06-09T23:37:15Z

@tabatkins took a stab at equals take a look?

nainar

@tabatkins made the changes and had a follow up question.

nainar · 2017-06-13T09:35:07Z

css-typed-om/Overview.bs

+
+    1. [=list/If all=] |item|s of |thisAndValues| do not belong to the same class return false. Example: all items in |thisAndValues| must be {{CSSUnitValue}}.
+
+    2. [=list/If all=] |item|s in |thisAndValues| are {{CSSUnitValue}s and have the same {{CSSUnitValue/value}} return true.


nainar · 2017-06-13T09:35:16Z

css-typed-om/Overview.bs

+
+    2. [=list/If all=] |item|s in |thisAndValues| are {{CSSUnitValue}s and have the same {{CSSUnitValue/value}} return true.
+
+    4. =list/If all=] |item|s in |thisAndValues| are {{CSSMathValue}s :


nainar · 2017-06-13T09:35:42Z

css-typed-om/Overview.bs

+    2. [=list/If all=] |item|s in |thisAndValues| are {{CSSUnitValue}s and have the same {{CSSUnitValue/value}} return true.
+
+    4. =list/If all=] |item|s in |thisAndValues| are {{CSSMathValue}s :
+        1. If the {{CSSMathValue/operator}} are not equal for all {{CSSMathValues}} in |thisAndValues| return false.


nainar · 2017-06-13T09:38:59Z

css-typed-om/Overview.bs

+        1. If the {{CSSMathValue/operator}} are not equal for all {{CSSMathValues}} in |thisAndValues| return false.
+        2. If |thisAndValues| are all either {{CSSMathSum}}, {{CSSMathProduct}}, {{CSSMathMin}} or {{CSSMathMax}}, iterate over the {{CSSNumericArray}}s on |thisAndValues|,
+        	1. For each {{CSSUnitValue}} in each of the {{CSSNumericArray}}s go back to step 1 to check that the {{CSSUnitValue}}s on all items in |thisAndValue| are equal.
+    	3. If |this| and |value| are both either {{CSSMathNegate}} or {{CSSMathInvert}}:


Do you mean CSSMathProduct? Cause that is dealt with in the above step. Or do you mean the case where values in |thisAndValues| are not all of the same type? If the latter in that case we should return a false right?

Gonna go ahead and merge and fix things up myself.

Fix issue 325

0d7cedb

shans reviewed Jun 6, 2017

View reviewed changes

tabatkins requested changes Jun 6, 2017

View reviewed changes

CSSNumericValue.equals rewrite

2218b84

Change equals to accept an array of values and not just one value.

5460457

nainar commented Jun 6, 2017

View reviewed changes

tabatkins previously requested changes Jun 6, 2017

View reviewed changes

Add algo for equals()

ca93cbd

nainar changed the title ~~Fix issue 325~~ Fix issue 325 by adding algo for equals() Jun 9, 2017

fixing nits in the code. Removing superflous cases.

d65004c

nainar commented Jun 13, 2017

View reviewed changes

formatting nit

1a6d3f7

Merge branch 'master' into issue325

1b7c546

tabatkins merged commit 1d6f0a6 into w3c:master Jun 30, 2017

nainar mentioned this pull request Jul 25, 2017

[css-typed-om] .equals methods for CSSStyleValue subclasses #325

Closed


		1. If the \|value\| item is the same type as the {{CSSNumericValue}} the method is being called on continue to step 2, else return false.

		2. If the \|value\| item has the same type as the {{CSSNumericValue}} continue to the next step, else return false.

		2. If the \|value\| item has the same type as the {{CSSNumericValue}} continue to the next step, else return false.

		3. If the \|value\| item has the same unit as the {{CSSNumericValue}} return true, else return false.


		1. [=list/If all=] \|item\|s of \|thisAndValues\| do not belong to the same class return false. Example: all items in \|thisAndValues\| must be {{CSSUnitValue}}.

		2. [=list/If all=] \|item\|s in \|thisAndValues\| are {{CSSUnitValue}s and have the same {{CSSUnitValue/value}} return true.


		2. [=list/If all=] \|item\|s in \|thisAndValues\| are {{CSSUnitValue}s and have the same {{CSSUnitValue/value}} return true.

		4. =list/If all=] \|item\|s in \|thisAndValues\| are {{CSSMathValue}s :

Fix issue 325 by adding algo for equals() #412

Fix issue 325 by adding algo for equals() #412

Uh oh!

Conversation

nainar commented Jun 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tabatkins commented Jun 6, 2017

Uh oh!

nainar commented Jun 6, 2017

Uh oh!

nainar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tabatkins left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!