Summary: | PDF views should keep a separate scaling factor from shared text scaling factor | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Trey Matteson <trey> | ||||||||||
Component: | New Bugs | Assignee: | Trey Matteson <trey> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | sullivan | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 420+ | ||||||||||||
Hardware: | Mac | ||||||||||||
OS: | OS X 10.4 | ||||||||||||
Attachments: |
|
Description
Trey Matteson
2005-07-20 22:06:33 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 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?
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.
Created attachment 3115 [details]
patch #2 - still also fixes 4015
Created attachment 3117 [details]
patch #3 - in response to comments just fixing typos, one method rename
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.
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. 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 |