Fix parser functions

[asamuzaK]

• TYPES values ​​changed to allow bitwise operations.
• Fixed valueType() function.
• Other parser functions have been modified to match the changes to the valueType() function.
• Camelized variables (only in lib/parsers.js).

This should fix #189, closes #190

[domenic]

Thanks as always!

I'm happy to continue merging with only light review, as you are quickly becoming the person most knowledgeable about this package. However, as before, can you help provide more of an overview of what this PR accomplishes?

First, does this have any impact on observable behavior in jsdom? Or does it only impact the "internal" functions like parsers.parseXYZ()? (Some of the changes to add more system color keywords seem like they might impact jsdom?)

If it is only for the internal functions, why are these changes improvements? What was wrong with the previous behavior, and what does the new behavior help with?

I'm particularly a bit surprised by the introduction of new types UNDEFINED and NULL_OR_EMPTY_STR, as those don't seem like concepts from any CSS specs. What do they help with?

I'm also unsure why we would want the type to be compatible with bitwise operations. Does this mean values can have multiple types? What is an example of when that would be useful?

[asamuzaK]

First, does this have any impact on observable behavior in jsdom?

It does have some impacts.

• Fixed div.style.backgroundColor = undefined to not to throw. Trying to parse undefined will cause error in cssColor dependency's resolveColor #189
• Fixed css calc() handling, e.g. calc(2 / 3) will be serialized to calc(0.666667).
• Since css-color does not handle system colors, added them as TYPES.COLOR.
• Some parseXYZ() functions missed var() handler, so added them.

I'm particularly a bit surprised by the introduction of new types UNDEFINED and NULL_OR_EMPTY_STR, as those don't seem like concepts from any CSS specs. What do they help with?

I think the TYPES.FOO constants were used in place of tokens, e.g. <length>, <percent>, <string> etc.
However, they are meant for internal use, so not fully spec-compatible.

NULL_OR_EMPTY_STR has been used before (it was previously 10, now 1).

One of the new constant added this time is UNDEFINED.
Previously, undefined was returned undefined directly, but since TYPES.FOO are numeric constant values, I made UNDEFINED to return 0 for consistency.

Another new constant is UNIDENT.
We don't need to parse all types every time, so added the type which indicates unidentified.

And since they are numeric constant values, I thought it would be better to take advantage of that, so decided to use bitwise.

I believe this change has at least two benefits that lead to improved readability:

• Requires fewer characters.

  if (type === TYPES.FOO || type === TYPES.BAR || type === TYPES.BAZ) {
    // do something
  }
  

  vs

  if (type & (TYPES.FOO | TYPES.BAR | TYPES.BAZ)) {
    // do something
  }
  

• Flexible expression possible with fewer nested parens.

  if ((type === TYPES.FOO || type === TYPES.BAR || type === TYPES.BAZ) ||
      ((type === TYPES.QUX || type === QUUX) && parseFloat(val) === 0)) {
    // do something
  }
  

  vs

  if (type & (TYPES.FOO | TYPES.BAR | TYPES.BAZ) ||
      (type & (TYPES.QUX | TYPES.QUUX) && parseFloat(val) === 0)) {
    // do something
  }
  

[domenic]

Thanks for explaining!

• Fixed div.style.backgroundColor = undefined to not to throw.

The better fix for this would be to update the setter code to have Web IDL-conformant behavior: convert undefined (and null) to empty string at the boundary. I don't think undefined or null should ever reach the internals of the library.

And since they are numeric constant values, I thought it would be better to take advantage of that, so decided to use bitwise.

In my opinion, if you don't need to return multiple types at once, then using a bitwise pattern reduces readability, by requiring the reader to be familiar with bitwise operations and what they mean. It is fewer characters, but those characters are harder to understand. It also will be confusing for people who are used to bitwise enums meaning that a value can have multiple types at once. Basically, this usage of bitwise enums is very nonstandard.

So I'd prefer that we stick with normal equality operations, instead of using bitwise ones.

[asamuzaK]

The better fix for this would be to update the setter code to have Web IDL-conformant behavior: convert undefined (and null) to empty string at the boundary. I don't think undefined or null should ever reach the internals of the library.

I completely agree, but doing so would require a lot of fixes.
I haven't looked over the files under properties/* thoroughly yet, but I think all (or many) files under properties/* would probably need to be fixed.

Also, there are many properties that are missing.

I feel it would be better to tackle the files under properties/* in a separate PR.
How about implementing as proposed in this PR for now, and then fixing it in the near future?

So I'd prefer that we stick with normal equality operations, instead of using bitwise ones.

Got it.
I'll revert the change.

[asamuzaK]

My thoughts on this project:

• The implementations are based on a fairly old specification, so changes and fixes will be needed in many places.
• But trying to change everything at once may have big side effects.
• So the first step should be to make the smallest fixes possible.
• Continuing to make these small fixes will prepare us for the major fixes.
• After that, major fixes should be made.