Neerja Pancholi[:neerja] Assignee
	Description • 8 years ago

      No description provided.

	Jean-Yves Perrier [:teoli]
	Comment 5 • 8 years ago

Neerja, to evaluate the doc needs for this bug, does this bug actually unprefix the properties (keeping the prefixed versions as aliases)?

	Neerja Pancholi[:neerja] Assignee
	Comment 6 • 8 years 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.

	Daniel Holbert [:dholbert]
	Comment 7 • 8 years ago

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

	Neerja Pancholi[:neerja] Assignee
	Comment 8 • 8 years 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?

	Daniel Holbert [:dholbert]
	Comment 9 • 8 years ago

That sounds great! Thanks.

	Neerja Pancholi[:neerja] Assignee
	Comment 11 • 8 years ago

Try run after these changes:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ded03febd68f&selectedJob=28752034

	Neerja Pancholi[:neerja] Assignee
	Comment 12 • 8 years 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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 13 • 8 years 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

	Pulsebot
	Comment 15 • 8 years 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

	Pulsebot
	Comment 17 • 8 years ago

Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/6db12098aaa8
followup to update devtools' property database. r=bustage-fix

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 18 • 8 years ago

Backed out (with the follow-up folded into it) on request by developer and reviewer:

https://hg.mozilla.org/integration/autoland/rev/4c0eb3ab2ff4cf6593e0f44408e73536e0ffd76d

	Daniel Holbert [:dholbert]
	Comment 19 • 8 years ago

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

	Daniel Holbert [:dholbert]
	Comment 20 • 8 years ago

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

	Neerja Pancholi[:neerja] Assignee
	Comment 21 • 8 years 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!)

	Neerja Pancholi[:neerja] Assignee
	Comment 24 • 8 years 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!

	Daniel Holbert [:dholbert]
	Comment 25 • 8 years ago

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.

	Pulsebot
	Comment 27 • 8 years 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

	Daniel Holbert [:dholbert]
	Comment 28 • 8 years ago

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

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 29 • 8 years ago

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

	Daniel Holbert [:dholbert]
	Comment 30 • 8 years ago

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

	Daniel Holbert [:dholbert]
	Comment 31 • 8 years ago

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

	Neerja Pancholi[:neerja] Assignee
	Comment 33 • 8 years 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

	Greg Tatum [:gregtatum]
	Comment 34 • 8 years ago

Looks good to me! I'm working on fixing up that other issue.

	Neerja Pancholi[:neerja] Assignee
	Comment 35 • 8 years ago

Thanks Greg!

	Pulsebot
	Comment 36 • 8 years 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

	Pulsebot
	Comment 37 • 8 years ago

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

	Carsten Book [:Tomcat]
	Comment 38 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/276f973ad01c
https://hg.mozilla.org/mozilla-central/rev/18255363130e

	Kohei Yoshino [:kohei]
	Comment 39 • 8 years ago

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/css3-multi-column-properties-have-been-unprefixed/

	Daniel Holbert [:dholbert]
	Comment 40 • 8 years ago

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/

	Daniel Holbert [:dholbert]
	Comment 41 • 8 years ago

(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

	Chris Mills [:cmills]
	Comment 43 • 7 years ago

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!

	info
	Comment 44 • 7 years 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.

	info
	Comment 45 • 7 years 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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 46 • 7 years ago

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

	Daniel Holbert [:dholbert]
	Comment 47 • 7 years ago

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 )