Bug 189597 - Wrong 'border' serialization with both common and uncommon values
Summary: Wrong 'border' serialization with both common and uncommon 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: Oriol Brufau
URL:
Keywords: InRadar
Depends on:
Blocks: 188697
  Show dependency treegraph
 
Reported: 2018-09-13 11:47 PDT by Oriol Brufau
Modified: 2018-09-20 13:44 PDT (History)
7 users (show)

See Also:


Attachments
Patch (8.01 KB, patch)
2018-09-14 08:52 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (8.98 KB, patch)
2018-09-14 09:35 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (2.33 MB, application/zip)
2018-09-14 10:29 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews107 for mac-sierra-wk2 (3.25 MB, application/zip)
2018-09-14 10:53 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews116 for mac-sierra (3.03 MB, application/zip)
2018-09-14 11:05 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (2.27 MB, application/zip)
2018-09-14 11:38 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews203 for win-future (12.84 MB, application/zip)
2018-09-14 13:20 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews206 for win-future (12.82 MB, application/zip)
2018-09-14 13:39 PDT, EWS Watchlist
no flags Details
Patch (15.91 KB, patch)
2018-09-20 12:48 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oriol Brufau 2018-09-13 11:47:04 PDT
Run this code:

```js
element.style.cssText = "border: 5px solid blue; border-top: 5px dotted green";
element.style.border;
```

Expected: empty string, because the 'border' shorthand can't express that combination.
Result: "5px", because that's a common value for the 4 sides. The style and color have uncommon values so they are just omitted.
Comment 1 Oriol Brufau 2018-09-14 08:52:04 PDT
Created attachment 349763 [details]
Patch
Comment 2 Oriol Brufau 2018-09-14 09:35:58 PDT
Created attachment 349767 [details]
Patch
Comment 3 EWS Watchlist 2018-09-14 10:29:42 PDT
Comment on attachment 349767 [details]
Patch

Attachment 349767 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9217280

New failing tests:
fast/dom/css-shorthand-common-value.html
fast/css/getPropertyValue-border.html
Comment 4 EWS Watchlist 2018-09-14 10:29:44 PDT
Created attachment 349773 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 5 EWS Watchlist 2018-09-14 10:53:49 PDT
Comment on attachment 349767 [details]
Patch

Attachment 349767 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9217381

New failing tests:
fast/dom/css-shorthand-common-value.html
fast/css/getPropertyValue-border.html
Comment 6 EWS Watchlist 2018-09-14 10:53:50 PDT
Created attachment 349777 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 7 EWS Watchlist 2018-09-14 11:05:18 PDT
Comment on attachment 349767 [details]
Patch

Attachment 349767 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9217362

New failing tests:
fast/css/getPropertyValue-border.html
fast/dom/css-shorthand-common-value.html
Comment 8 EWS Watchlist 2018-09-14 11:05:20 PDT
Created attachment 349780 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 9 EWS Watchlist 2018-09-14 11:38:52 PDT
Comment on attachment 349767 [details]
Patch

Attachment 349767 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9217469

New failing tests:
fast/css/getPropertyValue-border.html
fast/dom/css-shorthand-common-value.html
Comment 10 EWS Watchlist 2018-09-14 11:38:54 PDT
Created attachment 349785 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 11 Simon Fraser (smfr) 2018-09-14 12:55:49 PDT
Comment on attachment 349767 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Remove CommonValueMode enum and make borderPropertyValue always return null
> +        when there are uncommon values (the previous ReturnNullOnUncommonValues mode).

Why was CommonValueMode added in the first place, and what's the justification for removing it?
Comment 12 EWS Watchlist 2018-09-14 13:20:11 PDT
Comment on attachment 349767 [details]
Patch

Attachment 349767 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9217908

New failing tests:
fast/dom/css-shorthand-common-value.html
fast/css/getPropertyValue-border.html
Comment 13 EWS Watchlist 2018-09-14 13:20:25 PDT
Created attachment 349794 [details]
Archive of layout-test-results from ews203 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews203  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 14 EWS Watchlist 2018-09-14 13:39:16 PDT
Comment on attachment 349767 [details]
Patch

Attachment 349767 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9218796

New failing tests:
fast/dom/css-shorthand-common-value.html
fast/css/getPropertyValue-border.html
Comment 15 EWS Watchlist 2018-09-14 13:39:28 PDT
Created attachment 349797 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 16 Oriol Brufau 2018-09-20 12:48:32 PDT
Created attachment 350250 [details]
Patch
Comment 17 Oriol Brufau 2018-09-20 12:58:56 PDT
(In reply to Simon Fraser (smfr) from comment #11)
> Why was CommonValueMode added in the first place, and what's the
> justification for removing it?

Initially, there was no CommonValueMode and the behavior was always like OmitUncommonValues. This produced a wrong output both for cssText and getPropertyValue("border").

Then bug 83026 added CommonValueMode to fix the cssText behavior, but preserving the getPropertyValue("border") one.

But it's still wrong: https://drafts.csswg.org/cssom/#serialize-a-css-value
> If there is no such shorthand or shorthand cannot exactly represent the values of all the properties in list, return the empty string.

So now I want to remove CommonValueMode and always behave like ReturnNullOnUncommonValues.
Comment 18 WebKit Commit Bot 2018-09-20 13:43:44 PDT
Comment on attachment 350250 [details]
Patch

Clearing flags on attachment: 350250

Committed r236278: <https://trac.webkit.org/changeset/236278>
Comment 19 WebKit Commit Bot 2018-09-20 13:43:46 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Radar WebKit Bug Importer 2018-09-20 13:44:38 PDT
<rdar://problem/44653562>