565718 - Show zoom indicator in UI if not at default zoom level

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Alex Limi (:limi) — Firefox UX Team]

Attachment: Zoom indicator in the URL bar

(Note: this is filed as part of the “Paper Cut” bugs — we assume that there may be multiple existing bugs on this. Please make them block this bug, and we will de-dupe if they are indeed exactly the same. Thanks!)

Currently, we don't have any UI in the main window to indicate or adjust the zoom level. I think going all the way (similar to IE and Opera) and showing the zoom control always is a bit unnecessary and causes clutter, but on the other hand we need to have a way for people to adjust this more easily — and more importantly: be able to restore the zoom level to the default when they accidentally changed it.

This is magnified by it being very easy to accidentally do a pinch gesture on Mac trackpads, which zooms in/out, and a lot of people don't realize how it happened. 

When we on top of that actually persist the zoom setting based on the domain, it's very frustrating when people don't know how to get back to the 100% zoom level.

Recommendation:
Introduce a main UI element that shows up *only when your zoom level is different from the default setting*. In this zoom menu, you can also opt to always keep the zoom widget visible.

The zoom control is page-specific, so in the new Firefox 4 UI should be in the visual hierarchy in a way that indicates this. Current sketch suggestion is attached.

Comment 1

[Kevin Brosnan]

Bug 401219 is for a slider for zoom levels. If either get implemented the other is a likely wontfix.

Comment 2

[Willy_ Foo_Foo]

Or we can show the indicator in status bar like opera does in the status bar

Comment 3

[Philip Chee]

> Or we can show the indicator in status bar like opera does in the status bar
Firefox doesn't have a status bar any more.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Stephen, are you on board with doing a simplified implementation of https://bugzilla.mozilla.org/attachment.cgi?id=445156 ? We could show the zoom number in a box like the Australis zoom-controls, where clicking on it would revert back to 100% (but would also make the button disappear?). I'm thinking this would only appear if the zoom-controls are not in the navbar.

Comment 5

[Justin Dolske [:Dolske]]

Taking this out of the cluster bug - we now have a zoom widget in the menu panel (which can be moved to the toolbar) that shows this info if you want it. Could still add a further widget to indicate this by default, as Chrome does, but not a high enough priority to track right now.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

The zoom widget that is placed in the menu panel by default provides an easy way for users to adjust their zoom level, but it is not a good tool to indicate when the zoom level is non-default for a site. 

We already know a couple ways that are easy to accidentally zoom a page such as using a scroll wheel then tapping Ctrl while the wheel is still moving or accidentally hitting Ctrl+ or Ctrl- when intending to use Ctrl+Backspace.

This feature exists primarily for users who don't move the zoom controls out of the menu panel, and we could disable it for those that do move it. But we should have a way in the primary UI to show users when a site is non-default and allow an easy way to fix it. The implementation in Chrome is done very nicely.

Comment 7

