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.
Created attachment 167590 [details] Patch
Heya eseidel, I moved my work from bug 98557 to here, and fixed this bug with it.
Comment on attachment 167590 [details] Patch Fantastic.
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.
(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.
Created attachment 167597 [details] Patch for landing
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.
Created attachment 167607 [details] Patch for landing
(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.
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.
Comment on attachment 167607 [details] Patch for landing Whoops, sorry, messed up. One sec.
Comment on attachment 167607 [details] Patch for landing Attachment 167607 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14214461
Comment on attachment 167607 [details] Patch for landing Attachment 167607 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/14223380
Created attachment 167611 [details] Patch for landing
(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.
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.
Created attachment 167620 [details] Patch for landing
(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.
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
Created attachment 167663 [details] Patch for landing
(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.
Comment on attachment 167663 [details] Patch for landing Looks like this causes a bunch of column tests to fail.
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 ]
(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.
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
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
Created attachment 168076 [details] Patch
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.
Comment on attachment 168076 [details] Patch Clearing flags on attachment: 168076 Committed r130997: <http://trac.webkit.org/changeset/130997>
All reviewed patches have been landed. Closing bug.