Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1955423004:
Include selection rect in paint invalidation for LayoutText. (Closed)
|
Created:
4 years, 7 months ago by wkorman Modified:
4 years, 7 months ago CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionInclude selection rect in paint invalidation for LayoutText.
This change is intended to fix the paint invalidation rect, and thus
the visual rect, as part of the linked bug http://crbug.com/605319
spun out of http://crbug.com/529938. There is more explanation of
the intended use of visual rects in my comment at
http://crbug.com/604883#c5
The visual rects that are incorrect without this change are not
currently used by live code, but will be used as part of
http://crrev.com/1484163002. This change fixes about 15 LayoutTests
with that patch applied, mainly under editing.
BUG=605319
Committed: https://crrev.com/76f9a545cdf9903cded65e98b616a14976428aa8
Cr-Commit-Position: refs/heads/master@{#394807}
Patch Set 1 #Patch Set 2 : Reworking to cover more cases. #Patch Set 3 : Rebaseline tests. #
Total comments: 8
Patch Set 4 : Document logical height mutation. #Patch Set 5 : Feedback. #Patch Set 6 : Add expectations, simplify uniting. #Patch Set 7 : Sync to head and add one test. #
Messages
Total messages: 33 (13 generated)
Description was changed from ========== Include selection newline in visual overflow for line breaks. BUG=606076,605319 ========== to ========== Include selection newline in visual overflow for line breaks. BUG=605319 ==========
wkorman@chromium.org changed reviewers: + chrishtr@chromium.org, eae@chromium.org, wangxianzhu@chromium.org
Background for eae@ or others: This change is intended to fix the paint invalidation rect, and thus the visual rect, as part of the linked bug http://crbug.com/605319 spun out of http://crbug.com/529938. There is more explanation of the intended use of visual rects in my comment at http://crbug.com/604883#c5 The visual rects that are incorrect without this change are not currently used by live code, but will be used as part of http://crrev.com/1484163002. This change fixes about 15 LayoutTests with that patch applied, mainly under editing, and those were mainly cases where LayoutText::isBR() returned true, though this change also incorporates fix for !isBR() where we just have text with a wrapped newline highlight at end. You can see the expectations we'll be rebaselining to here: https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/... I've looked at them and they seem reasonable but I am not yet as much an expert in interpreting repaint test text expectations as you all, so please keep me honest. I thought this change could fix http://crbug.com/606076, which I am also able to repro, but it does not, so something else must be going on there.
Description was changed from ========== Include selection newline in visual overflow for line breaks. BUG=605319 ========== to ========== Include selection newline in visual overflow for selected line breaks. BUG=605319 ==========
https://codereview.chromium.org/1955423004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutText.cpp (right): https://codereview.chromium.org/1955423004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutText.cpp:1579: } Should LayoutText::localSelectionRect() also have this logic?
Could you please add the description/background from your first comment into the CL description? https://codereview.chromium.org/1955423004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutText.cpp (right): https://codereview.chromium.org/1955423004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutText.cpp:1578: logicalHeight = std::max(logicalHeight, localSelectionRect().height()); 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.
https://codereview.chromium.org/1955423004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutText.cpp (right): https://codereview.chromium.org/1955423004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutText.cpp:1579: } On 2016/05/10 at 20:35:46, Xianzhu wrote: > Should LayoutText::localSelectionRect() also have this logic? 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.
Description was changed from ========== Include selection newline in visual overflow for selected line breaks. BUG=605319 ========== to ========== Include selection newline in visual overflow for selected line breaks. This change is intended to fix the paint invalidation rect, and thus the visual rect, as part of the linked bug http://crbug.com/605319 spun out of http://crbug.com/529938. There is more explanation of the intended use of visual rects in my comment at http://crbug.com/604883#c5 The visual rects that are incorrect without this change are not currently used by live code, but will be used as part of http://crrev.com/1484163002. This change fixes about 15 LayoutTests with that patch applied, mainly under editing, and those were mainly cases where LayoutText::isBR() returned true, though this change also incorporates fix for !isBR() where we just have text with a wrapped newline highlight at end. BUG=605319 ==========
Description was changed from ========== Include selection newline in visual overflow for selected line breaks. This change is intended to fix the paint invalidation rect, and thus the visual rect, as part of the linked bug http://crbug.com/605319 spun out of http://crbug.com/529938. There is more explanation of the intended use of visual rects in my comment at http://crbug.com/604883#c5 The visual rects that are incorrect without this change are not currently used by live code, but will be used as part of http://crrev.com/1484163002. This change fixes about 15 LayoutTests with that patch applied, mainly under editing, and those were mainly cases where LayoutText::isBR() returned true, though this change also incorporates fix for !isBR() where we just have text with a wrapped newline highlight at end. BUG=605319 ========== to ========== Include selection newline in visual overflow for selected line breaks. This change is intended to fix the paint invalidation rect, and thus the visual rect, as part of the linked bug http://crbug.com/605319 spun out of http://crbug.com/529938. There is more explanation of the intended use of visual rects in my comment at http://crbug.com/604883#c5 The visual rects that are incorrect without this change are not currently used by live code, but will be used as part of http://crrev.com/1484163002. This change fixes about 15 LayoutTests with that patch applied, mainly under editing, and those were mainly cases where LayoutText::isBR() returned true, though this change also incorporates fix for !isBR() where we just have text with a wrapped newline highlight at end. BUG=605319 ==========
https://codereview.chromium.org/1955423004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutText.cpp (right): https://codereview.chromium.org/1955423004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutText.cpp:1579: } On 2016/05/10 20:41:08, wkorman wrote: > On 2016/05/10 at 20:35:46, Xianzhu wrote: > > Should LayoutText::localSelectionRect() also have this logic? > > 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. 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?
https://codereview.chromium.org/1955423004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutText.cpp (right): https://codereview.chromium.org/1955423004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutText.cpp:1579: } On 2016/05/10 21:10:46, Xianzhu wrote: > On 2016/05/10 20:41:08, wkorman wrote: > > On 2016/05/10 at 20:35:46, Xianzhu wrote: > > > Should LayoutText::localSelectionRect() also have this logic? > > > > 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. > > 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? s/Selection change affects/With this CL, selection change will also affect/
https://codereview.chromium.org/1955423004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutText.cpp (right): https://codereview.chromium.org/1955423004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutText.cpp:1578: logicalHeight = std::max(logicalHeight, localSelectionRect().height()); On 2016/05/10 at 20:39:19, eae wrote: > 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. 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). https://codereview.chromium.org/1955423004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutText.cpp:1579: } On 2016/05/10 at 21:13:56, Xianzhu wrote: > On 2016/05/10 21:10:46, Xianzhu wrote: > > On 2016/05/10 20:41:08, wkorman wrote: > > > On 2016/05/10 at 20:35:46, Xianzhu wrote: > > > > Should LayoutText::localSelectionRect() also have this logic? > > > > > > 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. > > > > 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? > > s/Selection change affects/With this CL, selection change will also affect/ 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.
On 2016/05/10 21:33:01, wkorman wrote: > https://codereview.chromium.org/1955423004/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/layout/LayoutText.cpp (right): > > https://codereview.chromium.org/1955423004/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/LayoutText.cpp:1578: logicalHeight = > std::max(logicalHeight, localSelectionRect().height()); > On 2016/05/10 at 20:39:19, eae wrote: > > 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. > > 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). Ah, that makes sense. Thanks for clarifying (and documenting). Not limiting it to isBR seems fine. LGTM
https://codereview.chromium.org/1955423004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutText.cpp (right): https://codereview.chromium.org/1955423004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutText.cpp:1579: } On 2016/05/10 21:33:01, wkorman wrote: > On 2016/05/10 at 21:13:56, Xianzhu wrote: > > On 2016/05/10 21:10:46, Xianzhu wrote: > > > On 2016/05/10 20:41:08, wkorman wrote: > > > > On 2016/05/10 at 20:35:46, Xianzhu wrote: > > > > > Should LayoutText::localSelectionRect() also have this logic? > > > > > > > > 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. > > > > > > 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? > > > > s/Selection change affects/With this CL, selection change will also affect/ > > 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. 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.
On 2016/05/10 at 21:55:13, wangxianzhu wrote: > https://codereview.chromium.org/1955423004/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/layout/LayoutText.cpp (right): > > https://codereview.chromium.org/1955423004/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/LayoutText.cpp:1579: } > On 2016/05/10 21:33:01, wkorman wrote: > > On 2016/05/10 at 21:13:56, Xianzhu wrote: > > > On 2016/05/10 21:10:46, Xianzhu wrote: > > > > On 2016/05/10 20:41:08, wkorman wrote: > > > > > On 2016/05/10 at 20:35:46, Xianzhu wrote: > > > > > > Should LayoutText::localSelectionRect() also have this logic? > > > > > > > > > > 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. > > > > > > > > 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? > > > > > > s/Selection change affects/With this CL, selection change will also affect/ > > > > 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. > > 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. To revisit and summarize what we were discussing above, what would you think is best from a performance perspective: 1. Always include the visual overflow for potential newline selection highlight in LayoutText. 2. Only include when needed and presumably we'd need to setNeedsCompositingUpdate() or similar somewhere when selection state changes. 3. Only include the overflow for paint invalidation After a bit more investigation (1) seems not unreasonable. It doesn't look like selection state change currently leads to recalculating graphics layer bounds, and my still-somewhat-naive sense is that adding logic to do that can be expensive for certain pages. (3) is also an option but feels like something of a hack (hence your note re: keeping as known bug if we do that). So I am thinking to modify this for (1) and just ignore selection state but wanted to hear thoughts re: tradeoff first.
On 2016/05/18 22:22:00, wkorman wrote: > On 2016/05/10 at 21:55:13, wangxianzhu wrote: > > > https://codereview.chromium.org/1955423004/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/layout/LayoutText.cpp (right): > > > > > https://codereview.chromium.org/1955423004/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/layout/LayoutText.cpp:1579: } > > On 2016/05/10 21:33:01, wkorman wrote: > > > On 2016/05/10 at 21:13:56, Xianzhu wrote: > > > > On 2016/05/10 21:10:46, Xianzhu wrote: > > > > > On 2016/05/10 20:41:08, wkorman wrote: > > > > > > On 2016/05/10 at 20:35:46, Xianzhu wrote: > > > > > > > Should LayoutText::localSelectionRect() also have this logic? > > > > > > > > > > > > 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. > > > > > > > > > > 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? > > > > > > > > s/Selection change affects/With this CL, selection change will also > affect/ > > > > > > 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. > > > > 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. > > To revisit and summarize what we were discussing above, what would you think is > best from a performance perspective: > > 1. Always include the visual overflow for potential newline selection highlight > in LayoutText. > > 2. Only include when needed and presumably we'd need to > setNeedsCompositingUpdate() or similar somewhere when selection state changes. > > 3. Only include the overflow for paint invalidation > > After a bit more investigation (1) seems not unreasonable. It doesn't look like > selection state change currently leads to recalculating graphics layer bounds, > and my still-somewhat-naive sense is that adding logic to do that can be > expensive for certain pages. > > (3) is also an option but feels like something of a hack (hence your note re: > keeping as known bug if we do that). > > So I am thinking to modify this for (1) and just ignore selection state but > wanted to hear thoughts re: tradeoff first. I would prefer 3: With spv2, 1 and 3 will change: 1. We'll still miss contentVisualOverflowRect update of ancestors; 3. The 'known bug' will disappear. So 3 seems better for spv2. This also conform to the existing behavior of LayoutSVGRoot.
On 2016/05/18 at 22:34:44, wangxianzhu wrote: > On 2016/05/18 22:22:00, wkorman wrote: > > On 2016/05/10 at 21:55:13, wangxianzhu wrote: > > > > > https://codereview.chromium.org/1955423004/diff/40001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/layout/LayoutText.cpp (right): > > > > > > > > https://codereview.chromium.org/1955423004/diff/40001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/layout/LayoutText.cpp:1579: } > > > On 2016/05/10 21:33:01, wkorman wrote: > > > > On 2016/05/10 at 21:13:56, Xianzhu wrote: > > > > > On 2016/05/10 21:10:46, Xianzhu wrote: > > > > > > On 2016/05/10 20:41:08, wkorman wrote: > > > > > > > On 2016/05/10 at 20:35:46, Xianzhu wrote: > > > > > > > > Should LayoutText::localSelectionRect() also have this logic? > > > > > > > > > > > > > > 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. > > > > > > > > > > > > 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? > > > > > > > > > > s/Selection change affects/With this CL, selection change will also > > affect/ > > > > > > > > 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. > > > > > > 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. > > > > To revisit and summarize what we were discussing above, what would you think is > > best from a performance perspective: > > > > 1. Always include the visual overflow for potential newline selection highlight > > in LayoutText. > > > > 2. Only include when needed and presumably we'd need to > > setNeedsCompositingUpdate() or similar somewhere when selection state changes. > > > > 3. Only include the overflow for paint invalidation > > > > After a bit more investigation (1) seems not unreasonable. It doesn't look like > > selection state change currently leads to recalculating graphics layer bounds, > > and my still-somewhat-naive sense is that adding logic to do that can be > > expensive for certain pages. > > > > (3) is also an option but feels like something of a hack (hence your note re: > > keeping as known bug if we do that). > > > > So I am thinking to modify this for (1) and just ignore selection state but > > wanted to hear thoughts re: tradeoff first. > > I would prefer 3: > > With spv2, 1 and 3 will change: > 1. We'll still miss contentVisualOverflowRect update of ancestors; > 3. The 'known bug' will disappear. > > So 3 seems better for spv2. This also conform to the existing behavior of LayoutSVGRoot. 3 makes sense to me too. Nice discussion! These issues are surprisingly subtle.
Please update the comment in BoxClipper.cpp to say: // Selection may extend beyond visual overflow, so this optimization is invalid if selection // is present.
Description was changed from ========== Include selection newline in visual overflow for selected line breaks. This change is intended to fix the paint invalidation rect, and thus the visual rect, as part of the linked bug http://crbug.com/605319 spun out of http://crbug.com/529938. There is more explanation of the intended use of visual rects in my comment at http://crbug.com/604883#c5 The visual rects that are incorrect without this change are not currently used by live code, but will be used as part of http://crrev.com/1484163002. This change fixes about 15 LayoutTests with that patch applied, mainly under editing, and those were mainly cases where LayoutText::isBR() returned true, though this change also incorporates fix for !isBR() where we just have text with a wrapped newline highlight at end. BUG=605319 ========== to ========== Include selection newline in visual overflow for selected line breaks. This change is intended to fix the paint invalidation rect, and thus the visual rect, as part of the linked bug http://crbug.com/605319 spun out of http://crbug.com/529938. There is more explanation of the intended use of visual rects in my comment at http://crbug.com/604883#c5 The visual rects that are incorrect without this change are not currently used by live code, but will be used as part of http://crrev.com/1484163002. This change fixes about 15 LayoutTests with that patch applied, mainly under editing. BUG=605319 ==========
Description was changed from ========== Include selection newline in visual overflow for selected line breaks. This change is intended to fix the paint invalidation rect, and thus the visual rect, as part of the linked bug http://crbug.com/605319 spun out of http://crbug.com/529938. There is more explanation of the intended use of visual rects in my comment at http://crbug.com/604883#c5 The visual rects that are incorrect without this change are not currently used by live code, but will be used as part of http://crrev.com/1484163002. This change fixes about 15 LayoutTests with that patch applied, mainly under editing. BUG=605319 ========== to ========== Include selection rect in paint invalidation for LayoutText. This change is intended to fix the paint invalidation rect, and thus the visual rect, as part of the linked bug http://crbug.com/605319 spun out of http://crbug.com/529938. There is more explanation of the intended use of visual rects in my comment at http://crbug.com/604883#c5 The visual rects that are incorrect without this change are not currently used by live code, but will be used as part of http://crrev.com/1484163002. This change fixes about 15 LayoutTests with that patch applied, mainly under editing. BUG=605319 ==========
Revised for (3) as discussed previously, updated change description, updated BoxClipper comment, added test expectations. Layout test diffs available at: https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...
lgtm
The CQ bit was checked by wkorman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org Link to the patchset: https://codereview.chromium.org/1955423004/#ps100001 (title: "Add expectations, simplify uniting.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1955423004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1955423004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by wkorman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wangxianzhu@chromium.org, eae@chromium.org Link to the patchset: https://codereview.chromium.org/1955423004/#ps120001 (title: "Sync to head and add one test.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1955423004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1955423004/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Include selection rect in paint invalidation for LayoutText. This change is intended to fix the paint invalidation rect, and thus the visual rect, as part of the linked bug http://crbug.com/605319 spun out of http://crbug.com/529938. There is more explanation of the intended use of visual rects in my comment at http://crbug.com/604883#c5 The visual rects that are incorrect without this change are not currently used by live code, but will be used as part of http://crrev.com/1484163002. This change fixes about 15 LayoutTests with that patch applied, mainly under editing. BUG=605319 ========== to ========== Include selection rect in paint invalidation for LayoutText. This change is intended to fix the paint invalidation rect, and thus the visual rect, as part of the linked bug http://crbug.com/605319 spun out of http://crbug.com/529938. There is more explanation of the intended use of visual rects in my comment at http://crbug.com/604883#c5 The visual rects that are incorrect without this change are not currently used by live code, but will be used as part of http://crrev.com/1484163002. This change fixes about 15 LayoutTests with that patch applied, mainly under editing. BUG=605319 Committed: https://crrev.com/76f9a545cdf9903cded65e98b616a14976428aa8 Cr-Commit-Position: refs/heads/master@{#394807} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/76f9a545cdf9903cded65e98b616a14976428aa8 Cr-Commit-Position: refs/heads/master@{#394807} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
