Bug 65159 – column-count: 0 should not prevent margin-collapse through

• Component: CSS
• Status: RESOLVED FIXED

[Philippe Wittenbergh]

Created attachment 101962 [details]
test case (requires Ahem font)

Since bug 64975, -webkit-column-count: 0 incorrectly prevents collapsing of margins of children with the margins of the parent (multi-col) element.

The spec says (for column-count): "Values must be greater than 0"
http://dev.w3.org/csswg/css3-multicol/#cc

-webkit-column-count: 0 is then invalid and should be ignored.

Test case: there should be no red, and 20px space between the 3 blocks.

Opera 11.5 and Gecko 1.9.2+ handle this correctly.

[Tab Atkins]

Created attachment 167590 [details]
Patch

[Tab Atkins]

Heya eseidel, I moved my work from bug 98557 to here, and fixed this bug with it.

[Eric Seidel (no email)]

Comment on attachment 167590 [details]
Patch

Fantastic.

[Tony Chang]

Comment on attachment 167590 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167590&action=review

> Source/WebCore/css/CSSParser.cpp:2444
> -            validPrimitive = !id && validUnit(value, FInteger | FNonNeg, CSSQuirksMode);
> +            validPrimitive = !id && validUnit(value, FInteger | FPositive, CSSQuirksMode);

I would just use FNonNeg and also check that !value->fValue.  We do that in other parts of the code (see parseAspectRatio) and it's a little weird that you could pass FNonNeg | FPositive to validUnit.

[Tab Atkins]

