Bug 52552 – Printing causes view to scroll

• Component: Printing
• Status: NEW

[Yuzo Fujishima]

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.

[Alexey Proskuryakov]

I cannot reproduce this in Safari on Mac OS X (with shipping or nightly WebKit).

[Yuzo Fujishima]

Created attachment 79243 [details]
Patch

[Yuzo Fujishima]

(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.

[Alexey Proskuryakov]

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.

[Yuzo Fujishima]

Created attachment 79249 [details]
Screen recording of how to reproduce this

[Yuzo Fujishima]

(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.

[Alexey Proskuryakov]

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.

[Yuzo Fujishima]

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.

[Yuzo Fujishima]

Created attachment 79366 [details]
More descriptive ChangeLog

[Alexey Proskuryakov]

The explanation sounds good, thanks!

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

[Yuzo Fujishima]

(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:] ()
...

[Alexey Proskuryakov]

As a wild guess - what if we skip calling platformSetContentsSize() in ScrollView::setContentsSize()?

[Yuzo Fujishima]

(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(); }

[Alexey Proskuryakov]

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.

[Yuzo Fujishima]

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.

[Alexey Proskuryakov]

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.

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

[Alexey Proskuryakov]

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?

[Yuzo Fujishima]

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?

[Alexey Proskuryakov]

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?

Dan, Simon - any thoughts on Alexey's question?

[Simon Fraser (smfr)]

Comment on attachment 79366 [details]
More descriptive ChangeLog

Why do you need to save and restore the scroll position both from WebHTMLView and Frame?

[Alexey Proskuryakov]

Note that there is a significant refactoring ongoing on ScrollView - it might be easier to make a proper fix once that's done.

[Yuzo Fujishima]

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.

[Yuzo Fujishima]

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?

[Alexey Proskuryakov]

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.

Any updates on whether this affects Webkit2 ?

[Alexey Proskuryakov]

<rdar://problem/8975325>

[Yuzo Fujishima]

Hi, any updates on the refactoring?

[Alexey Proskuryakov]

Can you still reproduce the problem with ToT? Hyatt recently landed some changes that might have fixed it.

[Yuzo Fujishima]

Hi, Alexey,

I can reproduce this bug with r78451.

[Yuzo Fujishima]

I confirmed that the patch https://bugs.webkit.org/attachment.cgi?id=79366 still works.

[Alexey Proskuryakov]

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?

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

[Alexey Proskuryakov]

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.

[Yuzo Fujishima]

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.

@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.

[Alexey Proskuryakov]

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.

[Yuzo Fujishima]

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.