Color issue

[oswetto]

Hi @rbri in the color the char "/" is not supported
color: hsl(120 75% 50% / 80%);
Error in expression. Invalid token "/"

[rbri]

Yes correct, will try to fix that by making the color handling more spec conform

https://developer.mozilla.org/en-US/docs/Web/CSS/color_value

[paulushub]

Any progress on this issue?

I am new to JavaCC and do not know yet how to add the support for the "/" token, but could
add the support for all the color formats (if the token is supported):

• Functional notation: hsl[a](H, S, L[, A])
• Functional notation: hsl[a](H S L[ / A])
• Functional notation: rgb[a](R, G, B[, A])
• Functional notation: rgb[a](R G B[ / A])
• etc

I have ported the original library (from SourceForge) to C# before I found your improvements/simplifications, but
the lack of the "/" token support stalls further improvements in the color support.

I did further simplifications by turning CSSValueImpl.java into an abstract class without the value object and making RectImpl.java, RGBColorImpl.java, CounterImpl.java, LexicalUnitImpl.java, etc super class of CSSValueImpl.java.

[rbri]

Had already a look at this but had to put it aside because of others.
I think there is no need to change the parser here the magic is inside the RGBColorImpl ctor

public RGBColorImpl(final LexicalUnit lu) throws DOMException

This method is called with the various parameters (and separators) chained as lexical units.
Supporting more formats can be done here.

Will be great if you can make a PR for this (including more unit tests) and i will try to merge it as soon as possible.

[paulushub]

I think there is no need to change the parser here the magic is inside the RGBColorImpl ctor

Partially yes. Together with the functionInternal method below and hexcolorInternal (of AbstractCSSParser.java), a lot can be done...

    protected LexicalUnit functionInternal(final LexicalUnit prev, final String funct,
            final LexicalUnit params) {

        if ("counter(".equalsIgnoreCase(funct)) {
            return LexicalUnitImpl.createCounter(prev, params);
        }
        else if ("counters(".equalsIgnoreCase(funct)) {
            return LexicalUnitImpl.createCounters(prev, params);
        }
        else if ("attr(".equalsIgnoreCase(funct)) {
            return LexicalUnitImpl.createAttr(prev, params.getStringValue());
        }
        else if ("rect(".equalsIgnoreCase(funct)) {
            return LexicalUnitImpl.createRect(prev, params);
        }
        else if ("rgb(".equalsIgnoreCase(funct)) {
            return LexicalUnitImpl.createRgbColor(prev, params);
        }
        else if ("calc(".equalsIgnoreCase(funct)) {
            return LexicalUnitImpl.createCalc(prev, params);
        }
        return LexicalUnitImpl.createFunction(
            prev,
            funct.substring(0, funct.length() - 1),
            params);
    }

Supporting more formats can be done here.

I have not tested your modified codes, but the original throws exception for the rgba(120 75% 50% / 80%) because of the "/" token. Is this fixed in your case?

Will be great if you can make a PR for this (including more unit tests) and i will try to merge it as soon as possible.

I could easily contribute that, since I have already handled such cases in my C# codes. The problem is the unsupported token "/", which is used as a separator of the alpha-component of the color.

[rbri]

ok, will improve the parser tomorrow

[rbri]

made some fixes here and changed the parser
have to write more tests but i think we are getting closer

@paulushub
Will be great if you can have a look at least at the tests and provide more.

[paulushub]

@rbri That is a lot of work, thanks for the time. Will start testing immediately.

[rbri]

Another round of fixes, hsl should now work also

[paulushub]

By the new draft Specs: Also for legacy reasons, an rgba() function also exists, with an identical grammar and behavior to rgb().
5.1. The RGB functions: rgb() and rgba()

So, the function parameter in the constructors of RGBColorImpl and HSLColorImpl should be optional (if specified and not empty, should be verified to be either rgb or rgba for the RGBColorImpl).

public RGBColorImpl(final String function, final LexicalUnit lu) throws DOMException

