Bug 52552 - Printing causes view to scroll
Summary: Printing causes view to scroll
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Printing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-01-16 23:05 PST by Yuzo Fujishima
Modified: 2011-02-17 03:48 PST (History)
7 users (show)

See Also:


Attachments
Test case (4.77 KB, text/html)
2011-01-16 23:05 PST, Yuzo Fujishima
no flags Details
Patch (10.63 KB, patch)
2011-01-17 23:11 PST, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Screen recording of how to reproduce this (2.73 MB, video/quicktime)
2011-01-18 00:51 PST, Yuzo Fujishima
no flags Details
More descriptive ChangeLog (11.08 KB, patch)
2011-01-18 17:33 PST, Yuzo Fujishima
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yuzo Fujishima 2011-01-16 23:05:24 PST
Created attachment 79128 [details]
Test case

Printing a page causes the view to scroll upward.

How to reproduce:
1. Open the attached test case file.
2. Make the scroll bar appear by resizing the window.
3. Scroll to the bottom.
4. Print the page.
5. Observe the page has been scrolled upward.
Comment 1 Alexey Proskuryakov 2011-01-17 10:58:21 PST
I cannot reproduce this in Safari on Mac OS X (with shipping or nightly WebKit).
Comment 2 Yuzo Fujishima 2011-01-17 23:11:19 PST
Created attachment 79243 [details]
Patch
Comment 3 Yuzo Fujishima 2011-01-17 23:17:30 PST
(In reply to comment #1)
> I cannot reproduce this in Safari on Mac OS X (with shipping or nightly WebKit).

Hmm, I can reproduce this
- with r75931 on Snow Leopard and Leopard (run-safari).
- with Safari 5.0.2 (7533.18.5) on Windows XP
(Based on my examination with Chrome, I believe this issue has been there for more than a year.)

Also, the test included in the patch https://bugs.webkit.org/attachment.cgi?id=79243 fails
on my Snow Leopard if the printing code is not changed.
Comment 4 Alexey Proskuryakov 2011-01-18 00:06:59 PST
I would certainly like to be able to reproduce this. Can you make a screencast showing how this happens (e.g. with QuickTime Player on Snow Leopard)?

Looking at the patch, I'm wondering why you had to make changes to both WebCore and in WebKit/mac.
Comment 5 Yuzo Fujishima 2011-01-18 00:51:42 PST
Created attachment 79249 [details]
Screen recording of how to reproduce this
Comment 6 Yuzo Fujishima 2011-01-18 01:47:08 PST
(In reply to comment #4)
> I would certainly like to be able to reproduce this. Can you make a screencast showing how this happens (e.g. with QuickTime Player on Snow Leopard)?

I've uploaded a screen recording.

> 
> Looking at the patch, I'm wondering why you had to make changes to both WebCore and in WebKit/mac.

My understanding is that WebHTMLView.mm is used by Safari while Frame.cpp is used by LayoutTestController, Chromium, etc.
Comment 7 Alexey Proskuryakov 2011-01-18 09:15:28 PST
I can reproduce when the window is really small, like on the screencast. The view is scrolled immediately when the Print sheet appears, not when printing. Scroll thumb sometimes appears inactive after dismissing the Print sheet.

Did you find out what causes scrolling? In most cases, scroll position is preserved just fine, so what goes wrong with tiny windows?

We should try to identify the root cause if possible.

> My understanding is that WebHTMLView.mm is used by Safari while Frame.cpp is used
> by LayoutTestController, Chromium, etc.

You're right, we should probably change Mac WebKit1 to use PrintContext::begin(), too.
Comment 8 Yuzo Fujishima 2011-01-18 17:17:47 PST
Thank you for the review.

(In reply to comment #7)
> I can reproduce when the window is really small, like on the screencast. The view is scrolled immediately when the Print sheet appears, not when printing. Scroll thumb sometimes appears inactive after dismissing the Print sheet.
> 
> Did you find out what causes scrolling? In most cases, scroll position is preserved just fine, so what goes wrong with tiny windows?

When the paper is larger than the window, maximum scroll position on paper becomes
smaller than that of on window. If the scroll position on window is greater than the
maximum on paper, the position is trimmed down to the latter in changing media type
from screen to print and then back to screen again. 

(I'll add the above description to the ChangeLog.)

> 
> We should try to identify the root cause if possible.
> 
> > My understanding is that WebHTMLView.mm is used by Safari while Frame.cpp is used
> > by LayoutTestController, Chromium, etc.
> 
> You're right, we should probably change Mac WebKit1 to use PrintContext::begin(), too.
Comment 9 Yuzo Fujishima 2011-01-18 17:33:26 PST
Created attachment 79366 [details]
More descriptive ChangeLog
Comment 10 Alexey Proskuryakov 2011-01-18 17:35:50 PST
The explanation sounds good, thanks!

Can we just skip the scroll position check that causes this when in print mode?
Comment 11 Yuzo Fujishima 2011-01-19 00:45:52 PST
(In reply to comment #10)
> The explanation sounds good, thanks!

You are welcome. :)

> 
> Can we just skip the scroll position check that causes this when in print mode?

It seems that the scroll position is changed outside WebKit as a result of setting contents size of a platform specific view (please see the stack trace below).

I tweaked the code a bit but there seems to be no easy way to suppress that or an easy way not to set content size in printing.

#0  WebCore::FrameView::scrollPositionChangedViaPlatformWidget (this=0x111c3e800) at <src>/WebKit/Source/WebCore/page/FrameView.cpp:1306
#1  0x0000000100fb6817 in -[WebHTMLView(WebPrivate) _frameOrBoundsChanged] (self=0x111c3d940, _cmd=0x7fff883415fb) at <src>/WebKit/Source/WebKit/mac/WebView/WebHTMLView.mm:1258
#2  0x00007fff81af284e in _nsnote_callback ()
#3  0x00007fff83944a90 in __CFXNotificationPost ()
#4  0x00007fff83931008 in _CFXNotificationPostNotification ()
#5  0x00007fff81ae97b8 in -[NSNotificationCenter postNotificationName:object:userInfo:] ()
#6  0x00007fff80f2f479 in -[NSView _postBoundsChangeNotification] ()
#7  0x00007fff81044073 in -[NSView translateOriginToPoint:] ()
#8  0x00007fff8100707a in -[NSClipView _immediateScrollToPoint:] ()
#9  0x0000000100f55cff in -[WebClipView _immediateScrollToPoint:] (self=0x111c21c70, _cmd=0x7fff81606bce, newOrigin={x = 0, y = 799}) at <src>/WebKit/Source/WebKit/mac/WebView/WebClipView.mm:99
#10 0x00007fff81006c05 in -[NSClipView scrollToPoint:] ()
#11 0x00007fff81043bc6 in -[NSScrollView scrollClipView:toPoint:] ()
#12 0x00007fff80f81bbc in -[NSClipView _scrollTo:animate:] ()
#13 0x00007fff80f815d7 in -[NSClipView _reflectDocumentViewFrameChange] ()
#14 0x00007fff80eed8b7 in -[NSView _postFrameChangeNotification] ()
#15 0x00007fff80ee78f6 in -[NSView setFrameSize:] ()
#16 0x00007fff80f0f566 in -[NSControl setFrameSize:] ()
#17 0x0000000102038c31 in WebCore::ScrollView::platformSetContentsSize (this=0x111c3e800) at <src>/WebKit/Source/WebCore/platform/mac/ScrollViewMac.mm:135
#18 0x0000000102035f4a in WebCore::ScrollView::setContentsSize (this=0x111c3e800, newSize=@0x7fff5fbfdc00) at <src>/WebKit/Source/WebCore/platform/ScrollView.cpp:287
#19 0x00000001018edb3b in WebCore::FrameView::setContentsSize (this=0x111c3e800, size=@0x7fff5fbfdc00) at <src>/WebKit/Source/WebCore/page/FrameView.cpp:422
#20 0x00000001018ec0f0 in WebCore::FrameView::adjustViewSize (this=0x111c3e800) at <src>/WebKit/Source/WebCore/page/FrameView.cpp:447
#21 0x00000001018ed18b in WebCore::FrameView::forceLayoutForPagination (this=0x111c3e800, pageSize=@0x7fff5fbfdce0, maximumShrinkFactor=1.60000002, shouldAdjustViewSize=WebCore::Frame::AdjustViewSize) at <src>/WebKit/Source/WebCore/page/FrameView.cpp:2299
#22 0x0000000100fb339c in -[WebHTMLView layoutToMinimumPageWidth:height:maximumPageWidth:adjustingViewSize:] (self=0x111c3d940, _cmd=0x10109dcab, minPageWidth=698.75, minPageHeight=918.75, maxPageWidth=1118, adjustViewSize=1 '\001') at <src>/WebKit/Source/WebKit/mac/WebView/WebHTMLView.mm:3148
#23 0x0000000100fb02a7 in -[WebHTMLView _setPrinting:minimumPageWidth:height:maximumPageWidth:adjustViewSize:paginateScreenContent:] (self=0x111c3d940, _cmd=0x10109d9c6, printing=1 '\001', minPageWidth=698.75, minPageHeight=918.75, maxPageWidth=1118, adjustViewSize=1 '\001', paginateScreenContent=0 '\0') at <src>/WebKit/Source/WebKit/mac/WebView/WebHTMLView.mm:3927
#24 0x0000000100fa962e in -[WebHTMLView(WebPrivate) _beginPrintModeWithPageWidth:height:shrinkToFit:] (self=0x111c3d940, _cmd=0x10109e7cb, pageWidth=559, pageHeight=735, shrinkToFit=1 '\001') at <src>/WebKit/Source/WebKit/mac/WebView/WebHTMLView.mm:2236
#25 0x0000000100fbadb9 in -[WebHTMLView knowsPageRange:] (self=0x111c3d940, _cmd=0x7fff8162d9a8, range=0x7fff5fbfdf70) at <src>/WebKit/Source/WebKit/mac/WebView/WebHTMLView.mm:4034
#26 0x00007fff81516661 in -[NSView(NSPrinting2) _knowsPagesFirst:last:] ()
...
Comment 12 Alexey Proskuryakov 2011-01-19 01:00:20 PST
As a wild guess - what if we skip calling platformSetContentsSize() in ScrollView::setContentsSize()?
Comment 13 Yuzo Fujishima 2011-01-19 17:25:28 PST
(In reply to comment #12)
> As a wild guess - what if we skip calling platformSetContentsSize() in ScrollView::setContentsSize()?

That was the tweak I tried. Then the printing result looked odd -- too large characters were used to print.

To be more concrete, I tried the following:

diff --git a/Source/WebCore/page/FrameView.cpp b/Source/WebCore/page/FrameView.cpp
index 1187318..0f298a8 100644
--- a/Source/WebCore/page/FrameView.cpp
+++ b/Source/WebCore/page/FrameView.cpp
@@ -420,6 +420,8 @@ void FrameView::setContentsSize(const IntSize& size)
     m_deferSetNeedsLayouts++;
 
     ScrollView::setContentsSize(size);
+    Document* doc = m_frame->document();
+    ScrollView::updateScrollbarsToNewSize(doc ? doc->printing() : false);
 
     Page* page = frame() ? frame()->page() : 0;
     if (!page)
diff --git a/Source/WebCore/platform/ScrollView.cpp b/Source/WebCore/platform/ScrollView.cpp
index 6cf13a4..361cb6f 100644
--- a/Source/WebCore/platform/ScrollView.cpp
+++ b/Source/WebCore/platform/ScrollView.cpp
@@ -283,8 +283,13 @@ void ScrollView::setContentsSize(const IntSize& newSize)
     if (contentsSize() == newSize)
         return;
     m_contentsSize = newSize;
+}
+
+void ScrollView::updateScrollbarsToNewSize(bool isPrinting)
+{
     if (platformWidget())
-        platformSetContentsSize();
+        if (!isPrinting)
+            platformSetContentsSize();
     else
         updateScrollbars(scrollOffset());
 }
diff --git a/Source/WebCore/platform/ScrollView.h b/Source/WebCore/platform/ScrollView.h
index 2477f49..6c44dee 100644
--- a/Source/WebCore/platform/ScrollView.h
+++ b/Source/WebCore/platform/ScrollView.h
@@ -160,6 +160,7 @@ public:
     int contentsWidth() const { return contentsSize().width(); }
     int contentsHeight() const { return contentsSize().height(); }
     virtual void setContentsSize(const IntSize&);
+    void updateScrollbarsToNewSize(bool isPrinting);
    
     // Functions for querying the current scrolled position (both as a point, a size, or as individual X and Y values).
     IntPoint scrollPosition() const { return visibleContentRect().location(); }
Comment 14 Alexey Proskuryakov 2011-01-19 17:53:58 PST
OK. On one hand, it seems generally wrong to fix a problem by hiding it post factum, rather than preventing it from occurring. On the other hand, I don't have a better fix to suggest quickly.
Comment 15 Yuzo Fujishima 2011-01-23 21:49:50 PST
I agree with you about the general approach.
Do you think it is OK to fix this issue with the current approach, though?

(In reply to comment #14)
> OK. On one hand, it seems generally wrong to fix a problem by hiding it post factum, rather than preventing it from occurring. On the other hand, I don't have a better fix to suggest quickly.
Comment 16 Alexey Proskuryakov 2011-01-23 21:57:34 PST
Personally, I don't see this as a practical problem that needs to be fixed no matter what. Perhaps there are use cases that I'm missing.
Comment 17 jeffharris 2011-01-24 03:29:54 PST
Just FYI - this bug is blocking native printing from being launched in Google Docs. Specifically, with native printing implemented you lose your location in the document whenever you print on Webkit.

Hopefully Yuzo's change (or some variant) can be approved to fix the behavior in Webkit.

Jeff Harris
Docs PM
Comment 18 Alexey Proskuryakov 2011-01-24 08:41:51 PST
Per comments in this bug, this only happens when the document window is unusually small, meaning that it's almost impossible to see the bug in practice. Is the description incomplete?
Comment 19 Yuzo Fujishima 2011-01-24 17:32:08 PST
This happens pretty often to me.

I observe the scrolling when I open and print http://www.apple.com with Safari 5.0.1 (5533.17.8)
with window height greater than 800px.

This is more conspicuous for the Chromium port -- it scrolls to the top.

Hence I think this issue is well worth resolving.


(In reply to comment #18)
> Per comments in this bug, this only happens when the document window is unusually small, meaning that it's almost impossible to see the bug in practice. Is the description incomplete?
Comment 20 Alexey Proskuryakov 2011-01-24 17:42:37 PST
Yes, I can reproduce it:
- open www.apple.com;
- javascript:window.resizeTo(1024,800);
- scroll to bottom;
- print.
- observe that scroll position changes.

Note that scrolling is not completely undone here, because it probably still generates DOM events.

Dan, Simon, do you think that we should try to avoid scrolling here, or does restoring scroll position post factum make sense?
Comment 21 jeffharris 2011-01-26 20:14:58 PST
Dan, Simon - any thoughts on Alexey's question?
Comment 22 Simon Fraser (smfr) 2011-01-26 20:36:59 PST
Comment on attachment 79366 [details]
More descriptive ChangeLog

Why do you need to save and restore the scroll position both from WebHTMLView and Frame?
Comment 23 Alexey Proskuryakov 2011-01-27 14:18:47 PST
Note that there is a significant refactoring ongoing on ScrollView - it might be easier to make a proper fix once that's done.
Comment 24 Yuzo Fujishima 2011-02-02 17:59:41 PST
Thank you for the review.

The ScrollView refactoring may clean things up, but it may take some time. Do you have ETA?

I agree that the patch is not ideal, but it is simple and its effect is local.
Also considering that there are some real needs to fix it rather soon,
I think landing the patch can be justified. What do you think?


(In reply to comment #23)
> Note that there is a significant refactoring ongoing on ScrollView - it might be easier to make a proper fix once that's done.
Comment 25 Yuzo Fujishima 2011-02-02 18:02:52 PST
Thank you for the review and sorry for the late response.

It was because Mac uses WebHTMLView and other platforms use Frame.


(In reply to comment #22)
> (From update of attachment 79366 [details])
> Why do you need to save and restore the scroll position both from WebHTMLView and Frame?
Comment 26 Alexey Proskuryakov 2011-02-02 19:18:31 PST
I think that the refactoring I've been thinking about is mostly done.

I need to check what effect this has on WebKit2. Right now, its behavior is somewhat different, so I'm worried that this patch may not do good.
Comment 27 jeffharris 2011-02-08 07:26:10 PST
Any updates on whether this affects Webkit2 ?
Comment 28 Alexey Proskuryakov 2011-02-08 17:30:54 PST
<rdar://problem/8975325>
Comment 29 Yuzo Fujishima 2011-02-13 18:06:29 PST
Hi, any updates on the refactoring?
Comment 30 Alexey Proskuryakov 2011-02-13 19:49:29 PST
Can you still reproduce the problem with ToT? Hyatt recently landed some changes that might have fixed it.
Comment 31 Yuzo Fujishima 2011-02-13 20:55:49 PST
Hi, Alexey,

I can reproduce this bug with r78451.
Comment 32 Yuzo Fujishima 2011-02-15 02:30:37 PST
I confirmed that the patch https://bugs.webkit.org/attachment.cgi?id=79366 still works.
Comment 33 Alexey Proskuryakov 2011-02-15 16:20:28 PST
My testing shows that window.onscroll is not fired below a print sheet despite scrolling with WebKit1, but it is fired with WebKit2. So, this fix isn't really sufficient.

I took a stab at making it so that scrolling didn't occur, and it proved difficult for me, too.

I'm still very much unsure if the user observable issue is big enough to warrant such attention to this bug. Could you please explain what makes it so important?
Comment 34 jeffharris 2011-02-15 18:35:46 PST
Hey Alexey - The issue I'm seeing is that we're starting to switch to native printing in Google Docs on Webkit (instead of needing to download and open a PDF from the server). Whenever anyone prints, they lose their position in the document and are taken to the top of the doc. We've had quite a number of people report it internally, and I'm concerned that when we launch native printing publicly, that we'll get a flood of complaints from Webkit users.

It might just be that people print documents much more often than they print regular pages, but lots of people have noticed. And this bug does affect regular sites too. I'm worried that we all agree that there's a bug, but that no one seems to be suggesting an alternate way of fixing it.

If you don't think this change is right, could you help us figure out a more suitable fix? Or could we check this in now, and commit to implementing a better fix when you guys decide what that should be?

Thanks for your help on the review: I know you're just looking out for code quality.
Jeff
Comment 35 Alexey Proskuryakov 2011-02-16 00:18:32 PST
Well, we have thousands of open bugs, but it doesn't mean that every one of them deserves a stopgap fix.

> are taken to the top of the doc

This sounds like it could be a different issue then. Here, the document is scrolled slightly up, and only if you're close to the very bottom of it when initiating printing.
Comment 36 Yuzo Fujishima 2011-02-16 02:05:53 PST
Hi, Alexey,

For Chromium port, it scrolls up to the top.

Applying patch 79366 reduces the amount of scroll, although scrolling is not completely suppressed.
(Perhaps more proper fix may completely suppress scrolling.)

I'd appreciate it if anyone more familiar with this part of code than I can look into this.

(In reply to comment #35)
> Well, we have thousands of open bugs, but it doesn't mean that every one of them deserves a stopgap fix.
> 
> > are taken to the top of the doc
> 
> This sounds like it could be a different issue then. Here, the document is scrolled slightly up, and only if you're close to the very bottom of it when initiating printing.
Comment 37 jeffharris 2011-02-16 06:13:58 PST
@Alexey - I know. I'm not saying every bug needs a stopgap. I am saying that pretty soon a large chunk of Webkit users of Docs (probably hundreds of thousands)  will start noticing and being bothered by this. I'm hoping that we'll at least have some idea of how we can fix the problem and a timeline for when we can tell them it'll be fixed.

In any case, it sounds like Yuzo's fix isn't sufficient. If you've got suggestions on what we should do to fix this properly then that would be awesome. And if we've got to wait until we're sure it bugs lots of people, then I guess we can do that.
Comment 38 Alexey Proskuryakov 2011-02-16 08:51:14 PST
It sounds like there are several separate things to look into:
1) Why does Chromium scroll to top? That sounds like a medium priority issue, and likely to be in Chromium specific WebKit code.
2) Why does Mac Safari sometimes scroll a little when printing? It seems really unclear - there is no particular reason for it to change scroll position. This is likely to be fixed in Mac WebKit in WebDynamicScrollBarsView code. The issue seems pretty minor.
3) What do other ports do?
4) Can printing stop fooling with platform views? Maybe it can at least create separate temporary ones? That's mostly WebCore code.

Chromium and Safari issues in particular should be tracked in separate bugs.
Comment 39 Yuzo Fujishima 2011-02-17 03:14:12 PST
Hi, Alexey,

I've forked https://bugs.webkit.org/show_bug.cgi?id=54632 from this bug to track the Chromium-specific part of the bug.

I'd propose tracking the item (2) of your list by this bug, 52552.

(In reply to comment #38)
> It sounds like there are several separate things to look into:
> 1) Why does Chromium scroll to top? That sounds like a medium priority issue, and likely to be in Chromium specific WebKit code.
> 2) Why does Mac Safari sometimes scroll a little when printing? It seems really unclear - there is no particular reason for it to change scroll position. This is likely to be fixed in Mac WebKit in WebDynamicScrollBarsView code. The issue seems pretty minor.
> 3) What do other ports do?
> 4) Can printing stop fooling with platform views? Maybe it can at least create separate temporary ones? That's mostly WebCore code.
> 
> Chromium and Safari issues in particular should be tracked in separate bugs.