Bug 137132 – Remove webkit prefix from CSS columns

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Dean Jackson]

Columns are ready for unprefixing (at least the column-* properties).

[Radar WebKit Bug Importer]

<rdar://problem/18462224>

[Said Abou-Hallawa]

Created attachment 240450 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Benjamin Poulain]

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.

[Said Abou-Hallawa]

Created attachment 240587 [details]
Patch

[Said Abou-Hallawa]

(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.

[Build Bot]

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

[Said Abou-Hallawa]

Created attachment 240618 [details]
Patch

[Said Abou-Hallawa]

(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.

[Said Abou-Hallawa]

(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.

[Dean Jackson]

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.

[Said Abou-Hallawa]

Created attachment 240712 [details]
Patch

[Said Abou-Hallawa]

Created attachment 240718 [details]
Patch

[Said Abou-Hallawa]

Created attachment 240742 [details]
Patch

[Said Abou-Hallawa]

(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.

[Dean Jackson]

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):
> +
> [phone]  Said Abou-Hallawa  <[e-mail]>
> +
> +        Remove webkit prefix from CSS columns.
> +        https://bugs.webkit.org/show_bug.cgi?id=137132.
> +

Oops. Duplicate entries here.

[Dean Jackson]

Committed in https://trac.webkit.org/r175421