Bug 128271

Summary: CSS properties are exposed to JS through CSSStyleDeclaration even when features are runtime disabled
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: CSSAssignee: Sergio Villar Senin <svillar>
Status: NEW ---    
Severity: Normal CC: allan.jensen, andersca, buildbot, bunhere, commit-queue, dbates, eoconnor, esprehn+autocc, glenn, gyuyoung.kim, jer.noble, jfernandez, kangil.han, kling, kondapallykalyan, macpherson, menard, oliver, rakuco, rego, rniwa, sergio, simon.fraser, svillar, syoichi
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Temptative patch
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2
none
Patch v2
none
Patch none

Description Simon Fraser (smfr) 2014-02-05 13:54:48 PST
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).
Comment 1 Simon Fraser (smfr) 2014-02-05 13:55:03 PST
<rdar://problem/15032846>
Comment 2 Manuel Rego Casasnovas 2014-02-05 14:32:00 PST
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.
Comment 3 Sergio Villar Senin 2014-02-06 02:18:05 PST
(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.
Comment 4 Sergio Villar Senin 2014-02-17 02:47:40 PST
Adjusting the title. In the end it was decided to bring the feature flag back and enable the runtime flag by default.
Comment 5 Sergio Villar Senin 2014-02-19 01:17:45 PST
Created attachment 224607 [details]
Temptative patch

Let's see how EWS deal with this
Comment 6 WebKit Commit Bot 2014-02-19 01:19:26 PST
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.
Comment 7 Build Bot 2014-02-19 02:53:59 PST
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
Comment 8 Build Bot 2014-02-19 02:54:02 PST
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
Comment 9 Build Bot 2014-02-19 03:41:49 PST
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
Comment 10 Build Bot 2014-02-19 03:41:54 PST
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
Comment 11 Sergio Villar Senin 2014-02-19 05:16:06 PST
Created attachment 224625 [details]
Patch v2

Make EWS happy, still need to figure out what happens with EFL
Comment 12 Simon Fraser (smfr) 2014-02-19 09:07:46 PST
Comment on attachment 224625 [details]
Patch v2

This doesn't actually fix the bug, right? It just adds an ENABLE flag.
Comment 13 Sergio Villar Senin 2014-02-19 10:23:03 PST
(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?
Comment 14 Sergio Villar Senin 2014-02-20 02:09:40 PST
Created attachment 224736 [details]
Patch
Comment 15 Sergio Villar Senin 2014-02-20 02:13:00 PST
I'd specially appreciate any Appler telling me about the xcconfig files changes because I am honestly not sure if they're correct.
Comment 16 Simon Fraser (smfr) 2014-02-20 11:27:54 PST
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".
Comment 17 Sergio Villar Senin 2014-02-21 00:29:11 PST
(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.
Comment 18 Sergio Villar Senin 2014-02-21 00:30:33 PST
(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.
Comment 19 Sergio Villar Senin 2014-02-21 00:33:50 PST
Comment on attachment 224736 [details]
Patch

Obsoleting the patch, I'm moving it to another bug.
Comment 20 Simon Fraser (smfr) 2014-02-21 09:04:59 PST
Sounds good!
Comment 21 Sergio Villar Senin 2014-02-21 09:09:28 PST
(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