Bug 259621
| Summary: | Potentially remove 'DarkGreyValue' from 'noshade' attribute of HTMLHRElement | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Ahmad Saleem <ahmad.saleem792> |
| Component: | CSS | Assignee: | Nobody <webkit-unassigned> |
| Status: | NEW | ||
| Severity: | Normal | CC: | karlcow, ntim, webkit-bug-importer |
| Priority: | P2 | Keywords: | BrowserCompat, InRadar |
| Version: | Safari Technology Preview | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
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!
| Attachments | ||
|---|---|---|
| Add attachment proposed patch, testcase, etc. |
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.