Bug 137132 - Remove webkit prefix from CSS columns
Summary: Remove webkit prefix from CSS columns
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-09-25 16:33 PDT by Dean Jackson
Modified: 2014-10-31 13:43 PDT (History)
5 users (show)

See Also:


Attachments
Patch (35.59 KB, patch)
2014-10-24 20:29 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (802.36 KB, application/zip)
2014-10-25 00:28 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (1.33 MB, application/zip)
2014-10-25 00:47 PDT, Build Bot
no flags Details
Patch (64.09 KB, patch)
2014-10-28 18:36 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (1.23 MB, application/zip)
2014-10-28 20:44 PDT, Build Bot
no flags Details
Patch (65.92 KB, patch)
2014-10-29 13:35 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (65.84 KB, patch)
2014-10-30 16:04 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (68.62 KB, patch)
2014-10-30 18:23 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (68.62 KB, patch)
2014-10-31 10:30 PDT, Said Abou-Hallawa
dino: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2014-09-25 16:33:21 PDT
Columns are ready for unprefixing (at least the column-* properties).
Comment 1 Radar WebKit Bug Importer 2014-09-25 16:33:36 PDT
<rdar://problem/18462224>
Comment 2 Said Abou-Hallawa 2014-10-24 20:29:25 PDT
Created attachment 240450 [details]
Patch
Comment 3 Build Bot 2014-10-25 00:28:30 PDT
Created attachment 240454 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 4 Build Bot 2014-10-25 00:47:29 PDT
Created attachment 240455 [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 5 Benjamin Poulain 2014-10-25 20:38:35 PDT
Comment on attachment 240450 [details]
Patch

Please update the tests to use the unprefixed version.

When unprefixing, you should also look into importing tests from W3C and the other vendors (blink and mozilla). We must make sure we have outstanding test coverage before unprefixing.
Comment 6 Said Abou-Hallawa 2014-10-28 18:36:54 PDT
Created attachment 240587 [details]
Patch
Comment 7 Said Abou-Hallawa 2014-10-28 19:06:09 PDT
(In reply to comment #5)
> Comment on attachment 240450 [details]
> Patch
> 
> Please update the tests to use the unprefixed version.
> 
> When unprefixing, you should also look into importing tests from W3C and the
> other vendors (blink and mozilla). We must make sure we have outstanding
> test coverage before unprefixing.

I ported few tests from W3C and Mozilla. I found the bug https://bugs.webkit.org/show_bug.cgi?id=138159 while trying to port one of the Mozilla tests.
Comment 8 Build Bot 2014-10-28 20:44:16 PDT
Created attachment 240591 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 9 Said Abou-Hallawa 2014-10-29 13:35:21 PDT
Created attachment 240618 [details]
Patch
Comment 10 Said Abou-Hallawa 2014-10-29 13:49:13 PDT
(In reply to comment #8)
> Created attachment 240591 [details]
> Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2
> 
> The attached test failures were seen while running run-webkit-tests on the
> mac-wk2-ews.
> Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

The failure in LayoutTests/svg/custom/svg-fonts-in-html.html was happening because this test was using the un-prefixed 'columns' property.  The test was part of the patch which fixes https://bugs.webkit.org/show_bug.cgi?id=16980.  The intention for using the un-prefixed 'columns' property at the time, this test was added, can not be seen from the ChangeLog.  The 'columns' property has no effect since the test was added and it seems to be unrelated to the test.  The property seems to be left by mistake, maybe after realizing WebKit was not supporting the css multi column yet.  So I am fixing this failure by removing the un-prefixed 'columns' property.  By doing that we are going to get the layout of the page as it is used to be.
Comment 11 Said Abou-Hallawa 2014-10-30 14:21:18 PDT
(In reply to comment #5)
> Comment on attachment 240450 [details]
> Patch
> 
> Please update the tests to use the unprefixed version.

This is what I am doing.  The test I added (but changed its name to be fast/multicol/multicol-aliases.html) is about making sure the prefixed column properties are actually aliases to the un-prefixed ones.

> 
> When unprefixing, you should also look into importing tests from W3C and the
> other vendors (blink and mozilla). We must make sure we have outstanding
> test coverage before unprefixing.

Done. I added some test from W3C and mozilla.
Comment 12 Dean Jackson 2014-10-30 15:09:50 PDT
Comment on attachment 240618 [details]
Patch

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

Please make sure you have consistent indentation in the <style> content for the new tests (sometimes you have 2 spaces, other times 4). Similarly be consistent with having a space after the "property:". Lastly, sometimes your scripts use double quotes for strings - other times they use single quotes.

> Source/WebCore/ChangeLog:31
> +        Add new un-prefixed column properties and make the -web-kit* ones be aliases

Typo: -web-kit -> -webkit

> LayoutTests/ChangeLog:22
> +        Test for the effect of the rtl settings on column flow; it is ported from Mozilla

Nit: Add "."

> LayoutTests/ChangeLog:26
> +        Ensure the prefixed and the un-prefixed column properties are behaving exactly the same

Same here.

> LayoutTests/ChangeLog:35
> +        Remove the use of the un-prefixed columns property since it seems unrelated to the test and from
> +        the ChangLog of https://bugs.webkit.org/show_bug.cgi?id=16980 nothing can be gotten why this was
> +        left since it has been doing nothing to the test since it was added 

And here.

I suggest just saying "Remove the use of the un-prefixed columns property since it seems unrelated to the test."

> LayoutTests/fast/multicol/column-box-alignment-rtl-expected.html:7
> +      margin: 0 0;
> +      padding: 0 0;

can just be "0"

> LayoutTests/fast/multicol/column-box-alignment-rtl-expected.html:21
> +      style="direction:rtl;
> +      text-align:right;"

You've copied the style attribute in.
Comment 13 Said Abou-Hallawa 2014-10-30 16:04:19 PDT
Created attachment 240712 [details]
Patch
Comment 14 Said Abou-Hallawa 2014-10-30 18:23:16 PDT
Created attachment 240718 [details]
Patch
Comment 15 Said Abou-Hallawa 2014-10-31 10:30:12 PDT
Created attachment 240742 [details]
Patch
Comment 16 Said Abou-Hallawa 2014-10-31 12:02:34 PDT
(In reply to comment #12)
> Comment on attachment 240618 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240618&action=review
> 
> Please make sure you have consistent indentation in the <style> content for
> the new tests (sometimes you have 2 spaces, other times 4). Similarly be
> consistent with having a space after the "property:". Lastly, sometimes your
> scripts use double quotes for strings - other times they use single quotes.
> 
> > Source/WebCore/ChangeLog:31
> > +        Add new un-prefixed column properties and make the -web-kit* ones be aliases
> 
> Typo: -web-kit -> -webkit
> 

Done.

> > LayoutTests/ChangeLog:22
> > +        Test for the effect of the rtl settings on column flow; it is ported from Mozilla
> 
> Nit: Add "."
> 

Done.

> > LayoutTests/ChangeLog:26
> > +        Ensure the prefixed and the un-prefixed column properties are behaving exactly the same
> 
> Same here.
> 

Done.

> > LayoutTests/ChangeLog:35
> > +        Remove the use of the un-prefixed columns property since it seems unrelated to the test and from
> > +        the ChangLog of https://bugs.webkit.org/show_bug.cgi?id=16980 nothing can be gotten why this was
> > +        left since it has been doing nothing to the test since it was added 
> 
> And here.
> 

Done.

> I suggest just saying "Remove the use of the un-prefixed columns property
> since it seems unrelated to the test."
> 

Done.

> > LayoutTests/fast/multicol/column-box-alignment-rtl-expected.html:7
> > +      margin: 0 0;
> > +      padding: 0 0;
> 
> can just be "0"
> 

Done.

> > LayoutTests/fast/multicol/column-box-alignment-rtl-expected.html:21
> > +      style="direction:rtl;
> > +      text-align:right;"
> 
> You've copied the style attribute in.

Done.
Comment 17 Dean Jackson 2014-10-31 13:38:09 PDT
Comment on attachment 240742 [details]
Patch

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

Excellent. I'll land this manually right now.

> Source/WebCore/ChangeLog:66
> +        * rendering/style/RenderStyle.cpp:
> +        (WebCore::RenderStyle::colorIncludingFallback):
> +
> +2014-10-30  Said Abou-Hallawa  <sabouhallawa@apple.com>
> +
> +        Remove webkit prefix from CSS columns.
> +        https://bugs.webkit.org/show_bug.cgi?id=137132.
> +

Oops. Duplicate entries here.
Comment 18 Dean Jackson 2014-10-31 13:43:13 PDT
Committed in https://trac.webkit.org/r175421