Zoom indicator (urlbar-zoom-button) is gone when moved to new window

VERIFIED FIXED in Firefox 53

Status

()

Firefox
Address Bar
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: magicp, Assigned: dao)

Tracking

Trunk
Firefox 54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 wontfix, firefox52 wontfix, firefox53 verified, firefox54 verified)

Details

MozReview Requests

()

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

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
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.
(Reporter)

Updated

2 years ago
Blocks: 565718
Has Regression Range: --- → yes
Has STR: --- → yes
status-firefox51: --- → affected
Component: Untriaged → Location Bar
OS: Unspecified → All
Hardware: Unspecified → All
(Reporter)

Updated

2 years ago
See Also: → bug 1300377
Possible duplicate of 1297970?
Aurora 51.0a2 (2016-09-27) and Nightly 52.0a1 (2016-09-27) are also affected.
status-firefox52: --- → affected
Flags: qe-verify+

Updated

2 years ago
Depends on: 1297970
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).

Updated

2 years ago
See Also: → bug 1313863
(Reporter)

Comment 4

2 years ago
Created attachment 8805806 [details]
steps-to-reproduce-20161030034116.mp4
Comment hidden (mozreview-request)

Comment 6

2 years ago
mozreview-review
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?
Attachment #8806207 - Flags: review?(gijskruitbosch+bugs) → review+
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)

Comment 8

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

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dad84422bcfc
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
(Assignee)

Comment 11

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

Comment 12

2 years ago
I've backed this out: https://hg.mozilla.org/integration/mozilla-inbound/rev/325acea6a61c07a698e6c738c6a8deb597795570
Status: RESOLVED → REOPENED
status-firefox52: fixed → affected
Resolution: FIXED → ---

Comment 13

2 years ago
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

Comment 14

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

Comment 15

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/38dcc78edc6d
fix bustage on a CLOSED TREE
Comment hidden (mozreview-request)

Comment 17

2 years ago
mozreview-review
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"
Attachment #8815084 - Flags: review?(jaws) → review+
(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.
Flags: needinfo?(jaws)
(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)
Flags: needinfo?(jaws)
Comment hidden (mozreview-request)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Comment 21

2 years ago
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.
Attachment #8815084 - Flags: review?(dao+bmo)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Updated

2 years ago
Attachment #8806207 - Attachment is obsolete: true

Comment 22

2 years ago
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?
Assignee: jaws → kbroida
Target Milestone: Firefox 52 → ---
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!
Assignee: kbroida → nobody
Jared, do you know anyone who might be able to take this over?
status-firefox51: affected → wontfix
status-firefox53: --- → affected
Flags: needinfo?(jaws)
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.
Flags: needinfo?(jaws) → needinfo?(dolske)

Comment 26

a year ago
Will this fix also fix Bug 1316907, which has been parked pending resolution of this bug?
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8815084 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Assignee: nobody → dao+bmo
Blocks: 1297970
status-firefox52: affected → wontfix
status-firefox54: --- → affected
No longer depends on: 1297970

Comment 28

a year ago
mozreview-review
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?
Attachment #8842346 - Flags: review?(jaws) → review+

Comment 29

a year ago
mozreview-review
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.

Comment 30

a year ago
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 31

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ff033748d4c6
Status: REOPENED → RESOLVED
Last Resolved: 2 years agoa year ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
(Assignee)

Comment 32

a year ago
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]: /
Attachment #8842346 - Flags: approval-mozilla-aurora?
Flags: needinfo?(dolske)

Comment 33

a year ago
[bugday-20170301]
status-firefox54: fixed → verified
(Assignee)

Updated

a year ago
Status: RESOLVED → VERIFIED
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).
Attachment #8842346 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 35

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/f904536fd210
status-firefox53: affected → fixed
Verified fixed Fx 53.0a2 (2017-03-06) Win 10
status-firefox53: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.