342594 - Reload button and menu-item should be disabled when about:blank page is loaded

Status: RESOLVED FIXED

(Camino Graveyard :: Toolbars & Menus, enhancement)

Description

[philippe (part-time)]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en; rv:1.9a1) Gecko/20060624 Camino/1.2+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en; rv:1.9a1) Gecko/20060624 Camino/1.2+

Set about:blank as home page URI
Open new tab/window

Reload button, the menu item and the contextual menu item for 'Reload' are all active, but there is nothing to reload. Technically, about:blank is not empty, but still...

In Firefox and Safari those items are disabled.

Reproducible: Always



Expected Results:  
Reload button, the menu item and the contextual menu item disabled when loading about:blank page

Comment 1

[froodian (Ian Leue)]

This seems legitimate enough as part of our illusion of about:blank not being a URI (leaving the location bar empty, etc).

Comment 2

[froodian (Ian Leue)]

Attachment: Patch

Comment 3

[Chris Lawson (gone)]

Comment on attachment 226895 [details] [diff] [review]
Patch

r=me (code-only, didn't test the patch).

cl

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 226895 [details] [diff] [review]
Patch

The tab context menu "Reload Tab" is still enabled with this patch.  The View menu item and the toolbar button are properly disabled, though.

Comment 5

[froodian (Ian Leue)]

Attachment: Patch

Good catch Smokey.  Got some cross-patchination going on here.  This fixes everything mentioned above and bug 319922.

Comment 6

[Bruce Davidson]

Comment on attachment 226914 [details] [diff] [review]
Patch

+  if (action == @selector(doReload:))
+    return (browserController && 
+            ![browserController bookmarkManagerIsVisible] &&
+            ![MainController isBlankURL:[[browserController getBrowserWrapper] getCurrentURI]]);
+
   if (action == @selector(biggerTextSize:))
     return (browserController &&
             ![[browserController getBrowserWrapper] isEmpty] &&
             [[[browserController getBrowserWrapper] getBrowserView] canMakeTextBigger]);

Why do we have two different checks for a blank URL, [BrowserWrapper isEmpty]
and [MainController isBlankURL], and use them for the same purpose within
5 lines of code!?

I'd like to see this rationalised, and the isEmpty check looks neater. (Though
currently isEmpty doesn't return YES for an empty or nil URL string.)

Also the "Reload" context menu on the page for about:blank is still enabled
and should really be disabled.

Comment 7

[froodian (Ian Leue)]

Attachment: Patch

> Why do we have two different checks for a blank URL, [BrowserWrapper isEmpty]
> and [MainController isBlankURL], and use them for the same purpose within
> 5 lines of code!?
> 
> I'd like to see this rationalised, and the isEmpty check looks neater. (Though
> currently isEmpty doesn't return YES for an empty or nil URL string.)

As I understand it, the two checks both need to exist, since one checks what's currently loaded in the wrapper, and one just says if an NSString is a "blank" uri (which is why isEmpty doesn't return yes for empty or nil URL strings, since they're never loaded in wrapper as those).  For this situation you're right, isEmpty is better.  Also fixing the call I stole the isBlankURL call from. ;)

> Also the "Reload" context menu on the page for about:blank is still enabled

This one's a bit tougher.  Actually, it's not.  The problem is, it's broken for context menus from background tabs.  The validateMenuItem method can't tell what tab the call came from, so it's just checking what page is currently being viewed.  Obviously this won't do, but I wrestled with the code for a while, and didn't come up with a way to do this and still have the items be dynamic (I could move the menu from the nib into code and set the validation state when I make the menu, but then it's not dynamic, right?).  If anybody has any ideas, I'm open.

Comment 8

[Bruce Davidson]

Comment on attachment 226951 [details] [diff] [review]
Patch

r+=me
Please raise a separate bug for the page context menu though.

Comment 9

[Bruce Davidson]

Comment on attachment 226951 [details] [diff] [review]
Patch

Err... froodian just pointed out this doesn't set the correct state in tab context menus for background tabs.

(Memo to self: test with some empty and some non-empty tabs, not all empty!)

Comment 10

[froodian (Ian Leue)]

Attachment: Patch

This Patch:

- Creates a helper method for validating "Reload" items, and uses it for the menu item, toolbar button, tab context menu item, and page context menu item.
- Validates the tab context menu item "Reload All Tabs" based on whether there is more than one tab open
- Replaces |isBlankURL| with |isEmpty| for the sendURL action (see comments 6 and 7)

Comment 11

[Stuart Morgan]

Comment on attachment 231478 [details] [diff] [review]
Patch

>+- (BOOL)shouldAllowReload;

canReload

>+- (BOOL)shouldAllowReload
>+{
>+  NSString* curURI = [[[self getBrowserView] getCurrentURI] lowercaseString];
>+  return (!([curURI isEqualToString:@"about:bookmarks"] || [curURI isEqualToString:@"about:history"]) &&
>+          ![self isEmpty]);
>+}

Is there any about: URL that it makes sense to allow reload for?  Maybe instead you should just have an isInternalURL that checks for the about scheme

>+      action == @selector(reloadAllTabs:))
>     return ([mTabBrowser numberOfTabViewItems] > 1);

Why can't someone reload all tabs if they have one tab?

>   if (action == @selector(sendURL:))
>-  {
>-    NSString* urlString = [[[self getMainWindowBrowserController] getBrowserWrapper] getCurrentURI];
>-    return ![MainController isBlankURL:urlString];
>-  }
>+    return ![[browserController getBrowserWrapper] isEmpty];

This should also be disabled for any internal page, since sending someone, e.g., about:bookmarks, doesn't make sense any more that about:blank does.

Comment 12

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #11)
> Is there any about: URL that it makes sense to allow reload for?  Maybe instead
> you should just have an isInternalURL that checks for the about scheme

We discussed this here: http://wiki.caminobrowser.org/Development:Planning:Toolbar_item_validation#Reload_Page

Comment 13

[froodian (Ian Leue)]

Attachment: Patch

- Renames shouldAllowReload to canReload.
- Adds about:config to the invalidation list per the link in smokey's previous comment.
- Since we don't need the proposed isInternalURL function for this bug, filing the Email Page Location validation as bug 347498
- Reenables "reload all tabs" when 1 tab is open, and changes the menu item to match.

Comment 14

[Stuart Morgan]

Comment on attachment 232290 [details] [diff] [review]
Patch

>+  if (action == @selector(doReloadAllTabs:))
>+    return (browserController);

This doesn't compile; you can't return a BWC from a BOOL method.

Comment 15

[froodian (Ian Leue)]

Attachment: Patch

Sorry 'bout that.

Comment 16

[Stuart Morgan]

Comment on attachment 232650 [details] [diff] [review]
Patch

r=me

Comment 17

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 232650 [details] [diff] [review]
Patch

sr=pink

Comment 18

[Mark Mentovai]

Checked in on the trunk and MOZILLA_1_8_BRANCH.