User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0 Build ID: 20160903030202 Steps to reproduce: 1. Start Nightly 2. Open any sites in a new tab 3. Zoom in (or zoom out) the tab > Zoom indicator is displayed 4. Move the tab to new window 5. Check zoom indicator Actual results: Zoom indicator (urlbar-zoom-button) is gone when moved to new window. Regression range: https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=8725f14625e0c87776f9acf66e6712ceca301000&tochange=ef0801fdc7abe018a8b026c00e8bdebcc690001c Expected results: Zoom indicator should be displayed when moved to new window.

Possible duplicate of 1297970?

Aurora 51.0a2 (2016-09-27) and Nightly 52.0a1 (2016-09-27) are also affected.

I observed the following case: presuming that in the current window you have open a random page (with A zoom level), if you open a new window with another page (with B zoom level) and move this last tab to the first window, the zoom indicator from the second tab will lose the B value and will get the A value. This may be an explanation for the zoom indicator fadeaway stated in comment 0: zoom indicator from a tab moved to another window is getting the zoom level value from the new window (namely 100% - default zoom level).

Comment on attachment 8806207 [details] Bug 1300376 - Update the zoom indicator status on XULBrowserWindow location changes to catch cases where the indicator wasn't getting updated such as swapping doc shells. https://reviewboard.mozilla.org/r/89692/#review89200 So, this looks good to me, but note that there has been previous discussion of some of this in bug 1297970. I believe this patch should also fix bug 1297970. ::: browser/base/content/browser.js:4593 (Diff revision 1) > gCustomizeMode.exit(); > } > } > UpdateBackForwardCommands(gBrowser.webNavigation); > ReaderParent.updateReaderButton(gBrowser.selectedBrowser); > + URLBarZoom.updateZoomButton(gBrowser.selectedBrowser, "browser-fullZoom:location-change"); Seems both this and the readermode thing should be inside the "isTopLevel" if block... ::: browser/modules/URLBarZoom.jsm:18 (Diff revision 1) > > - init: function(aWindow) { > + init(aWindow) { > // Register ourselves with the service so we know when the zoom prefs change. > - Services.obs.addObserver(updateZoomButton, "browser-fullZoom:zoomChange", false); > - Services.obs.addObserver(updateZoomButton, "browser-fullZoom:zoomReset", false); > - Services.obs.addObserver(updateZoomButton, "browser-fullZoom:location-change", false); > + 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); Nit: this seems a bit footgunny because now |this| references in the method won't work. Can you just add |this| as the observer and write an observe() method that forwards to updateZoomButton?

Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/dad84422bcfc Update the zoom indicator status on XULBrowserWindow location changes to catch cases where the indicator wasn't getting updated such as swapping doc shells. r=Gijs

