Bug 4091 - PDF views should keep a separate scaling factor from shared text scaling factor
Summary: PDF views should keep a separate scaling factor from shared text scaling factor
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Trey Matteson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-07-20 22:06 PDT by Trey Matteson
Modified: 2006-09-14 15:47 PDT (History)
1 user (show)

See Also:


Attachments
proposed patch - also fixed 4015 (20.16 KB, patch)
2005-07-20 22:48 PDT, Trey Matteson
sullivan: review-
Details | Formatted Diff | Diff
This is a good testing file (453 bytes, text/html)
2005-07-26 14:01 PDT, Trey Matteson
no flags Details
patch #2 - still also fixes 4015 (40.19 KB, patch)
2005-07-27 12:13 PDT, Trey Matteson
no flags Details | Formatted Diff | Diff
patch #3 - in response to comments just fixing typos, one method rename (40.43 KB, patch)
2005-07-27 13:47 PDT, Trey Matteson
sullivan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Trey Matteson 2005-07-20 22:06:33 PDT
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.
Comment 1 Trey Matteson 2005-07-20 22:48:48 PDT
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.
Comment 2 John Sullivan 2005-07-21 09:43:28 PDT
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?
Comment 3 Trey Matteson 2005-07-26 14:01:10 PDT
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.
Comment 4 Trey Matteson 2005-07-27 12:13:32 PDT
Created attachment 3115 [details]
patch #2 - still also fixes 4015
Comment 5 Trey Matteson 2005-07-27 13:47:06 PDT
Created attachment 3117 [details]
patch #3 - in response to comments just fixing typos, one method rename
Comment 6 John Sullivan 2005-07-27 14:07:16 PDT
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.
Comment 7 John Sullivan 2005-07-27 14:55:49 PDT
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.
Comment 8 David Harrison 2006-09-14 15:47:23 PDT
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