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

      No description provided.

	Sebastian Zartner [:sebo]
	Comment 1 • 7 years ago

Once the boilerplate is implemented, this needs to be mentioned at https://developer.mozilla.org/en-US/Firefox/Experimental_features and the compatibility note for Firefox at https://developer.mozilla.org/en-US/docs/Web/CSS/column-span needs to be updated to reflect that this feature is behind a preference.

Sebastian

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

This bug is *just* about the style-system aspect (parsing the property, & being able to correctly report the computed value).  The actual layout/rendering changes will happen elsewhere (in bug 616436, or a dependent bug).  And the style-system code will be default-disabled in all builds until then (so that the in-progress implementation isn't detectable to web content).

I'd say it's probably not useful to update any MDN pages to publicize incomplete support for this feature until we've actually updated our layout code to react to it & render something useful from it.

	Sebastian Zartner [:sebo]
	Comment 4 • 7 years ago

(In reply to Daniel Holbert [:dholbert] from comment #2)
> This bug is *just* about the style-system aspect (parsing the property, &
> being able to correctly report the computed value).  The actual
> layout/rendering changes will happen elsewhere (in bug 616436, or a
> dependent bug).  And the style-system code will be default-disabled in all
> builds until then (so that the in-progress implementation isn't detectable
> to web content).
> 
> I'd say it's probably not useful to update any MDN pages to publicize
> incomplete support for this feature until we've actually updated our layout
> code to react to it & render something useful from it.

You're right. It makes more sense to publicize it when there's actually something to show.

Sebastian

	Daniel Holbert [:dholbert]
	Comment 5 • 7 years ago
mozreview-review

Comment on attachment 8837910 [details]
Bug 1339298 - Add boilerplate code for CSS3 multicol column-span parsing and define pref 'layout.css.column-span.enabled' to toggle it.

https://reviewboard.mozilla.org/r/112904/#review114590

This is nearly there! Just some minor nits -- and I'll take a quick final look after nits are addressed; hence, marking r- for the moment.

::: layout/style/nsComputedDOMStyle.h:513
(Diff revision 1)
>    already_AddRefed<CSSValue> DoGetColumnCount();
> +  already_AddRefed<CSSValue> DoGetColumnSpan();
>    already_AddRefed<CSSValue> DoGetColumnFill();
>    already_AddRefed<CSSValue> DoGetColumnWidth();

Let's bump the new line here to be just after "DoGetColumnFill", for more-consistent ordering (most of this patch is inserting column-span stuff directly after column-fill stuff).

(Also, this'll keep these 4 properties alphabetically sorted, which is kind of nice.)

::: layout/style/nsComputedDOMStyle.cpp:1189
(Diff revision 1)
>  already_AddRefed<CSSValue>
> +nsComputedDOMStyle::DoGetColumnSpan()
> +{

Same here -- please move the implementation of this function just below the implementation of DoGetColumnFill(), for consistency.

::: layout/style/nsRuleNode.cpp:9283
(Diff revision 1)
>             column->mColumnFill, conditions,
>             SETVAL_ENUMERATED | SETVAL_UNSET_INITIAL,
>             parent->mColumnFill,
>             NS_STYLE_COLUMN_FILL_BALANCE);
>  
> +  // column-span: all, none

Just say "enum" in the comment here, like we do for column-fill.

(I believe the comment is meant to document "what type of *units* do we expect this property to have".  And in this case, all the values are enums (in our keyword table), rather than being e.g. the eCSSUnit_None special unit.)

::: layout/style/nsStyleStruct.h:3577
(Diff revision 1)
>    uint8_t      mColumnFill;  // [reset] see nsStyleConsts.h
> +  uint8_t      mColumnSpan;  // [reset], none

Just copypaste the mColumnFill code-comment here.

(These comments are meant to document "what are the values that this variable can take". We typically say "see nsStyleConsts.h" for enumerated values whose numeric values are defined in that header. And that's the case here, so let's just use the standard comment.)

::: layout/style/nsStyleStruct.cpp:820
(Diff revision 1)
>        mColumnCount != aNewData.mColumnCount) {
>      // We force column count changes to do a reframe, because it's tricky to handle
>      // some edge cases where the column count gets smaller and content overflows.
>      // XXX not ideal
>      return nsChangeHint_ReconstructFrame;
>    }
>  
> +  if (mColumnSpan != aNewData.mColumnSpan) {
> +    return nsChangeHint_ReconstructFrame;
> +  }

Let's just merge this into the existing "if" check before it (which already considers some other properties and has the same result).

::: layout/style/test/property_database.js:7811
(Diff revision 1)
> +  gCSSProperties["column-span"] = {
> +  domProp: "columnSpan",
> +  inherited: false,
> +  type: CSS_TYPE_LONGHAND,
> +  initial_values: [ "none" ],
> +  other_values: [ "all" ],
> +  invalid_values: [ "-1", "0", "auto", "2px" ]

Please add 2 more spaces of indentation to the "domProp", "inherited", etc. lines (the whole block inside of "= { ... }"

	Daniel Holbert [:dholbert]
	Comment 7 • 7 years ago
mozreview-review

Comment on attachment 8837910 [details]
Bug 1339298 - Add boilerplate code for CSS3 multicol column-span parsing and define pref 'layout.css.column-span.enabled' to toggle it.

https://reviewboard.mozilla.org/r/112904/#review114622

Looks good - thanks! r=me

	Phil Ringnalda (:philor)
	Comment 8 • 7 years ago

Needs a rebase, the automation can't manage to apply it anymore.

	Neerja Pancholi[:neerja] Assignee
	Comment 10 • 7 years ago

(In reply to Phil Ringnalda (:philor) from comment #8)
> Needs a rebase, the automation can't manage to apply it anymore.

I rebased this. Resetting checkin-needed. Thanks Phil!

	Phil Ringnalda (:philor)
	Comment 11 • 7 years ago

Odd, Autoland still says it can't rebase, still from conflicts in dom/ipc/ContentPrefs.cpp.

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

ContentPrefs.cpp actually just got a disclaimer added at the top saying that changes to that file need DOM peer review.  Discussing with billm in #developers right now, to sanity-check whether we even need that change after all.

	Mozilla Autoland
	Comment 14 • 7 years ago

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s ec77dec02540 -d e87b41eb5ac0: rebasing 378700:ec77dec02540 "Bug 1339298 - Add boilerplate code for CSS3 multicol column-span parsing and define pref 'layout.css.column-span.enabled' to toggle it. r=dholbert" (tip)
merging dom/ipc/ContentPrefs.cpp
merging layout/style/nsCSSPropList.h
merging layout/style/nsCSSProps.cpp
merging layout/style/nsCSSProps.h
merging layout/style/nsComputedDOMStyle.cpp
merging layout/style/nsComputedDOMStyle.h
merging layout/style/nsComputedDOMStylePropertyList.h
merging layout/style/nsRuleNode.cpp
merging layout/style/nsStyleConsts.h
merging layout/style/nsStyleStruct.cpp
merging layout/style/nsStyleStruct.h
merging layout/style/test/property_database.js
merging modules/libpref/init/all.js
warning: conflicts while merging dom/ipc/ContentPrefs.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

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

MozReview still shows that this patch is making whitespace-only changes to ContentPrefs.cpp:
  https://reviewboard.mozilla.org/r/112904/diff/4/
Unfortunately it doesn't show *what* those changes are -- but the direct view of the hg commit kinda does -- it shows all of the lines being removed and then re-added, for some reason:
 https://reviewboard-hg.mozilla.org/gecko/rev/04dfc4a7af7af36fe49533c35e394deb1a3b8e9d#l1.1

Maybe your text editor or mercurial client is changing the line-ending character for every single line in that file, after touching it? Not sure. Might be worth doing, too, but not as part of this patch. :)  (And that makes sense for why it's hitting conflicts -- any changes to that patch would conflict with such a change.)

So:
 - please revert all changes to ContentPrefs.cpp here (including whitespace changes), with "hg revert" or similar
 - Also, let's wait until Monday to land this -- the Firefox version number is getting bumped that day, and we might as well hold this new property until after the version-number increase, so that this is more likely to ship in the same version where the layout implementation of this property lands.  (That'll reduce the possibility of confusion, for users who go poking around in about:config.  Not a huge deal, since it's preffed off by default; I'm only mentioning it because the version number bump is literally a few days away at this point.)

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

Thanks -- looks like this latest version doesn't touch ContentPrefs.cpp, so it should be good to land on ~Monday.

Incidentally -- following up on the whitespace changes themselves:
(In reply to Daniel Holbert [:dholbert] from comment #15)
> Maybe your text editor or mercurial client is changing the line-ending
> character for every single line in that file, after touching it? Not sure.
> Might be worth doing, too, but not as part of this patch. :)

I opened version 4 of the patch[1] in emacs, and it shows that the patch was indeed *adding* DOS newline characters to the end of every line. We'd prefer not to do that. emacs shows these changes like so (the ^M is a stray DOS newline character):
-const char* mozilla::dom::ContentPrefs::gInitPrefs[] = {
+const char* mozilla::dom::ContentPrefs::gInitPrefs[] = {^M

If it's possible to figure out what part of your toolchain introduced those stray DOS endline characters, that would be worth fixing too, to keep that from happening in the future.

Some initial detective work to track down how/why these were added:
 - The first affected version was "version 3"[2] (from comment 9).  (The previous version, version 2, just shows just a 1-line insertion in ContentPrefs.cpp, in its raw-patch view. But version 3 shows every line being added and removed in its raw-patch-view, along with the 1-line insertion.)
 - The mercurial history of ContentPrefs.cpp shows that it *used to have DOS line endings* in the tree, e.g. as of mozilla-central revision de301f4f1519.  But those have since been fixed.
 - So something about the rebase operation in comment 9 must've changed your commit to restore those (gone-in-current-mozilla-central) DOS line endings.

[1] https://reviewboard.mozilla.org/r/112904/diff/4/raw/
[2] https://reviewboard.mozilla.org/r/112904/diff/3/raw/

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

Side note on this end-of-line character stuff: this bug prompted me to file:
 - bug 1344010 on better-exposing end-of-line-character changes in MozReview
 - bug 1343975 on getting rid of all remaining DOS end-of-line characters from mozilla C++ code (patch up, awaiting review)

	Neerja Pancholi[:neerja] Assignee
	Comment 19 • 7 years ago

(In reply to Daniel Holbert [:dholbert] from comment #17)
> Thanks -- looks like this latest version doesn't touch ContentPrefs.cpp, so
> it should be good to land on ~Monday.
> 
> Incidentally -- following up on the whitespace changes themselves:
> (In reply to Daniel Holbert [:dholbert] from comment #15)
> > Maybe your text editor or mercurial client is changing the line-ending
> > character for every single line in that file, after touching it? Not sure.
> > Might be worth doing, too, but not as part of this patch. :)
> 
> I opened version 4 of the patch[1] in emacs, and it shows that the patch was
> indeed *adding* DOS newline characters to the end of every line. We'd prefer
> not to do that. emacs shows these changes like so (the ^M is a stray DOS
> newline character):
> -const char* mozilla::dom::ContentPrefs::gInitPrefs[] = {
> +const char* mozilla::dom::ContentPrefs::gInitPrefs[] = {^M
> 
> If it's possible to figure out what part of your toolchain introduced those
> stray DOS endline characters, that would be worth fixing too, to keep that
> from happening in the future.
> 
> Some initial detective work to track down how/why these were added:
>  - The first affected version was "version 3"[2] (from comment 9).  (The
> previous version, version 2, just shows just a 1-line insertion in
> ContentPrefs.cpp, in its raw-patch view. But version 3 shows every line
> being added and removed in its raw-patch-view, along with the 1-line
> insertion.)
>  - The mercurial history of ContentPrefs.cpp shows that it *used to have DOS
> line endings* in the tree, e.g. as of mozilla-central revision de301f4f1519.
> But those have since been fixed.
>  - So something about the rebase operation in comment 9 must've changed your
> commit to restore those (gone-in-current-mozilla-central) DOS line endings.
> 
> [1] https://reviewboard.mozilla.org/r/112904/diff/4/raw/
> [2] https://reviewboard.mozilla.org/r/112904/diff/3/raw/

Thanks for catching this! I'm guessing that Diffmerge caused this since version '3'
of the patch was the rebased one, as you noted. Diffmerge did pop a UI to show me the merged
result but since the preference to "Ignore line endings" was on, I didn't see any problems.
I've disabled any setting that ignore line ending (or whitespaces or tabs) so I'll notice 
if this happens again. On diffing with revision de301f4f1519 I do see that it detects the 
difference in line endings. (Also, I'll make sure to recheck the diff in moz-review to be sure)
Will rebase this again on Monday and try to autoland like you suggested.
Thanks!

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

Sounds good - thanks for digging into that!  It totally makes sense that a graphical merge tool like DiffMerge might hide these away when doing a rebase operation.

	Neerja Pancholi[:neerja] Assignee
	Comment 22 • 7 years ago

(In reply to Neerja[:neerja] from comment #21)
> Comment on attachment 8837910 [details]
> Bug 1339298 - Add boilerplate code for CSS3 multicol column-span parsing and
> define pref 'layout.css.column-span.enabled' to toggle it.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/112904/diff/5-6/

This change is for adjusting this to use discrete animation rather than no animation, since I just realized that's what the spec calls for (and per discussion with dholbert)"

	Neerja Pancholi[:neerja] Assignee
	Comment 24 • 7 years ago

As discussed in comment 19, I rebased this patch again and checked the diff. It looks identical to before as none of these files haven been changed since 3 days ago. 
Please checkin, thanks!

	Pulsebot
	Comment 25 • 7 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/475af30afdad
Add boilerplate code for CSS3 multicol column-span parsing and define pref 'layout.css.column-span.enabled' to toggle it. r=dholbert

	Carsten Book [:Tomcat]
	Comment 26 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/475af30afdad

	Sebastian Zartner [:sebo]
	Comment 27 • 7 years ago

Added column-span to the experimental features:
https://developer.mozilla.org/en-US/Firefox/Experimental_features#column-span

Sebastian