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

RESOLVED FIXED in Camino1.5

Status

enhancement
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: phiw2, Assigned: froodian)

Tracking

({fixed1.8.1})

unspecified
Camino1.5
PowerPC
macOS
Dependency tree / graph

Details

Attachments

(1 attachment, 5 obsolete attachments)

7.24 KB, patch
stuart.morgan+bugzilla
: review+
mikepinkerton
: superreview+
Details | Diff | Splinter Review
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
This seems legitimate enough as part of our illusion of about:blank not being a URI (leaving the location bar empty, etc).
Assignee: nobody → stridey
Status: UNCONFIRMED → NEW
Ever confirmed: true
Target Milestone: --- → Camino1.1
Posted patch Patch (obsolete) — Splinter Review
Attachment #226895 - Flags: review?(bugzilla)
Comment on attachment 226895 [details] [diff] [review]
Patch

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

cl
Attachment #226895 - Flags: review?(bugzilla) → review+
Attachment #226895 - Flags: review?(mozilla)
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.
Attachment #226895 - Flags: review-
Posted patch Patch (obsolete) — Splinter Review
Good catch Smokey.  Got some cross-patchination going on here.  This fixes everything mentioned above and bug 319922.
Attachment #226895 - Attachment is obsolete: true
Attachment #226914 - Flags: review?(bugzilla)
Attachment #226895 - Flags: review?(mozilla)
Blocks: 319922
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.
Attachment #226914 - Flags: review-
Posted patch Patch (obsolete) — Splinter Review
> 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.
Attachment #226914 - Attachment is obsolete: true
Attachment #226914 - Flags: review?(bugzilla)
Comment on attachment 226951 [details] [diff] [review]
Patch

r+=me
Please raise a separate bug for the page context menu though.
Attachment #226951 - Flags: review+
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!)
Attachment #226951 - Flags: review+ → review-
Posted patch Patch (obsolete) — Splinter Review
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)
Attachment #226951 - Attachment is obsolete: true
Attachment #231478 - Flags: review?(stuart.morgan)
Status: NEW → ASSIGNED
Attachment #231478 - Flags: review?(mozilla)
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.
Attachment #231478 - Flags: review?(stuart.morgan) → review-
(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
Posted patch Patch (obsolete) — Splinter Review
- 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.
Attachment #231478 - Attachment is obsolete: true
Attachment #231478 - Flags: review?(mozilla)
Attachment #232290 - Flags: review?(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.
Attachment #232290 - Flags: review?(stuart.morgan) → review-
Posted patch PatchSplinter Review
Sorry 'bout that.
Attachment #232290 - Attachment is obsolete: true
Attachment #232650 - Flags: review?(stuart.morgan)
Comment on attachment 232650 [details] [diff] [review]
Patch

r=me
Attachment #232650 - Flags: superreview?(mikepinkerton)
Attachment #232650 - Flags: review?(stuart.morgan)
Attachment #232650 - Flags: review+
Comment on attachment 232650 [details] [diff] [review]
Patch

sr=pink
Attachment #232650 - Flags: superreview?(mikepinkerton) → superreview+
Checked in on the trunk and MOZILLA_1_8_BRANCH.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Whiteboard: [needs checkin]
You need to log in before you can comment on or make changes to this bug.