The default bug view has changed. See this FAQ.

Show zoom indicator in UI if not at default zoom level

VERIFIED FIXED in Firefox 51

Status

()

Firefox
General
VERIFIED FIXED
7 years ago
14 days ago

People

(Reporter: limi, Assigned: ktbee, Mentored)

Tracking

(Depends on: 4 bugs, Blocks: 1 bug)

Trunk
Firefox 51
Points:
8
Dependency tree / graph

Firefox Tracking Flags

(firefox51 verified, firefox52 verified, relnote-firefox 51+)

Details

(Whiteboard: [Advo][outreachy-12], URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 4 obsolete attachments)

Created attachment 445156 [details]
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

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

Comment 2

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

Comment 3

5 years ago
> 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.

Updated

4 years ago
Whiteboard: [Advo]
Blocks: 1248735
Mentor: jaws@mozilla.com
Points: --- → 8
Whiteboard: [Advo] → [Advo][outreachy-12]
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.
Flags: needinfo?(shorlander)

Comment 5

11 months ago
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.
No longer blocks: 1248735
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.
(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
Flags: needinfo?(shorlander)
Flags: needinfo?(jaws)
Yeah, that looks great. Katie, this will be a good bug for you to work on next.
Assignee: nobody → kbroida
Status: NEW → ASSIGNED
Flags: needinfo?(jaws)
(Assignee)

Comment 9

10 months ago
Sounds good!
(Assignee)

Comment 10

10 months ago
Created attachment 8760899 [details]
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/
Attachment #8760899 - Flags: review?(jaws)
(Assignee)

Comment 11

10 months ago
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.
(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 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?
Attachment #8760899 - Flags: review?(jaws) → review-
(Assignee)

Comment 14

9 months ago
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.
(Assignee)

Comment 15

9 months ago
Created attachment 8764019 [details] [diff] [review]
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.
Attachment #8760899 - Attachment is obsolete: true
Attachment #8764019 - Flags: review?(dao+bmo)
(Assignee)

Comment 16

9 months ago
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.
(Assignee)

Comment 17

9 months ago
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.

Updated

9 months ago
Attachment #445156 - Attachment is obsolete: true
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.
Attachment #8764019 - Flags: review?(dao+bmo) → review-
Created attachment 8764208 [details]
patch v2 screenshot
(Assignee)

Comment 20

9 months ago
(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.
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 21

9 months ago
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.
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.
Flags: needinfo?(dao+bmo)
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.
(Assignee)

Comment 24

9 months ago
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/
Attachment #8760899 - Attachment is obsolete: false
Attachment #8760899 - Flags: review- → review?(dao+bmo)
(Assignee)

Comment 25

9 months ago
(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 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.
Attachment #8760899 - Flags: review?(dao+bmo) → review-
(Assignee)

Comment 27

9 months ago
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/
Attachment #8760899 - Flags: review- → review?(dao+bmo)
(Assignee)

Comment 28

9 months ago
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.
Yes, please move it back to browser.xul.

Updated

9 months ago
Attachment #8760899 - Flags: review?(dao+bmo)
(Assignee)

Comment 30

9 months ago
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/
Attachment #8760899 - Flags: review?(dao+bmo)
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.
Attachment #8760899 - Flags: review?(dao+bmo) → review-
Created attachment 8770121 [details]
screenshot
Attachment #8764019 - Attachment is obsolete: true
Attachment #8764208 - Attachment is obsolete: true
(Assignee)

Comment 33

9 months ago
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/
Attachment #8760899 - Flags: review- → review?(dao+bmo)
(Assignee)

Comment 34

9 months ago
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 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].
Attachment #8760899 - Flags: review?(dao+bmo) → review-
(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.
Flags: needinfo?(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
(Assignee)

Comment 38

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=761af7690ac2
(Assignee)

Comment 39

8 months ago
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/
Attachment #8760899 - Attachment description: Bug 565718 - Adds module for a zoom indicator in the browser's URL bar. This indicator doesn't show if there is already zoom controls added from Customize Mode → Bug 565718 - Adds module for a zoom indicator in the browser's URL bar.
Attachment #8760899 - Flags: review- → review?(dao+bmo)
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
Attachment #8760899 - Flags: review?(dao+bmo)
(Assignee)

Comment 41

8 months ago
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/
Attachment #8760899 - Flags: review?(dao+bmo)
(Assignee)

Comment 42

8 months ago
(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 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
Attachment #8760899 - Flags: review?(dao+bmo) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 45

8 months ago
(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

8 months ago
mozreview-review
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.
Attachment #8760899 - Flags: review-
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.
Attachment #8760899 - Flags: review-
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.
Attachment #8760899 - Flags: review?(dao+bmo) → review-
Comment hidden (mozreview-request)

Comment 50

8 months ago
mozreview-review
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 hidden (mozreview-request)

Updated

8 months ago
Attachment #8770121 - Attachment is obsolete: true
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!
Attachment #8760899 - Flags: review?(dao+bmo) → review+
(Assignee)

Comment 53

8 months ago
(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 hidden (mozreview-request)
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

Updated

8 months ago
Attachment #8760899 - Flags: review?(dao+bmo) → review+

Comment 56

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ef0801fdc7ab
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51

Updated

8 months ago
Duplicate of this bug: 1215638

Updated

8 months ago
Duplicate of this bug: 595686

Updated

8 months ago
Depends on: 1295769

Updated

7 months ago
Flags: needinfo?(shorlander)

Updated

7 months ago
Depends on: 1297970

Updated

7 months ago
Depends on: 1298449

Updated

7 months ago
Depends on: 1300376

Updated

7 months ago
Depends on: 1300377

Updated

7 months ago
Depends on: 1238637
I think we'll want this tracked and tested as a small feature, since it will be very visible to users. Andrei, any thoughts?
Flags: needinfo?(andrei.vaida)
Removing the needinfo since this was already reviewed once, and is being tracked by the Engineering QA team.
Flags: needinfo?(andrei.vaida)

Updated

6 months ago
Depends on: 1305195
Hi :ktbee, 
May I know if this will be shipped in 51?
Flags: needinfo?(kbroida)
(In reply to Gerry Chang [:gchang] from comment #61)
> Hi :ktbee, 
> May I know if this will be shipped in 51?

Yes, it will.
Flags: needinfo?(kbroida)
[Test Plan]:
https://wiki.mozilla.org/QA/Zoom_Indicator
QA Contact: iulia.cristescu

Updated

5 months ago
Depends on: 1089246
Depends on: 1310758
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
QA Whiteboard: [bugday-20161109]
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]

Updated

4 months ago
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified
status-firefox52: --- → verified

Updated

4 months ago
Depends on: 1318830

Updated

3 months ago
Depends on: 1327131

Updated

3 months ago
Depends on: 1327145
Depends on: 1317686
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.
relnote-firefox: --- → 51+

Updated

2 months ago
Depends on: 1335572

Updated

2 months ago
Depends on: 1327642

Updated

a month ago
No longer depends on: 1317686

Comment 67

a month ago
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.

Updated

14 days ago
Blocks: 1348122
You need to log in before you can comment on or make changes to this bug.