Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(97)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 709613003: To ensure consistency between layouts, we need to reset any overriden top for centering the textfie… (Closed)

Created:
6 years, 1 month ago by pals
Modified:
6 years ago
CC:
blink-reviews, blink-reviews-rendering, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, rune+blink, zoltan1
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Project:
blink
Visibility:
Public.

Description

To ensure consistency between layouts, we need to reset any overriden top for centering the textfield. BUG=379964

Patch Set 1 #

Patch Set 2 : tests #

Patch Set 3 : #

Total comments: 12

Patch Set 4 : Updates #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+60 lines, -0 lines) Patch
A LayoutTests/fast/forms/positioned-input-text-cursor.html View 1 2 3 1 chunk +38 lines, -0 lines 1 comment Download
A LayoutTests/fast/forms/positioned-input-text-cursor-expected.html View 1 2 3 1 chunk +15 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderTextControlSingleLine.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/rendering/RenderTextControlSingleLine.cpp View 1 2 3 3 chunks +6 lines, -0 lines 1 comment Download

Messages

Total messages: 8 (4 generated)
pals
Please take a look.
6 years, 1 month ago (2014-11-11 13:17:01 UTC) #4
Julien - ping for review
https://codereview.chromium.org/709613003/diff/80001/LayoutTests/fast/repaint/positioned-input-text-cursor-repaint-expected.html File LayoutTests/fast/repaint/positioned-input-text-cursor-repaint-expected.html (right): https://codereview.chromium.org/709613003/diff/80001/LayoutTests/fast/repaint/positioned-input-text-cursor-repaint-expected.html#newcode1 LayoutTests/fast/repaint/positioned-input-text-cursor-repaint-expected.html:1: <!DOCTYPE html> If you only want to measure layout-related ...
6 years, 1 month ago (2014-11-11 16:57:58 UTC) #5
pals
I have changed the logic. Please review. https://codereview.chromium.org/709613003/diff/80001/LayoutTests/fast/repaint/positioned-input-text-cursor-repaint-expected.html File LayoutTests/fast/repaint/positioned-input-text-cursor-repaint-expected.html (right): https://codereview.chromium.org/709613003/diff/80001/LayoutTests/fast/repaint/positioned-input-text-cursor-repaint-expected.html#newcode1 LayoutTests/fast/repaint/positioned-input-text-cursor-repaint-expected.html:1: <!DOCTYPE html> ...
6 years, 1 month ago (2014-11-12 13:29:08 UTC) #7
Julien - ping for review
6 years, 1 month ago (2014-11-19 01:13:27 UTC) #8 
https://codereview.chromium.org/709613003/diff/80001/LayoutTests/fast/repaint...
File LayoutTests/fast/repaint/positioned-input-text-cursor-repaint-expected.html
(right):

https://codereview.chromium.org/709613003/diff/80001/LayoutTests/fast/repaint...
LayoutTests/fast/repaint/positioned-input-text-cursor-repaint-expected.html:1:
<!DOCTYPE html>
On 2014/11/12 13:29:08, sanjoy_pal wrote:
> On 2014/11/11 16:57:58, Julien Chaffraix - PST wrote:
> > If you only want to measure layout-related properties, you should do a
> > check-layout.js test as the output is more readable, it's faster and less
> likely
> > to shoot yourself in the foot (ref-test only test what's in the viewport,
> > anything else is ignored).
> 
> I dont want to measure the layout-related properties. I want to test if the
> cursor is rendered out of the input field.

The logical top is a layout-related property and that's what you're fixing.
ref-tests are OK but they have downsides and we should use check-layout.js tests
as much as possible.

https://codereview.chromium.org/709613003/diff/80001/LayoutTests/fast/repaint...
File LayoutTests/fast/repaint/positioned-input-text-cursor-repaint.html (right):

https://codereview.chromium.org/709613003/diff/80001/LayoutTests/fast/repaint...
LayoutTests/fast/repaint/positioned-input-text-cursor-repaint.html:8:
refreshIntervalId = setInterval(function(){moveInputElement();}, 10);
On 2014/11/12 13:29:08, sanjoy_pal wrote:
> On 2014/11/11 16:57:58, Julien Chaffraix - PST wrote:
> > setInterval is *almost never* a good solution. If you need to force a
layout,
> > just call document.body.offsetTop;
> The bug does not reproduce in case of fulllayout. The logicalTop
> keeps accumulating overriden value in case of simplifiedLayout while centering
> the innerEditor. how to force a simplifiedLayout?

simplifiedlayout is just a type of layout. You should be able to force it the
same way you do a normal layout: document.body.offsetTop;

https://codereview.chromium.org/709613003/diff/120001/LayoutTests/fast/forms/...
File LayoutTests/fast/forms/positioned-input-text-cursor.html (right):

https://codereview.chromium.org/709613003/diff/120001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/positioned-input-text-cursor.html:37: <!-- The cursor
should not be rendered outside the input textfield. -->
The description should be dumped into the ouput, unless there is a good reason
not to (makes the test platform-specific, prevents the bug from reproducing...).

Dumping it here is a no lose move AFAICT.

https://codereview.chromium.org/709613003/diff/120001/Source/core/rendering/R...
File Source/core/rendering/RenderTextControlSingleLine.cpp (right):

https://codereview.chromium.org/709613003/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderTextControlSingleLine.cpp:174:
centerContainerIfNeeded(containerRenderer);
This also overrides the logical top under some conditions so shouldn't we set
the new boolean here too?

Overall this approach is not great: we prefer to check the previous layout's
values to determine if we need to do another layout or recompute extra
properties.

But more fundamentally, I think you're patching the problem, not the cause of
the problem. It seems like the formula to compute the logical top shouldn't use
a previous layout's information (if we end up not relaying
|innerEditorRenderer|). Forcing a layout forces the logical top to be reset,
which fixes the problem, but pays a high price for it by missing the root cause.

Powered by Google App Engine
This is Rietveld 408576698