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).
<rdar://problem/15032846>
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.
(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.
Adjusting the title. In the end it was decided to bring the feature flag back and enable the runtime flag by default.
Created attachment 224607 [details] Temptative patch Let's see how EWS deal with this
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 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
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 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
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
Created attachment 224625 [details] Patch v2 Make EWS happy, still need to figure out what happens with EFL
Comment on attachment 224625 [details] Patch v2 This doesn't actually fix the bug, right? It just adds an ENABLE flag.
(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?
Created attachment 224736 [details] Patch
I'd specially appreciate any Appler telling me about the xcconfig files changes because I am honestly not sure if they're correct.
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".
(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.
(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 on attachment 224736 [details] Patch Obsoleting the patch, I'm moving it to another bug.
Sounds good!
(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