Bug 77433

Created attachment 124722 [details] Patch

I don't yet have a layout test for this (will have one shortly) but I'm uploading this to share and get early feedback.

Attachment 124722 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource First, rewinding head to replay your work on top of it... Applying: Fix compilation errors on build-webkit --debug --no-workers on mac. Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging LayoutTests/platform/qt/Skipped CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped Auto-merging Source/WebCore/ChangeLog CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Failed to merge in the changes. Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac. When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 135102 [details] Sample HTML file to reproduce the bug

Steps to reproduce the bug in chromium: 1. Run chromium with the --enable-viewport flag. 2. Open the attached HTML file. 3. Vertically resize the window to be larger, which increases the fixed layout size. The corner box incorrectly repositions itself by moving up instead of down. (Note: If there are visible scrollbars, you may have to repeat this step to see the effect.) This does not appear to reproduce on a horizontal resize. Steps to reproduce a (very likely) related bug in chromium: 1. Run chromium with the flags --window-size=1000,800 and --enable-fixed-layout=800,600. 2. Open the attached HTML file. 3. Vertically/horizontally resize the window. The corner box incorrectly repositions itself by moving in the direction opposite to the window resize direction (it should not move at all when the window is resized, since the layout size is fixed).

Created attachment 136779 [details] Sample HTML file to reproduce the bug

Created attachment 136786 [details] Patch

(In reply to comment #7) > Created an attachment (id=136786) [details] > Patch This patch is for feedback and discussion only. It fixes the bug for the attached HTML file, but will not work in general since setFixedLayoutSize is called in FrameView::setFixedVisibleContentRect. I am unclear about the original intended purpose of the member variable m_fixedVisibleContentRect in ScrollView and how this patch would impact its use elsewhere. It would be greatly appreciated if the developer who added this member variable could offer some feedback.

(In reply to comment #8) > (In reply to comment #7) > > Created an attachment (id=136786) [details] [details] > > Patch > > This patch is for feedback and discussion only. It fixes the bug for the attached HTML file, but will not work in general since setFixedLayoutSize is called in FrameView::setFixedVisibleContentRect. > > I am unclear about the original intended purpose of the member variable m_fixedVisibleContentRect in ScrollView and how this patch would impact its use elsewhere. It would be greatly appreciated if the developer who added this member variable could offer some feedback. FixedVisibleContentRect is the visibleContentRect when scrolling and clipping is performed outside of the ScrollView. This is why scrollbar width and height should not be subtracted from it. Why did you change this in ScrollView?

Comment on attachment 136786 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136786&action=review > Source/WebCore/page/FrameView.cpp:1705 > + ScrollView::setFixedLayoutSize(IntSize(visibleContentRect.width(), visibleContentRect.height())); This is wrong. The layout size and visible-content rect are not related. The visible content rect is what is visible when page zooming for instance. The fixed layout should stay the same when the visible rect changes. > Source/WebCore/platform/ScrollView.cpp:-237 > - if (!m_fixedVisibleContentRect.isEmpty()) > - return m_fixedVisibleContentRect; > - This should not be changed. The fixed visible content rect overrides the calculated content rect. > Source/WebKit/chromium/src/WebViewImpl.cpp:1291 > + if (mainFrameImpl() && mainFrameImpl()->frameView()) > + mainFrameImpl()->frameView()->setFixedVisibleContentRect(IntRect(0, 0, newSize.width, newSize.height)); > + I think you might be setting the wrong variable here. Perhaps you really mean to set fixed layout size here?

Comment on attachment 124722 [details] Patch Needs test.

It appears that the original bug has been fixed, or is otherwise not reproducible. The --enable-fixed-layout flag no longer works (on chromium) and appears to have no effect. However, I found a similar bug that still exists when the --enable-viewport flag is present. Steps to reproduce: 1. Run chromium with the --enable-viewport flag. 2. Open the sample HTML file uploaded by Terry (the first one). 3. Resize the window horizontally or vertically to make it smaller. - Note that the Corner box shifts away from the corner by about the width of a scrollbar 4. Resize the window from the corner to decrease both width and height. - Scroll bars now appear. 5. Repeat 4. - The Corner box has now moved partially, or fully, off of the screen with the scroll bars still present. Thoughts on why this happens: When resizing the window the ScrollView::updateScrollbars(const IntSize& desiredOffset) method is called multiple times, the first time the method is called contentsSize(), and visibleContentRect() sizes are equal to the previous window size and match, so nothing much happens. The second time updateScrollbars is called visibleContentRect() has been updated to the new size, while contentsSize() has not been updated, so the method believes scroll bars are required, and adds them. After adding scrollbars, the updateScrollbars recursively calls itself since the visible size has changed, but this time through the contentsSize() has been updated (since a Layout has been performed in the interim), and it is determined that scrollbars are no longer needed, so it removes them. However, the page no longer lays out correctly and you see the effects described above.

Created attachment 182586 [details] Patch

This bug (both versions as described in comment #5) is relevant for Chrome on Android. Related: https://bugs.webkit.org/show_bug.cgi?id=105486

Comment on attachment 182586 [details] Patch It's been a while since I've looked at this code: What does FixedVisibleContentRect do?

(In reply to comment #15) > (From update of attachment 182586 [details]) > It's been a while since I've looked at this code: What does FixedVisibleContentRect do? http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/page/FrameView.cpp&exact_package=chromium&q=FrameView::setFixedVisibleContentRect&l=1782 It starts by calling FrameView::setViewportConstrainedObjectsNeedLayout() which sets NeedsLayout to true for all "constrained objects" (I'm not exactly sure what constrained objects are). It then checks if the scroll offset has changed, but this shouldn't be the case because the method is called with the old scroll offset values. updateScrollbars() is then called with the new window size (which triggers a layout, which is what we need to update the scrollview contentsSize()).

--enable-viewport is not yet supported on desktop and there are several missing pieces. Chrome for Android also intentionally doesn't use fixedVisibleContentRect since its semantics aren't useful given that our compositor manages scrolling and clipping at a higher level. I'm not sure what you're trying to achieve here, are you trying to support viewport tag on desktop? If so we should take a step back and discuss the design.

Comment on attachment 182586 [details] Patch Attachment 182586 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15868617 New failing tests: fast/dom/vertical-scrollbar-in-rtl.html fast/dynamic/window-resize-scrollbars-test.html fast/dom/rtl-scroll-to-leftmost-and-resize.html

I have not looked at this in a very long time. Marking as available for someone else to take.

Comment on attachment 182586 [details] Patch Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.