Markus Stange [:mstange] Assignee
	Description • 10 years ago

browser-fullZoom.js duplicates a lot of the WheelPrefs code from EventStateManager.cpp, all in order to figure out whether a given wheel event will change the current page zoom, so that it can update the site-specific zoom prefs after the zoom change.

Is there a reason we can't just make EventStateManager::ChangeFullZoom (and maybe also EventStateManager::ChangeTextSize?) fire the browser-fullZoom:zoomChange observer notification, and make browser-fullZoom.js listen for that?

If we want to use async panning for wheel events in the browser (bug 1013364), this wheel event handler needs to go away, otherwise it will prevent the scroll gesture from starting until it has executed (once bug 1013412 is fixed).

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 1 • 10 years ago

(In reply to Markus Stange [:mstange] from comment #0)
> Is there a reason we can't just make EventStateManager::ChangeFullZoom (and
> maybe also EventStateManager::ChangeTextSize?) fire the
> browser-fullZoom:zoomChange observer notification, and make
> browser-fullZoom.js listen for that?

I think that it or something similar way (e.g., using custom DOM event) is right approach.

	Dão Gottwald [:dao]
	Comment 2 • 10 years ago

A DOM event seems about right. browser-fullZoom:zoomChange is application-global like all nsIObserver notifications. It was added for a test (arguably the first mistake) and then CustomizableWidgets.jsm misused it.

	Markus Stange [:mstange] Assignee
	Comment 3 • 10 years ago

nsDocumentViewer::SetFullZoom already sends a "FullZoomChange" chrome event. I'll try to reuse that.

	Markus Stange [:mstange] Assignee
	Comment 4 • 10 years ago

This seems to work. I still need to run it through try.

	Markus Stange [:mstange] Assignee
	Comment 6 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=67ff9ca3a5c8

	Dão Gottwald [:dao]
	Comment 7 • 10 years ago

Comment on attachment 8428813 [details] [diff] [review]
v1

