Bug 9410 – Support for CSS widows and orphans

• Component: CSS
• Status: RESOLVED FIXED

[David Latapie]

See the attached HTML and PDF. Problem occurs at page #3, on top. The text is in French, but you should have no problem any way — and lipsuming the whole whould have been longer.

The CSS is p {orphans:3 !important;widows:3 !important;}

[David Latapie]

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

[David Latapie]

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

[David Kilzer (:ddkilzer)]

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

[Joe Lewis]

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

[Hayato Ito]

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

[Dean Jackson]

<rdar://problem/12838699>

[Dean Jackson]

Created attachment 178288 [details]
Patch

[Early Warning System Bot]

Comment on attachment 178288 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 178288 [details]
Patch

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

[Build Bot]

Comment on attachment 178288 [details]
Patch

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

[EFL EWS Bot]

Comment on attachment 178288 [details]
Patch

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

[WebKit Review Bot]

Comment on attachment 178288 [details]
Patch

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

[Dean Jackson]

Created attachment 178297 [details]
Patch

[WebKit Review Bot]

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

[Darin Adler]

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?

[Build Bot]

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

[Dean Jackson]

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.

[Dean Jackson]

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).

[Dean Jackson]

(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.

[Dean Jackson]

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

[Allan Sandfeld Jensen]

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.

[Dean Jackson]

(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?