Bug 167255 - Styles should not show background-repeat-x/y, or -webkit-mask-repeat-x/y
Summary: Styles should not show background-repeat-x/y, or -webkit-mask-repeat-x/y
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Devin Rousso
Keywords: InRadar
Depends on:
Reported: 2017-01-20 14:14 PST by Simon Fraser (smfr)
Modified: 2017-01-27 10:59 PST (History)
7 users (show)

See Also:

Patch (10.24 KB, patch)
2017-01-26 14:42 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (10.31 KB, patch)
2017-01-26 15:55 PST, Devin Rousso
joepeck: review+
Details | Formatted Diff | Diff
Patch (16.54 KB, patch)
2017-01-27 01:10 PST, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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
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]
Comment 10 Joseph Pecoraro 2017-01-26 14:54:34 PST
Comment on attachment 299857 [details]

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]
Comment 12 Joseph Pecoraro 2017-01-26 19:56:44 PST
Comment on attachment 299869 [details]

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


> 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:

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)

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]
Comment 14 Joseph Pecoraro 2017-01-27 10:43:13 PST
Comment on attachment 299921 [details]

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

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.