Unprefix css3 multi-column properties (and add back -moz prefixed versions as aliases, for now)

RESOLVED FIXED in Firefox 52

Status

()

Core
Layout
RESOLVED FIXED
8 months ago
26 days ago

People

(Reporter: neerja, Assigned: neerja)

Tracking

(Blocks: 2 bugs, {dev-doc-complete, site-compat})

unspecified
mozilla52
dev-doc-complete, site-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

7 months ago
Summary: Remove -moz- prefix and add aliases for multicol properties → Remove all -moz- prefix and add aliases for prefixed multicol properties
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Neerja, to evaluate the doc needs for this bug, does this bug actually unprefix the properties (keeping the prefixed versions as aliases)?
Flags: needinfo?(npancholi)
Keywords: dev-doc-needed
(Assignee)

Comment 6

7 months ago
(In reply to Jean-Yves Perrier [:teoli] from comment #5)
> Neerja, to evaluate the doc needs for this bug, does this bug actually
> unprefix the properties (keeping the prefixed versions as aliases)?

Yes, that is correct. 
This is a step in the direction towards removing all the multicol prefixes altogether (Bug 698783) and as part of this step we will still have aliases to prefixed multicol properties. Eventually, these too will be removed.
Flags: needinfo?(npancholi)
Summary: Remove all -moz- prefix and add aliases for prefixed multicol properties → Unprefix css3 multi-column properties (and add back -moz prefixed versions as aliases, for now)
Drive-by nit on the commit message:
> Bug 1300895 - Remove -moz- prefix from nsCSSPropList.h and any other
> failing tests, add corresponding aliases to nsCSSPropAliasList.h

This should include the phrase "multi-column properties" somewhere, to make it clearer what it's doing.

Perhaps update to something like:
 "Unprefix CSS multi-column properties, but add back prefixed aliases via nsCSSPropAliasList.h"

(The test tweaks are nice to mention, but they might make the message a bit too long, particularly after we've added the clarifying bit about multi-column properties. So, probably ok to just leave those out of the message.)
(Assignee)

Comment 8

7 months ago
(In reply to Daniel Holbert [:dholbert] from comment #7)
> Drive-by nit on the commit message:
> > Bug 1300895 - Remove -moz- prefix from nsCSSPropList.h and any other
> > failing tests, add corresponding aliases to nsCSSPropAliasList.h
> 
> This should include the phrase "multi-column properties" somewhere, to make
> it clearer what it's doing.
> 
> Perhaps update to something like:
>  "Unprefix CSS multi-column properties, but add back prefixed aliases via
> nsCSSPropAliasList.h"
> 
> (The test tweaks are nice to mention, but they might make the message a bit
> too long, particularly after we've added the clarifying bit about
> multi-column properties. So, probably ok to just leave those out of the
> message.)

Ok, I changed the commit message to what you suggested. It should reflect the next time I push for review. I will add a summary of the test related changes in the lines following the commit message (so that the commit message itself isn't too long) Does that work?
That sounds great! Thanks.
Comment hidden (mozreview-request)
(Assignee)

Comment 11

7 months ago
Try run after these changes:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ded03febd68f&selectedJob=28752034
(Assignee)

Comment 12

7 months ago
mozreview-review
Comment on attachment 8797845 [details]
Bug 1300895 - Unprefix CSS multi-column properties, but add back prefixed aliases via nsCSSPropAliasList.h

https://reviewboard.mozilla.org/r/83446/#review82886

::: layout/style/nsComputedDOMStylePropertyList.h:108
(Diff revision 5)
>  COMPUTED_STYLE_PROP(caption_side,                  CaptionSide)
>  COMPUTED_STYLE_PROP(clear,                         Clear)
>  COMPUTED_STYLE_PROP(clip,                          Clip)
>  COMPUTED_STYLE_PROP(color,                         Color)
>  COMPUTED_STYLE_PROP(color_adjust,                  ColorAdjust)
> +COMPUTED_STYLE_PROP(column_count,                  ColumnCount)

Had to move these properties here because "layout/style/test/test_bug657143.html" expects all properties to be grouped into specific chunks in order i.e. first all CSS unprefixed, then moz-prefixed ... etc.

Comment 13

7 months ago
mozreview-review
Comment on attachment 8797845 [details]
Bug 1300895 - Unprefix CSS multi-column properties, but add back prefixed aliases via nsCSSPropAliasList.h

https://reviewboard.mozilla.org/r/83446/#review82914

r=dbaron with the following comment addressed.

You should also send an intent to ship to dev-platform if you haven't done so already, and that intent to ship should (a) reference this bug (and maybe also a metabug if there's a good one) and (b) mention the lack of column-span.

::: layout/style/test/test_transitions_per_property.html:70
(Diff revision 5)
> -    "-moz-column-count": [ test_pos_integer_or_auto_transition,
> +    "column-count": [ test_pos_integer_or_auto_transition,
>                             test_integer_at_least_one_clamping ],
> -    "-moz-column-gap": [ test_length_transition,
> +    "column-gap": [ test_length_transition,
>                           test_length_clamped ],
> -    "-moz-column-rule-color": [ test_color_transition,
> +    "column-rule-color": [ test_color_transition,
>                                  test_true_currentcolor_transition ],
> -    "-moz-column-rule-width": [ test_length_transition,
> +    "column-rule-width": [ test_length_transition,
>                                  test_length_clamped ],
> -    "-moz-column-width": [ test_length_transition,
> +    "column-width": [ test_length_transition,
>                             test_length_clamped ],

please fix the indentation of the second entries in these arrays to continue to line up with the first
Attachment #8797845 - Flags: review?(dbaron) → review+
Comment hidden (mozreview-request)

Comment 15

7 months ago
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1982d92ae180
Unprefix CSS multi-column properties, but add back prefixed aliases via nsCSSPropAliasList.h r=dbaron
(Assignee)

Comment 16

7 months ago
Created attachment 8799055 [details] [diff] [review]
Bug 1300895: followup to update devtools' property database
Attachment #8799055 - Flags: review+

Comment 17

7 months ago
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/6db12098aaa8
followup to update devtools' property database. r=bustage-fix
Backed out (with the follow-up folded into it) on request by developer and reviewer:

https://hg.mozilla.org/integration/autoland/rev/4c0eb3ab2ff4cf6593e0f44408e73536e0ffd76d
Flags: needinfo?(npancholi)
Comment on attachment 8799055 [details] [diff] [review]
Bug 1300895: followup to update devtools' property database

Aryx initially landed this for us, but I'm very uneasy about it because this devtools file is impossible to usefully diff / audit, which makes me uncomfortable taking changes to it in a bustage fix.  e.g. this file has lines that are over 100,000 characters long, which makes it impossible to know whether (autogenerated) changes to it make any sense at all.

Let's fix this file in bug 1308651 so that it can be usefully audited, and then update the main patch here to update this test file alongside the other test files, and then re-land. (Hopefully on Monday or early next week, if we can get the file format fixed quickly.)
Attachment #8799055 - Attachment is obsolete: true
I think for now, we're gated on a fix to bug 1308651 (which hopefully can come soon -- if gtatum can't get to it soon, perhaps neerja can reverse-engineer how the file is generated, and tweak the script to include some conveniently-placed newlines characters, so that changes can be represented in more targeted patches/hg-changesets)
--> adding dependency
Depends on: 1308651
(Assignee)

Comment 21

7 months ago
(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #13)
> Comment on attachment 8797845 [details]
> Bug 1300895 - Unprefix CSS multi-column properties, but add back prefixed
> aliases via nsCSSPropAliasList.h
> 
> https://reviewboard.mozilla.org/r/83446/#review82914
> 
> r=dbaron with the following comment addressed.
> 
> You should also send an intent to ship to dev-platform if you haven't done
> so already, and that intent to ship should (a) reference this bug (and maybe
> also a metabug if there's a good one) and (b) mention the lack of
> column-span.
> 
> ::: layout/style/test/test_transitions_per_property.html:70
> (Diff revision 5)
> > -    "-moz-column-count": [ test_pos_integer_or_auto_transition,
> > +    "column-count": [ test_pos_integer_or_auto_transition,
> >                             test_integer_at_least_one_clamping ],
> > -    "-moz-column-gap": [ test_length_transition,
> > +    "column-gap": [ test_length_transition,
> >                           test_length_clamped ],
> > -    "-moz-column-rule-color": [ test_color_transition,
> > +    "column-rule-color": [ test_color_transition,
> >                                  test_true_currentcolor_transition ],
> > -    "-moz-column-rule-width": [ test_length_transition,
> > +    "column-rule-width": [ test_length_transition,
> >                                  test_length_clamped ],
> > -    "-moz-column-width": [ test_length_transition,
> > +    "column-width": [ test_length_transition,
> >                             test_length_clamped ],
> 
> please fix the indentation of the second entries in these arrays to continue
> to line up with the first

Thanks David. I've made this change. 

Also, the link to the "Intent-to-ship" for dev-platform is here:
https://groups.google.com/forum/#!topic/mozilla.dev.platform/EammrHjrCpw
(Thanks a lot to dholbert for helping to put this together!)
Flags: needinfo?(npancholi)

Updated

7 months ago
Keywords: site-compat
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 24

7 months ago
Hi Daniel, 

This is a try run for the "Followup to update devtools' property database" patch before the fix for properties-db.js formatting (Bug 1308651) was applied. I did not re-run try after this formatting fix + rebase on top of autoland patches since I think the result will be equivalent. Let me know if you need me to rerun this.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd2c197bbe70

Thanks!
This looks good! Two notes:
 (1) please fold the followup into the main patch, as we just discussed. (The changes vs. the last reviewed version are small/obvious enough that I'm sure dbaron is OK with you carrying forward r=dbaron, particularly since I've sanity-checked them as well.)

 (2) the followup does include an unrelated chunk for "layout.css.masking.enabled".  That seems to be the script "catching up" this generated file to changes that already happened in bug 1308239.  Ideally we might want to split that change out (e.g. run the script before the rest of your changes here [which would probably just produce this one masking tweak], commit that, then apply the rest of your changes and run the script again).  But it doesn't matter too much, so let's not worry about it.
Comment hidden (mozreview-request)
(Assignee)

Updated

7 months ago
Attachment #8799518 - Attachment is obsolete: true
Attachment #8799518 - Flags: review?(dholbert)

Comment 27

7 months ago
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b7c0df58a2f4
Unprefix CSS multi-column properties, but add back prefixed aliases via nsCSSPropAliasList.h r=dbaron
(In reply to Daniel Holbert [:dholbert] from comment #25)
>  (2) the followup does include an unrelated chunk for
> "layout.css.masking.enabled" [...] let's not worry about it.

(Following up on this -- that about:config pref metadata is probably all getting removed from this devtools file soon anyway, per bug 1309040, and that means we shouldn't have many more instances of "catch-up" tweaks like this getting rolled into other unrelated patches. Which is great.)
Backed out for xpcshell failure in /test_css-properties-db.js:

https://hg.mozilla.org/integration/autoland/rev/9972b1dd43309d6166ca19fda41bd82e3d78e712

Push with failures: https://treeherder.mozilla.org/logviewer.html#?job_id=4816361&repo=autoland
Failure log: https://queue.taskcluster.net/v1/task/W0ra4A2pQOOhI9qvktpiDg/runs/0/artifacts/public%2Flogs%2Flive_backing.log

[task 2016-10-10T22:27:49.260746Z] 22:27:49  WARNING -  TEST-UNEXPECTED-FAIL | devtools/shared/tests/unit/test_css-properties-db.js | run_test/< - [run_test/< : 79] The static database and platform do not agree on the property "column-count" If this assertion fails, then the client side CSS properties list in devtools is out of sync with the CSS properties on the platform. To fix this assertion run `mach devtools-css-db` to re-generate the client side properties. - false == true
Flags: needinfo?(npancholi)
Yeah, it looks like that test is operating on the top chunk of the properties-db.js file (the exports.CSS_PROPERTIES definition), which the script didn't update for some reason.

In the latest version of the patch here, the (script-generated) unprefixing changes to properties-db.js are all in exports.PREFERENCES, which don't matter for this test.  So, we need to figure out why the script isn't updating exports.CSS_PROPERTIES, and fix it to do that (perhaps in a helper-bug).
(I filed bug 1309065 on addressing part of comment 30.  In the meantime, it seems like we can make "./mach devtools-css-db" produce a more useful/complete update [for the purposes of this bug], if we clear the data from its structures before running it.)
Comment hidden (mozreview-request)
(Assignee)

Comment 33

7 months ago
Hi Greg,

I generated a new properties-db.js file using the workaround that dholbert mentioned in Bug 1309065 comment#1 (at the end) and uploaded it here. Can you please sanity check this file for us to make sure that this might be a feasible work around? I ran this xpcshell test locally and I do see that it passes after this patch. 

There is also a try run for this at:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca938d82b741

Thanks!
Neerja
Flags: needinfo?(npancholi) → needinfo?(gtatum)
Looks good to me! I'm working on fixing up that other issue.
Flags: needinfo?(gtatum)
(Assignee)

Comment 35

7 months ago
Thanks Greg!

Comment 36

7 months ago
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/276f973ad01c
Unprefix CSS multi-column properties, but add back prefixed aliases via nsCSSPropAliasList.h r=dbaron

Comment 37

7 months ago
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/18255363130e
Touch CLOBBER DONTBUILD CLOSED TREE

Comment 38

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/276f973ad01c
https://hg.mozilla.org/mozilla-central/rev/18255363130e
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/css3-multi-column-properties-have-been-unprefixed/
Hmm - as discussed on the dev.platform thread[1], we probably need to back this out until we've got "column-span" implemented (in bug 616436).

In the 1 day that this has been on Nightly, we've already gotten a report[2] of a site[3] that uses unprefixed multicolumn CSS and (reasonably) depends on "column-span:all" in order for its other multicolumn styles to produce a nice rendering.  This bug's patches make us honor *some* of the site's unprefixed multicolumn CSS (but not its "column-span" styles), and so we end up producing a broken rendering as a result.

This suggests that we can't gracefully ship unprefixed multicolumn properties until we've fixed bug 616436 -- and we probably need to backout here, for the time being.

[1] https://groups.google.com/d/msg/mozilla.dev.platform/EammrHjrCpw/86F78swTBAAJ
[2] https://github.com/webcompat/web-bugs/issues/3429
[3] https://www.w3.org/blog/CSS/2016/10/13/minutes-telecon-302/
(Though, as implied by dbaron's response[4] on the thread, maybe this CSS Working Group blog is somewhat unique in relying on "column-span" so heavily.  I withdraw my "we probably need to back this out" - perhaps better to let this bake for a bit longer & see if there's any other fallout.)

[4] https://groups.google.com/d/msg/mozilla.dev.platform/EammrHjrCpw/7QLRxrYaBAAJ
Comment hidden (advocacy)
I've updated the browser support information on the reference pages:
https://developer.mozilla.org/en-US/docs/Web/CSS/column-count
https://developer.mozilla.org/en-US/docs/Web/CSS/column-fill
https://developer.mozilla.org/en-US/docs/Web/CSS/column-gap
https://developer.mozilla.org/en-US/docs/Web/CSS/column-rule
https://developer.mozilla.org/en-US/docs/Web/CSS/column-rule-color
https://developer.mozilla.org/en-US/docs/Web/CSS/column-rule-style
https://developer.mozilla.org/en-US/docs/Web/CSS/column-rule-width
https://developer.mozilla.org/en-US/docs/Web/CSS/column-width
https://developer.mozilla.org/en-US/docs/Web/CSS/columns

I've also added a note about the unprefixing to
https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Columns/Using_multi-column_layouts

I've also ensured that a note has been added to the Fx52 release notes:
https://developer.mozilla.org/en-US/Firefox/Releases/52

Let me know that's OK!
Keywords: dev-doc-needed → dev-doc-complete
Blocks: 775235

Comment 44

2 months ago
There is a bug on Firefox 52 with CSS columns. If the container has column-fill: auto it breaks the columns. It was not breaking the columns in version 51.0.1.

Comment 45

2 months ago
(In reply to info from comment #44)
> There is a bug on Firefox 52 with CSS columns. If the container has
> column-fill: auto it breaks the columns. It was not breaking the columns in
> version 51.0.1.

  -moz-column-fill: balance
  column-fill: auto
(In reply to info from comment #45)
>   -moz-column-fill: balance
>   column-fill: auto

If that's what you specified, then you'll get different behavior in 51 and 52, because 51 will get 'balance' and 52 (which supports unprefixed 'column-fill') will get 'auto'.
Put another way: if you've got comment 45's CSS and you want the Firefox 51 behavior, then you should just have:
  -moz-column-fill: balance
  column-fill: balance

(Alternately/also, if you do really want "auto" and it does the right thing in other browser engines but not in Firefox 52, then please file a bug about that and mention the bug number here:
 https://bugzilla.mozilla.org/enter_bug.cgi?product=Core&component=Layout )

Updated

26 days ago
Depends on: 1351551
You need to log in before you can comment on or make changes to this bug.