459614 - An infobar to be displayed when websites attempt to move/re-size the current window

Status: RESOLVED DUPLICATE of bug 413792

(Firefox :: General, enhancement)

Description

[Tony Lai]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.3) Gecko/2008092417 Firefox/3.0.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.3) Gecko/2008092417 Firefox/3.0.3

Certain websites attempt to move/resize the window. 
Currently, FireFox can disable window resizing from Javascript using cf. dom.disable_window_move_resize (the preference found in Javascript preferences), but it would be nice to have an infobar (similar to the popup blocker) which informs the user when this happens. 

The infobar could have options to allow the website to resize their window, or ignore it (by default)

Could this be written directly to the tree, or should it be done as an extension?

Currently I've been working on adding such an option through the tree.
If needed, I could redo this as an extension.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Tony Lai]

Attachment: Release 0.1, infobar displays but options are not complete.

This release patches FireFox so that an infobar is displayed when a webpage attempts to resize the browser. The infobar is only displayed if the preference cf. dom.disable_window_move_resize is disabled. (In other words, Javascript settings are set so that it is unable to move/resize the window)

The current release does not have working options.

Comment 2

[Tony Lai]

Attachment: Patch 0.2

Contains an Allow / Disable
as well as an Edit Options on Infobar.
disable_window_move_resize does not need to be true for infobar to display itself.

Next patch will rewrite the event to remember what the javascript command was (ResizeTo, ResizeBy, MoveTo, MoveBy), as well as the x and y parameters, allowing the "Allow" option to pick it up and run the command upon User input.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 348231 [details] [diff] [review]
Patch 0.2

Hey Tony, thanks for the patch! I have a few comments, mostly just minor stuff that might be premature if you're still cleaning up the patch. One comment that applies to the entire patch is that we don't use tabs in the mozilla codebase, so you should replace them with spaces.

>diff --git a/browser/base/content/browser-sets.inc b/browser/base/content/browser-sets.inc

>+    <!-- window resized blocked menu items -->
>+	<broadcaster id="blockedResizeAllowSite"
>+	             accesskey="A" 
>+				 oncommand="gResizedWindowObserver.allowResizing(event);" />
>+	<broadcaster id="blockedResizeDisableSite"
>+				 accesskey="D"
>+				 oncommand="gResizedWindowObserver.disableResizing(event);" />
>+	<broadcaster id="blockedResizeEditOptions" 
>+	             accesskey="O" 
>+	             oncommand="gResizedWindowObserver.editResizeOptions(event);" />

Why are these accesskeys defined on the broadcasters? The labels and accesskeys should just be defined on the menuitems themselves. They also need to be localizable, which means that they'll have to be defined in a DTD the same way the other accesskeys in this file are.

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+	fillResizedList: function (aEvent)

>+		 if (isNotResized) {
>+		     blockedResizeAllowSite.setAttribute("hidden", false);
>+			 blockedResizeDisableSite.setAttribute("hidden", true);
>+			 var allowString  = bundle_browser.getString("windowResizedAllow");
>+			 blockedResizeAllowSite.setAttribute("label", allowString);

Since these strings never change, you might as well just put them into the DTD rather than the .properties file, and refer to the directly from the menuitem. It doesn't seem like the broadcasters serve a purpose here either - you could just hide/unhide the menuitem itself, right?

>+	allowResizing: function (aEvent)
>+	{
>+	   try {
>+		 var pbi = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch2);
>+		 pbi.setBoolPref("dom.disable_window_move_resize", false);
>+		 BrowserReloadSkipCache();

Why BrowserReloadSkipCache() instead of BrowserReload()?

>+		 gBrowser.getNotificationBox().removeCurrentNotification();

Is this guaranteed to be the right notification? Seems like you should getNotificationWithValue() to make sure you're closing the right one.

>+	   }
>+	   catch (e) {
>+		 alert("Error" + e);

This is useful for debugging, but we can't alert()s like this. Use Components.utils.reportError(ex) if you really think this failure needs to be reported.

>+	disableResizing: function (aEvent) 

Same comments apply to this method. In fact, it's almost identical to allowResizing, so you should combine them into a single "toggleResizing" which takes a parameter to determine whether to enable or disable.

>+	editResizeOptions: function (aEvent) 
>+	{
>+	   try {
>+		 window.openDialog("chrome://browser/content/preferences/advanced-scripts.xul", "", "modal,centerscreen,resizable=no", null);
>+	   }
>+	   catch (e) {
>+	     alert("Error opening Advanced Javascript Options");
>+	   }
>+	},

The try/catch shouldn't be needed here, right? Just remove it and the alert().

>-          var buttons = [{
>-            label: popupButtonText,
>-            accessKey: popupButtonAccesskey,
>-            popup: "blockedPopupOptions",
>-            callback: null
>-          }];
>+           var buttons = [{
>+             label: popupButtonText,
>+             accessKey: popupButtonAccesskey,
>+             popup: "blockedPopupOptions",
>+             callback: null
>+           }];

This looks like a spurious whitespace change, which should generally be avoided.

>diff --git a/dom/src/base/nsGlobalWindow.cpp b/dom/src/base/nsGlobalWindow.cpp

>-
>+  
>   if (!CanMoveResizeWindows() || IsFrame()) {
>     return NS_OK;
>   }
>+  

>   if (!CanSetProperty("dom.disable_window_move_resize"))
>+  {
>     return PR_FALSE;
>+  }

More spurious changes - please remove!

>diff --git a/netwerk/protocol/http/src/nsHttpConnection.cpp b/netwerk/protocol/http/src/nsHttpConnection.cpp

>+    /* DEBUG_<username> is defined at build time, and Faculty is the username in the lab */
>+	#ifdef DEBUG_Faculty
>+	printf("Hello World!\n");
>+	#endif

This will need to be removed too, of course :)

>diff --git a/toolkit/content/widgets/browser.xml b/toolkit/content/widgets/browser.xml

>+	  <method name="onWindowResized">
>+	     <parameter name="evt"/>
>+		 <body>
>+		   <![CDATA[
>+		      if (!this.pageReport) {
>+			     this.pageReport = new Array();
>+			  }
>+			  this.pageReport.reported = false;
>+		      this.siteResizedWindow();

Won't this conflict with the other users of this.pageReport (popup blocking)? Seems like we need a different way of tracking this...

Looking at the patch overall, it seems rather odd to always post the event, and then have the listeners determine whether the event was allowed by checking the pref. It seems like we should fire different events based on whether or not the resize/move was blocked - and do we need to notify when it wasn't blocked? Not sure that's useful...

Comment 4

[Jesse Ruderman]

This is a duplicate of bug 413792, where you can see me arguing with myself over whether this is a good idea or not.

Comment 5

[Tony Lai]

Thanks for the comments Gavin!
I will change it ASAP in reflection to your suggestions.
And, since this is a duplicate, I think it's better we refer to Jesse's bug from now on.

Thanks!

Comment 6

[Tony Lai]

Hahahaa... the Hello World was part of a lab we did for Mozilla FireFox here at Seneca that I forgot to delete. My apologies! Hahahahaha....