237776 - Reload should be disabled for blank tabs

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Grey Nicholson (they/them)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.7b) Gecko/20040314 Firefox/0.8.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.7b) Gecko/20040314 Firefox/0.8.0+

If a tab is blank, the Reload button has no function; so it should be disabled.

Reproducible: Always
Steps to Reproduce:
1.Open a blank tab
2.
3.

Actual Results:  
The Reload button is available, but does nothing

Expected Results:  
The button should be disabled

Comment 1

[Benedikt Kantus]

-> NEW

Comment 2

[Mike Connor [:mconnor]]

it reloads about:blank :)

Comment 3

[Noam Avnery]

PMFJI.
Along the same lines, what about the "File" menu items?
Save Page As, Send Page, Print Preview, Print
I believe all of them should be disabled on a blank tab.
Should I open a new bug for it?

Comment 4

[Ben Goodger (use ben at mozilla dot org for email)]

-ing would consider patch.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

It does look a bit silly in my opinion.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

I'd like to take a stab at this bug, but I'm not even sure where these settings
are controlled.

Comment 7

[Mike Connor [:mconnor]]

Resummarizing so we don't fix this halfway:

Reload menu/context menu items should be disabled as well, as should the
keyboard command.  Using reload to match the stopCommand, and making the
keyboard shortcut and the UI elements all key off of that is the right fix.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Attachment: Disable reload button on blank pages/tabs

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Attachment: Same patch with -up8

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 171297 [details] [diff] [review]
Same patch with -up8

Obsoleting this patch until the patch from bug 273217 is checked in.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Attachment: Updated patch

Updated patch.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 172302 [details] [diff] [review]
Updated patch

Cripes, there's some extra garbage that I accidentally let get in there.
Resubmitting.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Attachment: Updated patch with no extra garbage

Comment 14

[Mike Connor [:mconnor]]

Comment on attachment 172350 [details] [diff] [review]
Updated patch with no extra garbage

clearing review request, new patch is supposed to be coming :)

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Attachment: New patch. Doesn't break Ctrl+F5 functionality.

OK, I've updated this patch to fix the issue with the last one where Ctrl+F5
functionality was broken. It's also diff'ed vs. the current trunk, so it should
patch with less complaining.

Comment 16

[Mike Connor [:mconnor]]

Comment on attachment 175522 [details] [diff] [review]
New patch. Doesn't break Ctrl+F5 functionality.


>+    this.stopCommand            = document.getElementById("Browser:Stop");
>+    this.reloadCommand          = document.getElementById("Browser:Reload");
>+    this.reloadCommandSkipCache = document.getElementById("Browser:ReloadSkipCache");

this.reloadSkipCacheCommand instead :)

>+    this.stopCommand            = null;
>+    this.reloadCommand          = null;
>+    this.reloadCommandSkipCache = null;

same here

>@@ -3109,16 +3113,25 @@ nsBrowserStatusHandler.prototype =
>     selectedBrowser.lastURI = aLocation;
> 
>     this.setOverLink("", null);
> 
>     var location = aLocation.spec;
> 
>     if (location == "about:blank")
>       location = "";
>+
>+    this.reloadCommand.removeAttribute("disabled");
>+    this.reloadCommandSkipCache.removeAttribute("disabled");
>+
>+    // Disable reload button on blank pages
>+    if (location == "") {
>+      this.reloadCommand.setAttribute("disabled", "true");
>+      this.reloadCommandSkipCache.setAttribute("disabled", "true");
>+      }

why remove the attribute only to set it again?	and why check a second time for
the about:blank check?

if (location == "about:blank") {
  location = "";
  this.reloadCommand.setAttribute("disabled", "true");
  this.reloadCommandSkipCache.setAttribute("disabled", "true");
} else {
  this.reloadCommand.removeAttribute("disabled");
  this.reloadCommandSkipCache.removeAttribute("disabled");
}

don't forget to rename the command too :)

