Bug 167255 – Styles should not show background-repeat-x/y, or -webkit-mask-repeat-x/y

• Component: Web Inspector
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

The styles sidebar shows the background-repeat-x/background-repeat-y properties, which are not real CSS properties. We only have them as CSSPropertyIDs to make parsing easier. They should never be exposed in the inspector; it should just show background-repeat and -webkit-mask-repeat.

[Joseph Pecoraro]

This is probably my fault. I think I added them to the ComputedStyles list as I was gather a list of all the CSSPropertyIDs that were missing.

[Joseph Pecoraro]

Hmm, no I didn't add them and they don't show up in the list I was thinking of (computedProperties in CSSComputedStyleDeclarations.cpp).

[Radar WebKit Bug Importer]

<rdar://problem/30126160>

[Simon Fraser (smfr)]

This is a hot topic in the css wg currently. Would be nice to fix this: https://github.com/w3c/csswg-drafts/issues/116

[Blaze Burg]

@Simon/Myles, did this progress with recent change to make this not web-visible?

[Simon Fraser (smfr)]

background-repeat-x/y were historically not web-visible. We temporarily made them visible by mistake, which I reverted yesterday. But I think the inspector has treated them as actual properties for a long time.

[Joseph Pecoraro]

Yes, Devin pointed out that they show up in the CSS autocompletion for example because it show sup in CSS.getSupportedCSSProperties. We should be excluding it from that list.

I believe Devin started looking at this already.

[Simon Fraser (smfr)]

These properties have "internal-only": true in the JSON now, so you should key off of that to decide what to hide in the UI.

[Devin Rousso]

Created attachment 299857 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 299857 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=299857&action=review

On the face, this looks good to me! Update the test and I'll give it a solid review.

> Source/WebCore/css/makeprop.pl:145
> +

Style: remove this blank line.

> Source/WebCore/css/makeprop.pl:402
> +bool internalCSSProperty(const CSSPropertyID&);

I'd name this "isInternalCSSProperty". And have it take a CSSPropertyID, not a reference. Since its just a numeric value.

> LayoutTests/inspector/css/property-validity.html:13
> +<script>
> +function test() {

I'd like to see this rewritten to use the TestSuite code. It wouldn't need to change much and I suspect it would be easier to read.

> LayoutTests/inspector/css/property-validity.html:72
> +    <p>Testing that the author rules returned by CSSStyleManager.stylesForNode have lowercase property names regardless of CSS source formatting.</p>

This description does not match the test.

[Devin Rousso]

Created attachment 299869 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 299869 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=299869&action=review

r=me

> Source/WebCore/css/makeprop.pl:260
> +  print GPERF "    case CSSPropertyID::CSSProperty" . $nameToId{$name} . ":\n";

Style: Perl indentation here should be 4 spaces.

> LayoutTests/inspector/css/property-members.html:26
> +    suite.addTestCase({
> +        name: "CSSProperty.valid",

This is beginning to look like an excellent unit test. Compare it to:
LayoutTests/inspector/unit-tests/wrapped-promise.html

We should follow that example and make this a unit-test for CSSProperty. Rename it to: (Keeping this in the css subdirectory because it depends on CSS domain behavior)
LayoutTests/inspector/css/css-property.html

Keep the suite name "CSSProperty" but name the individual test cases after the methods. So this one would be "CSSProperty.prototype.get valid".

The `validateProperty` function can go inside of here, since if you were to test other CSSProperty.prototype methods.

We can handle any other cleanup later when we start testing other CSSProperty methods. You may even want to test a slew of other CSSProperty.prototype methods while you are here!

> LayoutTests/inspector/css/property-members.html:64
> +                InspectorTest.log("DOM node not found.");

Nit: While you're here this might as well be `InspectorTest.fail` instead of `InspectorTest.log`.

> LayoutTests/inspector/css/property-members.html:73
> +    <p>Testing that member values of CSSProperty are correct.</p>

In the same spirit, just make this "Testing methods of CSSProperty."

[Devin Rousso]

Created attachment 299921 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 299921 [details]
Patch

New tests look good for me!

[WebKit Commit Bot]

Comment on attachment 299921 [details]
Patch

Clearing flags on attachment: 299921

Committed r211289: <http://trac.webkit.org/changeset/211289>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.