Last Comment Bug 342594 - Reload button and menu-item should be disabled when about:blank page is loaded
: Reload button and menu-item should be disabled when about:blank page is loaded
Status: RESOLVED FIXED
: fixed1.8.1
Product: Camino Graveyard
Classification: Graveyard
Component: Toolbars & Menus (show other bugs)
: unspecified
: PowerPC Mac OS X
-- enhancement (vote)
: Camino1.5
Assigned To: froodian (Ian Leue)
:
:
Mentors:
Depends on:
Blocks: 319922
  Show dependency treegraph
 
Reported: 2006-06-23 19:06 PDT by philippe (part-time)
Modified: 2006-08-15 08:35 PDT (History)
2 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch (4.48 KB, patch)
2006-06-23 19:48 PDT, froodian (Ian Leue)
bugzilla-graveyard: review+
alqahira: review-
Details | Diff | Splinter Review
Patch (5.39 KB, patch)
2006-06-24 04:35 PDT, froodian (Ian Leue)
mozilla: review-
Details | Diff | Splinter Review
Patch (5.27 KB, patch)
2006-06-24 17:02 PDT, froodian (Ian Leue)
mozilla: review-
Details | Diff | Splinter Review
Patch (6.45 KB, patch)
2006-07-31 13:46 PDT, froodian (Ian Leue)
stuart.morgan+bugzilla: review-
Details | Diff | Splinter Review
Patch (7.23 KB, patch)
2006-08-05 01:00 PDT, froodian (Ian Leue)
stuart.morgan+bugzilla: review-
Details | Diff | Splinter Review
Patch (7.24 KB, patch)
2006-08-07 20:12 PDT, froodian (Ian Leue)
stuart.morgan+bugzilla: review+
mikepinkerton: superreview+
Details | Diff | Splinter Review

Description User image philippe (part-time) 2006-06-23 19:06:19 PDT
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 User image froodian (Ian Leue) 2006-06-23 19:12:44 PDT
This seems legitimate enough as part of our illusion of about:blank not being a URI (leaving the location bar empty, etc).
Comment 2 User image froodian (Ian Leue) 2006-06-23 19:48:54 PDT
Created attachment 226895 [details] [diff] [review]
Patch
Comment 3 User image Chris Lawson (gone) 2006-06-23 19:55:12 PDT
Comment on attachment 226895 [details] [diff] [review]
Patch

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

cl
Comment 4 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-06-23 23:02:16 PDT
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 User image froodian (Ian Leue) 2006-06-24 04:35:28 PDT
Created attachment 226914 [details] [diff] [review]
Patch

Good catch Smokey.  Got some cross-patchination going on here.  This fixes everything mentioned above and bug 319922.
Comment 6 User image Bruce Davidson 2006-06-24 09:29:42 PDT
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 User image froodian (Ian Leue) 2006-06-24 17:02:17 PDT
Created attachment 226951 [details] [diff] [review]
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 User image Bruce Davidson 2006-07-01 06:47:26 PDT
Comment on attachment 226951 [details] [diff] [review]
Patch

r+=me
Please raise a separate bug for the page context menu though.
Comment 9 User image Bruce Davidson 2006-07-01 06:54:03 PDT
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 User image froodian (Ian Leue) 2006-07-31 13:46:05 PDT
Created attachment 231478 [details] [diff] [review]
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 User image Stuart Morgan 2006-08-04 01:37:49 PDT
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 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-08-04 15:03:21 PDT
(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 User image froodian (Ian Leue) 2006-08-05 01:00:31 PDT
Created attachment 232290 [details] [diff] [review]
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 User image Stuart Morgan 2006-08-05 22:07:55 PDT
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 User image froodian (Ian Leue) 2006-08-07 20:12:45 PDT
Created attachment 232650 [details] [diff] [review]
Patch

Sorry 'bout that.
Comment 16 User image Stuart Morgan 2006-08-12 19:46:21 PDT
Comment on attachment 232650 [details] [diff] [review]
Patch

r=me
Comment 17 User image Mike Pinkerton (not reading bugmail) 2006-08-14 05:40:26 PDT
Comment on attachment 232650 [details] [diff] [review]
Patch

sr=pink
Comment 18 User image Mark Mentovai 2006-08-15 08:35:55 PDT
Checked in on the trunk and MOZILLA_1_8_BRANCH.

Note You need to log in before you can comment on or make changes to this bug.