>Index: browser-sets.inc
>===================================================================
>RCS file: /cvsroot/mozilla/browser/base/content/browser-sets.inc,v
>retrieving revision 1.35
>diff -u -p -8 -r1.35 browser-sets.inc
>--- browser-sets.inc	1 Feb 2005 17:36:48 -0000	1.35
>+++ browser-sets.inc	25 Feb 2005 06:51:55 -0000
>@@ -275,20 +276,20 @@
>     <key id="goBackKb"  keycode="VK_LEFT" command="Browser:Back" modifiers="accel"/>
>     <key id="goForwardKb"  keycode="VK_RIGHT" command="Browser:Forward" modifiers="accel"/>
> #else
>     <key id="goBackKb"  keycode="VK_LEFT" command="Browser:Back" modifiers="alt"/>
>     <key id="goForwardKb"  keycode="VK_RIGHT" command="Browser:Forward" modifiers="alt"/>
> #endif
>     <key id="goHome" keycode="VK_HOME" command="Browser:Home" modifiers="alt"/>
>     <key id="key_fullScreen" keycode="VK_F11" command="View:FullScreen"/>
>-    <key keycode="VK_F5" oncommand="BrowserReload();"/>
>-    <key keycode="VK_F5" modifiers="accel" oncommand="BrowserReloadSkipCache();"/>
>-    <key id="key_reload"  key="&reloadCmd.commandkey;" oncommand="BrowserReload();" modifiers="accel"/>
>-    <key key="&reloadCmd.commandkey;" oncommand="BrowserReloadSkipCache();" modifiers="accel,shift"/>
>+    <key keycode="VK_F5" command="Browser:Reload"/>
>+    <key keycode="VK_F5" command="Browser:ReloadSkipCache" modifiers="accel"/>
>+    <key key="&reloadCmd.commandkey;" command="Browser:Reload" modifiers="accel" id="key_reload"/>
>+    <key key="&reloadCmd.commandkey;" command="Browser:Reload" modifiers="accel,shift"/>

why isn't this Browser:ReloadSkipCache directly?  You know its going there, why
mess with redirection and event checking?

almost! ;)

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Attachment: Updated patch based on mconnor's comments

This patch encompasses the changes Mike suggested in his review of the patch.
One minor issue I've run into, however, is that the reload button doesn't get
disabled on browser startup if the default homepage is about:blank. If you then
hit the Home button, though, the reload button disables.

If anyone reading this has an idea of what needs to be done to fix that, please
let me know.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Attachment: Fixed patch

Gah, I figured out the problem. It's fixed in this patch.

Do your worst, Mike :)

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Attachment: Adding missing entries to disabled items list in browser.js (Checked in)

Just realized that I forgot to add the functions to the disabled entries list
in browser.js. This remedies that.

OK, I swear I'm done submitting patches until I get a review :P

Comment 20

[Ian Neal]

Comment on attachment 175692 [details] [diff] [review]
Adding missing entries to disabled items list in browser.js (Checked in)

Checking in browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.390; previous revision: 1.389
done
Checking in browser.xul;
/cvsroot/mozilla/browser/base/content/browser.xul,v  <--  browser.xul
new revision: 1.257; previous revision: 1.256
done
Checking in browser-context.inc;
/cvsroot/mozilla/browser/base/content/browser-context.inc,v  <-- 
browser-context.inc
new revision: 1.11; previous revision: 1.10
done
Checking in browser-menubar.inc;
/cvsroot/mozilla/browser/base/content/browser-menubar.inc,v  <-- 
browser-menubar.inc
new revision: 1.44; previous revision: 1.43
done
Checking in browser-sets.inc;
/cvsroot/mozilla/browser/base/content/browser-sets.inc,v  <--  browser-sets.inc
new revision: 1.38; previous revision: 1.37
done

Comment 21

[Asaf Romano (gone)]

regression: bug 284130.

Comment 22

[tabmix.onemen]

bug 287105 is regressed by this bug fix....

Comment 23

[Boris Zbarsky [:bzbarsky]]

This patch looks wrong to me, since onLocationChange for subframes will end up
in this method.  Shouldn't that new code be inside the |if
(aWebProgress.DOMWindow == content)| block or something?

Comment 24

[Boris Zbarsky [:bzbarsky]]

Note that even after bug 285738 is fixed, we still need to fix the code that was
checked in here.  Let me know if you want me to file a bug on this, please.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

(In reply to comment #24)
> Note that even after bug 285738 is fixed, we still need to fix the code that was
> checked in here.  Let me know if you want me to file a bug on this, please.

Can we just reopen this bug?

Comment 26

[Boris Zbarsky [:bzbarsky]]

One bug per problem... The fact that the code is broken for frames is a separate
problem.  So that was actually a question as to whether you were going to file
the bug yourself.

Bug 288160 filed.