389795 - Hook up Gecko full content zoom

Status: RESOLVED FIXED

(Camino Graveyard :: Page Layout, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Bug 4821 adds the backend support for making Gecko zoom all page contents (text, images, whatever).  We should hook up some UI for it.

Sam asked earlier today if we had a bug filed on this.  We did not; now we do.

Comment 1

[philippe (part-time)]

For reference, the 'fox ui bug is bug 389628.

Comment 2

[Jesse Ruderman]

p-safari?  Really?  I only see text zoom in Safari.

Comment 3

[Samuel Sidler (old account; do not CC)]

I believe Safari 3 has it. If not, it probably will given that MobileSafari has it.

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

As of hot-off-the-processor builds, our mouse-zoom (ctrl-whatever) does full content zoom.  See bug 389628 comment 131; it might be changing, since that change was foisted on all Gecko apps without discussion.

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Stuart and I chatted tonight about "we also have to decide if we want it to replace text zoom" and I think we don't.  Bug 389628 and friends contain a litany of complaints about some part of the feature, and Stuart mentioned there are also many pages out there just using a very small font size for no good reason, where all you need to change is the text size, not the images.

Perhaps the easiest way to do this is to make one of them use our existing Cmd-+/Cmd-= Cmd-- and Cmd-0 keyboard shortcuts and Ctrl-Mouse-whatever, and then make the other use Opt-Cmd-keys and Opt-Ctrl-Mouse (and add matching toolbar buttons?).

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #4)
> As of hot-off-the-processor builds, our mouse-zoom (ctrl-whatever) does full
> content zoom.  See bug 389628 comment 131; it might be changing, since that
> change was foisted on all Gecko apps without discussion.

Bug 401214.

Comment 7

[philippe (part-time)]

* I'll vote to keep text zoom and add full page zoom as an option, as suggested in comment 5.
* Many calls to bring back the text zoom in the 'Fox UI in bug 401322.
* SeaMonkey's equivalent bug: bug 405133.

BTW - Safari 3.0 does not have full page zoom. The WebKit team is working on some sort of implementation:
http://bugs.webkit.org/show_bug.cgi?id=14998

Comment 8

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

This is something we should consider for 2.0.

Comment 9

[philippe (part-time)]

I tis now enabled in WebKit nightly builds, and judging by the comments:
http://webkit.org/blog/165/full-page-zoom/
having both 'Page zoom' and 'text zoom' is a much demanded feature (as finally implemented in Minefield). It is not clear what Apple will do, UI wise.

Comment 10

[Christopher Henderson]

Attachment: Source code changes

Changes made to MainController, BrowserWindowController, and CHBrowserView. Not much more than copying the text zoom methods and modifying them to use fullZoom() instead of textZoom().

Comment 11

[Christopher Henderson]

Attachment: Non-source modified files.

The modified project file (added two toolbar images to project), MainMenu.nib (added alternate entries - see MainMenu.png), two placeholder images for the toolbar.

Comment 12

[Christopher Henderson]

I have gotten full page zoom working by calling Gecko's page zoom functions. Works well for me, including menu validation. The zoom increments in 0.25 steps, like text zoom, between 0.1 and 10. I considered an exponential change so it doesn't take 40 clicks to get to 1000%, thought that without a popup menu showing the steps it would be confusing for the user. If someone wants to file a bug for a page zoom menu, I'd be happy to work on that as well.

The zoom doesn't work like for a PDF, due to CSS positioning rules; but that's an issue for Gecko, not Camino.

This is my first patch, so I hope I haven't made too many mistakes.

Comment 13

[Samuel Sidler (old account; do not CC)]

Comment on attachment 328058 [details] [diff] [review]
Source code changes

Sean, wanna take a first review pass at this?

Comment 14

[Samuel Sidler (old account; do not CC)]

Reassigning. Thanks a bunch for working on this!

Comment 15

[Sean Murphy]

Comment on attachment 328058 [details] [diff] [review]
Source code changes

For the most part, the code changes here look great.  Just a few minor points to address (or consider addressing):

>+- (void)makePageBigger
>+{
>+	[self incrementPageZoom:0.25 min:MIN_PAGE_ZOOM max:MAX_PAGE_ZOOM];
>+}
>+
>+- (void)makePageSmaller
>+{
>+	[self incrementPageZoom:-0.25 min:MIN_PAGE_ZOOM max:MAX_PAGE_ZOOM];
>+}

Maybe we should #define a PAGE_ZOOM_INCREMENT constant to use instead of the 0.25 magic number.

>+#define MIN_PAGE_ZOOM 0.1f
>+#define MAX_PAGE_ZOOM 10.0f

I think we might want to reduce the maximum page zoom allowed.  By the time 10.0 is reached, the page seems extremely zoomed.

>+- (BOOL)canMakePageBigger
>+{
>+  BrowserWrapper* wrapper = [self browserWrapper];
>+  return (![wrapper isEmpty] &&
>+          ![self bookmarkManagerIsVisible] &&
>+          [[wrapper browserView] isTextBasedContent] &&
>+          [[wrapper browserView] canMakePageBigger]);
>+}

In the canMakePage... methods, ensuring the browser contains text based content will exclude "image only" documents from being zoomed, which is otherwise possible.  If we want to allow this, what we'll really need to check for is a document that is entirely a plugin, such as an SWF or PDF, because that's when zooming is unavailable.

Also, just a few style nits: 

There are a few lines in your patch containing tab characters.  Please replace these with the Camino style of soft tabs (spaces) of size two (viewing invisibles in your editor can help you find them.)

Also, we like to ensure that blank lines do not have any characters on them, so just take a look in the patch and remove any lines with nothing other than an indent on them.

--

As for the nib, the menu items do not appear in the MainMenu.  This is due to the fact that the "treat as alternative to previous item" checkbox is enabled on each within Interface Builder.

Also, concerning the layout of the Page Zoom menu items, we might want to debate whether we want to intermingle them with the Text Zoom items.  To me, the different items are hard to discern all mixed among each other.  A screenshot of the new view menu is attached.

--

One last thing, if you could include your modifications to the Xcode project in the original diffed patch, that'd be great, as that's usually the way we handle such changes.

r=murph, with the minor source code changes listed above, the project patch included in the diff, and the nib menu item visibility problem addressed.

Comment 16

[Christopher Henderson]

Attachment: Updated source code patch

Incorporated Sean's recommendations. I added a function that determines whether the page's content type is a image, so we can zoom in a web page and an image, but not a plugin.

Comment 17

[Christopher Henderson]

Attachment: Updated non-source modified files

Removed the project file, since it is included in the new patch. Bumped the icons down a pixel to better align with the other toolbar icons. Modified MainMenu.nib to always show the page zoom items.

Comment 18

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 329373 [details] [diff] [review]
Updated source code patch

>Index: Camino.xcodeproj/project.pbxproj
>===================================================================
>-		035A4976079DC83E0084B05F /* Info-HistoryPrefPane.plist */ = {isa = PBXFileReference; lastKnownFileType = text.xml; path = "Info-HistoryPrefPane.plist"; sourceTree = SOURCE_ROOT; };
>+		035A4976079DC83E0084B05F /* Info-HistoryPrefPane.plist */ = {isa = PBXFileReference; lastKnownFileType = text.plist.xml; path = "Info-HistoryPrefPane.plist"; sourceTree = SOURCE_ROOT; };

Why are these .plist lines changing (it's happening all over the project file)?

>Index: resources/localized/English.lproj/Localizable.strings.in
>===================================================================
> "BigText" = "Bigger Text";
> "SmallText" = "Smaller Text";
>+"BigPage" = "Zoom In";
>+"SmallPage" = "Zoom Out";

> "BigTextToolTip" = "Enlarge the text on this page";
> "SmallTextToolTip" = "Shrink the text on this page";
>+"BigPageToolTip" = "Make this page bigger";
>+"SmallPageToolTip" = "Make this page smaller";

Conceptually I have an issue with "Make this page foo", since to me it sounds very much like "Make this window foo"--expanding the physical size of the window/page--whereas we're really making the page content appear larger/smaller.  If we want to avoid reusing "zoom" in the tooltip (especially since "zoom" in relation to windows has an entirely different meaning on Ma OS X), perhaps something like "Enlarge the content on this page" or "Make the page content larger" or something.  (I'm not really happy with any of those...I'll keep thinking.)

Ditto on the menu items, though there we also have to keep an eye on length of the item name.

(In reply to comment #15)
> As for the nib, the menu items do not appear in the MainMenu.  This is due to
> the fact that the "treat as alternative to previous item" checkbox is enabled
> on each within Interface Builder.
> 
> Also, concerning the layout of the Page Zoom menu items, we might want to
> debate whether we want to intermingle them with the Text Zoom items.  To me,
> the different items are hard to discern all mixed among each other.

I think that the menu still looks really crowded with the two sets; I think we should use alternates and make content zoom the default (moving the old text zoom over to the alternate cmd-opt-foo).  Doing that makes the feature visible without crowding the menu, and if the general theory (and the default in other browsers) is that content zoom is more commonly needed, it's the right one to expose and to take over the existing shortcuts.

Thanks for working on this, Christopher!

Comment 19

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

We discussed this at today's meeting, and we agreed that we should switch back to using alternates for the menu items, with the new zoom items taking over the cmd-foo shortcuts and the text shortcuts moving to the new cmd-opt-foo alternates.

No better ideas on the tooltip or menu item names yet, though.

Comment 20

[Stuart Morgan]

Comment on attachment 329373 [details] [diff] [review]
Updated source code patch

>@@ -1605,26 +1620,24 @@
>   [[browserWindow windowController] manageBookmarks:aSender];
> }
> 
> - (IBAction)openMenuBookmark:(id)aSender
> {
>   BookmarkItem*  item = [aSender representedObject];
>   EBookmarkOpenBehavior openBehavior = eBookmarkOpenBehavior_Preferred;
>   BOOL reverseBackgroundPref = NO;
>-
>   if ([aSender isAlternate]) {
>     reverseBackgroundPref = ([aSender keyEquivalentModifierMask] & NSShiftKeyMask) != 0;
>     if ([aSender keyEquivalentModifierMask] & NSCommandKeyMask)
>       openBehavior = eBookmarkOpenBehavior_NewPreferred;
>   }
>   // safeguard for bookmark menus that don't have alternates yet
>   else if ([[NSApp currentEvent] modifierFlags] & NSCommandKeyMask)
>     openBehavior = eBookmarkOpenBehavior_NewPreferred;
>-
>   [self loadBookmark:item withBWC:[self mainWindowBrowserController] openBehavior:openBehavior reverseBgToggle:reverseBackgroundPref];
> }
[...]
>@@ -2092,9 +2108,10 @@
> - (IBAction)setFileExtension:(id)aSender
> {
>   if ([[aSender title] isEqualToString:@"HTML"])
>     [[aSender representedObject] setRequiredFileType:@"html"];
>   else
>     [[aSender representedObject] setRequiredFileType:@"plist"];
> }
> 
>+
> @end

Please don't change whitespace in unrelated methods.

>+  if ([mimeType hasPrefix:@"image/"])
>+    return YES;
>+
>+  return NO;
>+}

Just |return [mimeType hasPrefix:@"image/"];|

>+- (BOOL)canMakePageBigger
>+{
>+  float zoom = [self pageZoom];
>+  return zoom < MAX_PAGE_ZOOM;
>+}
>+
>+- (BOOL)canMakePageSmaller
>+{
>+  float zoom = [self pageZoom];
>+  return zoom > MIN_PAGE_ZOOM;
>+}

No need for the temporary variable, just |return [self pageZoom [...]| (I know this is based on the text zoom code, but it doesn't have to be identical where we can improve.)

The code looks great! sr-'ing only because there will need to be changes for comments 18/19.

Comment 21

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I've collected some suggestions for the labels on http://wiki.caminobrowser.org/User:Sardisson/Content_Zoom_Scratchpad

All of the #1 items are the existing labels from Christopher's patch/nib; the others are things that have come to mind.  Comments, additional suggestions, etc., on that page; hopefully we can come to a decision at this week's meeting.

Comment 22

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Per the meeting, we'll go with "Zoom In"/"Zoom Out" for the toolbar button names, "Enlarge the content on this page"/"Shrink the content on this page" for the toolbar button tooltips, and "Zoom In"/"Actual Size"/"Zoom Out" for the menu items.

Comment 23

[philippe (part-time)]

Regarding the text-zoom issue. Has it been considered to have something like what Fx3+ does: adding 1 (one) menu item: 'zoom text only' ?

Zoom In
Actual Size
Zoom Out
--
Zoom Text Only

When checked, zoom only the text, using the same keyboard shortcuts as "Zoom In"/"Zoom Out".

I'm pretty sure I'm not the only one who vastly prefers 'text-zoom'. The keyboard shortcut for 'Zoom In' already requires the Shift key on my keyboard (apple-jis kbd). Adding the Option key makes text-zoom less conformable. 

Comment 24

[Christopher Henderson]

If the PTB want me to, I could add that feature, but I don't have any indication that's what they want.

You can always use the Keyboard & Mouse Preference Pane to simplify the shortcuts, or add the text zoom buttons to the toolbar.

Comment 25

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #23)
> Regarding the text-zoom issue. Has it been considered to have something like
> what Fx3+ does: adding 1 (one) menu item: 'zoom text only' ?

Stuart, Sam, and I discussed that (or something close to that) the other day, and the decision, after some back and forth, was to land things as described in comment 18/19 and evaluate feedback from nightlies and alphas.

Comment 26

[Christopher Henderson]

Attachment: Patch incorporating all fixes in the comments

Everything should be in here. Includes patch for the project file; I know there's a redundant line in there, but it doesn't change anything and it was the only way I could get it to work.

Comment 27

[Christopher Henderson]

Attachment: Non-source modified files

New MainMenu.nib with the labels we decided upon. Placeholder graphics still there until we get some new ones.

Comment 28

[Stuart Morgan]

Comment on attachment 334135 [details] [diff] [review]
Patch incorporating all fixes in the comments

Looks great, sr=smorgan. Thanks again for making this happen!

I realized I'd forgotten about the special key handling we do to treat + like = for this; rather than run through another round of reviews, I'll just post that minor fix as an additional patch.

One other thing we need to take care of before this lands: did you use 10.4 or 10.5 to make the nib? We have bizarre startup time problems with MainMenu.nib when edited from 10.5, so if that's what you used we'll need to recreate the nib changes on a 10.4 machine (one of us can take care of that if you don't have convenient access to one). Sorry we forgot to mention that in advance.

Comment 29

[Stuart Morgan]

Attachment: key handling tweak

This will need to be landed with the main patch.

Comment 30

[Stuart Morgan]

Oops, I forgot to test the toolbar buttons. You need to add the buttons to the copied resources of both Camino and CaminoStatic in the project so the are actually built into the application.

When you respin the patch for that, please fold in my change so there's just the one patch.

Comment 31

[Christopher Henderson]

Attachment: Patch with key handling and resource copying

Updated patch to address comments 29 and 30.

I edited MainMenu.nib in 10.5, and don't have access to a 10.4 dev environment, so someone will have to recreate it per comment 28.

Comment 32

[Eiichi]

Attachment: MainMenu.nib

This may not be sufficient, I opened MainMenu.nib and saved it on 10.4/IB2.5.6.

Comment 33

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Checked in on cvs trunk.  Christopher, thanks for the patch!  Eiichi, thanks for the nib; hopefully it will keep the perf numbers in line.