Bug 212856

Summary: Text form controls can scroll by 1px when value is the same length as size. No scrolling should happen.
Product: WebKit Reporter: Megan Gardner <megan_gardner>
Component: New BugsAssignee: Megan Gardner <megan_gardner>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, cdumez, changseok, esprehn+autocc, ews-watchlist, glenn, jbedard, kondapallykalyan, mifenton, mmaxfield, pdr, simon.fraser, thorton, tsavell, webkit-bug-importer, wenson_hsieh, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Megan Gardner 2020-06-05 16:31:49 PDT
Text form controls scroll by 1px when value is the same length and size, which shouldn't happen.
Comment 1 Megan Gardner 2020-06-05 16:41:44 PDT
Created attachment 401216 [details]
Patch
Comment 2 Megan Gardner 2020-06-05 17:13:56 PDT
Created attachment 401218 [details]
Patch
Comment 3 Megan Gardner 2020-06-09 14:22:48 PDT
Created attachment 401475 [details]
Patch
Comment 4 Megan Gardner 2020-06-09 21:43:27 PDT
Created attachment 401512 [details]
Patch
Comment 5 zalan 2020-06-10 07:05:25 PDT
I think this should match with the end padding adjustment in ComplexLineLayout::addOverflowFromInlineChildren(). Something along these lines:

diff --git a/Source/WebCore/rendering/ComplexLineLayout.cpp b/Source/WebCore/rendering/ComplexLineLayout.cpp
index 41bb52f06ef..7fb5e37c05d 100644
--- a/Source/WebCore/rendering/ComplexLineLayout.cpp
+++ b/Source/WebCore/rendering/ComplexLineLayout.cpp
@@ -2092,8 +2092,8 @@ void ComplexLineLayout::addOverflowFromInlineChildren()
 
     LayoutUnit endPadding = m_flow.hasOverflowClip() ? m_flow.paddingEnd() : 0_lu;
     // FIXME: Need to find another way to do this, since scrollbars could show when we don't want them to.
-    if (m_flow.hasOverflowClip() && !endPadding && m_flow.element() && m_flow.element()->isRootEditableElement() && style().isLeftToRightDirection())
-        endPadding = 1;
+    if (!endPadding)
+        endPadding = m_flow.endPaddingWidthForCaret();
     for (RootInlineBox* curr = firstRootBox(); curr; curr = curr->nextRootBox()) {
         m_flow.addLayoutOverflow(curr->paddedLayoutOverflowRect(endPadding));
         RenderFragmentContainer* fragment = m_flow.enclosingFragmentedFlow() ? curr->containingFragment() : nullptr;
diff --git a/Source/WebCore/rendering/RenderBlockFlow.h b/Source/WebCore/rendering/RenderBlockFlow.h
index b429b76921b..33cc0068017 100644
--- a/Source/WebCore/rendering/RenderBlockFlow.h
+++ b/Source/WebCore/rendering/RenderBlockFlow.h
@@ -403,6 +403,8 @@ public:
 
     void addFloatsToNewParent(RenderBlockFlow& toBlockFlow) const;
 
+    LayoutUnit endPaddingWidthForCaret() const;
+
 protected:
     bool shouldResetLogicalHeightBeforeLayout() const override { return true; }
 
@@ -652,6 +654,13 @@ inline LayoutIntegration::LineLayout* RenderBlockFlow::layoutFormattingContextLi
 {
     return hasLayoutFormattingContextLineLayout() ? WTF::get<std::unique_ptr<LayoutIntegration::LineLayout>>(m_lineLayout).get() : nullptr;
 }
+
+inline LayoutUnit RenderBlockFlow::endPaddingWidthForCaret() const
+{
+    if (element() && element()->isRootEditableElement() && hasOverflowClip() && style().isLeftToRightDirection())
+        return caretWidth;
+    return { };
+}
 #endif
 
 } // namespace WebCore
