698783 - Remove prefixing for CSS3 multicol

Status: RESOLVED FIXED

(Core :: Layout: Columns, task, P2)

Description

[Scott Johnson (:jwir3)]

Gecko's implementation of the CSS3 multicol spec is still prefixed. Once column-span and column-fill have been implemented, we should allow the following CSS properties to be recognized:

column-width
column-count
columns
column-gap
column-rule-color
column-rule-style
column-rule-width
column-rule
column-span
column-fill

Comment 1

[Scott Johnson (:jwir3)]

Attachment: b698783 - Manual Changes for b698783

Manual changes to deprefix columns. Automatic changes to follow.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Are the automatic changes going to follow?

Also, what's our testing story here?  The current opinion of the CSS WG is that implementations unprefixing a new feature should be contributing tests for it.  I think we have a bunch of tests that could be relatively-easily massaged into something we could contribute to the WG, though I'm not sure how thorough they are.

Comment 3

[Scott Johnson (:jwir3)]

(In reply to David Baron [:dbaron] from comment #2)
> Are the automatic changes going to follow?

Ah yes, sorry. I will post them right now. I apparently got distracted while I was in the midst of this. My apologies.

> Also, what's our testing story here?  The current opinion of the CSS WG is
> that implementations unprefixing a new feature should be contributing tests
> for it.  I think we have a bunch of tests that could be relatively-easily
> massaged into something we could contribute to the WG, though I'm not sure
> how thorough they are.

Well, I have a number of additional tests (mostly reftests) that I was planning on posting for bug 684062, but I can post these here, if you would prefer. This doesn't seem to be what you're asking about, though. If I understand correctly, when we deprefix we need to provide tests to the CSS WG? There seem to be some tests already available (e.g. http://test.csswg.org/suites/css3-multicol/nightly-unstable/ ). I'm not entirely clear on how the process works here - if we're the first vendor to remove prefixing, is it our responsibility to provide tests? Are these tests then automatically incorporated into the test suite? What is the standard for CSS WG tests (i.e. how much massagin will we have to do to get these to work?)

Comment 4

[Scott Johnson (:jwir3)]

Attachment: b698783 - Automatic Changes for b698783

To create this patch, I first moved my .hg directory to one directory higher in the tree, then ran the following command:

find ./ -type f -regextype posix-extended -iregex '.*\.(x?html|css|s?js)' -print | xargs sed -i 's/-moz-column/column/g'

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Given changing opinions and policies, is there any reason not to unprefix this right now?

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think that still depends on:
 * whether other browsers are shipping it unprefixed
 * whether their implementations are as buggy as (or more buggy than) ours
 * what sort of bugs we have

Comment 9

[Markus Popp]

(In reply to David Baron :dbaron: ⌚️UTC-4 (review requests must explain patch) from comment #8)
> I think that still depends on:
>  * whether other browsers are shipping it unprefixed

Basically every other major browser (Chrome as of version 50.0) is shipping with multicol unprefixed:

http://caniuse.com/#feat=multicolumn

Comment 10

[Jet Villegas (inactive)]

(In reply to David Baron :dbaron: ⌚️UTC+1 (mostly busy through August 4; review requests must explain patch) from comment #8)
> I think that still depends on:
>  * whether other browsers are shipping it unprefixed
>  * whether their implementations are as buggy as (or more buggy than) ours
>  * what sort of bugs we have

Neerja will be looking at getting this project over the wire.

Comment 11

[Neerja Pancholi[:neerja]]

Attachment: Bug 698783 - Updating the existing tests from CSS Working Group (Step 2a from dbarons comment#12)

Review commit: https://reviewboard.mozilla.org/r/69464/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69464/

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8778065 [details]
Bug 698783 - Updating the existing tests from CSS Working Group (Step 2a from dbarons comment#12)

https://reviewboard.mozilla.org/r/69464/#review66906

So as we discussed earlier today, I think there are a few things to do here that should be separated into a few different patches:

 (1) we should get rid of the _moz_ in the internal constants for these properties, anytime.  That's just code cleanup.

 (2) we should import the multicol reftests from the CSSWG test repository.  This has a few steps:
     (a) rerun layout/reftests/w3c-css/import-tests.py , and if there are any changes, make a patch with them and push it to try.
     (b) then, add importing of the multicol tests, and do the same.  This depends on (2a).
     (c) evaluate the failures of the multicol tests added in (b) and figure out if we're doing worse than other browsers that have unprefixed.  This depends on (2b).

 (3) write a patch to layout/style/nsCSSPropList.h to remove the -moz- prefixes there, but add them back as aliases in layout/style/nsCSSPropAliasList.h, and then make the same changes to layout/style/test/property_database.js and any other tests that show failures (probably a small number).  This depends on (2c) to decide whether we're ready to do it.

 (4) Substitute all of the rest of the tree to remove the -moz- prefixes for these properties.  This depends on (3).

 (5) Remove the aliases introduced in (3).  This depends on (3) and (4), and shouldn't be done until after we've successfully shipped (3).

All of these (1, 2a, 2b, 2c, 3, 4, and 5) could be different bugs, although it's ok if 2a and 2b are in a single bug, and it's also ok if 3 and 4 are in a single bug, or even 1, 3, and 4.  But things that happen at different times should be in separate bugs even if they could be in the same bug if done at the same time.

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 8778065 [details]
Bug 698783 - Updating the existing tests from CSS Working Group (Step 2a from dbarons comment#12)

(per our IRC discussion yesterday, I think you meant to cancel the review-request here.  --> canceling r?dholbert flag.)

Comment 14

[Neerja Pancholi[:neerja]]

Comment on attachment 8778065 [details]
Bug 698783 - Updating the existing tests from CSS Working Group (Step 2a from dbarons comment#12)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69464/diff/1-2/

Comment 15

[Neerja Pancholi[:neerja]]

Attachment: Bug 698783 - Added importing of multicol tests from CSSWG with py script and checking in these new tests + script (Step 2b from dbarons comment#12)

Review commit: https://reviewboard.mozilla.org/r/71506/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/71506/

Comment 16

[Kohei Yoshino]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/prefixed-css3-multi-column-properties-will-be-removed/

Comment 18

[Gérard Talbot]

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #12)

>      (c) evaluate the failures of the multicol tests added in (b) and figure
> out if we're doing worse than other browsers that have unprefixed. 


Current support of CSS3 Multi-column test suite (170 tests)
http://test.csswg.org/harness/review/css-multicol-1_dev/
with Firefox 57.0a1 buildID=20170820221144:

http://test.csswg.org/harness/results/css-multicol-1_dev/grouped/browser/Firefox/browser_version/57.0/

Gecko: Passed 66.47%, Coverage 100%

- - - - - - - - -

Other results:
http://test.csswg.org/harness/results/css-multicol-1_dev/grouped/

- - - - - - - - -

Current support of CSS3 Multi-column test suite (170 tests)
http://test.csswg.org/harness/review/css-multicol-1_dev/
with Chrome 62.0.3188.2 (dev channel):

http://test.csswg.org/harness/results/css-multicol-1_dev/grouped/engine/Blink/browser/Chrome/browser_version/62.0.3188.2/

Blink: Passed 70.59%, Coverage 100%



Preliminary comments on a few of those 170 tests
************************************************

http://test.csswg.org/harness/test/css-multicol-1_dev/multicol-width-ems-001/
is most likely (very high probability) that it is an imprecise test or the reference file is imprecise.

- - - - - - - - -

http://test.csswg.org/harness/test/css-multicol-1_dev/multicol-rule-fraction-003/
is imprecise. The reference file

http://test.csswg.org/suites/css-multicol-1_dev/nightly-unstable/html4/reference/multicol-rule-fraction-3-ref.htm

has its leftmost vertical blue stripe most likely is off by 1px because 
of fractional pixel:

line 34: #a1 {left: 2.43em;}
is off by 1px. Looks okay with left: 2.4em instead.
The whole reftest should probably be recoded to avoid fractional pixel. The reftest has _nothing to do_ with column-rule and has everything to do with absolute positioning.

- - - - - - - - -

wrong linkage of reference file in

http://test.csswg.org/harness/test/css-multicol-1_dev/multicol-fill-auto/

- - - - - - - - -

http://test.csswg.org/harness/test/css-multicol-1_dev/multicol-count-large-001/

http://test.csswg.org/harness/test/css-multicol-1_dev/multicol-count-large-002/

fail by 1px only in Firefox 57 and Chrome 62. Such 1px failure is most likely due to the intrinsic nature of the tests themselves. I do not think failing these 2 tests should mean anything, except that the tests are extreme, testing extreme limits.

Comment 19

[Mathew Hodson]

All the dependencies for this bug have been resolved. Can this be closed?

Comment 20

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

(In reply to Mathew Hodson from comment #19)

All the dependencies for this bug have been resolved. Can this be closed?

Yes, this bug can be closed.