Closed Bug 1300895 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: neerja, Assigned: neerja)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(1 file, 2 obsolete files)

Bug 1300895 - Unprefix CSS multi-column properties, but add back prefixed aliases via nsCSSPropAliasList.h
58 bytes, text/x-review-board-request
dbaron
: review+
Details
No description provided.
Summary: Remove -moz- prefix and add aliases for multicol properties → Remove all -moz- prefix and add aliases for prefixed multicol properties
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
(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.)
(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 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 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+
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
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
(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)
Keywords: site-compat
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.
Attachment #8799518 - Attachment is obsolete: true
Attachment #8799518 - Flags: review?(dholbert)
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.)
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)
Thanks Greg!
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
Pushed by kwierso@gmail.com: https://hg.mozilla.org/integration/autoland/rev/18255363130e Touch CLOBBER DONTBUILD CLOSED TREE
https://hg.mozilla.org/mozilla-central/rev/276f973ad01c https://hg.mozilla.org/mozilla-central/rev/18255363130e
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
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
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.
(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 )
Depends on: 1351551
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: