Include selection rect in paint invalidation for LayoutText.

third_party/WebKit/Source/core/layout/LayoutText.cpp

 /*
  * (C) 1999 Lars Knoll (knoll@kde.org)
  * (C) 2000 Dirk Mueller (mueller@kde.org)
  * Copyright (C) 2004, 2005, 2006, 2007 Apple Inc. All rights reserved.
  * Copyright (C) 2006 Andrew Wellington (proton@wiretapped.net)
  * Copyright (C) 2006 Graham Dennis (graham.dennis@gmail.com)
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
@@ skipping 1547 matching lines @@
     LayoutUnit logicalLeftSide = LayoutUnit::max();
     LayoutUnit logicalRightSide = LayoutUnit::min();
     for (InlineTextBox* curr = firstTextBox(); curr; curr = curr->nextTextBox()) {
         LayoutRect logicalVisualOverflow = curr->logicalOverflowRect();
         logicalLeftSide = std::min(logicalLeftSide, logicalVisualOverflow.x());
         logicalRightSide = std::max(logicalRightSide, logicalVisualOverflow.maxX());
     }
 
     LayoutUnit logicalTop = firstTextBox()->logicalTopVisualOverflow();
     LayoutUnit logicalWidth = logicalRightSide - logicalLeftSide;
-    LayoutUnit logicalHeight = lastTextBox()->logicalBottomVisualOverflow() - logicalTop;
+    InlineTextBox* lastBox = lastTextBox();
+    LayoutUnit logicalHeight = lastBox->logicalBottomVisualOverflow() - logicalTop;
+
+    // Include visual overflow for wrapped selection newline highlight if applicable.
+    if (lastBox->hasWrappedSelectionNewline() || (isBR() && getSelectionState() != SelectionNone)) {
+        // TODO(wkorman): The below is likely insufficient to handle RTL text.
+        // We already paint RTL wrapped newline selection highlights in
+        // contenteditable incorrectly. We should fix painting in tandem with
+        // this visual overflow rect. http://crbug.com/610813
+        logicalWidth += lastBox->newlineSpaceWidth();
+        logicalHeight = std::max(logicalHeight, localSelectionRect().height());

[eae, 2016/05/10 20:39:19]

Why is the height override needed? When is the localSelectionRect taller than
the logicalHeight? Seems like the newline indicator would only ever affect the
width.

[wkorman, 2016/05/10 21:33:01]

For isBR() case (note this block executes also in the case where
lastBox->hasWrappedSelectionNewline() in which case what you state is
(generally) true), the only text character is a line break character which
reports its logical width/height as zero.

I considered changing this to only mutate logicalHeight if isBR(), but it occurs
to me that it could be possible for LayoutText to exist where isBR() is false,
but the sole text content is still a line break character, which would mean we'd
still want this logic. Layout tests pass if we only do this for the isBR() case,
however.

I added a comment, and also changed to reference
lastBox->root().selectionHeight() instead. Actually it's incorrect
writing-mode-wise for me to have been mixing localSelectionRect() output here
(where we're in logical coords) with the output of localSelectionRect() (which
is in physical coords).

+    }

[Xianzhu, 2016/05/10 20:35:46]

Should LayoutText::localSelectionRect() also have this logic?

[wkorman, 2016/05/10 20:41:08]

Hmm. We already have essentially this logic in
InlineTextBox::localSelectionRect():

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

so LayoutText::localSelectionRect() is already incorporating a variant of this
logic through its use of ITB::localSelectionRect(), right? In fact we are
relying on that logic through our call to localSelectionRect() in line 1578 here
to get the height of the selection rectangle.

It may be that as part of fixing http://crbug.com/610813 we end up reworking
where the logic resides. I'm more familiar with things now than I was when this
was originally written.

So, I think this change makes sense as is pending that subsequent bug fix work,
but you may be aware of something paint invalidation/selection painting wise I
am not thinking about.

[Xianzhu, 2016/05/10 21:10:46]

Sorry, actually I didn't carefully read the added code and didn't notice it
calls localSelectionRect(). Thanks for the explanation.

I still have the following concerns:

- If we include selection overflow into visual overflow, can we ensure we update
visual overflow when selection changes? (It might not be a problem for
LayoutText itself because it doesn't cache visual overflow rect, but it may
affect visual overflow of ancestor.)

- Selection change affects graphics layer size (through
PaintLayer::boundingBoxForCompositing()). If selecting the new-line changes
visual overflow size, can we ensure the graphics layer size is adjusted to
contain the overflow? Considering this, should we always include the "potential
selection overflow" into visual overflow?

[Xianzhu, 2016/05/10 21:13:56]

s/Selection change affects/With this CL, selection change will also affect/

[wkorman, 2016/05/10 21:33:01]

Hmm, I think I see what you're saying. It seems it would be nice though to avoid
always including potential overflow, since selection is generally rare and
having a larger overflow means we'd invalidate/raster more than may be strictly
necessary.

On the other hand if we don't always include it maybe we'll have to introduce
layouts that today we don't have to have. I'm actually not sure of the answer to
your first point yet, re: whether when selection changes we're assured that
visual overflow is updated inclusive of ancestors, I will check.

In http://crrev.com/1817463004 jbroman@ and chrishtr@ were discussing selection
as a special case that is explicitly desired to not affect layout. jbroman@
pointed out: "visual overflow _doesn't_ affect layout (layout overflow does),
and exists exactly to handle purely visual (non-layout-effecting) effects, like
box-shadow." So this change is following the spirit of that conversation.
Perhaps selection is not a perfect fit since, unlike box shadow, it can easily
vary in response to user interactions.

Open to thoughts re: best path. In our longer term dreams selection highlighting
might be implemented as its own layer, and things could be more naturally
handled.

[Xianzhu, 2016/05/10 21:55:13]

I think "visual overflow doesn't affect layout" and "visual overflow affects
compositing" are two separate things. robhogan@ recently landed a CL to avoid
layout on box-shadow change, but it still get visual overflow and graphics
layers correctly updated.

If we can't ensure visual overflow and graphics layer size correctly updated on
selection change (let's keep it as a known bug), I would suggest we don't
include selection overflow into visual overflow, but just consider selection
overflow for paint invalidation:
  localOverflowRectForPaintInvalidation = uniteRect(visualOverflowRect(),
localSelectionRect()) + considering visibility, etc.

This is just like what LayoutSVGRoot::localOverflowRectForPaintInvalidation()
currently does.

 
     LayoutRect rect(logicalLeftSide, logicalTop, logicalWidth, logicalHeight);
     if (!style()->isHorizontalWritingMode())
         rect = rect.transposedRect();
     return rect;
 }
 
 LayoutRect LayoutText::localOverflowRectForPaintInvalidation() const
 {
     if (style()->visibility() != VISIBLE)
@@ skipping 120 matching lines @@
     for (InlineTextBox* box = firstTextBox(); box; box = box->nextTextBox()) {
         paintInvalidationContainer.invalidateDisplayItemClientOnBacking(*box, invalidationReason);
         if (box->truncation() != cNoTruncation) {
             if (EllipsisBox* ellipsisBox = box->root().ellipsisBox())
                 paintInvalidationContainer.invalidateDisplayItemClientOnBacking(*ellipsisBox, invalidationReason);
         }
     }
 }
 
 } // namespace blink