[Stephen Horlander [:shorlander]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> Stephen, are you on board with doing a simplified implementation of
> https://bugzilla.mozilla.org/attachment.cgi?id=445156 ? We could show the
> zoom number in a box like the Australis zoom-controls, where clicking on it
> would revert back to 100% (but would also make the button disappear?). I'm
> thinking this would only appear if the zoom-controls are not in the navbar.

Yeah, all this sounds good. The primary goal here is to notify people that they are in a non-default state and offer a quick way to revert it.

I mocked up how this could work (wait 5 seconds):

http://people.mozilla.org/~shorlander/mockups-interactive/firefox-ui/zoom-indicator-osx-yosemite.html

Notes:
- Show the zoom level if not 100%
- Subtle animation to hopefully catch your attention if you do this inadvertently
- Match tooltip from zoom widget that says "Reset zoom level"
- Pressing the button would reset to default and hide the indicator
- I agree we shouldn't show this if you have manually customized the zoom widget to the toolbar

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Yeah, that looks great. Katie, this will be a good bug for you to work on next.

Comment 9

[Katie Broida [:ktbee]]

Sounds good!

Comment 10

[Katie Broida [:ktbee]]

Attachment: Bug 565718 - Adds module for a zoom indicator in the browser's URL bar.

Review commit: https://reviewboard.mozilla.org/r/58300/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58300/

Comment 11

[Katie Broida [:ktbee]]

I've committed a version of the UI indicator, and when it looks okay I can add a test for it as well if you think that would be helpful. Also I've just recently realized that the 5 second delay for the indicator's appearance might have just been part of the mock up. Let me know if the user shouldn't actually have to wait 5 seconds to see their zoom change indicated in the URL bar.

Comment 12

[Stephen Horlander [:shorlander]]

(In reply to Katie Broida from comment #11)
> I've committed a version of the UI indicator, and when it looks okay I can
> add a test for it as well if you think that would be helpful. Also I've just
> recently realized that the 5 second delay for the indicator's appearance
> might have just been part of the mock up. Let me know if the user shouldn't
> actually have to wait 5 seconds to see their zoom change indicated in the
> URL bar.

Correct, the 5 second delay was just to let the page load so you could see what was happening. Zoom indicator should show up as soon as you zoom. Thanks!

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8760899 [details]
Bug 565718 - Adds module for a zoom indicator in the browser's URL bar.

https://reviewboard.mozilla.org/r/58300/#review55206

Everything looks pretty good, but I hope that we can reduce some of the style rules because I'm not sure all of them are necessary.

After you update this patch and add tests, can you please request review from Dao?

::: browser/base/content/browser.css:573
(Diff revision 1)
> +/* ::::: URL Bar Zoom Reset Button ::::: */
> +
> +@keyframes fade-in {
> +  0% {
> +    opacity: 0;
> +    display: none;

display is not an animatable property, so the transition from display:none to display:flex will just be an instant change and there won't be any noticeable opacity fade.

::: browser/base/content/browser.css:627
(Diff revision 1)
> +
> +  -moz-user-select:none;
> +  cursor: default;
> +
> +  animation-name: background-color;
> +  animation-delay: 5s;

As Stephen said, this line can be removed.

::: browser/base/content/browser.css:663
(Diff revision 1)
> +  -moz-margin-end: 4px;
> +  opacity: 1;
> +}
> +
> +#urlbar-zoom-button > .toolbarbutton-multiline-text  {
> +  display: inline !important;

Please stay away from using !important, unless it is absolutely necessary. Is there another rule that is using important? If so, please add a comment here explaining why important is necessary.

::: browser/base/content/browser.css:681
(Diff revision 1)
> +  position: absolute;
> +  top: 0;
> +  left: 0;
> +  height: 100%;
> +  width: 100%;

What are these rules supposed to do?

::: toolkit/themes/shared/icons/url-zoom-reset.svg:19
(Diff revision 1)
> +  <path id="cancel" d="M890.017,182.683l-3.322,3.323,3.3,3.3-1.67,1.67-3.3-3.3-3.274,3.273-1.67-1.67,3.274-3.273-3.372-3.372,1.67-1.67,3.372,3.372,3.322-3.323Z" transform="translate(-877 -178)"/>
> +  <path id="cancel-inverted" d="M890.017,182.683l-3.322,3.323,3.3,3.3-1.67,1.67-3.3-3.3-3.274,3.273-1.67-1.67,3.274-3.273-3.372-3.372,1.67-1.67,3.372,3.372,3.322-3.323Z" transform="translate(-877 -178)"/>
> +  <path id="cancel-system" d="M890.017,182.683l-3.322,3.323,3.3,3.3-1.67,1.67-3.3-3.3-3.274,3.273-1.67-1.67,3.274-3.273-3.372-3.372,1.67-1.67,3.372,3.372,3.322-3.323Z" transform="translate(-877 -178)"/>
> +  <path id="cancel-system-graytext" d="M890.017,182.683l-3.322,3.323,3.3,3.3-1.67,1.67-3.3-3.3-3.274,3.273-1.67-1.67,3.274-3.273-3.372-3.372,1.67-1.67,3.372,3.372,3.322-3.323Z" transform="translate(-877 -178)"/>

These path ids are not referenced anywhere. They're all the same, and all of the references to url-zoom-reset.svg lack the id hash at the end. Do we need all of these or can we just have one of them?

Comment 14

[Katie Broida [:ktbee]]

https://reviewboard.mozilla.org/r/58300/#review55206

Good point, I went back through the styles and found that I was able to remove a lot of them. I had stuck too closely to the mockup styling, and didn't realize that they weren't all needed when not a mockup.

Comment 15

[Katie Broida [:ktbee]]

Attachment: V2 for patch to add zoom indicator and tests

I've been having some trouble with pushing my latest patch to MozReview, so I'm uploading it as a patch for the time being. In this version I've reduced the unnecessary style declarations and added some tests for the zoom indicator. I'll respond to Jared's other comments on MozReview as well.

Comment 16

[Katie Broida [:ktbee]]

https://reviewboard.mozilla.org/r/58300/#review55206

> Please stay away from using !important, unless it is absolutely necessary. Is there another rule that is using important? If so, please add a comment here explaining why important is necessary.

Usually I try to stay away from !important tags, but there was some styling in xul.css that was preventing the button label text from showing. I tried to make the style selector as specific as possible, but the style from xul.css still overrode it and prevented it from displaying. I noticed that there is a comment in xul.css saying the styling in that file is locked down and that specific app styling should go in other files. I figured adding this !important tag would have less impact than editing the xul.css style declaration for toolbarbuttons. Let me know if I should go about it differently. I've added a comment with an explanation as well.

Comment 17

[Katie Broida [:ktbee]]

https://reviewboard.mozilla.org/r/58300/#review55206

> What are these rules supposed to do?

These rules give the background hbox it's size and placement. I found that if they're not there, the hbox has a height and width of 0 and/or is shifted off center.

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 8764019 [details] [diff] [review]
V2 for patch to add zoom indicator and tests

>--- a/browser/base/content/browser.css
>+++ b/browser/base/content/browser.css

Most if not all of these changes really belong in browser/themes/*/browser.css.

>+/* ::::: URL Bar Zoom Reset Button ::::: */
>+@keyframes fade-in {

This can be be simplified to a CSS transition rather than an animation, no?

>+@keyframes attention-pulse {

Please give animations more specific names such as urlbar-zoom-reset-...

>+  -moz-margin-end: 6px;

Please use margin-inline-end

>+  padding: 2px 8px;
>+  border-radius: 1em;
>+  color: hsl(0,0%,20%);

This looks like it's not going to work well with dark themes. Can you use graytext instead? Or just stick with the default font color.

>+  font-size: 12px;

Please translate this to a relative font-size (em).

>+#urlbar-zoom-button > .toolbarbutton-multiline-text  {
>+  /* The!important tag for display:inline overrides the display:none declaration for "toolbar[mode="icons"] .toolbarbutton-multiline-text" in xul.css */
>+  display: inline !important;

Uh, this isn't a multi-line button, so this element shouldn't even be relevant...? .toolbarbutton-text should be used here.

>+                <hbox id="urlbar-zoom-wrapper"
>+                       onclick="FullZoom.reset();">
>+                  <image id="zoom-reset-icon"/>
>+                  <toolbarbutton id="urlbar-zoom-button"
>+                       tooltiptext="&urlbarZoomReset.tooltip;"/>
>+                  <hbox id="zoomIndicator-backdrop"></hbox>
>+                </hbox>

The animated backdrop doesn't seem to work as intended, the layout is quite busted (screenshot coming).

The icon should just be added using list-style-image, no extra element needed.

So I think we should simplify this to something like this:

  <toolbarbutton id="urlbar-zoom-reset-button"
                 onclick="FullZoom.reset();"
                 tooltiptext="&urlbarZoomReset.tooltip;"/>

If shorlander feels strongly about the animated backdrop, we can think harder about how to properly implement that in a followup bug, but as a first step I think we can do without this.

>+<!ENTITY urlbarZoomReset.tooltip        "Reset Zoom Level">

Tooltips shouldn't use title capitalization.

>+  init: function () {
>+    // Register ourselves with the service so we know when the zoom prefs change.
>+    Services.obs.addObserver(this.updateZoomButton, "browser-fullZoom:zoomChange", false);
>+    Services.obs.addObserver(this.updateZoomButton, "browser-fullZoom:zoomReset", false);
>+    Services.obs.addObserver(this.updateZoomButton, "browser-fullZoom:location-change", false);

Replace these with Services.obs.addObserver(this, ...); and rename "updateZoomButton" to "observe".

>+  updateZoomButton: function() {
>+    let win = RecentWindow.getMostRecentBrowserWindow();

This isn't correct since the window you get the notification from isn't necessarily going to be the foreground window.

Please drop the unrelated whitespace changes from this patch. Feel free to file a separate bug on those and I'll review them there.

Comment 19

[Dão Gottwald [:dao]]

Attachment: patch v2 screenshot

Comment 20

[Katie Broida [:ktbee]]

(In reply to Dão Gottwald [:dao] from comment #18)

Thanks for taking a look at this Dão. I have a couple of follow up questions for you. 

> >--- a/browser/base/content/browser.css
> >+++ b/browser/base/content/browser.css
> 
> Most if not all of these changes really belong in
> browser/themes/*/browser.css.

Should I make these changes in each operating systems's browser.css folder (ie /windows/browser.css, /osx/browser.css, /linux/browser.css)? Since this zoom indicator should exist in all browsers versions, I would have thought it should go in /shared, but I don't see that the shared folder has a browser.css file. 

> >+/* ::::: URL Bar Zoom Reset Button ::::: */
> >+@keyframes fade-in {
> 
> This can be be simplified to a CSS transition rather than an animation, no?

I had wondered the same thing, but I'm not sure it will be much simpler as a transition because we need it to fade in as the button appears, not when the user takes an action like hovering over the button. I found I could trigger the transition using JavaScript to add and remove a class, but there needed to be a small delay in adding the class to allow the browser to process the request as a transition (This Stack Overflow post explains what I think is going on http://stackoverflow.com/questions/8210560/css-transitions-do-not-work-when-assigned-trough-javascript). Because of using JavaScript to trigger the animation, I'm not sure that that is simpler than just using an animation. However, if I'm not thinking about this the right way, let me know. 

> The animated backdrop doesn't seem to work as intended, the layout is quite
> busted (screenshot coming).

Thank you for the screenshot, I wasn't able to see it as broken on my Mac or Windows machines. What sort of set up are you using?

> The icon should just be added using list-style-image, no extra element
> needed.

If we add this icon as a list-style-image, can we still do a slide out CSS transition? I guess we could fade it in and out with the rest of the text.

> >+  updateZoomButton: function() {
> >+    let win = RecentWindow.getMostRecentBrowserWindow();
> 
> This isn't correct since the window you get the notification from isn't
> necessarily going to be the foreground window.

Would it make more sense to use the Focus Manager here? So add something like the code below to make sure we're getting the window that's in the foreground?

`XPCOMUtils.defineLazyServiceGetter(this, "_focusManager",
                                   "@mozilla.org/focus-manager;1",
                                   "nsIFocusManager");`
and

`let win = _focusManager.focusedWindow;`

Thanks again for your insight on this.

Comment 21

[Katie Broida [:ktbee]]

After chatting with Jared, I've had some of my questions answered. I understand now what needs to happen with the icon image for its sliding transition, and Jared said we can drop the text fade-in animation all together (it's not very visible in the mockup). Likewise with the missing "browser.css" file, he explained that that CSS is broken up into smaller files rather than create a large browser.css file. 

Dão, let me know if I should create a new CSS file in /shared for the zoom indicator or if I should put it in an existing one. Also your suggestions on the remaining questions are much appreciated.

Comment 22

[Dão Gottwald [:dao]]

If there's no fitting file in shared/ where this can be added to, you can just put it in the themes' individual browser.css files for now. I don't think we want to end up having hundreds of piecemeal CSS files for relatively tiny UI piece like this one.

(In reply to Katie Broida [:ktbee] from comment #20)
> > >+  updateZoomButton: function() {
> > >+    let win = RecentWindow.getMostRecentBrowserWindow();
> > 
> > This isn't correct since the window you get the notification from isn't
> > necessarily going to be the foreground window.
> 
> Would it make more sense to use the Focus Manager here?

No, using the focus manager is going to be just as wrong. Zoom changes can be triggered in background windows. You'll need to figure out which window the zoom change notification actually came from.

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8764019 [details] [diff] [review]
V2 for patch to add zoom indicator and tests

Review of attachment 8764019 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/modules/URLBarZoom.jsm
@@ +23,5 @@
> +    Services.obs.addObserver(this.updateZoomButton, "browser-fullZoom:location-change", false);
> +  },
> +
> +  updateZoomButton: function() {
> +    let win = RecentWindow.getMostRecentBrowserWindow();

CustomizableWidgets.jsm deals with this by:
1) onBuild is called with an `aDocument` argument.
2) Inside of onBuild, the updateZoomResetButton function is defined, which references `aDocument`.
3) Because `aDocument` is referenced in the updateZoomResetButton, its value is closed over causing it to get cached by the function object.
4) Each time the function is called in the future, the specific `aDocument` that was referenced when updateZoomResetButton was created is referenced.

This allows updateZoomResetButton to refer to `aDocument` as the current document and not have to look around for the most recent window or the currently focused window.

@@ +31,5 @@
> +    let zoomResetBackdrop = win.document.getElementById("zoomIndicator-backdrop");
> +    let zoomFactor = 100;
> +
> +    // Ensure that zoom controls haven't already been added to browser in Customize Mode
> +    if(customizableZoomControls === null || customizableZoomControls.getAttribute("cui-areatype") !== "toolbar") {

nit, please rewrite this as:

if (customizableZoomControls &&
    customizableZoomControls.getAttribute("cui-areatype") == "toolbar") {
  zoomResetWrapper.hidden = true;
  return;
}

and then run the rest of your code, which you can un-indent due to the early return. You shouldn't need to hide zoomResetBackdrop as it is a descendent of zoomResetWrapper, and by using the .hidden property (setting it to true or false as necessary) you won't need to specify the specific display values (flex/inline/none).

@@ +36,5 @@
> +      try {
> +        zoomFactor = Math.round(win.ZoomManager.zoom * 100);
> +      } catch (e) {}
> +      if (zoomFactor !== 100) {
> +        zoomResetButton.setAttribute("label", zoomFactor + "%");

This needs to be localizable. See CustomizableWidgets.jsm in the updateZoomResetButton function there, how we use a localizable string to create this.
> zoomResetButton.setAttribute("label", CustomizableUI.getLocalizedProperty(
>   buttons[1], "label", [updateDisplay ? zoomFactor : 100]
> ));

Not all locales may place the number in front of a % sign.

Comment 24

[Katie Broida [:ktbee]]

Comment on attachment 8760899 [details]
Bug 565718 - Adds module for a zoom indicator in the browser's URL bar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58300/diff/1-2/

Comment 25

[Katie Broida [:ktbee]]

(In reply to (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) from comment #23)

Thank you for the feedback! In this most recent patch, I've put the zoom indicator's CSS into each operating system's browser.css file. 

In terms of the other issues:

> CustomizableWidgets.jsm deals with this by:
> 1) onBuild is called with an `aDocument` argument.
> 2) Inside of onBuild, the updateZoomResetButton function is defined, which
> references `aDocument`.
> 3) Because `aDocument` is referenced in the updateZoomResetButton, its value
> is closed over causing it to get cached by the function object.
> 4) Each time the function is called in the future, the specific `aDocument`
> that was referenced when updateZoomResetButton was created is referenced.
> 
> This allows updateZoomResetButton to refer to `aDocument` as the current
> document and not have to look around for the most recent window or the
> currently focused window.

Part of the challenge with getting the window is that the zoom indicator isn't a customizable widget, so it doesn't have the onBuild event. However, I did find that I can pass a reference to the browser through two of the notification observers that I'm already using ("zoomChange" and "zoomReset"), which made it possible for me to grab the correct browser if some zoom event is happening in the non-focused tab. For the "location-change" observer, I wasn't able to get a browser reference, but I figured in this case it made sense to get the most recent browser since the location-change would cause the destination window to be the foreground window. (Let me know if my understanding is off on any of that)

> This needs to be localizable. See CustomizableWidgets.jsm in the
> updateZoomResetButton function there, how we use a localizable string to
> create this. 
> Not all locales may place the number in front of a % sign.

In order to make the button label localizable, I found that I need to start with an object for the button and then generate a XUL element with that object. This is because I need the "label" attribute to be null for the CustomizableUI.getLocalizedProperty() function to look up its value in the customizableWidgets.properties file. When the zoomResetButton started out as a XUL element, it's label would return ' ' instead of undefined even if it was never set. It was only undefined as a property on an object. 

Switching to an object for the button, I ended up reworking how it disappears and re-appears. Let me know if it will be really inefficient to add and remove an element like this.

Comment 26

[Dão Gottwald [:dao]]

Comment on attachment 8760899 [details]
Bug 565718 - Adds module for a zoom indicator in the browser's URL bar.

(In reply to Katie Broida [:ktbee] from comment #25)
> For the
> "location-change" observer, I wasn't able to get a browser reference, but I
> figured in this case it made sense to get the most recent browser since the
> location-change would cause the destination window to be the foreground
> window. (Let me know if my understanding is off on any of that)

I don't think this assumption holds. You can modify the code sending the location-change notification to give you the browser too.

> In order to make the button label localizable, I found that I need to start
> with an object for the button and then generate a XUL element with that
> object. This is because I need the "label" attribute to be null for the
> CustomizableUI.getLocalizedProperty() function to look up its value in the
> customizableWidgets.properties file. When the zoomResetButton started out as
> a XUL element, it's label would return ' ' instead of undefined even if it
> was never set. It was only undefined as a property on an object. 
> 
> Switching to an object for the button, I ended up reworking how it
> disappears and re-appears. Let me know if it will be really inefficient to
> add and remove an element like this.

You shouldn't use CustomizableUI.getLocalizedProperty here. Nor should you put the string in customizableWidgets.properties. Please use browser.properties.

Comment 27

[Katie Broida [:ktbee]]

Comment on attachment 8760899 [details]
Bug 565718 - Adds module for a zoom indicator in the browser's URL bar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58300/diff/2-3/

Comment 28

[Katie Broida [:ktbee]]

For localizing the label, I found that gNavigatorBundle.getFormattedString() didn't need the zoom indicator button to be created from an object (no need for the "label" property to be null). This means we can go back to having it be a button that was an addition to browser.xul rather than a node that gets added and removed from the toolbar based on the zoom level. This most recent patch has the button as a node that gets add/removed, but let me know if the first way is better.

Comment 29

[Dão Gottwald [:dao]]

Yes, please move it back to browser.xul.

Comment 30

[Katie Broida [:ktbee]]

Comment on attachment 8760899 [details]
Bug 565718 - Adds module for a zoom indicator in the browser's URL bar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58300/diff/3-4/

Comment 31

[Dão Gottwald [:dao]]

Comment on attachment 8760899 [details]
Bug 565718 - Adds module for a zoom indicator in the browser's URL bar.

This still makes the location bar taller than it should be. Screenshot coming.

Also, the transient X makes it hard to hit the history dropdown button when passing the zoom indicator with the pointer.

Comment 32

[Dão Gottwald [:dao]]

Attachment: screenshot

Comment 33

[Katie Broida [:ktbee]]

Comment on attachment 8760899 [details]
Bug 565718 - Adds module for a zoom indicator in the browser's URL bar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58300/diff/4-5/

Comment 34

[Katie Broida [:ktbee]]

That's a good point about the moving history dropdown button, Dão. I'm not sure what would be the best solution, but in this patch one fix I've implemented is making the containing element for the URL bar zoom button have a min-width that would be wide enough to contain the expanded zoom button without resizing. This way the history button wouldn't be moved by the containing element expanding. Alternatively, we could just have the X image always visible so that it didn't need to slide out and move the history button.

Comment 35

[Dão Gottwald [:dao]]

Comment on attachment 8760899 [details]
Bug 565718 - Adds module for a zoom indicator in the browser's URL bar.

I still see the problem shown in attachment 8770121 [details].

Comment 36

[Dão Gottwald [:dao]]

(In reply to Katie Broida [:ktbee] from comment #34)
> That's a good point about the moving history dropdown button, Dão. I'm not
> sure what would be the best solution, but in this patch one fix I've
> implemented is making the containing element for the URL bar zoom button
> have a min-width that would be wide enough to contain the expanded zoom
> button without resizing. This way the history button wouldn't be moved by
> the containing element expanding. Alternatively, we could just have the X
> image always visible so that it didn't need to slide out and move the
> history button.

Stephen, please chime in. Your mockup doesn't have the history dropdown and it's unclear how these elements are supposed to work together.

Comment 37

[Stephen Horlander [:shorlander]]

(In reply to Dão Gottwald [:dao] from comment #36)
> (In reply to Katie Broida [:ktbee] from comment #34)
> > That's a good point about the moving history dropdown button, Dão. I'm not
> > sure what would be the best solution, but in this patch one fix I've
> > implemented is making the containing element for the URL bar zoom button
> > have a min-width that would be wide enough to contain the expanded zoom
> > button without resizing. This way the history button wouldn't be moved by
> > the containing element expanding. Alternatively, we could just have the X
> > image always visible so that it didn't need to slide out and move the
> > history button.
> 
> Stephen, please chime in. Your mockup doesn't have the history dropdown and
> it's unclear how these elements are supposed to work together.

I need to build this and test it but some additional possible solutions (in addition to what Katie has implemented):
- Put the zoom control before the dropdown
- Use the same behavior as the hover effect on the dropdown so they are synced: show the X when you hover the toolbar
- Remove the X entirely and rely on the tooltip

Comment 38

[Katie Broida [:ktbee]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=761af7690ac2

Comment 39

[Katie Broida [:ktbee]]

Comment on attachment 8760899 [details]
Bug 565718 - Adds module for a zoom indicator in the browser's URL bar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58300/diff/5-6/

Comment 40

[Dão Gottwald [:dao]]

Comment on attachment 8760899 [details]
Bug 565718 - Adds module for a zoom indicator in the browser's URL bar.

URLBarZoom.jsm appears to be missing from this patch

Comment 41

[Katie Broida [:ktbee]]

Comment on attachment 8760899 [details]
Bug 565718 - Adds module for a zoom indicator in the browser's URL bar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58300/diff/6-7/

Comment 42

[Katie Broida [:ktbee]]

(In reply to Dão Gottwald [:dao] from comment #40)
> Comment on attachment 8760899 [details]
> Bug 565718 - Adds module for a zoom indicator in the browser's URL bar.
> 
> URLBarZoom.jsm appears to be missing from this patch

Sorry about that Dão! I had to re-import my patch at one point and didn't realize I didn't re-add some files to the patch. It should all be there in this most latest version.

Comment 43

[Dão Gottwald [:dao]]

Comment on attachment 8760899 [details]
Bug 565718 - Adds module for a zoom indicator in the browser's URL bar.

- Interaction with the history dropmarker is still suboptimal, and there's a similar problem with the reader mode icon. I think we should just remove the X at this point.

- The text in the button is larger than in the mockup. Please reduce the text size using em, e.g. font-size: .8em

- There seems to be insufficient margin before the button and too much margin after it.

- The bounce animation seems to big. It's cut off at the top and bottom where it exceeds the toolbar.

- The bounce animation is much slower than in the mockup.

- The bounce animation occurs when switching tabs. It should probably on occur when the user zooms in or out explicitly.

- The "#urlbar-zoom-button .toolbarbutton-text" rule should use the child selector, i.e. #urlbar-zoom-button > .toolbarbutton-text. Also, the comment in that rule erroneously refers to toolbarbutton-multiline-text.

- You should use display: -moz-box to show the text, not display: inline. !important shouldn't be needed either.

- nit: use 0 instead of 0px

Comment 44

[Katie Broida [:ktbee]]

Comment on attachment 8760899 [details]
Bug 565718 - Adds module for a zoom indicator in the browser's URL bar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58300/diff/7-8/

Comment 45

[Katie Broida [:ktbee]]

(In reply to Dão Gottwald [:dao] from comment #43)
> Comment on attachment 8760899 [details]
> Bug 565718 - Adds module for a zoom indicator in the browser's URL bar.
> 
> - The text in the button is larger than in the mockup. Please reduce the
> text size using em, e.g. font-size: .8em

It looks like the default size set for "em" is different for all of the operating systems, so I adjusted it for each one to get it closer to 12px (for Windows and Linux it's computed to 11.7333px).

> - There seems to be insufficient margin before the button and too much
> margin after it.

To make the margins more consistent, I made the zoom indicator have the same amount of space around it as the other URL bar icons (like reader mode). The URL bar icons each have a padding of 3px, so I gave the zoom indicator a left margin of 3px. The reload button already has some margin added on the indicator's right, so I made the indicator's right margin 0. 

> - The bounce animation is much slower than in the mockup.
I'm not 100% sure why the pulse animation seems slower than in the mockup since they have the same duration (500ms). However, I noticed that some of the other mockup animations happen over 250ms, which may make the whole animation appear to be shorter. I changed the duration to 250ms and the animation seems more consistent with the mockup now. 

> - The bounce animation occurs when switching tabs. It should probably on
> occur when the user zooms in or out explicitly.
This patch now listens to the observer topic and only adds the pulse's CSS class if the user has changed their zoom level and not if they change their location.

Comment 46

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8760899 [details]
Bug 565718 - Adds module for a zoom indicator in the browser's URL bar.

https://reviewboard.mozilla.org/r/58300/#review67940

::: browser/themes/linux/browser.css:957
(Diff revision 8)
> +  }
> +}
> +
> +#urlbar-zoom-button {
> +  -moz-appearance: none;
> +  margin: 0 0 0 3px;

This will break on RTL systems since everything gets flipped.

Replace this with:
margin-top: 0;
margin-inline-end: 0;
margin-bottom: 0;
margin-inline-start: 3px;

Here and in the other theme files.

Comment 47

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8760899 [details]
Bug 565718 - Adds module for a zoom indicator in the browser's URL bar.

I don't understand why mozreview marked this as r-. I didn't intend to set any review flag.

Comment 48

[Dão Gottwald [:dao]]

Comment on attachment 8760899 [details]
Bug 565718 - Adds module for a zoom indicator in the browser's URL bar.

> #urlbar-zoom-button {
>   -moz-appearance: none;
>   margin: 0 0 0 3px;

You should probably use margin: 0 3px; here, just like the urlbar-icon class except that we use padding there. If you really want the margin on one side only (why?), please follow Jared's instructions from comment 46.

> #urlbar-zoom-button > .toolbarbutton-text  {
>   display: -moz-box;
> }

nit: superfluous space before {

I think you also need to explicitly hide #urlbar-zoom-button > .toolbarbutton-icon, as this has a margin that makes the inside of the button misaligned.

Please use ThreeDLightShadow as the border color on Windows and Linux to ensure it's visible with dark themes / in high-contrast settings.

(In reply to Katie Broida [:ktbee] from comment #45)
> It looks like the default size set for "em" is different for all of the
> operating systems, so I adjusted it for each one to get it closer to 12px
> (for Windows and Linux it's computed to 11.7333px).

Font sizes aren't consistent across OSes, that's expected. em is a relative unit (so 0.8em is like 80%). I think we want this text to be smaller relative to the text size in the location bar, so using the same factor across platforms should be fine.

>     if( aTopic != "browser-fullZoom:location-change"){

nit: wonky use of spaces

>       zoomResetButton.className = "reset-zoom-pulse";
>     } else {
>       zoomResetButton.removeAttribute("class");

We tend to use attributes for these kind of per-element switches. E.g. zoomResetButton.setAttribute("animate", "true");

This patch still adds url-zoom-reset.svg without using it. Please remove that.

Comment 49

[Katie Broida [:ktbee]]

Comment on attachment 8760899 [details]
Bug 565718 - Adds module for a zoom indicator in the browser's URL bar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58300/diff/8-9/

Comment 50

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8760899 [details]
Bug 565718 - Adds module for a zoom indicator in the browser's URL bar.

https://reviewboard.mozilla.org/r/58300/#review68112

::: browser/modules/URLBarZoom.jsm:51
(Diff revisions 8 - 9)
> -      zoomResetButton.className = "reset-zoom-pulse";
> +      zoomResetButton.setAttribute("class", "reset-zoom-pulse");
>      } else {
>        zoomResetButton.removeAttribute("class");

Please use a different attribute name here because "class" also can get used for other classNames that would be referenced in CSS. Either using .className or .setAttribute("class", "..") have the same outcome.

You can switch this to .setAttribute("animate", "true") and then later, .removeAttribute("animate")

Comment 51

[Katie Broida [:ktbee]]

Comment on attachment 8760899 [details]
Bug 565718 - Adds module for a zoom indicator in the browser's URL bar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58300/diff/9-10/

Comment 52

[Dão Gottwald [:dao]]

Comment on attachment 8760899 [details]
Bug 565718 - Adds module for a zoom indicator in the browser's URL bar.

>+const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
>+
>+this.EXPORTED_SYMBOLS = [ "URLBarZoom" ];
>+
>+Cu.import("resource://gre/modules/Services.jsm");

nit: just call Components.utils.import directly and get rid of the Cc/Ci/Cu declarations.

>+var URLBarZoom = {
>+
>+  init: function(aWindow) {
>+    // Register ourselves with the service so we know when the zoom prefs change.
>+    Services.obs.addObserver(this, "browser-fullZoom:zoomChange", false);
>+    Services.obs.addObserver(this, "browser-fullZoom:zoomReset", false);
>+    Services.obs.addObserver(this, "browser-fullZoom:location-change", false);

Hmm, don't we need to remove these observers at some point? I guess this is fine if it doesn't leak on the Try server...

You can also just do Services.obs.addObserver(updateZoomButton, ...); directly and get rid of the 'observe' method.

>+  let urlBarParent = win.document.getElementById("urlbar-icons");

This is unused, please remove.

>+  let zoomFactor = 100;
[...]
>+  try {
>+    zoomFactor = Math.round(win.ZoomManager.zoom * 100);
>+  } catch (e) {}

This shouldn't ever throw an exception, should it? So please remove the try/catch and the earlier let zoomFactor = 100; declaration.

>+  if (zoomFactor !== 100) {

nit: just use !=

>+    } else {
>+      zoomResetButton.setAttribute("animate", "false");

Just use removeAttribute here.

Looks great overall!

Comment 53

[Katie Broida [:ktbee]]

(In reply to Dão Gottwald [:dao] from comment #52)
> Comment on attachment 8760899 [details]
> Bug 565718 - Adds module for a zoom indicator in the browser's URL bar.
> 
> >+var URLBarZoom = {
> >+
> >+  init: function(aWindow) {
> >+    // Register ourselves with the service so we know when the zoom prefs change.
> >+    Services.obs.addObserver(this, "browser-fullZoom:zoomChange", false);
> >+    Services.obs.addObserver(this, "browser-fullZoom:zoomReset", false);
> >+    Services.obs.addObserver(this, "browser-fullZoom:location-change", false);
> 
> Hmm, don't we need to remove these observers at some point? I guess this is
> fine if it doesn't leak on the Try server...
> 

Jared and I ran some tests on this patch on the Try server, and it seems like the leaks are intermittent:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c9368691c44

I've made the other changes, so I'll go ahead and push those and add "checkin-needed"

Comment 54

[Katie Broida [:ktbee]]

Comment on attachment 8760899 [details]
Bug 565718 - Adds module for a zoom indicator in the browser's URL bar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58300/diff/10-11/

Comment 55

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/ef0801fdc7abe018a8b026c00e8bdebcc690001c
Bug 565718 - Adds module for a zoom indicator in the browser's URL bar. r=dao

Comment 56

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ef0801fdc7ab

Comment 59

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

I think we'll want this tracked and tested as a small feature, since it will be very visible to users. Andrei, any thoughts?

Comment 60

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Removing the needinfo since this was already reviewed once, and is being tracked by the Engineering QA team.

Comment 61

[Gerry Chang [:gchang]]

Hi :ktbee, 
May I know if this will be shipped in 51?

Comment 62

[Dão Gottwald [:dao]]

(In reply to Gerry Chang [:gchang] from comment #61)
> Hi :ktbee, 
> May I know if this will be shipped in 51?

Yes, it will.

Comment 63

[Andrei Vaida [:avaida]]

[Test Plan]:
https://wiki.mozilla.org/QA/Zoom_Indicator

Comment 64

[Nazir Ahmed Sabbir [:NaSb]]

I've managed this issue in Firefox 3.6.5pre from Ubuntu 14.04.5 (64 Bit) 

This Bug is now verified as fixed on Latest Firefox Dev Edition 51.0a2 (2016-11-13) (64-bit)

Build ID: 20161113004006
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:51.0) Gecko/20100101 Firefox/51.0
OS: Linux 4.4.0-47-generic; Ubuntu 14.04.5

Comment 65

[Mohammad Maruf Rahman]

I have reproduced this bug with Firefox 3.5.11pre on Windows 7 , 64 Bit !

This Bug is now verified as fixed on Latest Firefox Dev Edition 52.0a2 (2016-11-16) (64-bit)

Build   ID    20161116004016
User Agent    Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:52.0) Gecko/20100101 Firefox/52.0

[bugday-20161116]

Comment 66

[Sylvestre Ledru [:Sylvestre]]

In the release notes of 51 with "Zoom indicator is shown in the URL bar if the zoom level is not at default level" as wording.

Comment 67

[P]

The zoom indicator is unremovable?
Please NEVER add features unconditionally, unremovably. Make them hideable.
Do not force me and many users to use ClassicThemeRestorer or similar addon.
It occupies precious space, shortens the visible part of the URL.
It is superfluous (redundant) if the zoom level is already shown with for example the zoompage addon.
And even confusing: at least once I saw the indicator showing 120% while the zoompage showing 100% - as I recall.

Comment 68

[yxu]

Agree with P in comment #67

Is there any possible we make a boolean for this feature in about:config? I think it's better for users if they can hide the indicator in customize menu, but It looks like need a lot of work compared to the previous suggestion.

Comment 69

[Michael Motes]

I had to read come here and read this thread and waste my time to fix your "improvement". I change to zoom often on webpages especially on my unusally sized screen devices, live the 62" TV I use as a monitor and use windows DPI scaling to make it viewable from 20ft on my couch. 

I don't want to see a number and percent sign in every urlbar, it is clutter and annoying. I get that you stupid people could never fathom the magic of ctrl+0 when you think that maybe a page is zoomed inappropriately and you want to return to normal, as you probably immediately blame someone else or the web page instead of considering the issue could be on your end, but the bug should have been marked as "not a bug - won't fix", or as has been said, it should be more easily removable with an about:config setting.


Anyone that wants to remove this can open C:\Program Files\Mozilla Firefox\browser\omni.ja with 7-zip or WinRAR and modify chrome\browser\skin\classic\browser\browser.css and remove all the CSS for ID #urlbar-zoom-button, that is remove all the green text found at this link: https://hg.mozilla.org/integration/fx-team/diff/ef0801fdc7ab/browser/themes/windows/browser.css