Bug 259621 – Potentially remove 'DarkGreyValue' from 'noshade' attribute of HTMLHRElement

• Status: NEW
• Component: CSS

[Ahmad Saleem]

Hi Team,

While going through 'HR' related failures on WPT, I just stumbled across following code:

case AttributeNames::noshadeAttr:
        if (!hasAttributeWithoutSynchronization(colorAttr)) {
            addPropertyToPresentationalHintStyle(style, CSSPropertyBorderStyle, CSSValueSolid);
            auto darkGrayValue = CSSValuePool::singleton().createColorValue(Color::darkGray);
            style.setProperty(CSSPropertyBorderColor, darkGrayValue.ptr());
            style.setProperty(CSSPropertyBackgroundColor, WTFMove(darkGrayValue));
        }

I noticed that we are assigning 'darkGrey' value using C++ but recently we added 'color' value from UA Stylesheet as below:

https://searchfox.org/wubkat/source/Source/WebCore/css/html.css#105

______

I looked into 'Firefox' to see if they have special color for 'noshade' attribute but they don't.

Test Case to check - https://www.quanzhanketang.com/tags/tryhtml_hr_noshade.html?filename=tryhtml_hr_noshade

____

I might be completely wrong but just wanted to raise this to check, whether we can get rid of this portion of code? [These lines]

            auto darkGrayValue = CSSValuePool::singleton().createColorValue(Color::darkGray);
            style.setProperty(CSSPropertyBorderColor, darkGrayValue.ptr());
            style.setProperty(CSSPropertyBackgroundColor, WTFMove(darkGrayValue));

Thanks!

[Tim Nguyen (:ntim)]

Removing the darkgray override seems fine to me.

[Ahmad Saleem]

We start failing this - https://jsfiddle.net/w589b0dh/show

As per spec, it is non-conforming standard with only support from Chrome and Safari only.

Spec: https://html.spec.whatwg.org/multipage/obsolete.html#non-conforming-features

Firefox Nightly fails a lot of tests from above.

Should we keep it or remove it altogether?

Or we can get rid of just this line:             

>> style.setProperty(CSSPropertyBorderColor, darkGrayValue.ptr());


and still make us pass above test.

[Tim Nguyen (:ntim)]

(In reply to Ahmad Saleem from comment #2)
> We start failing this - https://jsfiddle.net/w589b0dh/show
> 
> As per spec, it is non-conforming standard with only support from Chrome and
> Safari only.
> 
> Spec:
> https://html.spec.whatwg.org/multipage/obsolete.html#non-conforming-features
> 
> Firefox Nightly fails a lot of tests from above.
> 
> Should we keep it or remove it altogether?
> 
> Or we can get rid of just this line:             
> 
> >> style.setProperty(CSSPropertyBorderColor, darkGrayValue.ptr());
> 
> 
> and still make us pass above test.

I would follow the spec here: https://html.spec.whatwg.org/#the-hr-element-2 . We deviate in multiple ways from the spec:

* background-color should not be set when setting the color / noshade attributes
* size attribute should set the border-width to the value divided by 2 when color/noshade is set, it should set the height otherwise.

More minor differences:
* width should parse dimensions, not integers
* size should parse non-negative integers, not all integers.

[Radar WebKit Bug Importer]

<rdar://problem/113424073>

[Ahmad Saleem]

Based on Comment 03, few changes, which work:

Line 96 (Delete in 'colorAttr') - addHTMLColorToStyle(style, CSSPropertyBackgroundColor, value);

Line 103 (Delete in 'noshadeAttr') - style.setProperty(CSSPropertyBackgroundColor, WTFMove(darkGrayValue));

and change Line 107 (old and now new 105) as following:

 if (int size = parseHTMLInteger(value).value_or(0); size > 1)

to

 if (int size = parseHTMLNonNegativeInteger(value).value_or(0); size > 1)

_____

Still figuring out 'parseDimensions' part.

[Tim Nguyen (:ntim)]

(In reply to Ahmad Saleem from comment #5)
> Based on Comment 03, few changes, which work:
> 
> Line 96 (Delete in 'colorAttr') - addHTMLColorToStyle(style,
> CSSPropertyBackgroundColor, value);
> 
> Line 103 (Delete in 'noshadeAttr') -
> style.setProperty(CSSPropertyBackgroundColor, WTFMove(darkGrayValue));
> 
> and change Line 107 (old and now new 105) as following:
> 
>  if (int size = parseHTMLInteger(value).value_or(0); size > 1)
> 
> to
> 
>  if (int size = parseHTMLNonNegativeInteger(value).value_or(0); size > 1)
> 
> _____
> 
> Still figuring out 'parseDimensions' part.

There's a parseHTMLDimension method.