Commit d75fc484 authored by Xiaocheng Hu's avatar Xiaocheng Hu Committed by Commit Bot

Reland "Fix caret rendering when at the end of a block after an ineditable inline"

This reverts commit 6d77a353.

Reason for reland: Max recursion depth is reduced to avoid stack
overflow.

Original change's description:
> Revert "Fix caret rendering when at the end of a block after an ineditable inline"
> 
> This reverts commit e9aa20fd.
> 
> Reason for revert: this change breaks tests on win dbg bot
> 
> This is the first failure
> https://ci.chromium.org/p/chromium/builders/ci/Win10%20Tests%20x64%20%28dbg%29/6437
> 
> Test InsertListCommandTest.CleanupNodeSameAsDestinationNode is definitely fails because of this patch (I've verified it locally on Windows).
> The problem with this test that it's very deep recursion in function between functions
> AdjustBlockFlowPositionToInline
> ComputeInlineAdjustedPositionAlgorithm
> 
> and as a result stack overflow.
> 
> 
> Original change's description:
> > Fix caret rendering when at the end of a block after an ineditable inline
> >
> > AdjustBlockFlowPositionToInline() has an early-reject branch that avoids
> > infinite recursion, introduced in crrev.com/664b6437.
> >
> > However, the early reject condition is too aggressive that, it also
> > rejects some positions that don't enter an infinite recursion, like
> > the one used in the new unit test added in this CL.
> >
> > As now AdjustBlockFlowPositionToInline() has a hard limit on recursion
> > depth, this CL safely removes the over-aggressive reject condition, so
> > that caret can be correctly rendered for more positions.
> >
> > Note: Unit test InlineBoxPositionTest.ComputeInlineBoxPositionMixedEditable
> > uses another position that doesn't enter the infinite loop but is falsely
> > rejected by the condition. This patch allows caret rendering also for that
> > position, and therefore changes its test expectation.
> >
> > Bug: 936988
> > Change-Id: Idef91ffaa412e67cddd5fcf0dd61f54055de7189
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1497466
> > Auto-Submit: Xiaocheng Hu <xiaochengh@chromium.org>
> > Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
> > Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#637311}
> 
> TBR=yosin@chromium.org,yoichio@chromium.org,xiaochengh@chromium.org
> 
> Change-Id: I6fcff118f27d633e627d5942671b04554fef283b
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 936988
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1503579
> Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
> Commit-Queue: Vadym Doroshenko <dvadym@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#637676}

TBR=yosin@chromium.org,yoichio@chromium.org,dvadym@chromium.org,xiaochengh@chromium.org

Change-Id: Ic959d3d57558995a315269bb938b75307efb6ea8
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 936988
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1504226
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Reviewed-by: default avatarXiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#637832}
parent 83367864
...@@ -45,7 +45,7 @@ namespace blink { ...@@ -45,7 +45,7 @@ namespace blink {
namespace { namespace {
const int kBlockFlowAdjustmentMaxRecursionDepth = 1024; const int kBlockFlowAdjustmentMaxRecursionDepth = 256;
bool IsNonTextLeafChild(LayoutObject* object) { bool IsNonTextLeafChild(LayoutObject* object) {
if (object->SlowFirstChild()) if (object->SlowFirstChild())
...@@ -217,8 +217,7 @@ PositionWithAffinityTemplate<Strategy> AdjustBlockFlowPositionToInline( ...@@ -217,8 +217,7 @@ PositionWithAffinityTemplate<Strategy> AdjustBlockFlowPositionToInline(
} }
const PositionTemplate<Strategy>& upstream_equivalent = const PositionTemplate<Strategy>& upstream_equivalent =
UpstreamIgnoringEditingBoundaries(position); UpstreamIgnoringEditingBoundaries(position);
if (upstream_equivalent == position || if (upstream_equivalent == position)
DownstreamIgnoringEditingBoundaries(upstream_equivalent) == position)
return PositionWithAffinityTemplate<Strategy>(); return PositionWithAffinityTemplate<Strategy>();
return ComputeInlineAdjustedPositionAlgorithm( return ComputeInlineAdjustedPositionAlgorithm(
......
...@@ -49,12 +49,15 @@ TEST_F(InlineBoxPositionTest, ComputeInlineBoxPositionMixedEditable) { ...@@ -49,12 +49,15 @@ TEST_F(InlineBoxPositionTest, ComputeInlineBoxPositionMixedEditable) {
SetBodyContent( SetBodyContent(
"<div contenteditable id=sample>abc<input contenteditable=false></div>"); "<div contenteditable id=sample>abc<input contenteditable=false></div>");
Element* const sample = GetDocument().getElementById("sample"); Element* const sample = GetDocument().getElementById("sample");
Element* const input = GetDocument().QuerySelector("input");
const InlineBox* const input_wrapper_box =
ToLayoutBox(input->GetLayoutObject())->InlineBoxWrapper();
const InlineBoxPosition& actual = ComputeInlineBoxPosition( const InlineBoxPosition& actual = ComputeInlineBoxPosition(
PositionWithAffinity(Position::LastPositionInNode(*sample))); PositionWithAffinity(Position::LastPositionInNode(*sample)));
// Should not be in infinite-loop // Should not be in infinite-loop
EXPECT_EQ(nullptr, actual.inline_box); EXPECT_EQ(input_wrapper_box, actual.inline_box);
EXPECT_EQ(0, actual.offset_in_box); EXPECT_EQ(1, actual.offset_in_box);
} }
// http://crbug.com/841363 // http://crbug.com/841363
......
...@@ -972,4 +972,21 @@ TEST_P(ParameterizedLocalCaretRectTest, AfterCollapsedWhiteSpaceInRTLText) { ...@@ -972,4 +972,21 @@ TEST_P(ParameterizedLocalCaretRectTest, AfterCollapsedWhiteSpaceInRTLText) {
PositionWithAffinity(position, TextAffinity::kDownstream))); PositionWithAffinity(position, TextAffinity::kDownstream)));
} }
// https://crbug.com/936988
TEST_P(ParameterizedLocalCaretRectTest, AfterIneditableInline) {
// For LayoutNG, we also enable EditingNG to test NG caret rendering.
ScopedEditingNGForTest editing_ng(LayoutNGEnabled());
LoadAhem();
InsertStyleElement("div { font: 10px/10px Ahem }");
SetBodyContent(
"<div contenteditable><span contenteditable=\"false\">foo</span></div>");
const Element* div = GetDocument().QuerySelector("div");
const Node* text = div->firstChild()->firstChild();
const Position position = Position::LastPositionInNode(*div);
EXPECT_EQ(LocalCaretRect(text->GetLayoutObject(), LayoutRect(30, 0, 1, 10)),
LocalCaretRectOfPosition(PositionWithAffinity(position)));
}
} // namespace blink } // namespace blink
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment