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

Issue 568453002: Removing container's Left & Top border from computed scroll offset bounds. (Closed)

Created:
6 years, 3 months ago by Ambarish
Modified:
6 years, 2 months ago
CC:
arpitab_, arpitabahuguna, gnana, pals
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Project:
blink
Visibility:
Public.

Description

Element::scrollIntoViewIfNeeded computes the scroll bounding box by calling containerNode::boundingBox(), for performing autoscroll. This bounding box rect is passed to RenderLayerScrollableArea::exposeRect(), where actual scroll parent rect to be scrolled is computed. This bound is computed by using two factors. current Scroll Offset and the target Scroll Offset. target scroll offset is Y position of the element which has to be scrolled into view. This Y position computation includes top border of the parent container. for example. in the bug reported page. Parent grid has border of 5 px so, the position of first child would be 5 and the position of second child would be 55 and so on But [current]scroll offset is independent of top border. When target bounding box is computed, target scroll offset is substracted from current scroll offset which results in incorrect scroll offset. for example. in the bug reported page 1. When page loads grid is scrolled to 200. 2. Parents top border is 5, 3. Position of 2nd child is 55 4. so the target scroll offset is computed as, 55 - 200 = -145 , which is incorrect & should be, 50 - 200 = -150 5. Hence the borderTop included in the position causes the issue, and needs to be removed from computed scroll offset value. BUG=295848 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183917

Patch Set 1 #

Total comments: 7

Patch Set 2 : exposeRect() : Removing borderTop from localExposeRect of scrollable box #

Total comments: 7

Patch Set 3 : Removing parent's left & top border from computed scroll rect. #

Total comments: 13

Patch Set 4 : Modified Layout test to make it more readable and simple #

Total comments: 2

Patch Set 5 : Reduced html content from Layout test #

Patch Set 6 : Reduced layout test file contents from javascript, style and html content #

