User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:51.0) Gecko/20100101 Firefox/51.0 Build ID: 20161114004005 Steps to reproduce: Reproducible in FF 51 through 53. 1) Open any image / video. Sample image: http://i.imgur.com/Z0LU4lT.png. 2) Zoom in or out on the image so as to be at a zoom =/= 100%. 3) Now click on the image. Actual results: A) With the zoom percentage / + / - combo button (the one that looks like this - http://i.imgur.com/836AAdo.png) placed in the hamburger menu: the actual zoom of the image is reset but the indicator in the awesomebar isn't. However, the indicator in the hamburger menu has been correctly reset. Gif showing the bug: http://i.imgur.com/26wmoHd.gif (click marks the point at which the zoom is abruptly reset, STR 3). B) With the zoom percentage / + / - combo button placed in the nav-bar: the actual zoom of the image is reset but now even this combo button display an incorrect zoom level. Gif showing the bug: http://i.imgur.com/JEX4Oj5.gif (again, click marks the point at which the zoom is abruptly reset, STR 3). Expected results: Either the zoom level of the images / videos shouldn't be reset upon clicking, or the zoom indicators, whether the one in the awesomebar or the combo one, should show 100% zoom even upon clicking on the image / video.

I expect this is a frontend bug, made more noticeable because of the URL bar zoom indicator.

The basic problem here is that the code added in bug 565718, i.e., https://hg.mozilla.org/mozilla-central/rev/ef0801fdc7ab , doesn't interact with the image document code in https://searchfox.org/mozilla-central/source/dom/html/ImageDocument.cpp . I guess it seems a little bit odd to me that the notifications that it's hooked up to are global notifications for certain browser-triggered actions (which will update the zoom in every tab at once!) rather than notifications for changes to the relevant tab, from the code that controls the zoom for that tab. I think it would be preferable to observe the zoom more directly (and less globally), so moving this to the Firefox component rather than the image document component (neither of which is Layout: View Rendering).

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #2) > The basic problem here is that the code added in bug 565718, i.e., > https://hg.mozilla.org/mozilla-central/rev/ef0801fdc7ab , doesn't interact > with the image document code in > https://searchfox.org/mozilla-central/source/dom/html/ImageDocument.cpp . It looks like the image document code ends up firing a custom event from the document viewer here: https://searchfox.org/mozilla-central/rev/904bf9addd03b03d4cad11b82f19f43d875b7f27/layout/base/nsDocumentViewer.cpp#3083-3088 that frontend can listen to. > I guess it seems a little bit odd to me that the notifications that it's > hooked up to are global notifications for certain browser-triggered actions > (which will update the zoom in every tab at once!) Err, I don't think this is the case? Not when site-specific zoom is on, which is the default... > rather than notifications > for changes to the relevant tab, from the code that controls the zoom for > that tab. The indicator also needs updating for tab switches, or when new pages load in the same tab that then are subject to a different zoom level because of site-specific zoom.

(In reply to :Gijs Kruitbosch from comment #3) > > I guess it seems a little bit odd to me that the notifications that it's > > hooked up to are global notifications for certain browser-triggered actions > > (which will update the zoom in every tab at once!) > > Err, I don't think this is the case? Not when site-specific zoom is on, > which is the default... Er, I guess if the code is per-window, then still for the active tab in each window? Still seems like the wrong approach.

tracking 53+ for this bug since we should show the zoom level correctly.

Track 51+ as the zoom level is not shown correctly.

jaws, can you suggest someone who might work on this? Asking since you were the reviewer for the patch from bug 56718. Thanks.

Too late for 51, marking this fix-optional for 52/53 since we haven't yet found someone to work on this regression.

(In reply to :Gijs from comment #3) > (In reply to David Baron :dbaron: ⌚️UTC-8 from comment #2) > > The basic problem here is that the code added in bug 565718, i.e., > > https://hg.mozilla.org/mozilla-central/rev/ef0801fdc7ab , doesn't interact > > with the image document code in > > https://searchfox.org/mozilla-central/source/dom/html/ImageDocument.cpp . > > It looks like the image document code ends up firing a custom event from the > document viewer here: > > https://searchfox.org/mozilla-central/rev/ > 904bf9addd03b03d4cad11b82f19f43d875b7f27/layout/base/nsDocumentViewer. > cpp#3083-3088 > > that frontend can listen to. Gijs, should this be as simple as adding: > aWindow.document.addEventListener("FullZoomChange", e => { > updateZoomButton(e.target, "browser-fullZoom:zoomChange"); > }); to URLBarZoom.init? I tried that but it's not getting hit in the debugger.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #9) > (In reply to :Gijs from comment #3) > > (In reply to David Baron :dbaron: ⌚️UTC-8 from comment #2) > > > The basic problem here is that the code added in bug 565718, i.e., > > > https://hg.mozilla.org/mozilla-central/rev/ef0801fdc7ab , doesn't interact > > > with the image document code in > > > https://searchfox.org/mozilla-central/source/dom/html/ImageDocument.cpp . > > > > It looks like the image document code ends up firing a custom event from the > > document viewer here: > > > > https://searchfox.org/mozilla-central/rev/ > > 904bf9addd03b03d4cad11b82f19f43d875b7f27/layout/base/nsDocumentViewer. > > cpp#3083-3088 > > > > that frontend can listen to. > > Gijs, should this be as simple as adding: > > aWindow.document.addEventListener("FullZoomChange", e => { > > updateZoomButton(e.target, "browser-fullZoom:zoomChange"); > > }); > to URLBarZoom.init? I tried that but it's not getting hit in the debugger. The event will be in the content process, presumably? So the listener would need to be, too.

So, this patch *should* work, but win.ZoomManager.zoom doesn't always return the expected value. In fact, with this patch it never returns 100 unless I step the debugger through here, allowing enough time for the viewZoomOverlay.js's getZoomForBrowser -> remote-browser.xml's fullZoom to update. This seems very similar to bug 1310758. Gijs, do you have any ideas of what we can do to work around this? Preferably something quick-and-dirty because I don't have the time to spend working on a rewrite ;)

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #12) > So, this patch *should* work, but win.ZoomManager.zoom doesn't always return > the expected value. In fact, with this patch it never returns 100 unless I > step the debugger through here, allowing enough time for the > viewZoomOverlay.js's getZoomForBrowser -> remote-browser.xml's fullZoom to > update. This seems very similar to bug 1310758. > > Gijs, do you have any ideas of what we can do to work around this? > Preferably something quick-and-dirty because I don't have the time to spend > working on a rewrite ;) in the event listener, from looking at the code, I would expect that: docShell.contentViewer.fullZoom gives you the right value (1-based rather than 100-based). AIUI you should be able to pass it up from there and remove the synthetic document guard and use only that notification to update the zoom indicators for loads (as long as you cache per tab and update after tab switches). Of course, I could be wrong. :-)

(In reply to :Gijs from comment #13) > AIUI you should be able to pass it up from there and remove the synthetic > document guard and use only that notification to update the zoom indicators > for loads (as long as you cache per tab and update after tab switches). Of > course, I could be wrong. :-) Yeah, I agree that this is the better route to go for updating zoom values, but I don't have the time right now to rewrite our code and deal with potential regressions. The patch I'm attaching uses your feedback but only fixes this specific bug.

Comment on attachment 8838195 [details] Bug 1318830 - Listen for the FullZoomChange event on synthetic documents to trigger updating the zoom-control in the location bar. https://reviewboard.mozilla.org/r/113160/#review114666 ::: browser/modules/URLBarZoom.jsm:45 (Diff revision 2) > if (zoomFactor != 100) { > // Check if zoom button is visible and update label if it is > if (zoomResetButton.hidden) { > zoomResetButton.hidden = false; > } > // Only allow pulse animation for zoom changes, not tab switching Seems this comment should move to inside the observe function. Also, can we ensure somehow that the value is actually changing here? If it doesn't, and we get called, we should ideally not update. That's a bug that looks like it existed before, though (e.g. if you navigate from a page with 120% zoom to another page with 120% zoom) so no big deal if that's hard to do (followup for that and the other stuff we discussed, though).

Comment on attachment 8838195 [details] Bug 1318830 - Listen for the FullZoomChange event on synthetic documents to trigger updating the zoom-control in the location bar. https://reviewboard.mozilla.org/r/113160/#review114666 > Seems this comment should move to inside the observe function. > > Also, can we ensure somehow that the value is actually changing here? If it doesn't, and we get called, we should ideally not update. That's a bug that looks like it existed before, though (e.g. if you navigate from a page with 120% zoom to another page with 120% zoom) so no big deal if that's hard to do (followup for that and the other stuff we discussed, though). Err, s/update/*animate*/ being the operative word here. Sigh.

Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7dd9d61b5756 Listen for the FullZoomChange event on synthetic documents to trigger updating the zoom-control in the location bar. r=Gijs

(In reply to :Gijs from comment #16) > Also, can we ensure somehow that the value is actually changing here? If it > doesn't, and we get called, we should ideally not update. That's a bug that > looks like it existed before, though (e.g. if you navigate from a page with > 120% zoom to another page with 120% zoom) so no big deal if that's hard to > do (followup for that and the other stuff we discussed, though). I couldn't find a way to get this to occur so I didn't add code for it. I tried having two websites open both zoomed to 120% and then using the same tab to navigate from one to the other. There wasn't any animation. I did the same test with image documents.

[bugday-20170308] Status-firefox52: NOT FIXED Target Milestone : Firefox 55

[bugday-20170308] Status-firefox52: NOT FIXED Target Milestone : Firefox 55 Attachment : GIF has been attached reproduce the issue.

[bugday-20170308] Status-firefox55: NOT FIXED Attachment : GIF has been attached reproduce the issue. BuildID : 20170307030205

Jared, this doesn't look to have helped on Nightly. Or maybe it was broken by other changes in this area? On Nightly, I don't see any URL bar zoom indicator anymore when changing the zoom level using accel-mousewheel or the keyboard shortcuts.

(In reply to Madhuri from comment #25) > BuildID : 20170307030205 (In reply to :Gijs from comment #26) > Jared, this doesn't look to have helped on Nightly. Or maybe it was broken > by other changes in this area? > > On Nightly, I don't see any URL bar zoom indicator anymore when changing the > zoom level using accel-mousewheel or the keyboard shortcuts. The BuildID from comment 25 wouldn't have contained this fix as it wasn't merged to m-c until later that day, no?

This is working for me on Nightly 55.0a1 (2017-03-08) (64-bit) on Windows 10. I just confirmed by testing with https://msujaws.files.wordpress.com/2014/05/floating-head.png?w=75. accel-mousewheel and keyboard shortcuts get the zoom indicator to appear, and clicking on the image resets the zoom level to 100%. It sounds like RyanVM figured out why you weren't seeing it.

Madhuri, can you please confirm that this is fixed for you in today's nightly?

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #28) > This is working for me on Nightly 55.0a1 (2017-03-08) (64-bit) on Windows > 10. I just confirmed by testing with > https://msujaws.files.wordpress.com/2014/05/floating-head.png?w=75. > accel-mousewheel and keyboard shortcuts get the zoom indicator to appear, > and clicking on the image resets the zoom level to 100%. > > It sounds like RyanVM figured out why you weren't seeing it. Oh, yeah, so the additional confusing thing was that we don't show the in-URL-bar thing if you customize the control to the toolbar, and I'd forgotten that subtlety... Sorry for the confusion!

(In reply to Ryan VanderMeulen [:RyanVM] from comment #29) > Madhuri, can you please confirm that this is fixed for you in today's > nightly? Status-firefox-55.0a1 : NOT FIXED. Sure. With the latest Nightly updates available today, I can still see the issue if "zoom-in/zoom-in" option is used from toolbar. NOTE- you can refer the GIF attached in my comment#24, the issue can be reproduced with mentioned steps in the same. Details are - BuildID : 20170307030205 firefox-55.0a1(2017-03-08)(32 bit) OS: windows 10 Pro (64 bit)

20170308030207(In reply to Madhuri from comment #31) > (In reply to Ryan VanderMeulen [:RyanVM] from comment #29) > > Madhuri, can you please confirm that this is fixed for you in today's > > nightly? > > Status-firefox-55.0a1 : NOT FIXED. > > Sure. With the latest Nightly updates available today, I can still see the > issue if "zoom-in/zoom-in" option is used from toolbar. > > NOTE- you can refer the GIF attached in my comment#24, #32, the issue can be > reproduced with mentioned steps in the same. > > Details are - > > BuildID : 20170308030207 > > firefox-55.0a1(2017-03-08)(32 bit) > > OS: windows 10 Pro (64 bit)

(In reply to Ryan VanderMeulen [:RyanVM] from comment #29) > Madhuri, can you please confirm that this is fixed for you in today's > nightly? Sorry !! Build ID is 20170308030207.

I can reproduce what Madhuri is seeing on my win10 machine.

Okay, I can add a note to bug 1345375 about this not working if the zoom controls have been moved to the location bar. We need to update CustomizableWidgets.jsm too to listen for this.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #35) > Okay, I can add a note to bug 1345375 about this not working if the zoom > controls have been moved to the location bar. We need to update > CustomizableWidgets.jsm too to listen for this. Thanks! Marking this verified with the caveat that bug 1345375 will fix the 'full zoom control in the toolbar' case.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #35) > Okay, I can add a note to bug 1345375 about this not working if the zoom > controls have been moved to the location bar. We need to update > CustomizableWidgets.jsm too to listen for this. okay !! We will follow bug 1345375 for this. Thanks!! (In reply to :Gijs from comment #34) > I can reproduce what Madhuri is seeing on my win10 machine. Thanks!!

Are we ready to consider uplift requests here or did we want to wait for bug 1345375 as well?

(In reply to Ryan VanderMeulen [:RyanVM] from comment #38) > Are we ready to consider uplift requests here or did we want to wait for bug > 1345375 as well? We should wait for bug 1345375 but it just landed on autoland a couple hours ago. If we can wait another day or two then it will have gotten to Nightly and we can verify that bug through manual QA. At which point we can begin the uplift process.

Comment on attachment 8838195 [details] Bug 1318830 - Listen for the FullZoomChange event on synthetic documents to trigger updating the zoom-control in the location bar. Approval Request Comment [Feature/Bug causing the regression]: regression from bug 565718 [User impact if declined]: the zoom control may not update when viewing images directly [Is this code covered by automated tests?]: no [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: bug 1345375 should land on top of this [Is the change risky?]: no [Why is the change risky/not risky?]: changes are limited to the zoom control and have been on mozilla-central for multiple weeks now [String changes made/needed]:

[String changes made/needed]: none

Comment on attachment 8838195 [details] Bug 1318830 - Listen for the FullZoomChange event on synthetic documents to trigger updating the zoom-control in the location bar. Fix a zoom control regression and was verified. Aurora54+.

[Tracking Requested - why for this release]: Moving from + to ?, I would prefer that we *not* uplift this to 53beta since we're only two weeks away from merge date and the issue is not so severe (only the UI doesn't update but the image zoom does change).

Is this worth taking on ESR52 or should we let it ride the 54 train?

[bugday-20170419] Status: Fixed and verified Browser: Mozilla/5.0 (X11; Linux x86_64; rv:54.0) Gecko/20100101 Firefox/54.0 The issue is no longer reproducible on Firefox Aurora Tests were performed in Ubuntu 16.04.2 x64

We should let this ride the trains, it wasn't reported often and is low severity and low occurrence.