public HSLColorImpl(final String function, final LexicalUnit lu) throws DOMException

Currently, the test below fails

    /**
     * @throws Exception if any error occurs
     */
    @Test
    public void getCssText2() throws Exception {
        final LexicalUnit rgbLu = LexicalUnitImpl.createNumber(null, 10);
        LexicalUnit lu = LexicalUnitImpl.createComma(rgbLu);
        lu = LexicalUnitImpl.createNumber(lu, 20);
        lu = LexicalUnitImpl.createComma(lu);
        lu = LexicalUnitImpl.createNumber(lu, 30);
        lu = LexicalUnitImpl.createComma(lu);
        LexicalUnitImpl.createNumber(lu, 0);

        //NOTE: Here, I tried empty function name. null will throw exception.
        final RGBColorImpl rgb = new RGBColorImpl("", rgbLu);  

        assertEquals("rgba(10, 20, 30, 0)", rgb.toString());
        // Or: assertEquals("rgb(10, 20, 30, 0)", rgb.toString());
    }

[paulushub]

There is no set method for the commaSeparated_ member:

private boolean commaSeparated_

[paulushub]

In the hexColorInternal(final LexicalUnit prev, final Token t), the alpha-component is parsed as double, why not retain the integer?

protected LexicalUnit hexColorInternal(final LexicalUnit prev, final Token t) {
    // Step past the hash at the beginning
    final int i = 1;
    int r = 0;
    int g = 0;
    int b = 0;
    int a = -1;

    final int len = t.image.length() - 1;
    try {
        if (len == 3 || len == 4) {
            r = Integer.parseInt(t.image.substring(i + 0, i + 1), 16);
            g = Integer.parseInt(t.image.substring(i + 1, i + 2), 16);
            b = Integer.parseInt(t.image.substring(i + 2, i + 3), 16);
            r = (r << 4) | r;
            g = (g << 4) | g;
            b = (b << 4) | b;

            if (len == 4) {
                a = Integer.parseInt(t.image.substring(i + 3, i + 4), 16);
                a = (a << 4) | a;
            }
        }
        else if (len == 6 || len == 8) {
            r = Integer.parseInt(t.image.substring(i + 0, i + 2), 16);
            g = Integer.parseInt(t.image.substring(i + 2, i + 4), 16);
            b = Integer.parseInt(t.image.substring(i + 4, i + 6), 16);

            if (len == 8) {
                a = Integer.parseInt(t.image.substring(i + 6, i + 8), 16);
            }                
        }
        else {
            final String pattern = getParserMessage("invalidColor");
            throw new CSSParseException(MessageFormat.format(
                pattern, new Object[] {t}),
                getInputSource().getURI(), t.beginLine,
                t.beginColumn);
        }

        // Turn into an "rgb()"
        final LexicalUnit lr = LexicalUnitImpl.createNumber(null, r);
        final LexicalUnit lg = LexicalUnitImpl.createNumber(LexicalUnitImpl.createComma(lr), g);
        final LexicalUnit lb = LexicalUnitImpl.createNumber(LexicalUnitImpl.createComma(lg), b);

        if (a != -1) {
            LexicalUnitImpl.createNumber(LexicalUnitImpl.createComma(lb), a);

            return LexicalUnitImpl.createRgbColor(prev, "rgba", lr);
        }

        return LexicalUnitImpl.createRgbColor(prev, "rgb", lr);

    }
    catch (final NumberFormatException ex) {
        final String pattern = getParserMessage("invalidColor");
        throw new CSSParseException(MessageFormat.format(
            pattern, new Object[] {t}),
            getInputSource().getURI(), t.beginLine,
            t.beginColumn, ex);
    }
}

[rbri]

hexColorInternal

in the hex notation the value is between 0 and 255. We need to divide this by 255 to get the alpha value.

[rbri]

commaSeparated_ is determined at construction time, there is no need to change afterwards

[rbri]

npe inside the ctor is fixed

[paulushub]

in the hex notation the value is between 0 and 255. We need to divide this by 255 to get the alpha value.

I think that conversion should be left to the user. Java Color object, for instance, uses integer for the alpha.

Color(int r, int g, int b, int a)
Creates an sRGB color with the specified red, green, blue, and alpha values in the range (0 - 255).

int getAlpha()
Returns the alpha component in the range 0-255.

commaSeparated_ is determined at construction time, there is no need to change afterwards

I think your simplification went a bit far with elimination of CSSFormat and the getCssText(final CSSFormat format) method, so the formatting information is now part of the color class.

[paulushub]

@oswetto As the original poster of the issue and with application different from HtmlUnit, please provide a feedback.

[oswetto]

@rbri @paulushub thanks for me works!

[paulushub]

@rbri I missed this, please fix the get/set methods of the HSLColorImpl class, and add the get/set methods for the alpha component.:

    /**
     * @return the red part.
     */
    public CSSValueImpl getRed() {
        return hue_;
    }

    /**
     * Sets the red part to a new value.
     * @param red the new CSSPrimitiveValue
     */
    public void setRed(final CSSValueImpl red) {
        hue_ = red;
    }

    /**
     * @return the green part.
     */
    public CSSValueImpl getGreen() {
        return saturation_;
    }

    /**
     * Sets the green part to a new value.
     * @param green the new CSSPrimitiveValue
     */
    public void setGreen(final CSSValueImpl green) {
        saturation_ = green;
    }

    /**
     * @return the blue part.
     */
    public CSSValueImpl getBlue() {
        return lightness_;
    }

    /**
     * Sets the blue part to a new value.
     * @param blue the new CSSPrimitiveValue
     */
    public void setBlue(final CSSValueImpl blue) {
        lightness_ = blue;
    }

[rbri]

getter/setters are fixed

[rbri]

I think that conversion should be left to the user. Java Color object, for instance, uses integer for the alpha.

i think not :-) There is no chance for the user to figure out if the color was created from a hex notation or an rgb one. If you define one color in both ways the result should be consistent.

[rbri]

I think your simplification went a bit far with elimination of CSSFormat and the getCssText(final CSSFormat format) method, so the formatting information is now part of the color class.

The original idea was to trim the parser to the stuff i need to support my cases in HtmlUnit and be able to move forward (and do not have to take care of sac). This was the reason why all the other stuff is removed.

[rbri]

commaSeparated_

I tried to reproduce the orginal value format nothing more

[paulushub]

getter/setters are fixed

Sorry, there is more. In the constructors, some of the exception statements are without the throw keyword.
I was about to post a PR 😄

There is no chance for the user to figure out if the color was created from a hex notation or an rgb one.

The user does not have to, the primitive type will indicate an number/integer type like the rest of the RGB components, so for color systems using 0-255, no conversion will be needed.

I tried to reproduce the orginal value format nothing more

I do understand the requirements of HtmlUnit as a testing platform.

[paulushub]

@rbri Check the b in the exception statement "b requires at least three values."

htmlunit-cssparser/src/main/java/com/gargoylesoftware/css/dom/RGBColorImpl.java

Line 91 in fbebb3d

throw new DOMException(DOMException.SYNTAX_ERR, function_ + "b requires at least three values.");

The statement: throw new DOMException(DOMException.SYNTAX_ERR, "Color space rgb or rgba is requiredc");
should be throw new DOMException(DOMException.SYNTAX_ERR, "Color space hsl or hsla is required.");

htmlunit-cssparser/src/main/java/com/gargoylesoftware/css/dom/HSLColorImpl.java

Line 48 in fbebb3d

throw new DOMException(DOMException.SYNTAX_ERR, "Color space rgb or rgba is requiredc");

Attached is a test file (HSLColorImplTest.java) similar to the RGB version to flag the above issues.
HSLColorImplTest.zip

[rbri]

Looks like i forgot to close this. Thanks @paulushub for all the work.