Remove prefixing for CSS3 multicol

NEW
Unassigned

Status

()

P2
normal
8 years ago
a year ago

People

(Reporter: jwir3, Unassigned)

Tracking

(Depends on: 3 bugs, Blocks: 2 bugs, {css3, dev-doc-needed, site-compat})

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments)

(Reporter)

Description

8 years ago
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
(Reporter)

Comment 1

7 years ago
Manual changes to deprefix columns. Automatic changes to follow.
Attachment #584131 - Flags: review?(dbaron)
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.
(Reporter)

Comment 3

7 years ago
(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?)
(Reporter)

Comment 4

7 years ago
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'
Attachment #584980 - Flags: review?(dbaron)
(Reporter)

Updated

7 years ago
Priority: -- → P2
(Reporter)

Updated

7 years ago
Blocks: 629452
(Reporter)

Updated

7 years ago
Depends on: 724978, 743402, 616722
No longer depends on: 616436
(Reporter)

Updated

7 years ago
Blocks: 775235
(Reporter)

Updated

7 years ago
Attachment #584131 - Flags: review?(dbaron)
(Reporter)

Updated

7 years ago
Attachment #584980 - Flags: review?(dbaron)
(Reporter)

Updated

6 years ago
Duplicate of this bug: 876117

Updated

5 years ago
Depends on: 575614
Duplicate of this bug: 976982
Given changing opinions and policies, is there any reason not to unprefix this right now?
Flags: needinfo?(dbaron)
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
Flags: needinfo?(dbaron)

Comment 9

3 years ago
(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
Keywords: dev-doc-needed
Assignee: jaywir3 → npancholi
(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.
Attachment #8778065 - Flags: review?(dbaron)
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 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.)
Attachment #8778065 - Flags: review?(dholbert)
Comment hidden (mozreview-request)
Attachment #8781000 - Flags: review?(dholbert)
Attachment #8778065 - Flags: review?(dholbert)
Attachment #8781000 - Flags: review?(dholbert)
Attachment #8781000 - Flags: review?(dbaron)
Attachment #8778065 - Flags: review?(dbaron)
Depends on: 1295258
Depends on: 1295261
Depends on: 1295271
Depends on: 1299012
No longer depends on: 1299012
Depends on: 1300895
Depends on: 1308587
Depends on: 1308636

Comment 18

2 years ago
(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.
Assignee: npancholi → nobody
You need to log in before you can comment on or make changes to this bug.