(In reply to comment #4)
> (From update of attachment 167590 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167590&action=review
> 
> > Source/WebCore/css/CSSParser.cpp:2444
> > -            validPrimitive = !id && validUnit(value, FInteger | FNonNeg, CSSQuirksMode);
> > +            validPrimitive = !id && validUnit(value, FInteger | FPositive, CSSQuirksMode);
> 
> I would just use FNonNeg and also check that !value->fValue.  We do that in other parts of the code (see parseAspectRatio) and it's a little weird that you could pass FNonNeg | FPositive to validUnit.

That's not too weird.  You're just imposing two restrictions, one of which is a subset of the other.  It's no weirder than the fact that you can pass FInteger | FNumber to it (same thing).

I'd prefer to switch the other parts of the code over to FPositive, actually.  It's nice and clear!

Re: parseAspectRatio, whoops, the unofficial spec (my blog post) is imposing a bad restriction.  We purposely avoid open-ended ranges whenever possible, because they expose rounding behavior.  If this actually passed through the CSSWG, that would have been flagged and we would have found another way around it.

[Tab Atkins]

Created attachment 167597 [details]
Patch for landing

[Tony Chang]

Comment on attachment 167597 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=167597&action=review

> That's not too weird.  You're just imposing two restrictions, one of which is a subset of the other.  It's no weirder than the fact that you can pass FInteger | FNumber to it (same thing).

You're right, it's no weirder than what we already do.

> Source/WebCore/css/CSSParser.cpp:1572
> +        // FPositive only defines a closed range when the value is an integer.
> +        ASSERT(FInteger);

This assert doesn't do anything. FInteger is just an enum value. I think you mean unitflags & FInteger.  It would be more clear to just have the flag be FPositiveInteger or something if that's what you want.

[Tab Atkins]

Created attachment 167607 [details]
Patch for landing

[Tab Atkins]

(In reply to comment #7)
> > Source/WebCore/css/CSSParser.cpp:1572
> > +        // FPositive only defines a closed range when the value is an integer.
> > +        ASSERT(FInteger);
> 
> This assert doesn't do anything. FInteger is just an enum value. I think you mean unitflags & FInteger.

Oh, duh.  Sorry about that.

> It would be more clear to just have the flag be FPositiveInteger or something if that's what you want.

Good idea.  I've done so.

[WebKit Review Bot]

Attachment 167607 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/css/CSSParser.cpp:1571:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/css/CSSParser.h:530:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tab Atkins]

Comment on attachment 167607 [details]
Patch for landing

Whoops, sorry, messed up.  One sec.

[Gyuyoung Kim]

Comment on attachment 167607 [details]
Patch for landing

Attachment 167607 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14214461

[kov's GTK+ EWS bot]

Comment on attachment 167607 [details]
Patch for landing

Attachment 167607 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14223380

[Tab Atkins]

Created attachment 167611 [details]
Patch for landing

[Tab Atkins]

(In reply to comment #14)
> Created an attachment (id=167611) [details]
> Patch for landing

All right, ready to go.  Amended with Tony's suggestions, and properly building.

[WebKit Review Bot]

Attachment 167611 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/css/CSSParser.cpp:1571:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/css/CSSParser.h:530:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tab Atkins]

Created attachment 167620 [details]
Patch for landing

[Tab Atkins]

(In reply to comment #17)
> Created an attachment (id=167620) [details]
> Patch for landing

Ugh, I didn't realize land-safely didn't run style checks.  Bleh.  Anyway, it now makes the stylechecker happy.

[Eric Seidel (no email)]

If you'd like land-safely to run the style checks, it shoudl be easy to fix.  Just file a bug and CC me.

http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/tool/commands/upload.py#L224
http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/tool/steps/checkstyle.py

[Tab Atkins]

Created attachment 167663 [details]
Patch for landing

[Tab Atkins]

(In reply to comment #20)
> Created an attachment (id=167663) [details]
> Patch for landing

The bots have cycled around 6 times, and still claim that this patch makes ten thousand tests fail, all across WebKit.  Let's see if prodding them again helps any.

[Tony Chang]

Comment on attachment 167663 [details]
Patch for landing

Looks like this causes a bunch of column tests to fail.

[Tony Chang]

From the cr-linux ews bot:

Regressions: Unexpected image and text failures : (14)
  fast/line-grid/line-grid-inside-columns.html [ Failure ] 
  fast/line-grid/line-grid-into-columns.html [ Failure ] 
  fast/multicol/border-padding-pagination.html [ Failure ] 
  fast/multicol/client-rects.html [ Failure ] 
  fast/multicol/column-break-with-balancing.html [ Failure ] 
  fast/multicol/column-count-with-rules.html [ Failure ] 
  fast/multicol/column-rules-stacking.html [ Failure ] 
  fast/multicol/column-rules.html [ Failure ] 
  fast/multicol/columns-shorthand-parsing.html [ Failure ] 
  fast/multicol/flipped-blocks-border-after.html [ Failure ] 
  fast/multicol/span/anonymous-before-child-parent-crash.html [ Failure ] 
  fast/multicol/span/anonymous-split-block-crash.html [ Failure ] 
  fast/multicol/span/anonymous-style-inheritance.html [ Failure ] 
  fast/multicol/span/before-child-anonymous-column-block.html [ Failure ]

[Tab Atkins]

(In reply to comment #22)
> (From update of attachment 167663 [details])
> Looks like this causes a bunch of column tests to fail.

Yeah, I don't see how, though.  :/

I'll dig into it tomorrow and see what's up - doing spec work today with fantasai.

[Build Bot]

Comment on attachment 167663 [details]
Patch for landing

Attachment 167663 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14244632

New failing tests:
fast/multicol/span/before-child-anonymous-column-block.html
fast/css/remove-shorthand.html
compositing/geometry/composited-in-columns.html
fast/forms/number/number-spinbutton-in-multi-column.html
fast/multicol/span/span-as-immediate-child-property-removal.html
fast/multicol/span/anonymous-split-block-crash.html
fast/multicol/span/span-as-immediate-columns-child-removal.html
fast/line-grid/line-grid-inside-columns.html
fast/multicol/border-padding-pagination.html
fast/multicol/span/anonymous-style-inheritance.html
fast/multicol/span/span-as-immediate-child-generated-content.html
fast/multicol/span/span-as-immediate-columns-child.html
fast/multicol/span/span-as-nested-columns-child-dynamic.html
fast/line-grid/line-grid-into-columns.html
fast/multicol/span/clone-anonymous-block-non-inline-child-crash.html
fast/multicol/span/anonymous-before-child-parent-crash.html
fast/multicol/span/span-as-immediate-columns-child-dynamic.html
fast/multicol/span/generated-child-split-flow-crash.html
fast/dom/Element/getBoundingClientRect.html
fast/events/document-elementFromPoint.html
fast/forms/range/slider-in-multi-column.html
fast/multicol/span/span-as-nested-inline-block-child.html
fast/forms/select/listbox-in-multi-column.html
fast/multicol/span/span-as-immediate-child-complex-splitting.html
fast/loader/javascript-url-in-object.html
fast/block/float/float-not-removed-from-next-sibling4.html
fast/multicol/span/span-as-nested-columns-child.html
fast/multicol/break-properties.html

[WebKit Review Bot]

Comment on attachment 167663 [details]
Patch for landing

Attachment 167663 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14260074

New failing tests:
fast/multicol/hit-test-float.html
fast/css/remove-shorthand.html
fast/multicol/flipped-blocks-border-after.html
fast/forms/number/number-spinbutton-in-multi-column.html
fast/multicol/span/anonymous-before-child-parent-crash.html
fast/multicol/client-rects.html
fast/line-grid/line-grid-inside-columns.html
fast/forms/range/slider-delete-while-dragging-thumb.html
fast/multicol/float-truncation.html
fast/multicol/hit-test-end-of-column.html
fast/multicol/border-padding-pagination.html
fast/line-grid/line-grid-into-columns.html
fast/multicol/column-break-with-balancing.html
fast/forms/range/slider-mouse-events.html
fast/dom/Element/getBoundingClientRect.html
fast/multicol/column-rules-stacking.html
fast/events/document-elementFromPoint.html
fast/forms/range/slider-in-multi-column.html
http/tests/xmlhttprequest/web-apps/001.html
fast/multicol/image-inside-nested-blocks-with-border.html
fast/multicol/hit-test-end-of-column-with-line-height.html
fast/frames/frame-limit.html
fast/forms/select/listbox-in-multi-column.html
fast/multicol/inherit-column-values.html
fast/multicol/column-count-with-rules.html
fast/multicol/columns-shorthand-parsing.html
fast/multicol/column-rules.html
fast/forms/range/slider-onchange-event.html
fast/loader/local-CSS-from-local.html
fast/multicol/break-properties.html

[Tab Atkins]

Created attachment 168076 [details]
Patch

[Tab Atkins]

Comment on attachment 168076 [details]
Patch

Figured out what was wrong - I was modelling my addition after FNonNegative, but that's wrong - that's intended as a check done *after* you've already done the "type" checks.

Instead, I should have modelled it after FInteger, which is a type check itself.

My earlier patches worked because I didn't remove the type check (I just added FNonZero to the existing FInteger type check), so the layering was okay.  When I removed FInteger, we were left with *no* type check, and the conditional structure I was using then wouldn't run the FPositiveInteger check.

[WebKit Review Bot]

Comment on attachment 168076 [details]
Patch

Clearing flags on attachment: 168076

Committed r130997: <http://trac.webkit.org/changeset/130997>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.