diff --git a/Source/WebCore/rendering/RenderTextControlSingleLine.cpp b/Source/WebCore/rendering/RenderTextControlSingleLine.cpp
index 16602173095..ebfad354652 100644
--- a/Source/WebCore/rendering/RenderTextControlSingleLine.cpp
+++ b/Source/WebCore/rendering/RenderTextControlSingleLine.cpp
@@ -306,7 +306,7 @@ float RenderTextControlSingleLine::getAverageCharWidth()
 
 LayoutUnit RenderTextControlSingleLine::preferredContentLogicalWidth(float charWidth) const
 {
-    int factor;
+    int factor = 0;
     bool includesDecoration = inputElement().sizeShouldIncludeDecoration(factor);
     if (factor <= 0)
         factor = 20;
@@ -334,6 +334,9 @@ LayoutUnit RenderTextControlSingleLine::preferredContentLogicalWidth(float charW
     if (includesDecoration)
         result += inputElement().decorationWidth();
 
+    if (auto* innerRenderer = innerTextElement()->renderer())
+        result += innerRenderer->endPaddingWidthForCaret();
+
     return result;
 }
Comment 6 Wenson Hsieh 2020-06-10 08:54:42 PDT
Comment on attachment 401512 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401512&action=review

> Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:195
> -        self._interrupt_if_at_failure_limits(run_results)
> +

Please remember to revert this Tools/ change as well.
Comment 7 zalan 2020-06-10 10:32:28 PDT
> +inline LayoutUnit RenderBlockFlow::endPaddingWidthForCaret() const
> +{
> +    if (element() && element()->isRootEditableElement() &&
> hasOverflowClip() && style().isLeftToRightDirection())
> +        return caretWidth;
> +    return { };
> +}
>  #endif
if (element() && element()->isRootEditableElement() && hasOverflowClip() && style().isLeftToRightDirection() && !paddingEnd())
Comment 8 Megan Gardner 2020-06-11 12:16:07 PDT
Created attachment 401664 [details]
Patch
Comment 9 Megan Gardner 2020-06-11 14:08:34 PDT
Created attachment 401681 [details]
Patch
Comment 10 Megan Gardner 2020-06-11 15:37:23 PDT
Created attachment 401686 [details]
Patch
Comment 11 Megan Gardner 2020-06-11 16:22:57 PDT
Created attachment 401693 [details]
Patch
Comment 12 Megan Gardner 2020-06-11 19:05:17 PDT
Created attachment 401699 [details]
Patch
Comment 13 Megan Gardner 2020-06-11 22:30:50 PDT
Created attachment 401708 [details]
Patch
Comment 14 Megan Gardner 2020-06-11 22:38:02 PDT
Created attachment 401709 [details]
Patch
Comment 15 Megan Gardner 2020-06-11 23:57:17 PDT
Created attachment 401712 [details]
Patch
Comment 16 Megan Gardner 2020-06-12 13:31:52 PDT
Created attachment 401772 [details]
Patch
Comment 17 Megan Gardner 2020-06-12 15:24:47 PDT
Created attachment 401793 [details]
Patch
Comment 18 zalan 2020-06-15 09:20:35 PDT
Comment on attachment 401793 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401793&action=review

> Source/WebCore/rendering/RenderBlockFlow.h:662
> +    if (element() && element()->isRootEditableElement() && hasOverflowClip() && style().isLeftToRightDirection())
> +        return caretWidth;

I think this should include the "!paddingEnd()" check too (see Comment #7). -This also has an unfortunate side effect of having different renderer width when some (non-geometry related) style changes. Let's see if it causes any issues. We could always fall back to the simple, let's always add a padding approach.
Comment 19 Megan Gardner 2020-06-15 15:28:21 PDT
Created attachment 401949 [details]
Patch
Comment 20 Megan Gardner 2020-06-15 17:02:44 PDT
Created attachment 401957 [details]
Patch
Comment 21 Megan Gardner 2020-06-15 18:48:38 PDT
Created attachment 401967 [details]
Patch for landing
Comment 22 EWS 2020-06-15 19:17:07 PDT
Committed r263073: <https://trac.webkit.org/changeset/263073>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401967 [details].
Comment 23 Radar WebKit Bug Importer 2020-06-15 19:18:23 PDT
<rdar://problem/64389038>
Comment 24 Megan Gardner 2020-06-15 23:03:31 PDT
<rdar://problem/63541707>
Comment 25 Truitt Savell 2020-06-16 07:57:51 PDT
Updated the expected file for tables/mozilla_expected_failures/bugs/bug2479-5.html in https://trac.webkit.org/changeset/263086/webkit