Hook up Gecko full content zoom

RESOLVED FIXED in Camino2.0

Status

defect
RESOLVED FIXED
12 years ago
8 years ago

People

(Reporter: alqahira, Assigned: chris)

Tracking

(Depends on 1 bug)

Trunk
Camino2.0
PowerPC
macOS
Dependency tree / graph
Bug Flags:
camino2.0 +

Details

(Whiteboard: p-safari [needs nib generated with IB2.5.4 on 10.4])

Attachments

(3 attachments, 6 obsolete attachments)

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.
For reference, the 'fox ui bug is bug 389628.

Comment 2

12 years ago
p-safari?  Really?  I only see text zoom in Safari.
I believe Safari 3 has it. If not, it probably will given that MobileSafari has it.
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.
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?).
(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.
* 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
This is something we should consider for 2.0.
Flags: camino2.0?
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.
(Assignee)

Comment 10

11 years ago
Posted patch Source code changes (obsolete) — Splinter Review
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().
(Assignee)

Comment 11

11 years ago
Posted file Non-source modified files. (obsolete) —
The modified project file (added two toolbar images to project), MainMenu.nib (added alternate entries - see MainMenu.png), two placeholder images for the toolbar.
(Assignee)

Comment 12

11 years ago
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 on attachment 328058 [details] [diff] [review]
Source code changes

Sean, wanna take a first review pass at this?
Attachment #328058 - Flags: review?(murph)
Reassigning. Thanks a bunch for working on this!
Assignee: nobody → trendyhendy2000

Comment 15

11 years ago
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.
Attachment #328058 - Flags: review?(murph) → review+
(Assignee)

Comment 16

11 years ago
Posted patch Updated source code patch (obsolete) — Splinter Review
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.
Attachment #328058 - Attachment is obsolete: true
(Assignee)

Comment 17

11 years ago
Posted file Updated non-source modified files (obsolete) —
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.
Attachment #328059 - Attachment is obsolete: true

Updated

11 years ago
Attachment #329373 - Flags: superreview?(stuart.morgan)
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!
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

11 years ago
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.
Attachment #329373 - Flags: superreview?(stuart.morgan) → superreview-
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.
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.
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. 
(Assignee)

Comment 24

11 years ago
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.
(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.
(Assignee)

Comment 26

11 years ago
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.
Attachment #329373 - Attachment is obsolete: true
(Assignee)

Comment 27

11 years ago
New MainMenu.nib with the labels we decided upon. Placeholder graphics still there until we get some new ones.
Attachment #329374 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Attachment #334135 - Flags: superreview?(stuart.morgan)

Comment 28

11 years ago
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.
Attachment #334135 - Flags: superreview?(stuart.morgan+bugzilla) → superreview+

Comment 29

11 years ago
Posted patch key handling tweak (obsolete) — Splinter Review
This will need to be landed with the main patch.

Comment 30

11 years ago
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.
Flags: camino2.0? → camino2.0+
Target Milestone: --- → Camino2.0
(Assignee)

Comment 31

11 years ago
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.
Attachment #334135 - Attachment is obsolete: true
Attachment #334634 - Attachment is obsolete: true
Whiteboard: p-safari → p-safari [needs nib generated with IB2.5.4 on 10.4]

Comment 32

11 years ago
Posted file MainMenu.nib
This may not be sufficient, I opened MainMenu.nib and saved it on 10.4/IB2.5.6.
Checked in on cvs trunk.  Christopher, thanks for the patch!  Eiichi, thanks for the nib; hopefully it will keep the perf numbers in line.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Depends on: 452557
You need to log in before you can comment on or make changes to this bug.