Bug 212856 - Text form controls can scroll by 1px when value is the same length as size. No scrolling should happen.
Summary: Text form controls can scroll by 1px when value is the same length as size. N...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Megan Gardner
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-06-05 16:31 PDT by Megan Gardner
Modified: 2020-06-16 07:57 PDT (History)
17 users (show)

See Also:


Attachments
Patch (1.70 KB, patch)
2020-06-05 16:41 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (6.93 KB, patch)
2020-06-05 17:13 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (956.91 KB, patch)
2020-06-09 14:22 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (965.27 KB, patch)
2020-06-09 21:43 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (965.61 KB, patch)
2020-06-11 12:16 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (969.69 KB, patch)
2020-06-11 14:08 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (971.14 KB, patch)
2020-06-11 15:37 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (970.72 KB, patch)
2020-06-11 16:22 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (1019.49 KB, patch)
2020-06-11 19:05 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (1020.25 KB, patch)
2020-06-11 22:30 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (1022.69 KB, patch)
2020-06-11 22:38 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (934.02 KB, patch)
2020-06-11 23:57 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (940.43 KB, patch)
2020-06-12 13:31 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (940.37 KB, patch)
2020-06-12 15:24 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (940.38 KB, patch)
2020-06-15 15:28 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (926.04 KB, patch)
2020-06-15 17:02 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch for landing (926.04 KB, patch)
2020-06-15 18:48 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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