Bug 224807 - Clean up handling of `-webkit-inline-flex`/`-webkit-flex` CSS display values
Summary: Clean up handling of `-webkit-inline-flex`/`-webkit-flex` CSS display values
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Nguyen (:ntim)
URL:
Keywords: InRadar
Depends on: 224574
Blocks:
  Show dependency treegraph
 
Reported: 2021-04-20 02:05 PDT by Tim Nguyen (:ntim)
Modified: 2021-05-24 06:59 PDT (History)
11 users (show)

See Also:


Attachments
Patch (8.07 KB, patch)
2021-05-24 02:22 PDT, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Nguyen (:ntim) 2021-04-20 02:05:58 PDT
Context:

WebKit supports the `-webkit-inline-flex`/`-webkit-flex` values for the CSS `display` property. They are aliases for `inline-flex`/`flex` specifically.

Right now, those values are handled as separate `DisplayType` (see https://webkit-search.igalia.com/webkit/rev/240c1d5ec6ecbabd0053aad3ea5e985e53198714/Source/WebCore/rendering/style/RenderStyleConstants.h#908,910), instead we'd like to alias them directly at parse time in the `consumeDisplay` function of CSSPropertyParser.cpp.

This does change the serialization (what is returned when you do `element.style.display` or `getComputedStyle(element).display`) of `-webkit-flex`/`-webkit-inline-flex` to `flex`/`inline-flex`, but this change is ok. Some tests may need to be updated to reflect this though.


Steps to fix:

* Add special cases directly at the beginning of the `consumeDisplay` function:

```
    // Convert -webkit-flex/-webkit-inline-flex to flex/inline-flex
    if (range.peek().id() == CSSValueWebkitInlineFlex)
        return consumeIdent(range);
    if (range.peek().id() == CSSValueWebkitFlex)
        return consumeIdent(range);
```

* Move -webkit-flex/-webkit-inline-flex in Source/WebCore/css/CSSValueKeywords.in after `flow`, to avoid breaking `DisplayType display = static_cast<DisplayType>(m_value.valueID - CSSValueInline);` in CSSPrimitiveValue.cpp

* Remove the WebKitFlex/WebKitInlineFlex values from the `DisplayType` enum.

* Also remove all their usages too:
https://webkit-search.igalia.com/webkit/search?q=DisplayType%3A%3AWebKitFlex&path=&case=false&regexp=false
https://webkit-search.igalia.com/webkit/search?q=DisplayType%3A%3AWebKitInlineFlex&path=&case=false&regexp=false

* Fix all potential failing tests (You'll find those by running `Tools/Scripts/run-webkit-tests --no-retry --no-show LayoutTests/` or by submitting the patch on Bugzilla and checking EWS failures). The failures will be due tests expecting `-webkit-flex`/`-webkit-inline-flex` instead of the unprefixed version when serializing. The tests should be changed to reflect that.


Please feel free to reach out on the WebKit Slack for any questions (I'm @ntim).
Comment 1 Radar WebKit Bug Importer 2021-04-20 02:29:00 PDT
<rdar://problem/76888941>
Comment 2 Tim Nguyen (:ntim) 2021-04-20 02:35:24 PDT
Sorry, the snippet I suggested was wrong, here's one that should be correct:

+    // Convert -webkit-flex/-webkit-inline-flex to flex/inline-flex
+    CSSValueID nextValueID = range.peek().id();
+    if (nextValueID == CSSValueWebkitInlineFlex || nextValueID == CSSValueWebkitFlex) {
+        consumeIdent(range);
+        return CSSValuePool::singleton().createValue(
+            nextValueID == CSSValueWebkitInlineFlex ? CSSValueInlineFlex : CSSValueFlex);
+    }
Comment 3 Tim Nguyen (:ntim) 2021-05-24 01:31:06 PDT
I'll take this.
Comment 4 Tim Nguyen (:ntim) 2021-05-24 02:22:23 PDT
Created attachment 429513 [details]
Patch
Comment 5 EWS 2021-05-24 06:59:26 PDT
Committed r277949 (238076@main): <https://commits.webkit.org/238076@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 429513 [details].