1345375 - Use the FullZoomChange event to update zoom controls

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Dão Gottwald [:dao]]

Bug 1318830 implemented a URLBarZoom object in browser-content.js that dispatches a SyntheticDocument:ZoomChange message to support URLBarZoom.jsm. However, the browser-content.js code isn't really concerned with the URL bar -- and it shouldn't be, because this is in toolkit/. If we think the message is generally useful, we should rename the URLBarZoom object. Otherwise we should rename the message and move this code to browser/.

needinfo from jaws to figure out if this was supposed to be a general-purpose message.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1345375 - Use remote-browser.xml's FullZoomChange event to simplify zoom ui-controls as well as correctly update the zoom ui-controls on ImageDocuments.

Review commit: https://reviewboard.mozilla.org/r/118272/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118272/

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 8845054 [details]
Bug 1345375 - Use remote-browser.xml's FullZoomChange event to simplify zoom ui-controls as well as correctly update the zoom ui-controls on ImageDocuments.

https://reviewboard.mozilla.org/r/118272/#review120158

Apparently remote-browser.xml already forwards the FullZoomChange event. Shouldn't we make use of that?

::: browser/modules/URLBarZoom.jsm:85
(Diff revision 1)
>  }
>  
>  Services.obs.addObserver(fullZoomObserver, "browser-fullZoom:zoomChange", false);
>  Services.obs.addObserver(fullZoomObserver, "browser-fullZoom:zoomReset", false);
>  Services.obs.addObserver(fullZoomObserver, "browser-fullZoom:location-change", false);
> -Services.mm.addMessageListener("SyntheticDocument:ZoomChange", function(aMessage) {
> +Services.mm.addMessageListener("browser-fullZoom:zoomChange", function(aMessage) {

This message name is identical to that other notification name, which seems rather confusing.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Dão Gottwald [:dao] from comment #2)
> Comment on attachment 8845054 [details]
> Bug 1345375 - Move and rename the full zoom listener for synthetic documents
> to /browser since it is not needed in toolkit.
> 
> https://reviewboard.mozilla.org/r/118272/#review120158
> 
> Apparently remote-browser.xml already forwards the FullZoomChange event.
> Shouldn't we make use of that?

remote-browser.xml converts the FullZoomChange *message* to the FullZoom event, which is why bug 1318830 was necessary.

> 
> ::: browser/modules/URLBarZoom.jsm:85
> (Diff revision 1)
> >  }
> >  
> >  Services.obs.addObserver(fullZoomObserver, "browser-fullZoom:zoomChange", false);
> >  Services.obs.addObserver(fullZoomObserver, "browser-fullZoom:zoomReset", false);
> >  Services.obs.addObserver(fullZoomObserver, "browser-fullZoom:location-change", false);
> > -Services.mm.addMessageListener("SyntheticDocument:ZoomChange", function(aMessage) {
> > +Services.mm.addMessageListener("browser-fullZoom:zoomChange", function(aMessage) {
> 
> This message name is identical to that other notification name, which seems
> rather confusing.

I chose this name because it has very similar behavior to that of browser-fullZoom:zoomChange. Should we change this to browser-fullZoom:zoomChangeWithValue ?

Comment 4

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)
> (In reply to Dão Gottwald [:dao] from comment #2)
> > Comment on attachment 8845054 [details]
> > Bug 1345375 - Move and rename the full zoom listener for synthetic documents
> > to /browser since it is not needed in toolkit.
> > 
> > https://reviewboard.mozilla.org/r/118272/#review120158
> > 
> > Apparently remote-browser.xml already forwards the FullZoomChange event.
> > Shouldn't we make use of that?
> 
> remote-browser.xml converts the FullZoomChange *message* to the FullZoom
> event, which is why bug 1318830 was necessary.

And the message comes from the original event, so it's event -> message -> event. Anyway, I don't see your point. Why can't we listen to the forwarded event here?

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Note, while working on this we need to update CustomizableWidgets.jsm to listen for the same event/message in case the zoom-controls have been moved to the location bar.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8845054 [details]
Bug 1345375 - Use remote-browser.xml's FullZoomChange event to simplify zoom ui-controls as well as correctly update the zoom ui-controls on ImageDocuments.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118272/diff/1-2/

Comment 7

[Madhuri]

Attachment: 1318830_Bugday-20170308.gif

FOLLOW UP of bug 1318830

Status-firefox-55.0a1 : NOT FIXED.

With the latest Nightly updates available,I can still see the issue if "zoom-in/zoom-in" option is used from toolbar.

NOTE- you can refer the GIF attached so that 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)

Comment 8

[Madhuri]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> Note, while working on this we need to update CustomizableWidgets.jsm to
> listen for the same event/message in case the zoom-controls have been moved
> to the location bar.

Thanks for adding updates to be done as to follow bug 131880. I have attached the details and GIF file in my  comment#7 so that it will easily approachable what i found there.

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 8845054 [details]
Bug 1345375 - Use remote-browser.xml's FullZoomChange event to simplify zoom ui-controls as well as correctly update the zoom ui-controls on ImageDocuments.

https://reviewboard.mozilla.org/r/118272/#review120912

Can you please file a followup on removing the browser-fullZoom:zoomReset and browser-fullZoom:zoomChange notifications? (They were added in bug 869933.)

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 8845054 [details]
Bug 1345375 - Use remote-browser.xml's FullZoomChange event to simplify zoom ui-controls as well as correctly update the zoom ui-controls on ImageDocuments.

https://reviewboard.mozilla.org/r/118272/#review120918

::: browser/modules/URLBarZoom.jsm:31
(Diff revision 2)
>    // window is closing, we can just ignore this notification.
>    if (!aSubject.ownerGlobal) {
>      return;
>    }
>  
>    let animate = (aTopic != "browser-fullZoom:location-change");

Oh wait, this check makes no sense anymore. aTopic is always "browser-fullZoom:location-change" here.

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 8845054 [details]
Bug 1345375 - Use remote-browser.xml's FullZoomChange event to simplify zoom ui-controls as well as correctly update the zoom ui-controls on ImageDocuments.

https://reviewboard.mozilla.org/r/118272/#review120966

::: browser/modules/URLBarZoom.jsm:68
(Diff revision 2)
>      return;
>    }
>  
> -  let zoomFactor = Math.round((aValue || win.ZoomManager.zoom) * 100);
> +  let zoomFactor = Math.round(win.ZoomManager.zoom * 100);
>    if (zoomFactor != 100) {
>      // Check if zoom button is visible and update label if it is

While you're at it, can you remove or fix this misleading comment? This code doesn't care whether or not the button is visible (you can in fact remove that check too), it just always makes it visible.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Dão Gottwald [::dao] from comment #9)
> Comment on attachment 8845054 [details]
> Bug 1345375 - Use remote-browser.xml's FullZoomChange event to simplify zoom
> ui-controls as well as correctly update the zoom ui-controls on
> ImageDocuments.
> 
> https://reviewboard.mozilla.org/r/118272/#review120912
> 
> Can you please file a followup on removing the browser-fullZoom:zoomReset
> and browser-fullZoom:zoomChange notifications? (They were added in bug
> 869933.)

Yeah, I'll file a follow-up for this. It touches a few more files and I don't want to keep growing this patch.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1345375 - Use the FullZoomChange event instead of browser-fullZoom observers since FullZoomChange works on MediaDocuments.

Review commit: https://reviewboard.mozilla.org/r/119704/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/119704/

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 8846696 [details]
Bug 1345375 - Use the FullZoomChange event instead of browser-fullZoom observers since FullZoomChange works on MediaDocuments.

https://reviewboard.mozilla.org/r/119704/#review121544

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8bd3a85551ae
Use the FullZoomChange event instead of browser-fullZoom observers since FullZoomChange works on MediaDocuments. r=dao

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing browser_urlBar_zoom.js:

https://hg.mozilla.org/integration/autoland/rev/1ffab75a744e49b1602547292d0d12f97125d5f8

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=8bd3a85551ae943c8de2cff72decad0e3a0563ee&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=83449203&repo=autoland

[task 2017-03-13T19:21:11.009163Z] 19:21:11     INFO - TEST-START | browser/modules/test/browser/browser_urlBar_zoom.js
[task 2017-03-13T19:21:11.056132Z] 19:21:11     INFO - GECKO(1930) | JavaScript error: resource://app/modules/URLBarZoom.jsm, line 51: TypeError: win.gBrowser is undefined
[task 2017-03-13T19:21:11.134634Z] 19:21:11     INFO - GECKO(1930) | JavaScript error: resource://app/modules/URLBarZoom.jsm, line 51: TypeError: win.gBrowser is undefined
[task 2017-03-13T19:21:11.158373Z] 19:21:11     INFO - TEST-INFO | started process screentopng
[task 2017-03-13T19:21:12.542863Z] 19:21:12     INFO - TEST-INFO | screentopng: exit 0
[task 2017-03-13T19:21:12.543295Z] 19:21:12     INFO - Buffered messages logged at 19:21:11
[task 2017-03-13T19:21:12.546627Z] 19:21:12     INFO - Entering test bound 
[task 2017-03-13T19:21:12.548281Z] 19:21:12     INFO - Confirm whether the browser zoom is set to the default level
[task 2017-03-13T19:21:12.551359Z] 19:21:12     INFO - TEST-PASS | browser/modules/test/browser/browser_urlBar_zoom.js | Page zoom is set to default (100%) - 
[task 2017-03-13T19:21:12.552718Z] 19:21:12     INFO - TEST-PASS | browser/modules/test/browser/browser_urlBar_zoom.js | Zoom reset button is currently hidden - 
[task 2017-03-13T19:21:12.553059Z] 19:21:12     INFO - Change zoom and confirm zoom button appears
[task 2017-03-13T19:21:12.554514Z] 19:21:12     INFO - Console message: [JavaScript Error: "TypeError: win.gBrowser is undefined" {file: "resource://app/modules/URLBarZoom.jsm" line: 51}]
[task 2017-03-13T19:21:12.554898Z] 19:21:12     INFO - Zoom increased to 110%
[task 2017-03-13T19:21:12.556992Z] 19:21:12     INFO - TEST-PASS | browser/modules/test/browser/browser_urlBar_zoom.js | Zoom reset button is now visible - 
[task 2017-03-13T19:21:12.558910Z] 19:21:12     INFO - TEST-PASS | browser/modules/test/browser/browser_urlBar_zoom.js | Button label updated successfully to 110% - 
[task 2017-03-13T19:21:12.563092Z] 19:21:12     INFO - TEST-PASS | browser/modules/test/browser/browser_urlBar_zoom.js | Clicking zoom button successfully resets browser zoom to 100% - 
[task 2017-03-13T19:21:12.565040Z] 19:21:12     INFO - Buffered messages finished
[task 2017-03-13T19:21:12.568340Z] 19:21:12     INFO - TEST-UNEXPECTED-FAIL | browser/modules/test/browser/browser_urlBar_zoom.js | Zoom reset button returns to being hidden - Got false, expected true
[task 2017-03-13T19:21:12.570402Z] 19:21:12     INFO - Stack trace:
[task 2017-03-13T19:21:12.572457Z] 19:21:12     INFO -     chrome://mochikit/content/browser-test.js:test_is:911
[task 2017-03-13T19:21:12.574793Z] 19:21:12     INFO -     chrome://mochitests/content/browser/browser/modules/test/browser/browser_urlBar_zoom.js:null:33
[task 2017-03-13T19:21:12.578753Z] 19:21:12     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:735:9
[task 2017-03-13T19:21:12.581262Z] 19:21:12     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:655:7
[task 2017-03-13T19:21:12.588032Z] 19:21:12     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:791:59

Comment 17

[Dão Gottwald [:dao]]

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #16)
> JavaScript error: resource://app/modules/URLBarZoom.jsm, line 51: TypeError:
> win.gBrowser is undefined

That's weird and disturbing. URLBarZoom.init is only called from browser windows where win.gBrowser should never be undefined.

Btw, since I just looked at the patch again: Please group the addEventListener call for FullZoomChange with that one for EndSwapDocShells.

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8846696 [details]
Bug 1345375 - Use the FullZoomChange event instead of browser-fullZoom observers since FullZoomChange works on MediaDocuments.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119704/diff/1-2/

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/56b09fa0748c
Use the FullZoomChange event instead of browser-fullZoom observers since FullZoomChange works on MediaDocuments. r=dao

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Dão Gottwald [::dao] from comment #17)
> (In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on
> intermittent or backout) from comment #16)
> > JavaScript error: resource://app/modules/URLBarZoom.jsm, line 51: TypeError:
> > win.gBrowser is undefined
> 
> That's weird and disturbing. URLBarZoom.init is only called from browser
> windows where win.gBrowser should never be undefined.
> 
> Btw, since I just looked at the patch again: Please group the
> addEventListener call for FullZoomChange with that one for EndSwapDocShells.

Sorry, I didn't see your message here before landing. I'll file a follow-up and post a patch there.

Comment 21

[Dão Gottwald [:dao]]

What's up with the browser.xml change that apparently nobody reviewed?

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

Sorry, I should have requested review from you on that. I had to add it as the event was only being dispatched in remote-browser.xml but not in browser.xml, and thus failing browser_urlBar_zoom.js when run in non-e10s mode. Are you OK with that change or should I back it out?

Comment 23

[Dão Gottwald [:dao]]

I don't understand this offhand. remote-browser.xul doesn't introduce this event, it only forwards it. So yes, this should be backed out.

Comment 24

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0cc9dced786c
Backed out changeset 56b09fa0748c on request from dao

Comment 25

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Dão Gottwald [::dao] from comment #23)
> I don't understand this offhand. remote-browser.xul doesn't introduce this
> event, it only forwards it. So yes, this should be backed out.

Can you point me at the code where it forwards this event? I'm looking at these two places:

1) http://searchfox.org/mozilla-central/rev/ca7015fa45b30b29176fbaa70ba0a36fe9263c38/toolkit/content/widgets/remote-browser.xml#199

At this place, the "FullZoomChange" event is dispatched when the _fullZoom property is changed.

2) http://searchfox.org/mozilla-central/rev/ca7015fa45b30b29176fbaa70ba0a36fe9263c38/toolkit/content/widgets/remote-browser.xml#489

At this place, when the "FullZoomChange" message is received, a "FullZoomChange" event is created and then dispatched.

Comment 26

[Jared Wein [:jaws] (please needinfo? me)]

Okay, you must be talking about http://searchfox.org/mozilla-central/rev/ca7015fa45b30b29176fbaa70ba0a36fe9263c38/layout/base/nsDocumentViewer.cpp#3107. There are some subtle differences between if the event is coming form nsDocumentViewer.cpp or from remote-browser.xml, I'll try to work through them.

Comment 27

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #25)
> (In reply to Dão Gottwald [::dao] from comment #23)
> > I don't understand this offhand. remote-browser.xul doesn't introduce this
> > event, it only forwards it. So yes, this should be backed out.
> 
> Can you point me at the code where it forwards this event? I'm looking at
> these two places:

> 2)
> http://searchfox.org/mozilla-central/rev/
> ca7015fa45b30b29176fbaa70ba0a36fe9263c38/toolkit/content/widgets/remote-
> browser.xml#489
> 
> At this place, when the "FullZoomChange" message is received, a
> "FullZoomChange" event is created and then dispatched.

The FullZoomChange message originates from the content event. This is what I mean when I say the event gets forwarded (as already clarified in comment 4).

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #25)
> 1)
> http://searchfox.org/mozilla-central/rev/
> ca7015fa45b30b29176fbaa70ba0a36fe9263c38/toolkit/content/widgets/remote-
> browser.xml#199
> 
> At this place, the "FullZoomChange" event is dispatched when the _fullZoom
> property is changed.

*sigh* I guess I don't understand this either. Won't setting fullZoom make nsDocumentViewer.cpp dispatch the event again, so now we get two events?

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #26)
> Okay, you must be talking about
> http://searchfox.org/mozilla-central/rev/
> ca7015fa45b30b29176fbaa70ba0a36fe9263c38/layout/base/nsDocumentViewer.
> cpp#3107. There are some subtle differences between if the event is coming
> form nsDocumentViewer.cpp or from remote-browser.xml, I'll try to work
> through them.

I think you may need to use a capturing listener.

Comment 28

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8846696 [details]
Bug 1345375 - Use the FullZoomChange event instead of browser-fullZoom observers since FullZoomChange works on MediaDocuments.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119704/diff/2-3/

Comment 29

[Dão Gottwald [:dao]]

Comment on attachment 8846696 [details]
Bug 1345375 - Use the FullZoomChange event instead of browser-fullZoom observers since FullZoomChange works on MediaDocuments.

https://reviewboard.mozilla.org/r/119704/#review122614

::: browser/modules/URLBarZoom.jsm:39
(Diff revision 3)
>  function onEndSwapDocShells(event) {
>    updateZoomButton(event.originalTarget);
>  }
>  
> +function onFullZoomChange(event) {
> +  if (Services.appinfo.browserTabsRemoteAutostart) {

Since we can have a mix of remote and non-remote browsers, I think you need to actually check what the event target is here instead.

Comment 30

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [::dao] from comment #27)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #25)
> > 1)
> > http://searchfox.org/mozilla-central/rev/
> > ca7015fa45b30b29176fbaa70ba0a36fe9263c38/toolkit/content/widgets/remote-
> > browser.xml#199
> > 
> > At this place, the "FullZoomChange" event is dispatched when the _fullZoom
> > property is changed.
> 
> *sigh* I guess I don't understand this either. Won't setting fullZoom make
> nsDocumentViewer.cpp dispatch the event again, so now we get two events?

Unless you can make sense of this, we should also file a followup on removing this.

Comment 31

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Dão Gottwald [::dao] from comment #30)
> > *sigh* I guess I don't understand this either. Won't setting fullZoom make
> > nsDocumentViewer.cpp dispatch the event again, so now we get two events?
> 
> Unless you can make sense of this, we should also file a followup on
> removing this.

I can't make sense of it either. I can get two events to fire on about:preferences, but not on about:config or on http://webdev.cse.msu.edu/~beachjar/capstone/demo.html. I'll file a follow-up.

Comment 32

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8846696 [details]
Bug 1345375 - Use the FullZoomChange event instead of browser-fullZoom observers since FullZoomChange works on MediaDocuments.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119704/diff/3-4/

Comment 33

[Dão Gottwald [:dao]]

Comment on attachment 8846696 [details]
Bug 1345375 - Use the FullZoomChange event instead of browser-fullZoom observers since FullZoomChange works on MediaDocuments.

https://reviewboard.mozilla.org/r/119704/#review122626

::: browser/modules/URLBarZoom.jsm:39
(Diff revision 4)
>  function onEndSwapDocShells(event) {
>    updateZoomButton(event.originalTarget);
>  }
>  
> +function onFullZoomChange(event) {
> +  if (event.target.localName == "tabbrowser") {

Does this give you a strict warning if event.target is the document (and thus event.target.localName is undefined)?
You could probably check nodeType instead.

::: browser/modules/URLBarZoom.jsm:48
(Diff revision 4)
> +    // so we need to jump through some hoops to get to the <xul:browser>.
> +    let gBrowser = event.currentTarget.gBrowser;
> +    let topDoc = event.target.defaultView.top.document;
> +    let browser = gBrowser.getBrowserForDocument(topDoc);
> +    updateZoomButton(browser, true);
> +  }

I'd prefer cutting this down to one updateZoomButton call:

let browser;
if (...) {
  browser = event.originalTarget;
} else {
  ...
  browser = ...;
}
updateZoomButton(browser, true);

Comment 34

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8846696 [details]
Bug 1345375 - Use the FullZoomChange event instead of browser-fullZoom observers since FullZoomChange works on MediaDocuments.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119704/diff/4-5/

Comment 35

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [::dao] from comment #33)
> Comment on attachment 8846696 [details]
> Bug 1345375 - Use the FullZoomChange event instead of browser-fullZoom
> observers since FullZoomChange works on MediaDocuments.
> 
> https://reviewboard.mozilla.org/r/119704/#review122626
> 
> ::: browser/modules/URLBarZoom.jsm:39
> (Diff revision 4)
> >  function onEndSwapDocShells(event) {
> >    updateZoomButton(event.originalTarget);
> >  }
> >  
> > +function onFullZoomChange(event) {
> > +  if (event.target.localName == "tabbrowser") {
> 
> Does this give you a strict warning if event.target is the document (and
> thus event.target.localName is undefined)?
> You could probably check nodeType instead.

Comment 36

[Jared Wein [:jaws] (please needinfo? me)]

I don't see any strict warnings in the Browser Console but I'll upload a new patch that uses nodeType.

Comment 37

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8846696 [details]
Bug 1345375 - Use the FullZoomChange event instead of browser-fullZoom observers since FullZoomChange works on MediaDocuments.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119704/diff/5-6/

Comment 38

[Dão Gottwald [:dao]]

Comment on attachment 8846696 [details]
Bug 1345375 - Use the FullZoomChange event instead of browser-fullZoom observers since FullZoomChange works on MediaDocuments.

https://reviewboard.mozilla.org/r/119704/#review122726

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e263ffa2eb29
Use the FullZoomChange event instead of browser-fullZoom observers since FullZoomChange works on MediaDocuments. r=dao

Comment 40

[Jared Wein [:jaws] (please needinfo? me)]

For QA manual testing, please verify that zooming in and out (via ctrl+, ctrl-) and resetting the zoom
(ctrl 0) all update the zoom control in the location bar. The zoom control should hide when the value is equal to 100% (and also when it is reset, which sets it back to 100%).

The zoom control in the location bar shouldn't appear if the zoom-controls have been moved from the Firefox menu to the navigation toolbar. In all of the cases above the zoom-controls should update with the current zoom value.

This should all be tested on the following pages:
about:preferences
about:newtab
https://www.mozilla.org

Another way to adjust the zoom value is ctrl+mousewheel or through the View menu (Alt-V on Windows) > Zoom > Zoom In / Zoom Out / Reset.

Comment 41

[Madhuri]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #40)
> For QA manual testing, please verify that zooming in and out (via ctrl+,
> ctrl-) and resetting the zoom
> (ctrl 0) all update the zoom control in the location bar. The zoom control
> should hide when the value is equal to 100% (and also when it is reset,
> which sets it back to 100%).
> 
> The zoom control in the location bar shouldn't appear if the zoom-controls
> have been moved from the Firefox menu to the navigation toolbar. In all of
> the cases above the zoom-controls should update with the current zoom value.
> 
> This should all be tested on the following pages:
> about:preferences
> about:newtab
> https://www.mozilla.org
> 
> Another way to adjust the zoom value is ctrl+mousewheel or through the View
> menu (Alt-V on Windows) > Zoom > Zoom In / Zoom Out / Reset.

Other than follow up of bug 1318830, all above cases have been verified with the latest updates.

verified on buildID : 20170315030215

Comment 42

[Dão Gottwald [:dao]]

(In reply to Madhuri from comment #41)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #40)
> > For QA manual testing, please verify that zooming in and out (via ctrl+,
> > ctrl-) and resetting the zoom
> > (ctrl 0) all update the zoom control in the location bar. The zoom control
> > should hide when the value is equal to 100% (and also when it is reset,
> > which sets it back to 100%).
> > 
> > The zoom control in the location bar shouldn't appear if the zoom-controls
> > have been moved from the Firefox menu to the navigation toolbar. In all of
> > the cases above the zoom-controls should update with the current zoom value.
> > 
> > This should all be tested on the following pages:
> > about:preferences
> > about:newtab
> > https://www.mozilla.org
> > 
> > Another way to adjust the zoom value is ctrl+mousewheel or through the View
> > menu (Alt-V on Windows) > Zoom > Zoom In / Zoom Out / Reset.
> 
> Other than follow up of bug 1318830, all above cases have been verified with
> the latest updates.
> 
> verified on buildID : 20170315030215

Your build doesn't contain this patch which hasn't been merged to mozilla-central yet.

Comment 43

[Madhuri]

(In reply to Dão Gottwald [::dao] from comment #42)
> (In reply to Madhuri from comment #41)
> > (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #40)
> > > For QA manual testing, please verify that zooming in and out (via ctrl+,
> > > ctrl-) and resetting the zoom
> > > (ctrl 0) all update the zoom control in the location bar. The zoom control
> > > should hide when the value is equal to 100% (and also when it is reset,
> > > which sets it back to 100%).
> > > 
> > > The zoom control in the location bar shouldn't appear if the zoom-controls
> > > have been moved from the Firefox menu to the navigation toolbar. In all of
> > > the cases above the zoom-controls should update with the current zoom value.
> > > 
> > > This should all be tested on the following pages:
> > > about:preferences
> > > about:newtab
> > > https://www.mozilla.org
> > > 
> > > Another way to adjust the zoom value is ctrl+mousewheel or through the View
> > > menu (Alt-V on Windows) > Zoom > Zoom In / Zoom Out / Reset.
> > 
> > Other than follow up of bug 1318830, all above cases have been verified with
> > the latest updates.
> > 
> > verified on buildID : 20170315030215
> 
> Your build doesn't contain this patch which hasn't been merged to
> mozilla-central yet.

But i can see the scenarios to be fixed until zoom-in/out option is moved to toolbar(as issue mentioned in bug 1318830). Anyways, i will verify it once again when the patch will be merged.

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e263ffa2eb29

Comment 45

[Gerry Chang [:gchang]]

Hi :jaws,
From bug 1089246, FF53 & 54 are also affected, do you think it's worth uplifting to 54 at least?

Comment 46

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Gerry Chang [:gchang] from comment #45)
> Hi :jaws,
> From bug 1089246, FF53 & 54 are also affected, do you think it's worth
> uplifting to 54 at least?

Yes, I am waiting for this bug to get verified by QA before requesting uplift. We will need to uplift bug 1318830 as well.

@RyanVM, can you find someone to verify this? See comment 40 for str.

Comment 47

[Madhuri]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #46)
> (In reply to Gerry Chang [:gchang] from comment #45)
> > Hi :jaws,
> > From bug 1089246, FF53 & 54 are also affected, do you think it's worth
> > uplifting to 54 at least?
> 
> Yes, I am waiting for this bug to get verified by QA before requesting
> uplift. We will need to uplift bug 1318830 as well.
> 
> @RyanVM, can you find someone to verify this? See comment 40 for str.

The bugs seems to be fixed now.

Comment 48

[Madhuri]

[bugday-20170322] 

Status-ff55: verified and fixed

BuildID: 20170321030211 [ 55.0a1 (2017-03-21)(32 bit)

OS : Win 10(X64)

Comment 49

[Ciprian Muresan [:cmuresan], Ecosystem QA]

User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID 	20170321030211

I have verified this bug on Windows 10 x64, Mac OS 10.12 and Ubuntu 14.04 x64 and the zoom levels are correctly represented in the URL bar. 

I do have a question though. I haven't observed any visual differences between Build ID 20170321030211 and Build ID 20170315030215. 
Jared, was the verification meant to see if anything broke because of the work done in this bug? 

Do you want me to test anything else?

Comment 50

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Ciprian Muresan [:cmuresan] from comment #49)
> User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0)
> Gecko/20100101 Firefox/55.0
> Build ID 	20170321030211
> 
> I have verified this bug on Windows 10 x64, Mac OS 10.12 and Ubuntu 14.04
> x64 and the zoom levels are correctly represented in the URL bar. 
> 
> I do have a question though. I haven't observed any visual differences
> between Build ID 20170321030211 and Build ID 20170315030215. 
> Jared, was the verification meant to see if anything broke because of the
> work done in this bug? 

Yes, there should have been no visual change between these two builds. The testing here was to verify that nothing broke due to the work in this bug.
 
> Do you want me to test anything else?

Nope your testing is sufficient. Thanks!

Comment 51

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8846696 [details]
Bug 1345375 - Use the FullZoomChange event instead of browser-fullZoom observers since FullZoomChange works on MediaDocuments.

Approval Request Comment
[Feature/Bug causing the regression]: regression from bug 565718
[User impact if declined]: code cleanup from bug 1318830. the combined changes between bug 1318830 and this bug have been verified by QA, though there is no user-visible difference. including this change will make future uplifts easier.
[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 1318830 should land on before 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 a week now as well as having been verified manually by QA
[String changes made/needed]: none

Comment 52

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #51)
> including this change will make future uplifts easier.

Only if we uplift bug 1348122 too...

Comment 53

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Dão Gottwald [::dao] from comment #52)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #51)
> > including this change will make future uplifts easier.
> 
> Only if we uplift bug 1348122 too...

Yes, I was thinking that also, but that bug hasn't been verified yet. We can only uplift that to aurora once these bugs are uplifted anyways, and I didn't want to delay the first bug (1318830) much longer.

Comment 54

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #53)
> (In reply to Dão Gottwald [::dao] from comment #52)
> > (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #51)
> > > including this change will make future uplifts easier.
> > 
> > Only if we uplift bug 1348122 too...
> 
> Yes, I was thinking that also, but that bug hasn't been verified yet.

It also contains string changes, although that could be avoided if necessary by using urlbar-zoom-button.label on Aurora.

Comment 55

[Gerry Chang [:gchang]]

Comment on attachment 8846696 [details]
Bug 1345375 - Use the FullZoomChange event instead of browser-fullZoom observers since FullZoomChange works on MediaDocuments.

This is required for bug 1318830. Aurora54+.

Comment 56

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/135f5507e658

Comment 57

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I can confirm this issue is fixed on the latest aurora (20170331004006).