(In reply to :Gijs Kruitbosch from comment #6) > Comment on attachment 8806207 [details] > Bug 1300376 - Update the zoom indicator status on XULBrowserWindow location > changes to catch cases where the indicator wasn't getting updated such as > swapping doc shells. > > https://reviewboard.mozilla.org/r/89692/#review89200 > > So, this looks good to me, but note that there has been previous discussion > of some of this in bug 1297970. I believe this patch should also fix bug > 1297970. Yes, this indeed fixes bug 1297970.

*sigh* So this patch basically does what Katie proposed in bug 1297970 comment 6 and what I rejected in the following comment. I.e. it makes us call URLBarZoom.updateZoomButton twice per location change in the selected browser.

I've backed this out: https://hg.mozilla.org/integration/mozilla-inbound/rev/325acea6a61c07a698e6c738c6a8deb597795570

Pushed by cbook@mozilla.com: https://hg.mozilla.org/mozilla-central/rev/8002299dfc3f fixing conflicts in backout that likely causing tree closing timeouts on a CLOSED TREE r+a=sheriffduty

Backout by cbook@mozilla.com: https://hg.mozilla.org/mozilla-central/rev/8b845f0dfb42 Backed out changeset 8002299dfc3f causing es bustage on a CLOSED TREE

Pushed by cbook@mozilla.com: https://hg.mozilla.org/mozilla-central/rev/38dcc78edc6d fix bustage on a CLOSED TREE

Comment on attachment 8815084 [details] Bug 1300376 -- Ensure zoom indicator appears in browser after tab is transfered to a new window. https://reviewboard.mozilla.org/r/96076/#review98722 Sorry for taking so long to review it. I was hoping Dao would get to it sooner. This looks fine to me. If Dao doesn't respond to the review request in the next 3 days then we should land this patch with the minor change requested in URLBarZoom.jsm. ::: browser/base/content/tabbrowser.xml:2921 (Diff revision 1) > this.updateCurrentBrowser(true); > > if (modifiedAttrs.length) { > this._tabAttrModified(aOurTab, modifiedAttrs); > } > + Services.obs.notifyObservers(ourBrowser, "tabbrowser:swapBrowsersAndCloseOther", ""); I'm not necessarily fond of this topic name, but it does follow the convention used elsewhere and I can't think of anything better. ::: browser/modules/URLBarZoom.jsm:24 (Diff revision 1) > - let win = aSubject.ownerDocument.defaultView; > + // aSubject.ownerGlobal may no longer exist if a tab has been dragged to a > + // new window. In this case, aSubject.ownerGlobal will be supplied by > + // the notification from "tabbrowser:swapBrowsersAndCloseOther" in tabbrowser.xml > + if (!aSubject.ownerGlobal) { > + return; > + } > + let win = aSubject.ownerGlobal; The code here doesn't necessarily match the comment. We should only be getting reading aSubject.ownerGlobal if aTopic is "tabbrowser:swapBrowsersAndCloseOther"

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #17) > Comment on attachment 8815084 [details] > Bug 1300376 -- Ensure zoom indicator appears in browser after tab is > transfered to a new window. > > https://reviewboard.mozilla.org/r/96076/#review98722 > > Sorry for taking so long to review it. I was hoping Dao would get to it > sooner. This looks fine to me. If Dao doesn't respond to the review request > in the next 3 days then we should land this patch with the minor change > requested in URLBarZoom.jsm. No worries! Should I add a checkin-needed to this bug at the end of the week (assuming Dao doesn't request any changes)? > ::: browser/modules/URLBarZoom.jsm:24 > (Diff revision 1) > > - let win = aSubject.ownerDocument.defaultView; > > + // aSubject.ownerGlobal may no longer exist if a tab has been dragged to a > > + // new window. In this case, aSubject.ownerGlobal will be supplied by > > + // the notification from "tabbrowser:swapBrowsersAndCloseOther" in tabbrowser.xml > > + if (!aSubject.ownerGlobal) { > > + return; > > + } > > + let win = aSubject.ownerGlobal; > > The code here doesn't necessarily match the comment. We should only be > getting reading aSubject.ownerGlobal if aTopic is > "tabbrowser:swapBrowsersAndCloseOther" I struggled with the wording of that comment and I'm all for changing it. I'm not sure I understand what you're saying the edits should be though. This if-statement is there to prevent "win is null" errors (as discussed in bug 1297970) by just ending the function and assuming `win` will be supplied by later calls to updateZoomButton(). So we need to get aSubject.ownerGlobal no matter what the aTopic is, right? Like in case aTopic is something like "browser-fullZoom:zoomChange"? Let me know if I'm misunderstanding that.

(In reply to Katie Broida [:ktbee] from comment #18) > (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #17) > > Comment on attachment 8815084 [details] > > Bug 1300376 -- Ensure zoom indicator appears in browser after tab is > > transfered to a new window. > > > > https://reviewboard.mozilla.org/r/96076/#review98722 > > > > Sorry for taking so long to review it. I was hoping Dao would get to it > > sooner. This looks fine to me. If Dao doesn't respond to the review request > > in the next 3 days then we should land this patch with the minor change > > requested in URLBarZoom.jsm. > > No worries! Should I add a checkin-needed to this bug at the end of the week > (assuming Dao doesn't request any changes)? Yes, that is correct. > > ::: browser/modules/URLBarZoom.jsm:24 > > (Diff revision 1) > > > - let win = aSubject.ownerDocument.defaultView; > > > + // aSubject.ownerGlobal may no longer exist if a tab has been dragged to a > > > + // new window. In this case, aSubject.ownerGlobal will be supplied by > > > + // the notification from "tabbrowser:swapBrowsersAndCloseOther" in tabbrowser.xml > > > + if (!aSubject.ownerGlobal) { > > > + return; > > > + } > > > + let win = aSubject.ownerGlobal; > > > > The code here doesn't necessarily match the comment. We should only be > > getting reading aSubject.ownerGlobal if aTopic is > > "tabbrowser:swapBrowsersAndCloseOther" > > I struggled with the wording of that comment and I'm all for changing it. > I'm not sure I understand what you're saying the edits should be though. > This if-statement is there to prevent "win is null" errors (as discussed in > bug 1297970) by just ending the function and assuming `win` will be supplied > by later calls to updateZoomButton(). So we need to get aSubject.ownerGlobal > no matter what the aTopic is, right? Like in case aTopic is something like > "browser-fullZoom:zoomChange"? Let me know if I'm misunderstanding that. Okay, the comment made it seem to me that you were only reading aSubject.ownerGlobal when aTopic == "tabbrowser:swapBrowsersAndCloseOther". We should just update the comment since it is actually more generic than swapBrowsersAndCloseOther. Let's go with something like: // aSubject.ownerGlobal may no longer exist if a tab has been dragged to a // new window. In this case, aSubject.ownerGlobal will be supplied by the // later notification of "tabbrowser:swapBrowsersAndCloseOther". (the subtle tweak is in using the words "the later notification", which should make it clearer that we expect a future notification at which point we will be able to act upon this)

Comment on attachment 8815084 [details] Bug 1300376 -- Ensure zoom indicator appears in browser after tab is transfered to a new window. The tabbrowser:swapBrowsersAndCloseOther name doesn't really make sense from an API design point of view. It mentions two browsers but the subject is just a single browser. Also I think this might miss other cases where we swap browsers by calling tabbrowser.swapBrowser or browser.swapBrowsers rather than tabbrowser.swapBrowsersAndCloseOther. I'm actually not sure where we use these APIs, but they probably exist for a reason. I suspect the sanest thing to do would be to listen to the SwapDocShells event.

Over in bug 1318830 dbaron suggested an alternative way of keeping the zoom indicator up-to-date, assuming that the event linked in bug 1318830 comment 3 also fires for any other full zoom updates. Would switching to that help fix this issue?

Thanks for the feedback and suggestions Dao and Gijs. I won't be able to to keep up with this bug since I'll be starting a new job soon (!!!), but hopefully the next person will be able to wrap it up with the information here. If not, please let me know if you have any questions. Thanks again!

Jared, do you know anyone who might be able to take this over?

I propose we re-land the patch from comment 10. That patch will make URLBarZoom.updateZoomButton get called multiple times, but since we don't really understand what is going on here we don't have a better solution available. When choosing between correctness and performance, we should choose correctness. Dolske, are you OK with re-landing the patch in comment 10? Note that it was backed out due to comment 11 (and https://bugzilla.mozilla.org/show_bug.cgi?id=1297970#c7). This is not a high priority area for engineers to work on currently, and landing it would close a couple known regressions (this bug, bug 1257312, bug 1297970). I would prefer to land this, and then we can file a follow-up to remove redundant calls.

Will this fix also fix Bug 1316907, which has been parked pending resolution of this bug?

Comment on attachment 8842346 [details] Bug 1300376 - Update zoom indicator when moving a browser to another window. https://reviewboard.mozilla.org/r/116230/#review117858 Does this fix any tests? We should add a test for the closing window case. ::: commit-message-1bc2a:1 (Diff revision 1) > +Bug 1300376 - Update zoom indicator when moving a browser to another window. r?jaws I haven't seen a commit-message file before. I think this should be removed?

Comment on attachment 8842346 [details] Bug 1300376 - Update zoom indicator when moving a browser to another window. https://reviewboard.mozilla.org/r/116230/#review117874 ::: commit-message-1bc2a:1 (Diff revision 1) > +Bug 1300376 - Update zoom indicator when moving a browser to another window. r?jaws It's a mozreview change so that you can add comments to the commit message. It's not an actual file.

Pushed by dgottwald@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ff033748d4c6 Update zoom indicator when moving a browser to another window. r=jaws

Comment on attachment 8842346 [details] Bug 1300376 - Update zoom indicator when moving a browser to another window. Approval Request Comment [Feature/Bug causing the regression]: bug 565718 [User impact if declined]: see comment 0 [Is this code covered by automated tests?]: no [Has the fix been verified in Nightly?]: not yet [Needs manual test from QE? If yes, steps to reproduce]: yes, see comment 0 for STR [List of other uplifts needed for the feature/fix]: / [Is the change risky?]: no [Why is the change risky/not risky?]: fairly straightforward and isolated fix [String changes made/needed]: /

[bugday-20170301]

Comment on attachment 8842346 [details] Bug 1300376 - Update zoom indicator when moving a browser to another window. Regression from 51, let's uplift the fix to 53. If there are any followup issues please file new bugs (unless it's clear this should be backed out).

Verified fixed Fx 53.0a2 (2017-03-06) Win 10