>   handleEvent: function FullZoom_handleEvent(event) {
>     switch (event.type) {
>-      case "DOMMouseScroll":
>-        this._handleMouseScrolled(event);
>+      case "FullZoomChange":
>+      case "TextZoomChange":
>+        let browser = gBrowser.selectedBrowser;

event.target points to the browser, doesn't it?

	Markus Stange [:mstange] Assignee
	Comment 8 • 10 years ago

I think in the non-e10s case it points to the content document. Will confirm.
Also the try push didn't go so well, tests are timing out all over the place.

	Jim Mathies [:jimm]
	Comment 9 • 10 years ago

I'm in need these events for bug 691601 which blocks our m1 milestone of e10s. Is this bug something you're planning on coming back to markus, can I pick it up if you're not going to get back to it soon?

	Markus Stange [:mstange] Assignee
	Comment 10 • 10 years ago

I'm going to finish this up.

	Markus Stange [:mstange] Assignee
	Comment 11 • 10 years ago

Same as before, but also removes ACTION_ZOOM.

(In reply to Dão Gottwald [:dao] from comment #7)
> Comment on attachment 8428813 [details] [diff] [review]
> v1
> 
> >   handleEvent: function FullZoom_handleEvent(event) {
> >     switch (event.type) {
> >-      case "DOMMouseScroll":
> >-        this._handleMouseScrolled(event);
> >+      case "FullZoomChange":
> >+      case "TextZoomChange":
> >+        let browser = gBrowser.selectedBrowser;
> 
> event.target points to the browser, doesn't it?

In the non-e10s case it points to the contentDocument and in the e10s case it points to the tabbrowser. Not really useful in either case.

	Markus Stange [:mstange] Assignee
	Comment 12 • 10 years ago

The problem with the tryserver timeouts was the following: FullZoom now gets notified of zoom changes that it has made itself, and this causes it to get into an infinite loop if two FullZoom instances are involved (due to two open browser windows, for example):

Let's say we have two open browser windows, each with its own FullZoom instance, call them FullZoom1 and FullZoom2, and both windows have an active tab with a page from the same domain. Now change the zoom in window 1.
 - FullZoom1 reacts to the detected zoom change by storing the site specific zoom in a pref.
 - Setting the pref notifies both FullZoom1 and FullZoom2. FullZoom1 has _isNextContentPrefChangeInternal set and knows to ignore the notification, but FullZoom2 doesn't.
 - FullZoom2 reacts by setting the new zoom value from the pref on the active browser in window 2.
 - This triggers a FullZoomChange event in window 2. Jump to the first step with 1 and 2 reversed.

Not dispatching the FullZoomChange event when the value hasn't changed provides a simple way out of this loop.

	Markus Stange [:mstange] Assignee
	Comment 13 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=5975662a1ce8

	Dão Gottwald [:dao]
	Comment 14 • 10 years ago

(In reply to Markus Stange [:mstange] from comment #11)
> > >   handleEvent: function FullZoom_handleEvent(event) {
> > >     switch (event.type) {
> > >-      case "DOMMouseScroll":
> > >-        this._handleMouseScrolled(event);
> > >+      case "FullZoomChange":
> > >+      case "TextZoomChange":
> > >+        let browser = gBrowser.selectedBrowser;
> > 
> > event.target points to the browser, doesn't it?
> 
> In the non-e10s case it points to the contentDocument and in the e10s case
> it points to the tabbrowser. Not really useful in either case.

I guess that's because the browsers are anonymous. Does event.originalTarget point to the browser in the e10s case?

FullZoomChange and TextZoomChange can be dispatched in background browsers, can't they? In which case the above code would be wrong...

	Tom S [:evilpie]
	Comment 15 • 10 years ago

Comment on attachment 8439958 [details] [diff] [review]
part 2, v1: only dispatch events if the zoom actually changed

Looks good to me, but I am not sure I can actually review here.

	Dão Gottwald [:dao]
	Comment 16 • 10 years ago

Comment on attachment 8439942 [details] [diff] [review]
part 1, v2

see comment 14

	Markus Stange [:mstange] Assignee
	Comment 17 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=fc8cdc1cfd44

	Jim Mathies [:jimm]
	Comment 18 • 10 years ago

(In reply to Markus Stange [:mstange] from comment #17)
> Created attachment 8444402 [details] [diff] [review]
> new attempt
> 
> https://tbpl.mozilla.org/?tree=Try&rev=fc8cdc1cfd44

The problem here seems to be that FullZoom and TextZoom dom events fire prior to FullZoom.onLocationChange getting called when a tab loads, so we never get a chance to update the zoom on a loading tab before we call _applyPrefToZoom.

In the first failing test we:

1) set zoom != 1 for 'url' in tab 1
2) load 'url' into tab 2
3) check to see if zoom != 1 on tab 1 and 2

between steps 2 and 3, the zoom level for 'url' gets reset to 1, then in onLocationChange we set the zoom according to the pref, which is now 1.

	Markus Stange [:mstange] Assignee
	Comment 19 • 10 years ago

Thank you for looking into this. I've made some more progress since the try push, and I think the best course of action would be to *not* use the FullZoomChange / TextZoomChange events, and instead add a new event that is only fired in response to wheel events that cause zoom changes.
The main problem with the FullZoomChange / TextZoomChange events is that they are not only sent for user-initiated zoom level changes, but also for automatic zoom changes that happen on navigation. Our browser-fullZoom.js zoom listener can't discern between these types of causes and treats all events as intentional (and responds by updating the prefs, making the zoom change permanent).
Another problem are automatic zoom events from background tabs: browser-fullZoom.js does not keep the zoom level in background tabs up-to-date, it only corrects it once you switch to the tab. So if a foreground tab gets zoomed by the user to a new zoom level (and this new zoom level is stored in the pref correctly), but a page from the same domain is open in a background tab, this background tab might fire a zoom change event in response to a navigation, and we'd incorrectly set the zoom pref to the old level from the background tab.
And yet another problem is that the zoom change events can be attributed to the wrong document when navigating back/forward between documents. During such a navigation, the content viewer tries to restore the zoom level of the restored document, but it does so before it sets the new document as the active document inside the content viewer. So the resulting zoom change event will have the zoom level of the new page, but the browser's URL still reflects the old page. And we can't just swap the order here since we don't want to get to onLocationChange with the non-restored zoom level.

All in all, adding a new event is a lot easier and gets around all these problems. It also means browser-fullZoom.js doesn't need to do any extra work in order to ignore zoom change events it caused itself.

	Markus Stange [:mstange] Assignee
	Comment 22 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=297c4a605351

	Jim Mathies [:jimm]
	Comment 23 • 10 years ago

(In reply to Markus Stange [:mstange] from comment #19)
> Thank you for looking into this. I've made some more progress since the try
> push, and I think the best course of action would be to *not* use the
> FullZoomChange / TextZoomChange events, and instead add a new event that is
> only fired in response to wheel events that cause zoom changes.
> The main problem with the FullZoomChange / TextZoomChange events is that
> they are not only sent for user-initiated zoom level changes, but also for
> automatic zoom changes that happen on navigation. Our browser-fullZoom.js
> zoom listener can't discern between these types of causes and treats all
> events as intentional (and responds by updating the prefs, making the zoom
> change permanent).
> Another problem are automatic zoom events from background tabs:
> browser-fullZoom.js does not keep the zoom level in background tabs
> up-to-date, it only corrects it once you switch to the tab. So if a
> foreground tab gets zoomed by the user to a new zoom level (and this new
> zoom level is stored in the pref correctly), but a page from the same domain
> is open in a background tab, this background tab might fire a zoom change
> event in response to a navigation, and we'd incorrectly set the zoom pref to
> the old level from the background tab.
> And yet another problem is that the zoom change events can be attributed to
> the wrong document when navigating back/forward between documents. During
> such a navigation, the content viewer tries to restore the zoom level of the
> restored document, but it does so before it sets the new document as the
> active document inside the content viewer. So the resulting zoom change
> event will have the zoom level of the new page, but the browser's URL still
> reflects the old page. And we can't just swap the order here since we don't
> want to get to onLocationChange with the non-restored zoom level.
> 
> All in all, adding a new event is a lot easier and gets around all these
> problems. It also means browser-fullZoom.js doesn't need to do any extra
> work in order to ignore zoom change events it caused itself.

I ran into most of this as well. FWIW I was messing around with a different experimental fix - 

https://pastebin.mozilla.org/5544227

avoiding sending those ZoomChange events from document viewer when the zoom values match solved the individual mochitest-browser test failures in your previous push. (It may have regressed somewhere else, I didn't have a chance to push to try before you posted.)

	Markus Stange [:mstange] Assignee
	Comment 24 • 10 years ago

(In reply to Jim Mathies [:jimm] from comment #23)
> I ran into most of this as well. FWIW I was messing around with a different
> experimental fix - 
> 
> https://pastebin.mozilla.org/5544227

I tried that as well (see attachment 8439958 [details] [diff] [review] in this bug) and ran into different failures:
https://tbpl.mozilla.org/?tree=Try&rev=6b83e0e79d8c

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 25 • 10 years ago

Comment on attachment 8454385 [details] [diff] [review]
part 1: add ZoomChangeUsingMouseWheel event

>+      nsContentUtils::DispatchChromeEvent(mDocument, static_cast<nsIDocument*>(mDocument),
>+                                          NS_LITERAL_STRING("ZoomChangeUsingMouseWheel"),
>+                                          true, true);
true, false)

	Markus Stange [:mstange] Assignee
	Comment 26 • 10 years ago

Comment on attachment 8454386 [details] [diff] [review]
part 2: process ZoomChangeUsingMouseWheel events instead of DOMMouseScroll events in browser-fullZoom.js

>+    // With in-process content browsers, the event's target is the content
>+    // document.
>+    if (target.nodeType == Node.DOCUMENT_NODE)
>+      return gBrowser.getBrowserForDocument(target);

Bug 1039506 will make this faster.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 27 • 10 years ago

One way to go with this would be to use message passing everywhere. So you'd put your code in browser-content.js and browser.xml instead of browser-child.js and remote-browser.xml. And you'd have to give the event you fire on the <browser> element a different name--maybe "BrowserZoomChangeUsingMouseWheel". Then the browser-fullZoom.js would just handle the BrowserZoomChangeUsingMouseWheel event and it wouldn't need to worry about content documents.

	Markus Stange [:mstange] Assignee
	Comment 28 • 10 years ago

Dão, what do you think about that suggestion?

	Jim Mathies [:jimm]
	Comment 29 • 10 years ago

The work here is blocking an e10s M1 bug, which missed a deadline last week. Dao, any chance you might be able to get to this review this week? Alternatively, can I land bug 691601 with the current events and update the patches here to use the new ones?

	Dão Gottwald [:dao]
	Comment 30 • 10 years ago

Comment on attachment 8454386 [details] [diff] [review]
part 2: process ZoomChangeUsingMouseWheel events instead of DOMMouseScroll events in browser-fullZoom.js

(In reply to Markus Stange [:mstange] from comment #28)
> Dão, what do you think about that suggestion?

Sounds good to me at a first glance. Doesn't seem like it would make a big difference, though?

>+    window.addEventListener("ZoomChangeUsingMouseWheel", this);

Should the event listener be added to gBrowser? Do you want to catch browsers that aren't inside of gBrowser, e.g. in side bars?

>+      case "ZoomChangeUsingMouseWheel":
>+        let browser = this._getTargetedBrowser(event);
>+        if (browser) {
>+          this._ignorePendingZoomAccesses(browser);
>+          this._applyZoomToPref(browser);
>+        }

Why null-check browser here?

>+  _getTargetedBrowser: function FullZoom__getTargetedBrowser(event) {
>+    let target = event.originalTarget;
>+
>+    // With remote content browsers, the event's target is the browser
>+    // we're looking for.
>+    const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
>+    if (target instanceof window.XULElement &&
>+        target.localName == "browser" &&
>+        target.namespaceURI == XUL_NS)
>+      return target;
>+
>+    // With in-process content browsers, the event's target is the content
>+    // document.
>+    if (target.nodeType == Node.DOCUMENT_NODE)
>+      return gBrowser.getBrowserForDocument(target);
>+
>+    // Fall back to the currently selected browser.
>+    return gBrowser.selectedBrowser;

This fallback shouldn't be reached, should it? Can you just throw an exception here?

	Markus Stange [:mstange] Assignee
	Comment 31 • 10 years ago

(In reply to Dão Gottwald [:dao] from comment #30)
> Comment on attachment 8454386 [details] [diff] [review]
> part 2: process ZoomChangeUsingMouseWheel events instead of DOMMouseScroll
> events in browser-fullZoom.js
> 
> (In reply to Markus Stange [:mstange] from comment #28)
> > Dão, what do you think about that suggestion?
> 
> Sounds good to me at a first glance. Doesn't seem like it would make a big
> difference, though?

I've lost track of the actual problem. I think Bill's suggestion was a way of getting to the browser without _getTabForContentWindow which bug 1039506 changed in a non-e10s-compatible way or something?
Anyway, I'll use the existing approach and just try to get it landed.

> >+    window.addEventListener("ZoomChangeUsingMouseWheel", this);
> 
> Should the event listener be added to gBrowser? Do you want to catch
> browsers that aren't inside of gBrowser, e.g. in side bars?

I don't know. The existing listener was on the window, so I guess it caught those browsers? Would non-tabbrowser browsers even zoom in response to mouse wheel events?
I've changed it to gBrowser because I don't think it's going to make much of a difference, but I also don't see much point in changing the existing behavior.

> >+      case "ZoomChangeUsingMouseWheel":
> >+        let browser = this._getTargetedBrowser(event);
> >+        if (browser) {
> >+          this._ignorePendingZoomAccesses(browser);
> >+          this._applyZoomToPref(browser);
> >+        }
> 
> Why null-check browser here?

Probably unnecessary. I've removed it, Try mochitests will hopefully alert me if there are problems.

> >+  _getTargetedBrowser: function FullZoom__getTargetedBrowser(event) {
> >+    let target = event.originalTarget;
> >+
> >+    // With remote content browsers, the event's target is the browser
> >+    // we're looking for.
> >+    const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> >+    if (target instanceof window.XULElement &&
> >+        target.localName == "browser" &&
> >+        target.namespaceURI == XUL_NS)
> >+      return target;
> >+
> >+    // With in-process content browsers, the event's target is the content
> >+    // document.
> >+    if (target.nodeType == Node.DOCUMENT_NODE)
> >+      return gBrowser.getBrowserForDocument(target);
> >+
> >+    // Fall back to the currently selected browser.
> >+    return gBrowser.selectedBrowser;
> 
> This fallback shouldn't be reached, should it? Can you just throw an
> exception here?

I replaced it with: throw new Error("Unexpected ZoomChangeUsingMouseWheel event source");
https://tbpl.mozilla.org/?tree=Try&rev=cc2ed4485b49

	Markus Stange [:mstange] Assignee
	Comment 32 • 10 years ago

Hah, that try push didn't test much because I forgot to push part 1 along with it, so no ZoomChangeUsingMouseWheel would ever be fired. Disturbingly, no test failed as the result of it.
So it looks like I need to add a test that synthesizes ctrl+mouse wheel events at the page and checks whether zoom is persisted between tabs. How do I write such a test in an e10s-compatible manner? I'd need to fire the event on the page in the content process, wait until the result gets forwarded into the parent process and then check the zoom values there. Does somebody know of an existing test that does something similar? Or should I not worry about e10s test coverage for now?

	Jim Mathies [:jimm]
	Comment 33 • 10 years ago

(In reply to Markus Stange [:mstange] from comment #32)
> Hah, that try push didn't test much because I forgot to push part 1 along
> with it, so no ZoomChangeUsingMouseWheel would ever be fired. Disturbingly,
> no test failed as the result of it.
> So it looks like I need to add a test that synthesizes ctrl+mouse wheel
> events at the page and checks whether zoom is persisted between tabs. How do
> I write such a test in an e10s-compatible manner? I'd need to fire the event
> on the page in the content process, wait until the result gets forwarded
> into the parent process and then check the zoom values there. Does somebody
> know of an existing test that does something similar? Or should I not worry
> about e10s test coverage for now?

re: writing a new test - I'd suggest writing a regular in-process test and then try running it in e10s mode. a lot of tests should just work now that cpows are fully implemented. Just remember that in the parent, anything that points to content is a wrapper that requires ipc to get data. The key there is don't assume content updates immediately, when checking results of content manipulation try to rely on events to trigger your state checks.

	Jim Mathies [:jimm]
	Comment 34 • 10 years ago

any update on test work here Markus? This is blocking an old m1 e10s bug which is now almost a month behind in landing. I'd really appreciate you help in getting this finished up, or if you do not have the time, please just ask and I'll try to finish it up myself.

	Markus Stange [:mstange] Assignee
	Comment 35 • 10 years ago

I'm working on the test, hope to have it finished today.

	Markus Stange [:mstange] Assignee
	Comment 36 • 10 years ago

Very basic, and doesn't work under e10s. Does this look good to you?

	Markus Stange [:mstange] Assignee
	Comment 37 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=1bebe334d888

	Jim Mathies [:jimm]
	Comment 38 • 10 years ago

(In reply to Markus Stange [:mstange] from comment #37)
> https://tbpl.mozilla.org/?tree=Try&rev=1bebe334d888

Looks like something in your queue blew up on mac. Doubt it was the work here.

FWIW, this new test passes for me locally. I also tested a few other tests that use FullZoomHelper without issue.

	Jim Mathies [:jimm]
	Comment 39 • 10 years ago

Comment on attachment 8475399 [details] [diff] [review]
part 3: basic test

Review of attachment 8475399 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/general/browser.ini
@@ +276,5 @@
>  skip-if = buildapp == "mulet" || e10s # Bug ?????? - test directly manipulates content (strange - gets an element from a child which it tries to treat as a string, but that fails)
>  [browser_bug970746.js]
>  skip-if = e10s # Bug ?????? - test directly manipulates content (directly gets elements from the content)
> +[browser_bug1015721.js]
> +skip-if = e10s # Bug ?????? - FullZoomHelper uses promiseTabLoadEvent() which isn't e10s friendly

So this ?????? stuff is sloppy left overs from getting a basic set of tests running. If you wouldn't mind, please file the actual follow up and have it block 'e10s-tests'. We're going to do a mass triage of that here soonish to figure out what our testing strategy is going to be.

::: browser/base/content/test/general/browser_bug1015721.js
@@ +45,5 @@
> +
> +var finishTestStarted  = false;
> +function finishTest() {
> +  Task.spawn(function () {
> +    ok(!finishTestStarted, "finishTest called more than once");

nit - I don't see how this can get called twice, but it's a harmless check none the less I think.

	Markus Stange [:mstange] Assignee
	Comment 40 • 10 years ago

Thanks for the review! I filed bug 1056146 and removed the finishTestStarted test.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d6100d9ff552
https://hg.mozilla.org/integration/mozilla-inbound/rev/884941dfa5c8
https://hg.mozilla.org/integration/mozilla-inbound/rev/a50a835644e3

	neil@parkwaycc.co.uk
	Comment 41 • 10 years ago

Comment on attachment 8454386 [details] [diff] [review]
part 2: process ZoomChangeUsingMouseWheel events instead of DOMMouseScroll events in browser-fullZoom.js

>+              let event = document.createEvent("Events");
>+              event.initEvent("ZoomChangeUsingMouseWheel", true, false);
>+              this.dispatchEvent(event);
Aren't we supposed to be using this.dispatchEvent(new Event("ZoomChangeUsingMouseWheel", { bubbles: true, cancelable: false })); these days?

	neil@parkwaycc.co.uk
	Comment 42 • 10 years ago

Comment on attachment 8454386 [details] [diff] [review]
part 2: process ZoomChangeUsingMouseWheel events instead of DOMMouseScroll events in browser-fullZoom.js

>-    window.addEventListener("DOMMouseScroll", this, false);
Mind you, I thought DOMMouseScroll was deprecated too, so that's something.

	Ryan VanderMeulen [:RyanVM]
	Comment 43 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/d6100d9ff552
https://hg.mozilla.org/mozilla-central/rev/884941dfa5c8
https://hg.mozilla.org/mozilla-central/rev/a50a835644e3
https://hg.mozilla.org/mozilla-central/rev/a91d65abbcf8