Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1592493002:
Fix links in inline continuations in printed PDF (Closed)
|
Created:
4 years, 11 months ago by Xianzhu Modified:
4 years, 11 months ago Reviewers:
chrishtr CC:
blink-reviews, blink-reviews-paint_chromium.org, chromium-reviews, dshwang, slimming-paint-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix links in inline continuations in printed PDF
We output links as clickable rects to PDF when printing. If a
link contains continuations, the paint rect intersection check
of the containing anonymous block may cause miss of output of
the PDF rect, because the visual overflow rect of the anonymous
block is empty or is smaller than the link PDF rect.
Now when printing, we let the parent block check for intersection
for the anonymous block of the link.
BUG=535514
TEST=PrintContextTest.LinkTargetComplex
Committed: https://crrev.com/83c02707dd7c8d48c47503d10734d67dd18ce254
Cr-Commit-Position: refs/heads/master@{#370260}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Remove an extra condition #
Total comments: 3
Patch Set 3 : Fix unit test #Patch Set 4 : #
Total comments: 1
Messages
Total messages: 28 (5 generated)
wangxianzhu@chromium.org changed reviewers: + chrishtr@chromium.org
https://codereview.chromium.org/1592493002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/BlockPainter.cpp (right): https://codereview.chromium.org/1592493002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/BlockPainter.cpp:215: if (paintInfo.isPrinting() && m_layoutBlock.isAnonymousBlock() && !m_layoutBlock.previousSibling() && m_layoutBlock.childrenInline()) { Why can't we always use this path? Is it really too inefficient?
https://codereview.chromium.org/1592493002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/BlockPainter.cpp (right): https://codereview.chromium.org/1592493002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/BlockPainter.cpp:215: if (paintInfo.isPrinting() && m_layoutBlock.isAnonymousBlock() && !m_layoutBlock.previousSibling() && m_layoutBlock.childrenInline()) { On 2016/01/14 23:29:32, chrishtr wrote: > Why can't we always use this path? Is it really too inefficient? Do you mean use the path also for non-printing painting? It's just unnecessary, and the normal path works fine.
https://codereview.chromium.org/1592493002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/BlockPainter.cpp (right): https://codereview.chromium.org/1592493002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/BlockPainter.cpp:215: if (paintInfo.isPrinting() && m_layoutBlock.isAnonymousBlock() && !m_layoutBlock.previousSibling() && m_layoutBlock.childrenInline()) { On 2016/01/14 at 23:41:41, Xianzhu wrote: > On 2016/01/14 23:29:32, chrishtr wrote: > > Why can't we always use this path? Is it really too inefficient? > > Do you mean use the path also for non-printing painting? It's just unnecessary, and the normal path works fine. I mean: why can't we have the same cull rect behavior for PDF and non-PDF? If the other one misses things because PDF links are different size, then how about we delete that logic in favor of the logic you added here?
https://codereview.chromium.org/1592493002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/BlockPainter.cpp (right): https://codereview.chromium.org/1592493002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/BlockPainter.cpp:215: if (paintInfo.isPrinting() && m_layoutBlock.isAnonymousBlock() && !m_layoutBlock.previousSibling() && m_layoutBlock.childrenInline()) { On 2016/01/14 23:56:00, chrishtr wrote: > On 2016/01/14 at 23:41:41, Xianzhu wrote: > > On 2016/01/14 23:29:32, chrishtr wrote: > > > Why can't we always use this path? Is it really too inefficient? > > > > Do you mean use the path also for non-printing painting? It's just > unnecessary, and the normal path works fine. > > I mean: why can't we have the same cull rect behavior for PDF and non-PDF? If > the other one > misses things because PDF links are different size, then how about we delete > that logic in favor of the logic you added here? The added logic will cause unnecessary paints which is not a big issue for printing. It's also only applicable to the anonymous block container of inline with continuations, so we can't delete the original logic.
https://codereview.chromium.org/1592493002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/BlockPainter.cpp (right): https://codereview.chromium.org/1592493002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/BlockPainter.cpp:215: if (paintInfo.isPrinting() && m_layoutBlock.isAnonymousBlock() && !m_layoutBlock.previousSibling() && m_layoutBlock.childrenInline()) { On 2016/01/15 at 01:51:00, Xianzhu wrote: > On 2016/01/14 23:56:00, chrishtr wrote: > > On 2016/01/14 at 23:41:41, Xianzhu wrote: > > > On 2016/01/14 23:29:32, chrishtr wrote: > > > > Why can't we always use this path? Is it really too inefficient? > > > > > > Do you mean use the path also for non-printing painting? It's just > > unnecessary, and the normal path works fine. > > > > I mean: why can't we have the same cull rect behavior for PDF and non-PDF? If > > the other one > > misses things because PDF links are different size, then how about we delete > > that logic in favor of the logic you added here? > > The added logic will cause unnecessary paints which is not a big issue for printing. It's also only applicable to the anonymous block container of inline with continuations, so we can't delete the original logic. Ok. Now my question is: why are the rects broken in PDF output exactly? Does PDF have different dimensions that don't exhibit in regular web pages? https://codereview.chromium.org/1592493002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/InlinePainter.cpp (left): https://codereview.chromium.org/1592493002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/InlinePainter.cpp:34: // FIXME: When Skia supports annotation rect covering (https://code.google.com/p/skia/issues/detail?id=3872), Why are these comments now obsolete?
https://codereview.chromium.org/1592493002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/BlockPainter.cpp (right): https://codereview.chromium.org/1592493002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/BlockPainter.cpp:215: if (paintInfo.isPrinting() && m_layoutBlock.isAnonymousBlock() && !m_layoutBlock.previousSibling() && m_layoutBlock.childrenInline()) { On 2016/01/15 02:20:02, chrishtr wrote: > On 2016/01/15 at 01:51:00, Xianzhu wrote: > > On 2016/01/14 23:56:00, chrishtr wrote: > > > On 2016/01/14 at 23:41:41, Xianzhu wrote: > > > > On 2016/01/14 23:29:32, chrishtr wrote: > > > > > Why can't we always use this path? Is it really too inefficient? > > > > > > > > Do you mean use the path also for non-printing painting? It's just > > > unnecessary, and the normal path works fine. > > > > > > I mean: why can't we have the same cull rect behavior for PDF and non-PDF? > If > > > the other one > > > misses things because PDF links are different size, then how about we delete > > > that logic in favor of the logic you added here? > > > > The added logic will cause unnecessary paints which is not a big issue for > printing. It's also only applicable to the anonymous block container of inline > with continuations, so we can't delete the original logic. > > Ok. Now my question is: why are the rects broken in PDF output exactly? Does PDF > have different dimensions > that don't exhibit in regular web pages? The PDF clickable rects are to let the PDF viewers know which parts of the document are clickable and the URLs to open when clicked. The counterpart in regular web pages is implemented with hit testing. It's special for PDF printing during painting. The PDF rect of a link is the bounding box of the outline of the link. If the link has continuations, the bounding box also covers the continuations. If the link did have an actual outline, we would have added the outline area to the visual overflow of the link and it's container blocks, and the normal intersection checking path would work. But when printing, the link doesn't have actual outline, so the continuation part of the link are not in the visual overflow, causing the intersection check returns early even we have "outline" to output. For example, <a href="x"><div>Target</div></a> The layout tree is: LayoutBlock anonymous LayoutInline A with empty content LayoutBlock anonymous LayoutBlock DIV LayoutText Target LayoutBlock anonymous LayoutInline A with empty content The first anonymous container block of <a> has empty visual overflow. With the old code, the anonymous block's paint() method returns immediately because intersectsPaintRect returns false. We miss painting of the <a> element and thus miss output of the PDF rect of the link. https://codereview.chromium.org/1592493002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/InlinePainter.cpp (left): https://codereview.chromium.org/1592493002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/InlinePainter.cpp:34: // FIXME: When Skia supports annotation rect covering (https://code.google.com/p/skia/issues/detail?id=3872), On 2016/01/15 02:20:02, chrishtr wrote: > Why are these comments now obsolete? The bug has been marked WontFix because unfeasiblity, so the FIXMEs also no longer apply.
https://codereview.chromium.org/1592493002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/BlockPainter.cpp (right): https://codereview.chromium.org/1592493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/BlockPainter.cpp:215: if (paintInfo.isPrinting() && m_layoutBlock.isAnonymousBlock() && m_layoutBlock.childrenInline()) { How about: if (paintInfo.isPrinting()) return true; Micro-performance of printing is not important. I'm also not so convinced this fix is general enough. Also, can you point me to the code that outputs these clickable rects?
https://codereview.chromium.org/1592493002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/BlockPainter.cpp (right): https://codereview.chromium.org/1592493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/BlockPainter.cpp:215: if (paintInfo.isPrinting() && m_layoutBlock.isAnonymousBlock() && m_layoutBlock.childrenInline()) { On 2016/01/15 18:16:35, chrishtr wrote: > How about: > > if (paintInfo.isPrinting()) > return true; > > Micro-performance of printing is not important. > During printing, we paint each page separately (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...), so without this check we'll paint everything for each page. (However, it seems we have a bug that we do paint everything for each page. Not very sure. Verifying.) > I'm also not so convinced this fix is general enough. Also, can you point me > to the code that outputs these clickable rects? https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
On 2016/01/15 at 18:36:29, wangxianzhu wrote: > https://codereview.chromium.org/1592493002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/paint/BlockPainter.cpp (right): > > https://codereview.chromium.org/1592493002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/paint/BlockPainter.cpp:215: if (paintInfo.isPrinting() && m_layoutBlock.isAnonymousBlock() && m_layoutBlock.childrenInline()) { > On 2016/01/15 18:16:35, chrishtr wrote: > > How about: > > > > if (paintInfo.isPrinting()) > > return true; > > > > Micro-performance of printing is not important. > > > > During printing, we paint each page separately (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...), so without this check we'll paint everything for each page. (However, it seems we have a bug that we do paint everything for each page. Not very sure. Verifying.) > > > I'm also not so convinced this fix is general enough. Also, can you point me > > to the code that outputs these clickable rects? > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... This code looks like it collects the visual overflow as the bounds. Why would visual overflow as collected by addElementVisualOverflowRects differ from the visual overflow used in intersectsPaintRect?
On 2016/01/15 20:40:45, chrishtr wrote: > On 2016/01/15 at 18:36:29, wangxianzhu wrote: > > > https://codereview.chromium.org/1592493002/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/paint/BlockPainter.cpp (right): > > > > > https://codereview.chromium.org/1592493002/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/paint/BlockPainter.cpp:215: if > (paintInfo.isPrinting() && m_layoutBlock.isAnonymousBlock() && > m_layoutBlock.childrenInline()) { > > On 2016/01/15 18:16:35, chrishtr wrote: > > > How about: > > > > > > if (paintInfo.isPrinting()) > > > return true; > > > > > > Micro-performance of printing is not important. > > > > > > > During printing, we paint each page separately > (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...), > so without this check we'll paint everything for each page. (However, it seems > we have a bug that we do paint everything for each page. Not very sure. > Verifying.) > > > > > I'm also not so convinced this fix is general enough. Also, can you point me > > > to the code that outputs these clickable rects? > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > This code looks like it collects the visual overflow as the bounds. Why would > visual overflow as collected by addElementVisualOverflowRects differ from the > visual overflow used > in intersectsPaintRect? Continuations cause different tree structures of DOM tree and layout tree. For the example in #8, there is no direct counterpart in the layout tree for <a> because it's split into the first part (i.e. the head of continuations) and following parts (i.e. continuations). The element overflow of <a> covers all of the parts, but there is no layout object counterpart having the same visual overflow (except when <a> has outline we add continuation visual overflows into <a>'s visual overflow).
Description was changed from ========== Fix links in inline continuations in printed PDF We output links as clickable rects to PDF when printing. If a link contains continuations, the paint rect intersection check of the containing anonymous block may cause miss of output of the PDF rect, because the visual overflow rect of the anonymous block is empty or is smaller than the link PDF rect. Now when printing, we let the parent block check for intersection for the anonymous block of the link. BUG=535514 TEST=PrintContextTest.InlineContinuation ========== to ========== Fix links in inline continuations in printed PDF We output links as clickable rects to PDF when printing. If a link contains continuations, the paint rect intersection check of the containing anonymous block may cause miss of output of the PDF rect, because the visual overflow rect of the anonymous block is empty or is smaller than the link PDF rect. Now when printing, we let the parent block check for intersection for the anonymous block of the link. BUG=535514 TEST=PrintContextTest.LinkTargetComplex ==========
On 2016/01/15 at 21:56:01, wangxianzhu wrote: > On 2016/01/15 20:40:45, chrishtr wrote: > > On 2016/01/15 at 18:36:29, wangxianzhu wrote: > > > > > https://codereview.chromium.org/1592493002/diff/20001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/paint/BlockPainter.cpp (right): > > > > > > > > https://codereview.chromium.org/1592493002/diff/20001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/paint/BlockPainter.cpp:215: if > > (paintInfo.isPrinting() && m_layoutBlock.isAnonymousBlock() && > > m_layoutBlock.childrenInline()) { > > > On 2016/01/15 18:16:35, chrishtr wrote: > > > > How about: > > > > > > > > if (paintInfo.isPrinting()) > > > > return true; > > > > > > > > Micro-performance of printing is not important. > > > > > > > > > > During printing, we paint each page separately > > (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...), > > so without this check we'll paint everything for each page. (However, it seems > > we have a bug that we do paint everything for each page. Not very sure. > > Verifying.) > > > > > > > I'm also not so convinced this fix is general enough. Also, can you point me > > > > to the code that outputs these clickable rects? > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > This code looks like it collects the visual overflow as the bounds. Why would > > visual overflow as collected by addElementVisualOverflowRects differ from the > > visual overflow used > > in intersectsPaintRect? > > Continuations cause different tree structures of DOM tree and layout tree. For the example in #8, there is no direct counterpart in the layout tree for <a> because it's split into the first part (i.e. the head of continuations) and following parts (i.e. continuations). The element overflow of <a> covers all of the parts, but there is no layout object counterpart having the same visual overflow (except when <a> has outline we add continuation visual overflows into <a>'s visual overflow). Ah it's because addElementVisualOverflowRects is implemented in terms of outline? Also, any update on whether performance is a concern or not?
On 2016/01/16 01:24:47, chrishtr wrote: > On 2016/01/15 at 21:56:01, wangxianzhu wrote: > > On 2016/01/15 20:40:45, chrishtr wrote: > > > On 2016/01/15 at 18:36:29, wangxianzhu wrote: > > > > > > > > https://codereview.chromium.org/1592493002/diff/20001/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/core/paint/BlockPainter.cpp (right): > > > > > > > > > > > > https://codereview.chromium.org/1592493002/diff/20001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/core/paint/BlockPainter.cpp:215: if > > > (paintInfo.isPrinting() && m_layoutBlock.isAnonymousBlock() && > > > m_layoutBlock.childrenInline()) { > > > > On 2016/01/15 18:16:35, chrishtr wrote: > > > > > How about: > > > > > > > > > > if (paintInfo.isPrinting()) > > > > > return true; > > > > > > > > > > Micro-performance of printing is not important. > > > > > > > > > > > > > During printing, we paint each page separately > > > > (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...), > > > so without this check we'll paint everything for each page. (However, it > seems > > > we have a bug that we do paint everything for each page. Not very sure. > > > Verifying.) > > > > > > > > > I'm also not so convinced this fix is general enough. Also, can you > point me > > > > > to the code that outputs these clickable rects? > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > > > This code looks like it collects the visual overflow as the bounds. Why > would > > > visual overflow as collected by addElementVisualOverflowRects differ from > the > > > visual overflow used > > > in intersectsPaintRect? > > > > Continuations cause different tree structures of DOM tree and layout tree. For > the example in #8, there is no direct counterpart in the layout tree for <a> > because it's split into the first part (i.e. the head of continuations) and > following parts (i.e. continuations). The element overflow of <a> covers all of > the parts, but there is no layout object counterpart having the same visual > overflow (except when <a> has outline we add continuation visual overflows into > <a>'s visual overflow). > > Ah it's because addElementVisualOverflowRects is implemented in terms of > outline? > I think they are just interchangeable: the outline needs to enclose all visual parts of the element, so element's visual overflow rects are just the layout object's outline rects. > Also, any update on whether performance is a concern or not? Just confirmed that we are not painting the whole web page for each printed page. Had the impression because I used a test case containing only one big block. We should not let this CL cause the regression by always returning true from intersectsPaintRect().
On 2016/01/15 at 21:56:01, wangxianzhu wrote: > On 2016/01/15 20:40:45, chrishtr wrote: > > On 2016/01/15 at 18:36:29, wangxianzhu wrote: > > > > > https://codereview.chromium.org/1592493002/diff/20001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/paint/BlockPainter.cpp (right): > > > > > > > > https://codereview.chromium.org/1592493002/diff/20001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/paint/BlockPainter.cpp:215: if > > (paintInfo.isPrinting() && m_layoutBlock.isAnonymousBlock() && > > m_layoutBlock.childrenInline()) { > > > On 2016/01/15 18:16:35, chrishtr wrote: > > > > How about: > > > > > > > > if (paintInfo.isPrinting()) > > > > return true; > > > > > > > > Micro-performance of printing is not important. > > > > > > > > > > During printing, we paint each page separately > > (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...), > > so without this check we'll paint everything for each page. (However, it seems > > we have a bug that we do paint everything for each page. Not very sure. > > Verifying.) > > > > > > > I'm also not so convinced this fix is general enough. Also, can you point me > > > > to the code that outputs these clickable rects? > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > This code looks like it collects the visual overflow as the bounds. Why would > > visual overflow as collected by addElementVisualOverflowRects differ from the > > visual overflow used > > in intersectsPaintRect? > > Continuations cause different tree structures of DOM tree and layout tree. For the example in #8, there is no direct counterpart in the layout tree for <a> because it's split into the first part (i.e. the head of continuations) and following parts (i.e. continuations). The element overflow of <a> covers all of the parts, but there is no layout object counterpart having the same visual overflow (except when <a> has outline we add continuation visual overflows into <a>'s visual overflow). What if we always did that? (add continuation visual overflow into <a>'s visual overflow regardless of outline) That seems an inconsistency.
On 2016/01/19 17:00:15, chrishtr wrote: > On 2016/01/15 at 21:56:01, wangxianzhu wrote: > > On 2016/01/15 20:40:45, chrishtr wrote: > > > On 2016/01/15 at 18:36:29, wangxianzhu wrote: > > > > > > > > https://codereview.chromium.org/1592493002/diff/20001/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/core/paint/BlockPainter.cpp (right): > > > > > > > > > > > > https://codereview.chromium.org/1592493002/diff/20001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/core/paint/BlockPainter.cpp:215: if > > > (paintInfo.isPrinting() && m_layoutBlock.isAnonymousBlock() && > > > m_layoutBlock.childrenInline()) { > > > > On 2016/01/15 18:16:35, chrishtr wrote: > > > > > How about: > > > > > > > > > > if (paintInfo.isPrinting()) > > > > > return true; > > > > > > > > > > Micro-performance of printing is not important. > > > > > > > > > > > > > During printing, we paint each page separately > > > > (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...), > > > so without this check we'll paint everything for each page. (However, it > seems > > > we have a bug that we do paint everything for each page. Not very sure. > > > Verifying.) > > > > > > > > > I'm also not so convinced this fix is general enough. Also, can you > point me > > > > > to the code that outputs these clickable rects? > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > > > This code looks like it collects the visual overflow as the bounds. Why > would > > > visual overflow as collected by addElementVisualOverflowRects differ from > the > > > visual overflow used > > > in intersectsPaintRect? > > > > Continuations cause different tree structures of DOM tree and layout tree. For > the example in #8, there is no direct counterpart in the layout tree for <a> > because it's split into the first part (i.e. the head of continuations) and > following parts (i.e. continuations). The element overflow of <a> covers all of > the parts, but there is no layout object counterpart having the same visual > overflow (except when <a> has outline we add continuation visual overflows into > <a>'s visual overflow). > > What if we always did that? (add continuation visual overflow into <a>'s visual > overflow regardless of outline) That seems an inconsistency. The inconsistency occurs only for printing PDF rects. Still use the example in #8: DOM: <a href="x"><div>Target</div></a> Layout tree: LayoutBlock anonymous LayoutInline A with empty content LayoutBlock anonymous LayoutBlock DIV LayoutText Target LayoutBlock anonymous LayoutInline A with empty content In DOM tree, the <a> element covers the div, but in layout tree, there is no LayoutObject covering the whole <a>, so no LayoutObject should have visual overflow covering the whole <a>. It's a different story if <a> has outline, because the outline covers the continuations, so we need to let the visual overflow of first LayoutInline and the first anonymous cover the whole continuations. It's expensive to collect outline rects for continuations, so we should avoid it when it's not necessary. Especially we may want to make printing a bit dirty to keep normal painting clean and efficient.
On 2016/01/19 at 18:06:53, wangxianzhu wrote: > On 2016/01/19 17:00:15, chrishtr wrote: > > On 2016/01/15 at 21:56:01, wangxianzhu wrote: > > > On 2016/01/15 20:40:45, chrishtr wrote: > > > > On 2016/01/15 at 18:36:29, wangxianzhu wrote: > > > > > > > > > > > https://codereview.chromium.org/1592493002/diff/20001/third_party/WebKit/Sour... > > > > > File third_party/WebKit/Source/core/paint/BlockPainter.cpp (right): > > > > > > > > > > > > > > > > https://codereview.chromium.org/1592493002/diff/20001/third_party/WebKit/Sour... > > > > > third_party/WebKit/Source/core/paint/BlockPainter.cpp:215: if > > > > (paintInfo.isPrinting() && m_layoutBlock.isAnonymousBlock() && > > > > m_layoutBlock.childrenInline()) { > > > > > On 2016/01/15 18:16:35, chrishtr wrote: > > > > > > How about: > > > > > > > > > > > > if (paintInfo.isPrinting()) > > > > > > return true; > > > > > > > > > > > > Micro-performance of printing is not important. > > > > > > > > > > > > > > > > During printing, we paint each page separately > > > > > > (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...), > > > > so without this check we'll paint everything for each page. (However, it > > seems > > > > we have a bug that we do paint everything for each page. Not very sure. > > > > Verifying.) > > > > > > > > > > > I'm also not so convinced this fix is general enough. Also, can you > > point me > > > > > > to the code that outputs these clickable rects? > > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > > > > > This code looks like it collects the visual overflow as the bounds. Why > > would > > > > visual overflow as collected by addElementVisualOverflowRects differ from > > the > > > > visual overflow used > > > > in intersectsPaintRect? > > > > > > Continuations cause different tree structures of DOM tree and layout tree. For > > the example in #8, there is no direct counterpart in the layout tree for <a> > > because it's split into the first part (i.e. the head of continuations) and > > following parts (i.e. continuations). The element overflow of <a> covers all of > > the parts, but there is no layout object counterpart having the same visual > > overflow (except when <a> has outline we add continuation visual overflows into > > <a>'s visual overflow). > > > > What if we always did that? (add continuation visual overflow into <a>'s visual > > overflow regardless of outline) That seems an inconsistency. > > The inconsistency occurs only for printing PDF rects. Still use the example in #8: > > DOM: > <a href="x"><div>Target</div></a> > > Layout tree: > LayoutBlock anonymous > LayoutInline A with empty content > LayoutBlock anonymous > LayoutBlock DIV > LayoutText Target > LayoutBlock anonymous > LayoutInline A with empty content > > In DOM tree, the <a> element covers the div, but in layout tree, there is no LayoutObject covering the whole <a>, so no LayoutObject should have visual overflow covering the whole <a>. > > It's a different story if <a> has outline, because the outline covers the continuations, so we need to let the visual overflow of first LayoutInline and the first anonymous cover the whole continuations. > > It's expensive to collect outline rects for continuations, so we should avoid it when it's not necessary. Especially we may want to make printing a bit dirty to keep normal painting clean and efficient. Ok. But the proposed solution also looks hacky and fragile to me, especially use of the parent()'s visual overflow. visualOverflowRect() should include all painted effects. I can see why it's hard to integrate painting directly into that, since printing is not a persistent object state but rather a painting mode. How about instead implementing a visualOverflowRectForPrinting() (or similar, maybe just inline it into BlockPainter::intersectsPaintRect) which calls addElementVisualOverflowRects to include painted effects for printing?
https://codereview.chromium.org/1592493002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/BlockPainter.cpp (right): https://codereview.chromium.org/1592493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/BlockPainter.cpp:221: return BlockPainter(toLayoutBlock(*m_layoutBlock.parent())).intersectsPaintRect(paintInfo, paintOffset); Now call m_layoutBlock.addElementVisualOverflowRects() instead of delegating to parent.
https://codereview.chromium.org/1592493002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/BlockPainter.cpp (right): https://codereview.chromium.org/1592493002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/BlockPainter.cpp:218: if (paintInfo.isPrinting() && m_layoutBlock.isAnonymousBlock() && m_layoutBlock.childrenInline()) { Last nit I promise: is childrenInline() necessary?
On 2016/01/19 23:25:42, chrishtr wrote: > https://codereview.chromium.org/1592493002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/paint/BlockPainter.cpp (right): > > https://codereview.chromium.org/1592493002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/paint/BlockPainter.cpp:218: if > (paintInfo.isPrinting() && m_layoutBlock.isAnonymousBlock() && > m_layoutBlock.childrenInline()) { > Last nit I promise: > > is childrenInline() necessary? Yes. We need this path only when the anonymous block is the container of the head of inline continuation.
The CQ bit was checked by chrishtr@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1592493002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1592493002/60001
Message was sent while issue was closed.
Description was changed from ========== Fix links in inline continuations in printed PDF We output links as clickable rects to PDF when printing. If a link contains continuations, the paint rect intersection check of the containing anonymous block may cause miss of output of the PDF rect, because the visual overflow rect of the anonymous block is empty or is smaller than the link PDF rect. Now when printing, we let the parent block check for intersection for the anonymous block of the link. BUG=535514 TEST=PrintContextTest.LinkTargetComplex ========== to ========== Fix links in inline continuations in printed PDF We output links as clickable rects to PDF when printing. If a link contains continuations, the paint rect intersection check of the containing anonymous block may cause miss of output of the PDF rect, because the visual overflow rect of the anonymous block is empty or is smaller than the link PDF rect. Now when printing, we let the parent block check for intersection for the anonymous block of the link. BUG=535514 TEST=PrintContextTest.LinkTargetComplex ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Fix links in inline continuations in printed PDF We output links as clickable rects to PDF when printing. If a link contains continuations, the paint rect intersection check of the containing anonymous block may cause miss of output of the PDF rect, because the visual overflow rect of the anonymous block is empty or is smaller than the link PDF rect. Now when printing, we let the parent block check for intersection for the anonymous block of the link. BUG=535514 TEST=PrintContextTest.LinkTargetComplex ========== to ========== Fix links in inline continuations in printed PDF We output links as clickable rects to PDF when printing. If a link contains continuations, the paint rect intersection check of the containing anonymous block may cause miss of output of the PDF rect, because the visual overflow rect of the anonymous block is empty or is smaller than the link PDF rect. Now when printing, we let the parent block check for intersection for the anonymous block of the link. BUG=535514 TEST=PrintContextTest.LinkTargetComplex Committed: https://crrev.com/83c02707dd7c8d48c47503d10734d67dd18ce254 Cr-Commit-Position: refs/heads/master@{#370260} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/83c02707dd7c8d48c47503d10734d67dd18ce254 Cr-Commit-Position: refs/heads/master@{#370260} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
