Bug 9410 - Support for CSS widows and orphans
: Support for CSS widows and orphans
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: CSS
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Dean Jackson
http://www.blooberry.com/indexdot/css...
: InRadar
Depends on: 40821
Blocks: 39735
  Show dependency treegraph
 
Reported: 2006-06-11 21:24 PDT by David Latapie
Modified: 2012-12-10 20:49 PST (History)
17 users (show)

See Also:


Attachments
Test - PDF, French, orphan (45.65 KB, application/pdf)
2006-06-11 21:26 PDT, David Latapie
no flags Details
Test - XHTML, French, orphan (4.14 KB, application/xhtml+xml)
2006-06-11 21:27 PDT, David Latapie
no flags Details
Patch (37.20 KB, patch)
2012-12-07 15:00 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (37.17 KB, patch)
2012-12-07 15:54 PST, Dean Jackson
darin: review+
webkit.review.bot: commit‑queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Latapie 2006-06-11 21:24:40 PDT
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;}
Comment 1 David Latapie 2006-06-11 21:26:13 PDT
Created attachment 8818 [details]
Test - PDF, French, orphan
Comment 2 David Latapie 2006-06-11 21:27:13 PDT
Created attachment 8819 [details]
Test - XHTML, French, orphan
Comment 3 David Kilzer (:ddkilzer) 2006-06-12 03:55:33 PDT
Confirming bug.  Looks like Opera is the only browser that supports this per Blooberry.
Comment 4 Joe Lewis 2009-01-02 09:33:52 PST
Adding my vote - would like to see support of missing print properties (@page page-break-inside, orphans, widows)
Comment 5 Hayato Ito 2010-05-06 04:34:25 PDT
I am working on this. Please see a comment at:
https://bugs.webkit.org/show_bug.cgi?id=34155#c1
Comment 6 Dean Jackson 2012-12-07 14:17:19 PST
<rdar://problem/12838699>
Comment 7 Dean Jackson 2012-12-07 15:00:07 PST
Created attachment 178288 [details]
Patch
Comment 8 Early Warning System Bot 2012-12-07 15:07:17 PST
Comment on attachment 178288 [details]
Patch

Attachment 178288 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15182696
Comment 9 Early Warning System Bot 2012-12-07 15:11:02 PST
Comment on attachment 178288 [details]
Patch

Attachment 178288 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15187467
Comment 10 Build Bot 2012-12-07 15:25:39 PST
Comment on attachment 178288 [details]
Patch

Attachment 178288 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15189454
Comment 11 EFL EWS Bot 2012-12-07 15:34:16 PST
Comment on attachment 178288 [details]
Patch

Attachment 178288 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15175863
Comment 12 WebKit Review Bot 2012-12-07 15:52:35 PST
Comment on attachment 178288 [details]
Patch

Attachment 178288 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15182707
Comment 13 Dean Jackson 2012-12-07 15:54:03 PST
Created attachment 178297 [details]
Patch
Comment 14 WebKit Review Bot 2012-12-07 21:24:01 PST
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 15 Darin Adler 2012-12-08 11:15:03 PST
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 16 Build Bot 2012-12-08 11:51:19 PST
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 17 Dean Jackson 2012-12-10 12:35:32 PST
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 18 Dean Jackson 2012-12-10 12:36:41 PST
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).
Comment 19 Dean Jackson 2012-12-10 13:13:56 PST
(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.
Comment 20 Dean Jackson 2012-12-10 13:25:46 PST
Committed r137200: <http://trac.webkit.org/changeset/137200>
Comment 21 Allan Sandfeld Jensen 2012-12-10 15:59:42 PST
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.
Comment 22 Dean Jackson 2012-12-10 20:49:58 PST
(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?