Closed Bug 1339298 Opened 3 years ago Closed 3 years ago

Add boilerplate parsing support for CSS3 multicol column-span

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: neerja, Assigned: neerja)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

No description provided.
Assignee: nobody → npancholi
Blocks: column-span
Summary: Add parsing support for CSS3 multicol column-span and define pref 'layout.css.column-span.enabled' to toggle it → Add parsing support for CSS3 multicol column-span
Summary: Add parsing support for CSS3 multicol column-span → Add boilerplate parsing support for CSS3 multicol column-span
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
Keywords: dev-doc-needed
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.
(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
Keywords: dev-doc-needed
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 "= { ... }"
Attachment #8837910 - Flags: review?(dholbert) → 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
Attachment #8837910 - Flags: review?(dholbert) → review+
Keywords: checkin-needed
Needs a rebase, the automation can't manage to apply it anymore.
Flags: needinfo?(npancholi)
Keywords: checkin-needed
(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!
Flags: needinfo?(npancholi)
Keywords: checkin-needed
Odd, Autoland still says it can't rebase, still from conflicts in dom/ipc/ContentPrefs.cpp.
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.
Keywords: checkin-needed
Keywords: checkin-needed
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)
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.)
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/
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)
(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!
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.
(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)"
Keywords: checkin-needed
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!
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/475af30afdad
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Added column-span to the experimental features:
https://developer.mozilla.org/en-US/Firefox/Experimental_features#column-span

Sebastian
You need to log in before you can comment on or make changes to this bug.