Bug 9410

Created attachment 8818 [details] Test - PDF, French, orphan

Created attachment 8819 [details] Test - XHTML, French, orphan

Confirming bug. Looks like Opera is the only browser that supports this per Blooberry.

Adding my vote - would like to see support of missing print properties (@page page-break-inside, orphans, widows)

I am working on this. Please see a comment at: https://bugs.webkit.org/show_bug.cgi?id=34155#c1

<rdar://problem/12838699>

Created attachment 178288 [details] Patch

Comment on attachment 178288 [details] Patch Attachment 178288 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15182696

Comment on attachment 178288 [details] Patch Attachment 178288 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15187467

Comment on attachment 178288 [details] Patch Attachment 178288 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15189454

Comment on attachment 178288 [details] Patch Attachment 178288 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15175863

Comment on attachment 178288 [details] Patch Attachment 178288 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15182707

Created attachment 178297 [details] Patch

Comment on attachment 178297 [details] Patch Attachment 178297 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15189557 New failing tests: fast/events/ondrop-text-html.html editing/pasteboard/onpaste-text-html.html svg/css/getComputedStyle-basic.xhtml

Comment on attachment 178297 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178297&action=review > Source/WebCore/css/CSSParser.cpp:2157 > + case CSSPropertyOrphans: // <integer> | inherit | auto (We've added support for auto for backwards compatibility) > + case CSSPropertyWidows: // <integer> | inherit | auto (Ditto) Auto seems like a very strange way to say "do nothing" here. Is there some better way to say "don't try to stop these"? > Source/WebCore/rendering/RenderBlock.cpp:6778 > +void RenderBlock::setBreakAtLineToAvoidWidow(RootInlineBox* lineToBreak) We should assert this is non-zero, since the function to clear is separate. > Source/WebCore/rendering/RenderBlock.h:310 > + bool shouldBreakAtLineToAvoidWidow() const { return m_rareData ? m_rareData->m_shouldBreakAtLineToAvoidWidow : false; } I like && for expressions like this instead of ? :, what do you think? > Source/WebCore/rendering/RenderBlock.h:317 > + void clearShouldBreakAtLineToAvoidWidow() const > + { > + if (!m_rareData) > + return; > + m_rareData->m_shouldBreakAtLineToAvoidWidow = false; > + m_rareData->m_lineBreakToAvoidWidow = 0; > + } Even if you want this to be inline, I think it would be better to put this outside the class definition in the header, to keep things tighter. But also, does this really need to be inline?

Comment on attachment 178297 [details] Patch Attachment 178297 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15218325 New failing tests: svg/css/getComputedStyle-basic.xhtml

Comment on attachment 178297 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178297&action=review >> Source/WebCore/css/CSSParser.cpp:2157 >> + case CSSPropertyWidows: // <integer> | inherit | auto (Ditto) > > Auto seems like a very strange way to say "do nothing" here. Is there some better way to say "don't try to stop these"? I originally thought to use "none", but I was worried that it implied avoiding widows and orphans, rather than ignoring them. There is a value keyword "ignore". I think I'll use that instead! Asking Ted if he agrees. >> Source/WebCore/rendering/RenderBlock.cpp:6778 >> +void RenderBlock::setBreakAtLineToAvoidWidow(RootInlineBox* lineToBreak) > > We should assert this is non-zero, since the function to clear is separate. OK >> Source/WebCore/rendering/RenderBlock.h:310 >> + bool shouldBreakAtLineToAvoidWidow() const { return m_rareData ? m_rareData->m_shouldBreakAtLineToAvoidWidow : false; } > > I like && for expressions like this instead of ? :, what do you think? Good point. >> Source/WebCore/rendering/RenderBlock.h:317 >> + } > > Even if you want this to be inline, I think it would be better to put this outside the class definition in the header, to keep things tighter. But also, does this really need to be inline? I don't think it should be inline.

Comment on attachment 178297 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178297&action=review >>> Source/WebCore/css/CSSParser.cpp:2157 >>> + case CSSPropertyWidows: // <integer> | inherit | auto (Ditto) >> >> Auto seems like a very strange way to say "do nothing" here. Is there some better way to say "don't try to stop these"? > > I originally thought to use "none", but I was worried that it implied avoiding widows and orphans, rather than ignoring them. There is a value keyword "ignore". I think I'll use that instead! Asking Ted if he agrees. Ted didn't like "ignore" and suggests "auto" is the right thing to do here (if you read it as letting the user agent make the choice).

(In reply to comment #14) > New failing tests: > fast/events/ondrop-text-html.html > editing/pasteboard/onpaste-text-html.html I'm not sure why these would fail. > svg/css/getComputedStyle-basic.xhtml I have fixed this.

Committed r137200: <http://trac.webkit.org/changeset/137200>

Comment on attachment 178297 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178297&action=review > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1699 > + // We now want to break at this line. Remember for next layout and trigger relayout. > + setBreakAtLineToAvoidWidow(lineBox); > + markLinesDirtyInBlockRange(lastRootBox()->lineBottomWithLeading(), lineBox->lineBottomWithLeading(), lineBox); Isn't this going to break if it happens on two different page-breaks? The first causes any later to be invalid.

(In reply to comment #21) > (From update of attachment 178297 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=178297&action=review > > > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1699 > > + // We now want to break at this line. Remember for next layout and trigger relayout. > > + setBreakAtLineToAvoidWidow(lineBox); > > + markLinesDirtyInBlockRange(lastRootBox()->lineBottomWithLeading(), lineBox->lineBottomWithLeading(), lineBox); > > Isn't this going to break if it happens on two different page-breaks? The first causes any later to be invalid. I think the latter will still break. All it means is on the next layout, make sure we add a pagination strut at the marked line. It as if that line doesn't fit on the page. Normal layout happens from then on. Or do I misunderstand?