Unified diffs Side-by-side diffs Delta from patch set Stats (+72 lines, -19 lines) Patch
M LayoutTests/TestExpectations View 1 2 3 4 1 chunk +10 lines, -0 lines 0 comments Download
M LayoutTests/fast/forms/listbox-selection-2.html View 1 1 chunk +2 lines, -2 lines 0 comments Download
M LayoutTests/fast/forms/listbox-selection-2-expected.txt View 1 1 chunk +2 lines, -2 lines 0 comments Download
A LayoutTests/fast/scroll-behavior/bordered-container-child-scroll.html View 1 2 3 4 5 1 chunk +42 lines, -0 lines 0 comments Download
A + LayoutTests/fast/scroll-behavior/bordered-container-child-scroll-expected.txt View 1 2 3 4 5 1 chunk +5 lines, -5 lines 0 comments Download
M LayoutTests/fast/spatial-navigation/snav-div-overflow-scrol-hidden-expected.txt View 1 1 chunk +2 lines, -2 lines 0 comments Download
M LayoutTests/platform/linux/editing/input/caret-at-the-edge-of-contenteditable-expected.png View 1 2 Binary file 0 comments Download
M LayoutTests/platform/linux/editing/input/caret-at-the-edge-of-contenteditable-expected.txt View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/platform/linux/editing/input/reveal-caret-of-multiline-contenteditable-expected.png View 1 Binary file 0 comments Download
M LayoutTests/platform/linux/editing/input/reveal-caret-of-multiline-contenteditable-expected.txt View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/platform/linux/editing/input/reveal-caret-of-multiline-input-expected.png View 1 Binary file 0 comments Download
M LayoutTests/platform/linux/editing/input/reveal-caret-of-multiline-input-expected.txt View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/platform/linux/fast/forms/listbox-hit-test-zoomed-expected.png View 1 Binary file 0 comments Download
M LayoutTests/platform/linux/fast/forms/listbox-hit-test-zoomed-expected.txt View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/platform/linux/fast/forms/select-initial-position-expected.png View 1 Binary file 0 comments Download
M LayoutTests/platform/linux/fast/forms/select-initial-position-expected.txt View 1 2 3 4 2 chunks +2 lines, -2 lines 0 comments Download
M LayoutTests/platform/linux/fast/forms/select/menulist-appearance-basic-expected.png View 1 Binary file 0 comments Download
M LayoutTests/platform/linux/fast/forms/select/menulist-appearance-basic-expected.txt View 1 2 3 4 2 chunks +2 lines, -2 lines 0 comments Download
M LayoutTests/platform/linux/fast/forms/suggestion-picker/date-suggestion-picker-appearance-with-scroll-bar-expected.png View 1 2 Binary file 0 comments Download
M LayoutTests/platform/linux/fast/forms/suggestion-picker/month-suggestion-picker-appearance-with-scroll-bar-expected.png View 1 2 Binary file 0 comments Download
M LayoutTests/platform/linux/fast/forms/suggestion-picker/week-suggestion-picker-appearance-with-scroll-bar-expected.png View 1 2 Binary file 0 comments Download
M Source/core/rendering/RenderLayerScrollableArea.cpp View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 27 (6 generated)
arpitabahuguna
https://codereview.chromium.org/568453002/diff/1/Source/core/dom/Element.cpp File Source/core/dom/Element.cpp (right): https://codereview.chromium.org/568453002/diff/1/Source/core/dom/Element.cpp#newcode472 Source/core/dom/Element.cpp:472: // Fix start : for crbug.com/295848. You don't need ...
6 years, 3 months ago (2014-09-11 14:14:59 UTC) #4
arpitabahuguna
https://codereview.chromium.org/568453002/diff/1/Source/core/dom/Element.cpp File Source/core/dom/Element.cpp (right): https://codereview.chromium.org/568453002/diff/1/Source/core/dom/Element.cpp#newcode472 Source/core/dom/Element.cpp:472: // Fix start : for crbug.com/295848. You don't need ...
6 years, 3 months ago (2014-09-11 14:14:59 UTC) #5
pals
https://codereview.chromium.org/568453002/diff/1/Source/core/dom/Element.cpp File Source/core/dom/Element.cpp (right): https://codereview.chromium.org/568453002/diff/1/Source/core/dom/Element.cpp#newcode475 Source/core/dom/Element.cpp:475: // border, so this border top should be removed ...
6 years, 3 months ago (2014-09-12 13:40:08 UTC) #6
Ambarish
On 2014/09/12 13:40:08, sanjoy_pal wrote: > https://codereview.chromium.org/568453002/diff/1/Source/core/dom/Element.cpp > File Source/core/dom/Element.cpp (right): > > https://codereview.chromium.org/568453002/diff/1/Source/core/dom/Element.cpp#newcode475 > ...
6 years, 3 months ago (2014-09-24 10:48:16 UTC) #7
pals
https://codereview.chromium.org/568453002/diff/20001/LayoutTests/fast/scroll-behavior/bordered-container-child-scroll.html File LayoutTests/fast/scroll-behavior/bordered-container-child-scroll.html (right): https://codereview.chromium.org/568453002/diff/20001/LayoutTests/fast/scroll-behavior/bordered-container-child-scroll.html#newcode10 LayoutTests/fast/scroll-behavior/bordered-container-child-scroll.html:10: overflow: auto; Indentations. https://codereview.chromium.org/568453002/diff/20001/LayoutTests/fast/scroll-behavior/bordered-container-child-scroll.html#newcode22 LayoutTests/fast/scroll-behavior/bordered-container-child-scroll.html:22: <body> Not needed. https://codereview.chromium.org/568453002/diff/20001/LayoutTests/fast/scroll-behavior/bordered-container-child-scroll.html#newcode23 ...
6 years, 3 months ago (2014-09-24 12:31:36 UTC) #8
Ambarish
On 2014/09/24 12:31:36, sanjoy_pal wrote: > https://codereview.chromium.org/568453002/diff/20001/LayoutTests/fast/scroll-behavior/bordered-container-child-scroll.html > File LayoutTests/fast/scroll-behavior/bordered-container-child-scroll.html > (right): > > https://codereview.chromium.org/568453002/diff/20001/LayoutTests/fast/scroll-behavior/bordered-container-child-scroll.html#newcode10 ...
6 years, 2 months ago (2014-09-29 10:26:24 UTC) #9
Ambarish
ping
6 years, 2 months ago (2014-10-06 11:01:31 UTC) #11
Ambarish
Dear Levi, Please review this patch. Kindly waiting for your feedback on this patch.
6 years, 2 months ago (2014-10-08 15:32:17 UTC) #13
leviw_travelin_and_unemployed
The code change looks good. Spend some time cleaning up the test and we should ...
6 years, 2 months ago (2014-10-08 20:18:42 UTC) #14
leviw_travelin_and_unemployed
https://codereview.chromium.org/568453002/diff/40001/LayoutTests/fast/scroll-behavior/bordered-container-child-scroll.html File LayoutTests/fast/scroll-behavior/bordered-container-child-scroll.html (right): https://codereview.chromium.org/568453002/diff/40001/LayoutTests/fast/scroll-behavior/bordered-container-child-scroll.html#newcode20 LayoutTests/fast/scroll-behavior/bordered-container-child-scroll.html:20: background: black; A lot of these styles are simply ...
6 years, 2 months ago (2014-10-08 20:19:14 UTC) #15
Ambarish
On 2014/10/08 20:18:42, leviw wrote: > The code change looks good. Spend some time cleaning ...
6 years, 2 months ago (2014-10-09 17:52:03 UTC) #16
leviw_travelin_and_unemployed
On 2014/10/09 at 17:52:03, ambarish.r wrote: > On 2014/10/08 20:18:42, leviw wrote: > > The ...
6 years, 2 months ago (2014-10-09 22:35:23 UTC) #17
Ambarish
PTAL https://codereview.chromium.org/568453002/diff/40001/LayoutTests/TestExpectations File LayoutTests/TestExpectations (right): https://codereview.chromium.org/568453002/diff/40001/LayoutTests/TestExpectations#newcode176 LayoutTests/TestExpectations:176: # Below test's behavior is changed due to ...
6 years, 2 months ago (2014-10-10 11:28:45 UTC) #18
leviw_travelin_and_unemployed
https://codereview.chromium.org/568453002/diff/80001/LayoutTests/fast/scroll-behavior/bordered-container-child-scroll.html File LayoutTests/fast/scroll-behavior/bordered-container-child-scroll.html (right): https://codereview.chromium.org/568453002/diff/80001/LayoutTests/fast/scroll-behavior/bordered-container-child-scroll.html#newcode60 LayoutTests/fast/scroll-behavior/bordered-container-child-scroll.html:60: <td><div class="column"></div></td> Can't this be done with a small ...
6 years, 2 months ago (2014-10-11 01:49:16 UTC) #19
Ambarish
https://codereview.chromium.org/568453002/diff/80001/LayoutTests/fast/scroll-behavior/bordered-container-child-scroll.html File LayoutTests/fast/scroll-behavior/bordered-container-child-scroll.html (right): https://codereview.chromium.org/568453002/diff/80001/LayoutTests/fast/scroll-behavior/bordered-container-child-scroll.html#newcode60 LayoutTests/fast/scroll-behavior/bordered-container-child-scroll.html:60: <td><div class="column"></div></td> On 2014/10/11 01:49:16, leviw wrote: > Can't ...
6 years, 2 months ago (2014-10-12 11:59:56 UTC) #20
Ambarish
Dear Leviw, The layout test contents are reduced and test is updated in Patch Set ...
6 years, 2 months ago (2014-10-16 15:01:12 UTC) #21
leviw_travelin_and_unemployed
Honestly, this test could still be about half a long and demonstrate the issue.
6 years, 2 months ago (2014-10-16 18:21:48 UTC) #22
Ambarish
On 2014/10/16 18:21:48, leviw wrote: > Honestly, this test could still be about half a ...
6 years, 2 months ago (2014-10-17 12:53:43 UTC) #23
leviw_travelin_and_unemployed
LGTM. Thanks for staying with this.
6 years, 2 months ago (2014-10-17 18:12:05 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/568453002/290001
6 years, 2 months ago (2014-10-17 18:12:29 UTC) #26
commit-bot: I haz the power
6 years, 2 months ago (2014-10-17 19:26:46 UTC) #27
Message was sent while issue was closed.
Committed patchset #6 (id:290001) as 183917

Powered by Google App Engine
This is Rietveld 408576698