Color issue about htmlunit-cssparser HOT 25 CLOSED

ok, will improve the parser tomorrow

from htmlunit-cssparser.

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.

from htmlunit-cssparser.

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

from htmlunit-cssparser.

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.

from htmlunit-cssparser.

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.

from htmlunit-cssparser.

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.

from htmlunit-cssparser.

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

from htmlunit-cssparser.

Another round of fixes, hsl should now work also

from htmlunit-cssparser.

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());
    }

from htmlunit-cssparser.

There is no set method for the commaSeparated_ member:

private boolean commaSeparated_

from htmlunit-cssparser.

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);
    }
}

from htmlunit-cssparser.

hexColorInternal

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

from htmlunit-cssparser.

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

from htmlunit-cssparser.

npe inside the ctor is fixed

from htmlunit-cssparser.

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.

from htmlunit-cssparser.

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

from htmlunit-cssparser.

@rbri @paulushub thanks for me works!

from htmlunit-cssparser.

@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;
    }

from htmlunit-cssparser.

getter/setters are fixed

from htmlunit-cssparser.

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.

from htmlunit-cssparser.

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.

from htmlunit-cssparser.

commaSeparated_

I tried to reproduce the orginal value format nothing more

from htmlunit-cssparser.

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.

from htmlunit-cssparser.

@rbri Check the b in the exception statement "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.");

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

from htmlunit-cssparser.

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

from htmlunit-cssparser.