Closed Bug 1345375 Opened 7 years ago Closed 7 years ago

Use the FullZoomChange event to update zoom controls

Categories

(Firefox :: Toolbars and Customization, defect)

Product:
Firefox
Component:
Toolbars and Customization
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox54 --- verified
firefox55 --- verified

People

(Reporter: dao, Assigned: jaws)

References

Details

Attachments

(2 files, 1 obsolete file)

1318830_Bugday-20170308.gif
2.04 MB, image/gif
Details
Bug 1345375 - Use the FullZoomChange event instead of browser-fullZoom observers since FullZoomChange works on MediaDocuments.
59 bytes, text/x-review-board-request
dao
: review+
gchang
: approval-mozilla-aurora+
Details 
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.
Flags: needinfo?(jaws)
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Flags: needinfo?(jaws)
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.
Attachment #8845054 - Flags: review?(dao+bmo) → review-
(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 ?
(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?
Flags: needinfo?(jaws)
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.
Flags: needinfo?(jaws)
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)
(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 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.)
Attachment #8845054 - Flags: review?(dao+bmo) → review+
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.
Attachment #8845054 - Flags: review+ → review-
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.
Component: General → Toolbars and Customization
Product: Toolkit → Firefox
Summary: browser-content.js: rename URLBarZoom or the SyntheticDocument:ZoomChange message → Use the FullZoomChange event to update zoom controls
Blocks: 1257312
No longer blocks: 1257312
Blocks: 1089246
(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.
Attachment #8845054 - Attachment is obsolete: true
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
Attachment #8846696 - Flags: review?(dao+bmo) → review+
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8bd3a85551ae
Use the FullZoomChange event instead of browser-fullZoom observers since FullZoomChange works on MediaDocuments. r=dao
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
Flags: needinfo?(jaws)
(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.
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/56b09fa0748c
Use the FullZoomChange event instead of browser-fullZoom observers since FullZoomChange works on MediaDocuments. r=dao
(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.
Flags: needinfo?(jaws)
Depends on: 1347413
What's up with the browser.xml change that apparently nobody reviewed?
Flags: needinfo?(jaws)
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?
Flags: needinfo?(jaws)
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.
Attachment #8846696 - Flags: review+ → review-
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0cc9dced786c
Backed out changeset 56b09fa0748c on request from dao
(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.
Flags: needinfo?(dao+bmo)
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.
Flags: needinfo?(dao+bmo)
(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.
Attachment #8846696 - Flags: review- → review?(dao+bmo)
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.
Attachment #8846696 - Flags: review?(dao+bmo) → review-
(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.
(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 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);
(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.
Flags: needinfo?(jaws)
I don't see any strict warnings in the Browser Console but I'll upload a new patch that uses nodeType.
Flags: needinfo?(jaws)
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
Attachment #8846696 - Flags: review?(dao+bmo) → review+
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e263ffa2eb29
Use the FullZoomChange event instead of browser-fullZoom observers since FullZoomChange works on MediaDocuments. r=dao
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.
Keywords: qawanted
Flags: qe-verify+
Keywords: qawanted
(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
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/e263ffa2eb29
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Hi :jaws,
From bug 1089246, FF53 & 54 are also affected, do you think it's worth uplifting to 54 at least?
Flags: needinfo?(jaws)
(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.
Flags: needinfo?(jaws) → needinfo?(ryanvm)
Flags: needinfo?(ryanvm) → needinfo?(rares.bologa)
(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.
[bugday-20170322] 

Status-ff55: verified and fixed

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

OS : Win 10(X64)
Flags: needinfo?(rares.bologa) → needinfo?(ciprian.muresan)
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?
Flags: needinfo?(ciprian.muresan) → needinfo?(jaws)
(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!
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
Flags: needinfo?(jaws)
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
Attachment #8846696 - Flags: approval-mozilla-aurora?
(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...
(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.
(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 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+.
Attachment #8846696 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I can confirm this issue is fixed on the latest aurora (20170331004006).
status-firefox54: fixed → verified
Flags: qe-verify+
Depends on: 1378784
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: