Bug 128271 – CSS properties are exposed to JS through CSSStyleDeclaration even when features are runtime disabled

• Status: NEW
• Component: CSS

[Simon Fraser (smfr)]

http://www.mobilexweb.com/blog/safari-ios7-html5-problems-apis-review:

CSS grid properties are exposed to JS through CSSStyleDeclaration even when grid layout is disabled (as it is in shipping Safari).

[Simon Fraser (smfr)]

<rdar://problem/15032846>

[Manuel Rego Casasnovas]

JFYI, ENABLE_CSS_GRID_LAYOUT was dropped back in May 2012 (r117613). So I guess that since then the properties are exposed as the #if was removed from css/CSSPropertyNames.in and css/CSSComputedStyleDeclaration.cpp.

There's a setting set to false by default at page/Settings.in to enable/disable Grid Layout support, however it's not being used to remove the exposed properties.

[Sergio Villar Senin]

(In reply to comment #0)
> http://www.mobilexweb.com/blog/safari-ios7-html5-problems-apis-review:
> 
> CSS grid properties are exposed to JS through CSSStyleDeclaration even when grid layout is disabled (as it is in shipping Safari).

Correct me if I'm wrong but this is not a problem of grid layout per se. As far as I know in WebKit we do not hide properties that are runtime disabled. What we normally do is to enable/disable their parsing as it's done for example for CSS shapes, CSS regions etc.

This is easy to check, if you just disable one of those features and run  Object.getOwnPropertyDescriptor(document.body.style, property) you'll see how all those properties are exposed even though they're disabled at runtime.

So if we consider this an issue, I guess we do, we should remove grid layout from the title and start thinking about how to fix it. Maybe we should just filter the static list of properties removing the ones that are runtime disabled. As a reference Eric fixed this in Blink last year, see https://codereview.chromium.org/14324009.

[Sergio Villar Senin]

Adjusting the title. In the end it was decided to bring the feature flag back and enable the runtime flag by default.

[Sergio Villar Senin]

Created attachment 224607 [details]
Temptative patch

Let's see how EWS deal with this

[WebKit Commit Bot]

Attachment 224607 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 49 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 224607 [details]
Temptative patch

Attachment 224607 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5022678033367040

New failing tests:
fast/ruby/ruby-base-merge-block-children-crash-2.html
fast/css-grid-layout/grid-disabled-by-default.html

[Build Bot]

Created attachment 224617 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 224607 [details]
Temptative patch

Attachment 224607 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6087694362869760

New failing tests:
fast/ruby/ruby-base-merge-block-children-crash-2.html
fast/css-grid-layout/grid-disabled-by-default.html

[Build Bot]

Created attachment 224619 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Sergio Villar Senin]

Created attachment 224625 [details]
Patch v2

Make EWS happy, still need to figure out what happens with EFL

[Simon Fraser (smfr)]

Comment on attachment 224625 [details]
Patch v2

This doesn't actually fix the bug, right? It just adds an ENABLE flag.

[Sergio Villar Senin]

(In reply to comment #12)
> (From update of attachment 224625 [details])
> This doesn't actually fix the bug, right? It just adds an ENABLE flag.

Yeah, I'm bringing back the feature flag so ports might decide not to ship it.

I don't know if you followed this thread in the mailing list:
https://lists.webkit.org/pipermail/webkit-dev/2014-February/026218.html

most of people suggested this kind of "fix". Probably we should open another different bug if we want to hide those properties even for features that are already built but not runtime enabled. What do you think Simon?

[Sergio Villar Senin]

Created attachment 224736 [details]
Patch

[Sergio Villar Senin]

I'd specially appreciate any Appler telling me about the xcconfig files changes because I am honestly not sure if they're correct.

[Simon Fraser (smfr)]

Comment on attachment 224736 [details]
Patch

FeatureDefines changes look OK. But I think this patch belongs in a different bug that should be titled "Add ENABLE flag for grid layout".

[Sergio Villar Senin]

(In reply to comment #16)
> (From update of attachment 224736 [details])
> FeatureDefines changes look OK. But I think this patch belongs in a different bug that should be titled "Add ENABLE flag for grid layout".

Fair enough, I'll create another one.

[Sergio Villar Senin]

(In reply to comment #17)
> (In reply to comment #16)
> > (From update of attachment 224736 [details] [details])
> > FeatureDefines changes look OK. But I think this patch belongs in a different bug that should be titled "Add ENABLE flag for grid layout".
> 
> Fair enough, I'll create another one.

BTW Simon, what do you think about renaming this one just to

"CSS properties are exposed to JS through CSSStyleDeclaration even when features are runtime disabled"

just to be explicit about the fact that this happens for every single runtime feature.

[Sergio Villar Senin]

Comment on attachment 224736 [details]
Patch

Obsoleting the patch, I'm moving it to another bug.

[Simon Fraser (smfr)]

Sounds good!

[Sergio Villar Senin]

(In reply to comment #17)
> (In reply to comment #16)
> > (From update of attachment 224736 [details] [details])
> > FeatureDefines changes look OK. But I think this patch belongs in a different bug that should be titled "Add ENABLE flag for grid layout".
> 
> Fair enough, I'll create another one.

Done, see bug 129153