Bug 4091 – PDF views should keep a separate scaling factor from shared text scaling factor

• Component: New Bugs
• Status: RESOLVED FIXED

[Trey Matteson]

As of the to 3766909 on 2005-06-14, WebPDFView implements the _web_WebDocumentTextSizing 
protocol, which brought the benefit that cmd--and cmd-+ would work on PDF docs.  In addition, the 
current zoom factor for the window became shared by PDF views.  I think this latter aspect turns out to 
be a lose for a few reasons:

1) PDF files tend to have smaller text than most web pages.  I believe this is an artifact of them being 
pre-laid out in pages for printing on paper.  Examples include the typical TEX academic research paper 
format (http://www.ctan.org/tex-archive/info/lshort/english/lshort.pdf).  The practical result is that as 
the user moves between HTML and PDF content, no one zoom factor works for both, and either the 
HTML looks too big or the PDF looks too small.

2) The PDF autoscale feature does not fit well with the Safari model.  Example:  Open a pdf doc.  Resize 
the window so that one page fits at a time.  Zoom in three times.  Choose autoscale.  The zoom factor 
should be similar to what you started with.  Zoom out.  The user expects to now be looking at a 
relatively "more distant" image, but instead the image is further magnified.  The reason is that the 
change to autosize does not update the zoom factor stored for the whole window.

2') If we fixed this by updating the zoom factor in the window when you switched to autosize mode, we 
have a new problem.  We've decided that it would be nice for the various PDF view settings to be sticky.  
Let's say the user prefers autoscaling to be on.  Now everytime a new PDF doc with a different size is 
opened, the window's zoom factor changes, affecting HTML pages in erratic ways.  Even just resizing an 
autosize window affects the scale of subsequent HTML docs.

My proposal is that the scale factor for PDFs be kept separate from the window's scale factor.  I would 
also have the WebPDFView implement the action messages makeTextSmaller: and makeTextLarger: to 
be the same as ZoomIn and ZoomOut in the context menu.  It is a bit of a fudge to have these slightly 
bent semantics of these commands, but I think few users would think of it enough to realize the 
inconsistency of it, compared to the faults raised above.

[Trey Matteson]

Created attachment 3040 [details]
proposed patch - also fixed 4015

This worked out so well I am attaching a patch.  I like these semantics better,
but I am also relieved that all the weird PDFKit crashing and midbehavior I was
getting when the textScale mechanism was poking the PDFKit scale factors has
gone away.

[John Sullivan]

Comment on attachment 3040 [details]
proposed patch - also fixed 4015

You convinced me that trying to use zoom factor and text scaling
interchangeably has UI problems that are worse than the UI problems of reusing
the Make Text Smaller/Larger menu items in a related but non-interdependent
way. So I think this approach is great, and the code looks really good too. I
just have a few comments and questions, so I'm marking it review- for this
round.


Typo in ChangeLog: "Sace scrollState"


+		 // Parent is the clipview of the DynamicScrollView the
WebFrame installs
+		 point = [parent bounds].origin;

I'd add an ASSERT here to match the comment. I realize that this code didn't
change, but as the surrounding code gets more complex I think the ASSERT is a
useful safety measure.

It looks like the WebHistoryItem private data structure now has a scrollPoint
item and a viewState item that contains a scrollPoint item. Can the standalone
scrollPoint item be eliminated, and the viewState always used?

What's the story with removing these lines:

-
-@interface WebHTMLView (TextSizing) <_web_WebDocumentTextSizing>
-@end
-

We're not very consistent with our naming of internal-to-webkit protocols, but
I think that _WebDocumentViewState should be _web_WebDocumentViewState to match
_web_WebDocumentTextSizing.


With this patch, when are "Make Text Larger/Smaller" dimmed in the menu when
viewing a PDF flie?

[Trey Matteson]

Created attachment 3092 [details]
This is a good testing file

Here's a good testing file that has 4 frames with HTML, PDF, plain text and an
image.

[Trey Matteson]

Created attachment 3115 [details]
patch #2 - still also fixes 4015

[Trey Matteson]

Created attachment 3117 [details]
patch #3 - in response to comments just fixing typos, one method rename

[John Sullivan]

Comment on attachment 3117 [details]
patch #3 - in response to comments just fixing typos, one method rename

Thanks for the persistence on this. I'll commit this soon.

[John Sullivan]

Checked this in. Trey and I found a couple of loose ends that he will write up separately, but neither of 
them is a regression from the old behavior.

[David Harrison]

This caused <rdar://problem/4494340> REGRESSION: Making the font size bigger/smaller in an HTML message doesn't affect the body until you reopen it