Closed Bug 1417725 Opened 7 years ago Closed 7 years ago

Add a prefixed alias (behind a pref) for column-span property since all other multicol properties have a -moz- alias

Categories

(Core :: Layout, enhancement, P3)

Product:
Core
Component:
Layout
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: neerja, Assigned: neerja)

References

Details

Attachments

(2 files)

Bug 1417725 - (Gecko) Add -moz-column-span alias for column-span property.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1417725 - (Servo) Add -moz-column-span alias for column-span property.
59 bytes, text/x-review-board-request
emilio
: review+
Details 
The column-span feature is in development (See Bug 616436) and parsing support for un-prefixed column-span was added in Bug 1339298. Since most existing tests and real world usages of column-span are prefixed, add a -moz-column-span alias for column-span for consistency and convenience. 

This alias should be removed whenever the aliases for all the other multicol properties are removed.
Depends on: 1417728
Attachment #8928771 - Flags: review?(dbaron)
I guess this is OK.  Have you seen usage of this in the wild?

Have you checked that the layout.css.column-span.enabled preference controls both the main property and the alias with the servo style system enabled?

Also, do you need to add to the exports.PREFERENCES in devtools/shared/css/generated/properties-db.js to keep the property disabled, or is that unnecessary?  I don't see any other examples of pref-controlled aliases, so I'm not sure.

(Perhaps heycam is a better reviewer for this sort of addition these days?)
Flags: needinfo?(npancholi)
I'm pretty suspicious on the pref stuff, though, since the -moz-column-span addition isn't in a if (IsCSSPropertyPrefEnabled("layout.css.column-span.enabled"))... which means if this patch passed try as-is, then it's incorrectly exposing a property that it shouldn't be.
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #4)
> I'm pretty suspicious on the pref stuff, though, since the -moz-column-span
> addition isn't in a if
> (IsCSSPropertyPrefEnabled("layout.css.column-span.enabled"))... which means
> if this patch passed try as-is, then it's incorrectly exposing a property
> that it shouldn't be.

My original impression here was that the alias and the preference were two different ways of achieving the same thing i.e. not exposing the unprefixed column-span property. But after talking to Dbaron it seems reasonable that a prefixed -moz-column-span exposed without a pref to control it can be interpreted as the implementation of column-span is ready to be tried out by web developers. So, I've added this alias behind the column-span pref and updated the patch. 

 (In reply to David Baron :dbaron: ⌚️UTC-8 from comment #3)
> I guess this is OK.  Have you seen usage of this in the wild?
No, I haven't but I believe we will eventually need this anyway because when I add CSSWG column-span tests to try using import-tests.py, all instances of column-span will be prefixed and so these tests will fail on try (Unless we decide to remove all prefixes for multicol before landing patches for Bug 616436)
Also, this makes it more convenient for testing in case there are instances of only -moz-column-span in the wild. 
 
> Have you checked that the layout.css.column-span.enabled preference controls
> both the main property and the alias with the servo style system enabled?
Yes, with the latest patch I have checked that with both servo on and off, I can see that the pref now controls both column-span and -moz-column-span. 

> Also, do you need to add to the exports.PREFERENCES in
> devtools/shared/css/generated/properties-db.js to keep the property
> disabled, or is that unnecessary?  I don't see any other examples of
> pref-controlled aliases, so I'm not sure.
This also had to be done to add -moz-column-span behind a pref. 

> (Perhaps heycam is a better reviewer for this sort of addition these days?)
Please let me know if I should pass this to heycam.

Thanks!
Flags: needinfo?(npancholi)
Summary: Add prefixed alias for column-span property since all other multicol properties have a -moz- alias → Add prefixed alias behind a pref for column-span property since all other multicol properties have a -moz- alias
Summary: Add prefixed alias behind a pref for column-span property since all other multicol properties have a -moz- alias → Add a prefixed alias (behind a pref) for column-span property since all other multicol properties have a -moz- alias
Attachment #8928771 - Flags: review?(dbaron) → review?(cam)
Passing to heycam for review since dbaron is busy.
Comment on attachment 8928771 [details]
Bug 1417725 - (Gecko) Add -moz-column-span alias for column-span property.

https://reviewboard.mozilla.org/r/200044/#review207956

r=me.  You'll need to split the servo/ change out and land it through a Servo PR.
Attachment #8928771 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #8)

Will do, thanks Cameron!
No longer blocks: column-span
Comment on attachment 8935218 [details]
Bug 1417725 - (Servo) Add -moz-column-span alias for column-span property.

https://reviewboard.mozilla.org/r/206092/#review211830

r=me, thanks!
Attachment #8935218 - Flags: review?(emilio) → review+
Let me know if you need help landing the servo patch, I can help with it.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #13)
> Let me know if you need help landing the servo patch, I can help with it.

Thanks Emilio! Let me know what I need to do to land this for Servo :)

(I updated the Gecko patch with a minor change) Try run looks ok with the latest patch -
https://treeherder.mozilla.org/#/jobs?repo=try&revision=faa9c050c9adf10714953b8c5d5af67683992437&selectedJob=151353901
(In reply to Neerja Pancholi[:neerja] from comment #16)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #13)
> > Let me know if you need help landing the servo patch, I can help with it.
> 
> Thanks Emilio! Let me know what I need to do to land this for Servo :)

Use ni? for the next time, since otherwise it's easy to miss it.

You need to create a pull request to https://github.com/servo/servo, you can see how to use it here[1].

[1]: https://help.github.com/articles/creating-a-pull-request/
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/711ce1005e5e
(Gecko) Add -moz-column-span alias for column-span property. r=heycam on a CLOSED TREE
https://hg.mozilla.org/mozilla-central/rev/711ce1005e5e
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: