Zoom controls percentage label doesn't update when it's in the toolbar and you navigate page

VERIFIED FIXED in Firefox 29

Status

()

Firefox
Toolbars and Customization
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: Alice0775 White, Assigned: mikedeboer)

Tracking

(Blocks: 1 bug)

29 Branch
Firefox 29
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:P3])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

4 years ago
Build Identifier:
http://hg.mozilla.org/integration/fx-team/rev/b70ad79e2d7c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140102200527

Bug 934951 does not fix this problem.

Steps To Reproduce:
1. Move "zoom controls" to toolbar by customize.
2. Open a page(ex. https://www.mozilla.org/en-US/about/ ) and Zoom in
   --- Zoom controls percentage label is changed to 110% ---- as expected
3. Open New tab
   --- Zoom controls percentage label is changed to 100% ---- as expected
4. Load the same page of step2(i.e. https://www.mozilla.org/en-US/about/ )
   --- Zoom controls percentage label is keeping to stay 100% ---- bug

Actual Results:
Zoom controls percentage label is not changed.

Expected Results:
Zoom controls percentage label should be changed to the site specific zoom level.

Updated

4 years ago
Blocks: 934951
Whiteboard: [Australis:P3]

Updated

4 years ago
OS: Windows 7 → All
Hardware: x86_64 → All

Comment 1

4 years ago
So AFAICT in order to get this right, we need to update the label *after* the browser full zoom code has done its work by checking the content preference service and so on. Which means using that notification which we used to use and then removed, because it was "just for tests" and now is actually called "FullZoom:TESTS:location-change". Drew, what should we be doing here? Because the update is async (AFAICT) we do need some sort of notification... :-\
Flags: needinfo?(adw)

Comment 2

4 years ago
(In reply to :Gijs Kruitbosch from comment #1)
> Because the update is async (AFAICT) we do need some sort of
> notification... :-\

Yes, whenever a zoom setting for a site isn't cached in memory, we have to check the CPS's database, and in those cases the zoom change is async.

Looking at bug 897410 comment 15 and 17, I think the reason I didn't like this notification being used outside tests is that sometimes the browser that triggers it can be destroyed before it's broadcasted, which observers do not expect, resulting in the JS errors in that bug for example.  So if you rely on this notification, you'll need to make sure that the browser that changed locations is still alive before you use it, e.g., by checking that browser.docShell is still defined.  And actually, the browser isn't included in the notification, but it seems like you would need it in this case, so it probably should be.
Flags: needinfo?(adw)
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Created attachment 8361588 [details] [diff] [review]
Patch 1: Update zoom controls percentage label upon navigation
Attachment #8361588 - Flags: review?(gijskruitbosch+bugs)
Created attachment 8361590 [details] [diff] [review]
Patch 1.1: Update zoom controls percentage label upon navigation

updated comment.
Attachment #8361588 - Attachment is obsolete: true
Attachment #8361588 - Flags: review?(gijskruitbosch+bugs)
Attachment #8361590 - Flags: review?(gijskruitbosch+bugs)

Comment 5

4 years ago
Comment on attachment 8361588 [details] [diff] [review]
Patch 1: Update zoom controls percentage label upon navigation

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

In principle this looks fine, but a try run would not go amiss for a change like this considering the errors we've seen in the past. (in fact, adding a test for this particular issue would also be a good idea...)

Note that when updating the tests you missed browser_privatebrowsing_zoomrestore.js:

http://mxr.mozilla.org/mozilla-central/search?string=TESTS%3Alocation-change

Regardless of whether you add a test or do a try run, please fix that before landing. :-)
Attachment #8361588 - Attachment is obsolete: false

Comment 6

4 years ago
Comment on attachment 8361588 [details] [diff] [review]
Patch 1: Update zoom controls percentage label upon navigation

Ugh, bugzilla, you suck.
Attachment #8361588 - Attachment is obsolete: true

Comment 7

4 years ago
Comment on attachment 8361590 [details] [diff] [review]
Patch 1.1: Update zoom controls percentage label upon navigation

For the review comment, see my previous comment. Seems you picked up the private browsing one in this version already (forgot to qref?) so that's fine. A test specifically for this issue would be useful though.
Attachment #8361590 - Flags: review?(gijskruitbosch+bugs) → review+
Created attachment 8361602 [details] [diff] [review]
Patch 2: unit test.
Attachment #8361602 - Flags: review?(gijskruitbosch+bugs)

Comment 9

4 years ago
Comment on attachment 8361602 [details] [diff] [review]
Patch 2: unit test.

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

::: browser/components/customizableui/test/browser_934951_zoom_in_toolbar.js
@@ +35,5 @@
> +  // Test zoom label updates while navigating pages in the same tab.
> +  FullZoom.enlarge();
> +  yield zoomChangePromise;
> +  is(parseInt(zoomResetButton.label, 10), 110, "Zoom is changed to 110% for about:mozilla");
> +  yield promiseTabLoadEvent(tab1, "about:home");

If this doesn't return the test will time out and we'll all be very sad (including subsequent tests). Can you reject the promise after some timeout (inside both promiseTabLoadEvent and promiseTabNavigate) and deal will fallout in some kind of cleanup to remove the added tabs?

@@ +87,5 @@
> + * @param aEvent
> + *        The load event type to wait for.  Defaults to "load".
> + * @return {Promise} resolved when the event is handled.
> + */
> +function promiseTabLoadEvent(aTab, aURL, aEventType="load") {

This should probably live in head.js. Did you copy it from somewhere?

@@ +104,5 @@
> +  aTab.linkedBrowser.loadURI(aURL);
> +  return deferred.promise;
> +}
> +
> +function promiseTabNavigate(back = true) {

Ditto.
Attachment #8361602 - Flags: review?(gijskruitbosch+bugs) → feedback+
Created attachment 8361618 [details] [diff] [review]
Patch 2.1: unit test.
Attachment #8361602 - Attachment is obsolete: true
Attachment #8361618 - Flags: review?(gijskruitbosch+bugs)

Comment 11

4 years ago
Comment on attachment 8361618 [details] [diff] [review]
Patch 2.1: unit test.

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

r+ but nits below.

::: browser/components/customizableui/test/head.js
@@ +16,5 @@
>  
>  let {synthesizeDragStart, synthesizeDrop} = ChromeUtils;
>  
>  const kNSXUL = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> +const {Ci: interfaces} = Components;

This is broken; it's defined above already.

@@ +17,5 @@
>  let {synthesizeDragStart, synthesizeDrop} = ChromeUtils;
>  
>  const kNSXUL = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> +const {Ci: interfaces} = Components;
> +const kTimeoutInMS = 20000;

This should have a more specific name.

@@ +356,5 @@
> +    gBrowser.removeEventListener("pageshow", listener, true);
> +    clearTimeout(timeoutId);
> +    // Make sure to resolve the promise until other async ops are finished (ex.
> +    // FullZoom ops).
> +    setTimeout(() => deferred.resolve(), 2000);

I'd prefer adding a condition callback function and:

if (aConditionCallbackFunction) {
  yield waitForCondition(aConditionCallbackFunction)
}

over the settimeout.

@@ +360,5 @@
> +    setTimeout(() => deferred.resolve(), 2000);
> +  }
> +  gBrowser.addEventListener("pageshow", listener, true);
> +
> +  if (aBack)

We don't seem to ever use the other version of this, and it seems counterintuitive to me that "navigate" goes *back* rather than... somewhere else, and that it has a boolean that indicates when you want to go forward, ie:

promiseTabNavigation(false); // This actually navigates forward

That seems weird.

Maybe name it promiseTabHistoryNavigation or something, and use an int for direction? IIRC there's a gBrowser or navigator method that'll take such an int directly, too. I dunno. As you like.
Attachment #8361618 - Flags: review?(gijskruitbosch+bugs) → review+
Created attachment 8361632 [details] [diff] [review]
Patch 2.2: unit test.
Attachment #8361632 - Flags: review?(gijskruitbosch+bugs)
Attachment #8361618 - Attachment is obsolete: true

Comment 13

4 years ago
Comment on attachment 8361632 [details] [diff] [review]
Patch 2.2: unit test.

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

r=me with the nits addressed.

::: browser/components/customizableui/test/browser_934951_zoom_in_toolbar.js
@@ +57,5 @@
>    };
>    let timeoutId = setTimeout(() => {
>      Services.obs.removeObserver(notificationCallback, aObserver, false);
>      deferred.reject("Notification '" + aObserver + "' did not happen within 20 seconds.");
> +  }, kTabEventFailureTimeoutInMs);

Assign the value from this to an appropriately named variable at the top of the file. Sorry, I didn't realize you used it here as well...

::: browser/components/customizableui/test/head.js
@@ +17,5 @@
>  let {synthesizeDragStart, synthesizeDrop} = ChromeUtils;
>  
>  const kNSXUL = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> +const kTabEventFailureTimeoutInMs = 20000;
> +const kBack = -1;

Don't really need these, see below. If you want to, you could keep kBack for the default argument value, but otherwise...

@@ +309,5 @@
> + * Starts a load in an existing tab and waits for it to finish (via some event).
> + *
> + * @param aTab   The tab to load into.
> + * @param aUrl   The url to load.
> + * @param aEvent The load event type to wait for.  Defaults to "load".

You mean aEventType.

@@ +344,5 @@
> + * @param aDirection Number to indicate to move backward in history (kBack) or
> + *                   forward (kForward).
> + * @return {Promise} resolved when navigation has finished.
> + */
> +function promiseTabHistoryNavigation(aDirection = kBack, aConditionFn) {

There's no documentation for aConditionFn.

@@ +368,5 @@
> +
> +  if (aDirection == kBack)
> +    gBrowser.goBack();
> +  else if (aDirection == kForward)
> +    gBrowser.goForward();

content.history.go(aDirection);
Attachment #8361632 - Flags: review?(gijskruitbosch+bugs) → review+
Thanks!

remote: https://hg.mozilla.org/integration/fx-team/rev/14e817fe25e1
remote: https://hg.mozilla.org/integration/fx-team/rev/7c74e99d80c1
Backed out for mochitest-bc failures. That's two backouts in two days. Can you please run these through Try before pushing?
https://hg.mozilla.org/integration/fx-team/rev/00dca774ecd1

https://tbpl.mozilla.org/php/getParsedLog.php?id=33171646&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=33171720&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=33171783&tree=Fx-Team
Try build running with observer cleanup: https://tbpl.mozilla.org/?tree=Try&rev=7f0ebe2bb2b8
re-pushed with memleak fixes:

https://hg.mozilla.org/integration/fx-team/rev/ee6b70532489
https://hg.mozilla.org/integration/fx-team/rev/3843c855d7ec
https://hg.mozilla.org/mozilla-central/rev/ee6b70532489
https://hg.mozilla.org/mozilla-central/rev/3843c855d7ec
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29

Updated

4 years ago
QA Contact: cornel.ionce
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (X11; Linux i686; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:29.0) Gecko/20100101 Firefox/29.0

Verified fixed on Firefox 29 beta 5 (build ID: 20140403132807).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.