Bug 167255

Summary: Styles should not show background-repeat-x/y, or -webkit-mask-repeat-x/y
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: Web InspectorAssignee: Devin Rousso <hi>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, commit-queue, hi, inspector-bugzilla-changes, joepeck, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
joepeck: review+
Patch none

Description Simon Fraser (smfr) 2017-01-20 14:14:58 PST
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.
Comment 1 Joseph Pecoraro 2017-01-20 14:40:37 PST
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.
Comment 2 Joseph Pecoraro 2017-01-20 14:42:37 PST
Hmm, no I didn't add them and they don't show up in the list I was thinking of (computedProperties in CSSComputedStyleDeclarations.cpp).
Comment 3 Radar WebKit Bug Importer 2017-01-20 15:18:57 PST
<rdar://problem/30126160>
Comment 4 Simon Fraser (smfr) 2017-01-25 09:46:46 PST
This is a hot topic in the css wg currently. Would be nice to fix this: https://github.com/w3c/csswg-drafts/issues/116
Comment 5 BJ Burg 2017-01-26 10:27:46 PST
@Simon/Myles, did this progress with recent change to make this not web-visible?
Comment 6 Simon Fraser (smfr) 2017-01-26 11:32:57 PST
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.
Comment 7 Joseph Pecoraro 2017-01-26 11:38:40 PST
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.
Comment 8 Simon Fraser (smfr) 2017-01-26 12:06:12 PST
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.
Comment 9 Devin Rousso 2017-01-26 14:42:33 PST
Created attachment 299857 [details]
Patch
Comment 10 Joseph Pecoraro 2017-01-26 14:54:34 PST
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.
Comment 11 Devin Rousso 2017-01-26 15:55:27 PST
Created attachment 299869 [details]
Patch
Comment 12 Joseph Pecoraro 2017-01-26 19:56:44 PST
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."
Comment 13 Devin Rousso 2017-01-27 01:10:56 PST
Created attachment 299921 [details]
Patch
Comment 14 Joseph Pecoraro 2017-01-27 10:43:13 PST
Comment on attachment 299921 [details]
Patch

New tests look good for me!
Comment 15 WebKit Commit Bot 2017-01-27 10:59:33 PST
Comment on attachment 299921 [details]
Patch

Clearing flags on attachment: 299921

Committed r211289: <http://trac.webkit.org/changeset/211289>
Comment 16 WebKit Commit Bot 2017-01-27 10:59:39 PST
All reviewed patches have been landed.  Closing bug.