Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2320303002:
Reset VisualViewport position after same page navigation (Closed)
|
Created:
4 years, 3 months ago by chaopeng Modified:
4 years, 3 months ago Reviewers:
bokan CC:
chromium-reviews, blink-reviews, loading-reviews_chromium.org, tyoshino+watch_chromium.org, Nate Chapin, gavinp+loader_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReset VisualViewport position after same page navigation
This issue can be reproduced using pinch zoom in Android, ChromeOS and Mac.
After navigating to the **exact** same page, we create a new FrameView but
reuse the VisualViewport, meaning its scroll position remains unchanged even
though it was relative to an old FrameView.
In this patch, we reset the VisualViewport position in Page::didCommitLoad
since prior to here we would overwrite the old history item.
BUG=642279
Committed: https://crrev.com/4521a9bb2402ebfffd709f02413ba3ec5a70de46
Cr-Commit-Position: refs/heads/master@{#419167}
Patch Set 1 #Patch Set 2 : move the fix to RootFrameViewport #Patch Set 3 : move reset to Page::didCommitLoad and create test #
Total comments: 4
Patch Set 4 : fix test case #Patch Set 5 : add test html #
Total comments: 2
Patch Set 6 : add tests #Patch Set 7 : remove unuse code #
Total comments: 4
Patch Set 8 : update test html #Patch Set 9 : change test to async #
Total comments: 6
Patch Set 10 : update comments and test #
Total comments: 2
Patch Set 11 : change test to async test #
Total comments: 2
Patch Set 12 : update comment #
Messages
Total messages: 38 (11 generated)
Description was changed from ========== fix-642279 BUG=642279 ========== to ========== correct the scrollbar position for same page navigation after scale reset the scroll in VisualViewport when reset the page. BUG=642279 ==========
chaopeng@chromium.org changed reviewers: + bokan@chromium.org
Hey Chao, Change looks fine but this will require a test. I think a test in WebFrameTest.cpp is probably easiest. You'll want to load a page, zoom-in and scroll so that VisualViewport has some scroll offset, then navigate to the same page and make sure the VisualViewport offset is reset. Also, the title is a bit confusing. We're changing the "scroll position" not the "scrollbar position" (scrollbar calculates its position from the page's scroll position). I would word it at "Reset VisualViewport position after same page navigation"
On 2016/09/09 19:33:13, bokan wrote: > Hey Chao, > > Change looks fine but this will require a test. I think a test in > WebFrameTest.cpp is probably easiest. You'll want to load a page, zoom-in and > scroll so that VisualViewport has some scroll offset, then navigate to the same > page and make sure the VisualViewport offset is reset. > > Also, the title is a bit confusing. We're changing the "scroll position" not the > "scrollbar position" (scrollbar calculates its position from the page's scroll > position). I would word it at "Reset VisualViewport position after same page > navigation" Hi David, The testcase added.
Please fix up the CL description as noted in my previous message. https://codereview.chromium.org/2320303002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/Page.cpp (right): https://codereview.chromium.org/2320303002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/Page.cpp:439: frameHost().visualViewport().setScrollPosition(DoublePoint(), ProgrammaticScroll); Please add a comment that we need to reset the scroll position from here since the commitLoad happens after we create a new history item in FrameLoader. https://codereview.chromium.org/2320303002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/VisualViewportTest.cpp (right): https://codereview.chromium.org/2320303002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/VisualViewportTest.cpp:1939: // Ensure that scrobar position correct when navigate to same page after resize Nit: "Ensure that scrobar position correct when navigate to..." -> "Ensure that the scroll position is correct when we navigate to..." https://codereview.chromium.org/2320303002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/VisualViewportTest.cpp:1941: TEST_P(ParameterizedVisualViewportTest, ScrobarPositionForLinkToSamePageAfterResize) Why do we need the resize? That's not part of the bug, right?
Description was changed from ========== correct the scrollbar position for same page navigation after scale reset the scroll in VisualViewport when reset the page. BUG=642279 ========== to ========== Reset VisualViewport position after same page navigation BUG=642279 ==========
https://codereview.chromium.org/2320303002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/VisualViewportTest.cpp (right): https://codereview.chromium.org/2320303002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/VisualViewportTest.cpp:1941: TEST_P(ParameterizedVisualViewportTest, ScrobarPositionForLinkToSamePageAfterResize) On 2016/09/12 18:55:23, bokan wrote: > Why do we need the resize? That's not part of the bug, right? Is it resize == scale(zoom-in/zoom-out)? This bug related to scale.
On 2016/09/12 20:08:35, chaopeng wrote: > https://codereview.chromium.org/2320303002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/web/tests/VisualViewportTest.cpp (right): > > https://codereview.chromium.org/2320303002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/web/tests/VisualViewportTest.cpp:1941: > TEST_P(ParameterizedVisualViewportTest, > ScrobarPositionForLinkToSamePageAfterResize) > On 2016/09/12 18:55:23, bokan wrote: > > Why do we need the resize? That's not part of the bug, right? > > Is it resize == scale(zoom-in/zoom-out)? This bug related to scale. Nope, resize is like on desktop where you drag the mouse to make the window bigger. Zoom/scale comes from pinch-zoom. You want to use webViewImpl::setPageScaleFactor to increase/decrease zoom.
On 2016/09/13 01:28:58, bokan wrote: > On 2016/09/12 20:08:35, chaopeng wrote: > > > https://codereview.chromium.org/2320303002/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/web/tests/VisualViewportTest.cpp (right): > > > > > https://codereview.chromium.org/2320303002/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/web/tests/VisualViewportTest.cpp:1941: > > TEST_P(ParameterizedVisualViewportTest, > > ScrobarPositionForLinkToSamePageAfterResize) > > On 2016/09/12 18:55:23, bokan wrote: > > > Why do we need the resize? That's not part of the bug, right? > > > > Is it resize == scale(zoom-in/zoom-out)? This bug related to scale. > > Nope, resize is like on desktop where you drag the mouse to make the window > bigger. Zoom/scale comes from pinch-zoom. You want to use > webViewImpl::setPageScaleFactor to increase/decrease zoom. PTAL, THANKS
Description was changed from ========== Reset VisualViewport position after same page navigation BUG=642279 ========== to ========== Reset VisualViewport position after same page navigation BUG=642279 ==========
https://codereview.chromium.org/2320303002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/Page.cpp (right): https://codereview.chromium.org/2320303002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/Page.cpp:439: // Need reset visual viewport position, fix for crbug.com/642279 Comments should answer "why?" not "what?". This comment just says what the line below does (which we can easily see by looking at the line below). Instead, I'd want to know *why* we reset the visual viewport here and not in a more logical place. i.e. we need to reset the visual viewport position here since the setting it earlier will clobber the current history item and didCommitLoad happens after a new history item is created in FrameLoader. https://codereview.chromium.org/2320303002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/VisualViewportTest.cpp (right): https://codereview.chromium.org/2320303002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/VisualViewportTest.cpp:1957: frameView.layoutViewportScrollableArea()->scrollPositionDouble()); The bug is that the *visual viewport* isn't reset. This check should be against the visual viewport. Also, please run this test without your fix above and make sure it fails (i.e. having this test would have caught the bug).
On 2016/09/13 14:56:47, bokan wrote: > https://codereview.chromium.org/2320303002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/page/Page.cpp (right): > > https://codereview.chromium.org/2320303002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/page/Page.cpp:439: // Need reset visual viewport > position, fix for crbug.com/642279 > Comments should answer "why?" not "what?". This comment just says what the line > below does (which we can easily see by looking at the line below). Instead, I'd > want to know *why* we reset the visual viewport here and not in a more logical > place. i.e. we need to reset the visual viewport position here since the setting > it earlier will clobber the current history item and didCommitLoad happens after > a new history item is created in FrameLoader. > > https://codereview.chromium.org/2320303002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/web/tests/VisualViewportTest.cpp (right): > > https://codereview.chromium.org/2320303002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/web/tests/VisualViewportTest.cpp:1957: > frameView.layoutViewportScrollableArea()->scrollPositionDouble()); > The bug is that the *visual viewport* isn't reset. This check should be against > the visual viewport. Also, please run this test without your fix above and make > sure it fails (i.e. having this test would have caught the bug). PTAL, THANKS
On 2016/09/14 17:58:43, chaopeng wrote: > On 2016/09/13 14:56:47, bokan wrote: > > > https://codereview.chromium.org/2320303002/diff/80001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/page/Page.cpp (right): > > > > > https://codereview.chromium.org/2320303002/diff/80001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/page/Page.cpp:439: // Need reset visual > viewport > > position, fix for crbug.com/642279 > > Comments should answer "why?" not "what?". This comment just says what the > line > > below does (which we can easily see by looking at the line below). Instead, > I'd > > want to know *why* we reset the visual viewport here and not in a more logical > > place. i.e. we need to reset the visual viewport position here since the > setting > > it earlier will clobber the current history item and didCommitLoad happens > after > > a new history item is created in FrameLoader. > > > > > https://codereview.chromium.org/2320303002/diff/80001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/web/tests/VisualViewportTest.cpp (right): > > > > > https://codereview.chromium.org/2320303002/diff/80001/third_party/WebKit/Sour... > > third_party/WebKit/Source/web/tests/VisualViewportTest.cpp:1957: > > frameView.layoutViewportScrollableArea()->scrollPositionDouble()); > > The bug is that the *visual viewport* isn't reset. This check should be > against > > the visual viewport. Also, please run this test without your fix above and > make > > sure it fails (i.e. having this test would have caught the bug). > > PTAL, THANKS FYI, you should normally reply to each comment that a reviewer makes. A simple "Done." is fine as a way of letting the reviewer know that the comment has been addressed, or if it hasn't, an explanation. In this case, my comment above (in Page.cpp) hasn't been addressed.
https://codereview.chromium.org/2320303002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/scrolling/same-page-navigate.html (right): https://codereview.chromium.org/2320303002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/scrolling/same-page-navigate.html:7: width: 600px; Nit: unnecessary tab https://codereview.chromium.org/2320303002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/scrolling/same-page-navigate.html:29: . All this space and other links are just adding unnecessary clutter. Please remove everything but the one link you want to click. You can place it down the page by using `style='position: absolute; top: 2000px'` https://codereview.chromium.org/2320303002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/scrolling/same-page-navigate.html:217: return; Did the `waitUntilDone` and `notifyDone` methods not work? I'm a little nervous that this may have timing issues.
https://codereview.chromium.org/2320303002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/scrolling/same-page-navigate.html (right): https://codereview.chromium.org/2320303002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/scrolling/same-page-navigate.html:217: return; On 2016/09/14 21:38:11, bokan wrote: > Did the `waitUntilDone` and `notifyDone` methods not work? I'm a little nervous > that this may have timing issues. Yes, async test will give "missing actual" but I can print it.
On 2016/09/15 00:15:22, chaopeng wrote: > https://codereview.chromium.org/2320303002/diff/120001/third_party/WebKit/Lay... > File third_party/WebKit/LayoutTests/fast/scrolling/same-page-navigate.html > (right): > > https://codereview.chromium.org/2320303002/diff/120001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/fast/scrolling/same-page-navigate.html:217: > return; > On 2016/09/14 21:38:11, bokan wrote: > > Did the `waitUntilDone` and `notifyDone` methods not work? I'm a little > nervous > > that this may have timing issues. > > Yes, async test will give "missing actual" but I can print it. change test to `waitUntilDone` and `notifyDone`, PTAL. Thank you.
https://codereview.chromium.org/2320303002/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/scrolling/same-page-navigate.html (right): https://codereview.chromium.org/2320303002/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/scrolling/same-page-navigate.html:5: height: 800px; No need for this style, these are the defaults. https://codereview.chromium.org/2320303002/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/scrolling/same-page-navigate.html:21: testRunner.waitUntilDone(); Wrap this and notifyDone in `if(testRunner) ...` https://codereview.chromium.org/2320303002/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/scrolling/same-page-navigate.html:24: if (innerWidth < 800) { Lets be more explicit, use internals.pageScaleFactor() to check that scale == 2 https://codereview.chromium.org/2320303002/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/scrolling/same-page-navigate.html:26: console.log("scrollY=" + scrollY); Remove console messages, use asserts instead. https://codereview.chromium.org/2320303002/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/scrolling/same-page-navigate.html:38: eventSender.mouseMoveTo(x, y); Add a comment here that this clicks on the link which causes a reload of this page. https://codereview.chromium.org/2320303002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/Page.cpp (right): https://codereview.chromium.org/2320303002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/Page.cpp:439: // Need reset visual viewport position, fix for crbug.com/642279 Please fix the comment. See my comment on this line in PatchSet #5.
On 2016/09/15 15:03:47, bokan wrote: > https://codereview.chromium.org/2320303002/diff/160001/third_party/WebKit/Lay... > File third_party/WebKit/LayoutTests/fast/scrolling/same-page-navigate.html > (right): > > https://codereview.chromium.org/2320303002/diff/160001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/fast/scrolling/same-page-navigate.html:5: height: > 800px; > No need for this style, these are the defaults. > > https://codereview.chromium.org/2320303002/diff/160001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/fast/scrolling/same-page-navigate.html:21: > testRunner.waitUntilDone(); > Wrap this and notifyDone in `if(testRunner) ...` > > https://codereview.chromium.org/2320303002/diff/160001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/fast/scrolling/same-page-navigate.html:24: if > (innerWidth < 800) { > Lets be more explicit, use internals.pageScaleFactor() to check that scale == 2 > > https://codereview.chromium.org/2320303002/diff/160001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/fast/scrolling/same-page-navigate.html:26: > console.log("scrollY=" + scrollY); > Remove console messages, use asserts instead. > > https://codereview.chromium.org/2320303002/diff/160001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/fast/scrolling/same-page-navigate.html:38: > eventSender.mouseMoveTo(x, y); > Add a comment here that this clicks on the link which causes a reload of this > page. > > https://codereview.chromium.org/2320303002/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/page/Page.cpp (right): > > https://codereview.chromium.org/2320303002/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/page/Page.cpp:439: // Need reset visual viewport > position, fix for crbug.com/642279 > Please fix the comment. See my comment on this line in PatchSet #5. PTAL, Thank you
https://codereview.chromium.org/2320303002/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/scrolling/same-page-navigate-expected.txt (right): https://codereview.chromium.org/2320303002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/scrolling/same-page-navigate-expected.txt:1: link to self You shouldn't need this file anymore. Please delete it. https://codereview.chromium.org/2320303002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/Page.cpp (right): https://codereview.chromium.org/2320303002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/Page.cpp:442: // fix for crbug.com/642279 This change still doesn't address what I wrote before: "Comments should answer "why?" not "what?". This comment just says what the line below does (which we can easily see by looking at the line below). Instead, I'd want to know *why* we reset the visual viewport here and not in a more logical place. i.e. we need to reset the visual viewport position here since the setting it earlier will clobber the current history item and didCommitLoad happens after a new history item is created in FrameLoader." It doesn't say *why* this reset is happening in didCommitLoad.
Also, please add some discussion/context to the issue description. See https://codereview.chromium.org/2262373003 for an example of a good description. It should mention what's causing the bug, what the change is, and anything else that someone looking at this in a year or two would be helped by knowing. e.g. after navigating to the same page, we create a new FrameView but reuse the VisualViewport, meaning it's scroll position remains unchanged even though it was relative to an old FrameView.
Description was changed from ========== Reset VisualViewport position after same page navigation BUG=642279 ========== to ========== Reset VisualViewport position after same page navigation This issue can reproduce using pinch zoom in Android, ChromeOS and Mac. After navigating to the **exact** same page, we create a new FrameView but reuse the VisualViewport, meaning its scroll position remains unchanged even though it was relative to an old FrameView. BUG=642279 ==========
Description was changed from ========== Reset VisualViewport position after same page navigation This issue can reproduce using pinch zoom in Android, ChromeOS and Mac. After navigating to the **exact** same page, we create a new FrameView but reuse the VisualViewport, meaning its scroll position remains unchanged even though it was relative to an old FrameView. BUG=642279 ========== to ========== Reset VisualViewport position after same page navigation This issue can reproduce using pinch zoom in Android, ChromeOS and Mac. After navigating to the **exact** same page, we create a new FrameView but reuse the VisualViewport, meaning its scroll position remains unchanged even though it was relative to an old FrameView. In this patch, we reset the VisualViewport position in Page::didCommitLoad since it is after the history current item created. BUG=642279 ==========
On 2016/09/15 16:07:48, bokan wrote: > Also, please add some discussion/context to the issue description. See > https://codereview.chromium.org/2262373003 for an example of a good description. > It should mention what's causing the bug, what the change is, and anything else > that someone looking at this in a year or two would be helped by knowing. > > e.g. after navigating to the same page, we create a new FrameView but reuse the > VisualViewport, meaning it's scroll position remains unchanged even though it > was relative to an old FrameView. PTAL, Thank you
Just some improvements in wording In the description: "This issue can reproduce" -> "This issue can be reproduced" "since it is after the history current item created." -> "since prior to here we would overwrite the old history item". And if you haven't, please confirm that the test you added fails if you remove your fix. Thanks. https://codereview.chromium.org/2320303002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/Page.cpp (right): https://codereview.chromium.org/2320303002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/Page.cpp:440: // Need reset visual viewport position here since before this will change the history current item, Nit: "Need reset visual" -> "Need to reset the visual" Nit: "before this will change the history current item" -> "before commit load we would update the previous history item" https://codereview.chromium.org/2320303002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/Page.cpp:441: // Page::didCommitLoad is after a new history item is created in FrameLoader. Nit: "didCommitLoad is after" -> "didCommitLoad is called after"
On 2016/09/15 18:27:32, bokan wrote: > Just some improvements in wording > > In the description: > "This issue can reproduce" -> "This issue can be reproduced" > > "since it is after the history current item created." -> "since prior to here we > would overwrite the old history item". > > And if you haven't, please confirm that the test you added fails if you remove > your fix. > > Thanks. > > https://codereview.chromium.org/2320303002/diff/200001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/page/Page.cpp (right): > > https://codereview.chromium.org/2320303002/diff/200001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/page/Page.cpp:440: // Need reset visual viewport > position here since before this will change the history current item, > Nit: "Need reset visual" -> "Need to reset the visual" > Nit: "before this will change the history current item" -> "before commit load > we would update the previous history item" > > https://codereview.chromium.org/2320303002/diff/200001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/page/Page.cpp:441: // Page::didCommitLoad is > after a new history item is created in FrameLoader. > Nit: "didCommitLoad is after" -> "didCommitLoad is called after" Oh, and please wrap the description at 80 columns. Sorry, your tools should really do this automatically but they don't :(
Description was changed from ========== Reset VisualViewport position after same page navigation This issue can reproduce using pinch zoom in Android, ChromeOS and Mac. After navigating to the **exact** same page, we create a new FrameView but reuse the VisualViewport, meaning its scroll position remains unchanged even though it was relative to an old FrameView. In this patch, we reset the VisualViewport position in Page::didCommitLoad since it is after the history current item created. BUG=642279 ========== to ========== Reset VisualViewport position after same page navigation This issue can be reproduced using pinch zoom in Android, ChromeOS and Mac. After navigating to the **exact** same page, we create a new FrameView but reuse the VisualViewport, meaning its scroll position remains unchanged even though it was relative to an old FrameView. In this patch, we reset the VisualViewport position in Page::didCommitLoad since prior to here we would overwrite the old history item. BUG=642279 ==========
Description was changed from ========== Reset VisualViewport position after same page navigation This issue can be reproduced using pinch zoom in Android, ChromeOS and Mac. After navigating to the **exact** same page, we create a new FrameView but reuse the VisualViewport, meaning its scroll position remains unchanged even though it was relative to an old FrameView. In this patch, we reset the VisualViewport position in Page::didCommitLoad since prior to here we would overwrite the old history item. BUG=642279 ========== to ========== Reset VisualViewport position after same page navigation This issue can be reproduced using pinch zoom in Android, ChromeOS and Mac. After navigating to the **exact** same page, we create a new FrameView but reuse the VisualViewport, meaning its scroll position remains unchanged even though it was relative to an old FrameView. In this patch, we reset the VisualViewport position in Page::didCommitLoad since prior to here we would overwrite the old history item. BUG=642279 ==========
On 2016/09/15 18:28:05, bokan wrote: > On 2016/09/15 18:27:32, bokan wrote: > > Just some improvements in wording > > > > In the description: > > "This issue can reproduce" -> "This issue can be reproduced" > > > > "since it is after the history current item created." -> "since prior to here > we > > would overwrite the old history item". > > > > And if you haven't, please confirm that the test you added fails if you remove > > your fix. > > > > Thanks. > > > > > https://codereview.chromium.org/2320303002/diff/200001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/page/Page.cpp (right): > > > > > https://codereview.chromium.org/2320303002/diff/200001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/page/Page.cpp:440: // Need reset visual > viewport > > position here since before this will change the history current item, > > Nit: "Need reset visual" -> "Need to reset the visual" > > Nit: "before this will change the history current item" -> "before commit load > > we would update the previous history item" > > > > > https://codereview.chromium.org/2320303002/diff/200001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/page/Page.cpp:441: // Page::didCommitLoad is > > after a new history item is created in FrameLoader. > > Nit: "didCommitLoad is after" -> "didCommitLoad is called after" > > Oh, and please wrap the description at 80 columns. Sorry, your tools should > really do this automatically but they don't :( Fixed. And the test case will fail if I remove my fix.
On 2016/09/15 18:38:15, chaopeng wrote: > On 2016/09/15 18:28:05, bokan wrote: > > On 2016/09/15 18:27:32, bokan wrote: > > > Just some improvements in wording > > > > > > In the description: > > > "This issue can reproduce" -> "This issue can be reproduced" > > > > > > "since it is after the history current item created." -> "since prior to > here > > we > > > would overwrite the old history item". > > > > > > And if you haven't, please confirm that the test you added fails if you > remove > > > your fix. > > > > > > Thanks. > > > > > > > > > https://codereview.chromium.org/2320303002/diff/200001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/core/page/Page.cpp (right): > > > > > > > > > https://codereview.chromium.org/2320303002/diff/200001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/core/page/Page.cpp:440: // Need reset visual > > viewport > > > position here since before this will change the history current item, > > > Nit: "Need reset visual" -> "Need to reset the visual" > > > Nit: "before this will change the history current item" -> "before commit > load > > > we would update the previous history item" > > > > > > > > > https://codereview.chromium.org/2320303002/diff/200001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/core/page/Page.cpp:441: // Page::didCommitLoad is > > > after a new history item is created in FrameLoader. > > > Nit: "didCommitLoad is after" -> "didCommitLoad is called after" > > > > Oh, and please wrap the description at 80 columns. Sorry, your tools should > > really do this automatically but they don't :( > > Fixed. And the test case will fail if I remove my fix. Please address my comments in Page.cpp
On 2016/09/15 18:42:48, bokan wrote: > On 2016/09/15 18:38:15, chaopeng wrote: > > On 2016/09/15 18:28:05, bokan wrote: > > > On 2016/09/15 18:27:32, bokan wrote: > > > > Just some improvements in wording > > > > > > > > In the description: > > > > "This issue can reproduce" -> "This issue can be reproduced" > > > > > > > > "since it is after the history current item created." -> "since prior to > > here > > > we > > > > would overwrite the old history item". > > > > > > > > And if you haven't, please confirm that the test you added fails if you > > remove > > > > your fix. > > > > > > > > Thanks. > > > > > > > > > > > > > > https://codereview.chromium.org/2320303002/diff/200001/third_party/WebKit/Sou... > > > > File third_party/WebKit/Source/core/page/Page.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2320303002/diff/200001/third_party/WebKit/Sou... > > > > third_party/WebKit/Source/core/page/Page.cpp:440: // Need reset visual > > > viewport > > > > position here since before this will change the history current item, > > > > Nit: "Need reset visual" -> "Need to reset the visual" > > > > Nit: "before this will change the history current item" -> "before commit > > load > > > > we would update the previous history item" > > > > > > > > > > > > > > https://codereview.chromium.org/2320303002/diff/200001/third_party/WebKit/Sou... > > > > third_party/WebKit/Source/core/page/Page.cpp:441: // Page::didCommitLoad > is > > > > after a new history item is created in FrameLoader. > > > > Nit: "didCommitLoad is after" -> "didCommitLoad is called after" > > > > > > Oh, and please wrap the description at 80 columns. Sorry, your tools should > > > really do this automatically but they don't :( > > > > Fixed. And the test case will fail if I remove my fix. > > Please address my comments in Page.cpp Comment updated
Thank you, lgtm :)
The CQ bit was checked by chaopeng@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Reset VisualViewport position after same page navigation This issue can be reproduced using pinch zoom in Android, ChromeOS and Mac. After navigating to the **exact** same page, we create a new FrameView but reuse the VisualViewport, meaning its scroll position remains unchanged even though it was relative to an old FrameView. In this patch, we reset the VisualViewport position in Page::didCommitLoad since prior to here we would overwrite the old history item. BUG=642279 ========== to ========== Reset VisualViewport position after same page navigation This issue can be reproduced using pinch zoom in Android, ChromeOS and Mac. After navigating to the **exact** same page, we create a new FrameView but reuse the VisualViewport, meaning its scroll position remains unchanged even though it was relative to an old FrameView. In this patch, we reset the VisualViewport position in Page::didCommitLoad since prior to here we would overwrite the old history item. BUG=642279 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Reset VisualViewport position after same page navigation This issue can be reproduced using pinch zoom in Android, ChromeOS and Mac. After navigating to the **exact** same page, we create a new FrameView but reuse the VisualViewport, meaning its scroll position remains unchanged even though it was relative to an old FrameView. In this patch, we reset the VisualViewport position in Page::didCommitLoad since prior to here we would overwrite the old history item. BUG=642279 ========== to ========== Reset VisualViewport position after same page navigation This issue can be reproduced using pinch zoom in Android, ChromeOS and Mac. After navigating to the **exact** same page, we create a new FrameView but reuse the VisualViewport, meaning its scroll position remains unchanged even though it was relative to an old FrameView. In this patch, we reset the VisualViewport position in Page::didCommitLoad since prior to here we would overwrite the old history item. BUG=642279 Committed: https://crrev.com/4521a9bb2402ebfffd709f02413ba3ec5a70de46 Cr-Commit-Position: refs/heads/master@{#419167} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/4521a9bb2402ebfffd709f02413ba3ec5a70de46 Cr-Commit-Position: refs/heads/master@{#419167} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
