The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Camino1.5

Status

Camino Graveyard
Toolbars & Menus
--
enhancement
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: philippe (part-time), Assigned: froodian (Ian Leue))

Tracking

({fixed1.8.1})

unspecified
Camino1.5
PowerPC
Mac OS X
fixed1.8.1

Details

Attachments

(1 attachment, 5 obsolete attachments)

7.24 KB, patch
Stuart Morgan
: review+
Mike Pinkerton (not reading bugmail)
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
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
(Assignee)

Comment 1

11 years ago
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
(Assignee)

Updated

11 years ago
Target Milestone: --- → Camino1.1
(Assignee)

Comment 2

11 years ago
Created attachment 226895 [details] [diff] [review]
Patch
Attachment #226895 - Flags: review?(bugzilla)

Comment 3

11 years ago
Comment on attachment 226895 [details] [diff] [review]
Patch

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

cl
Attachment #226895 - Flags: review?(bugzilla) → review+
(Assignee)

Updated

11 years ago
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-
(Assignee)

Comment 5

11 years ago
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.
Attachment #226895 - Attachment is obsolete: true
Attachment #226914 - Flags: review?(bugzilla)
Attachment #226895 - Flags: review?(mozilla)
(Assignee)

Updated

11 years ago
Blocks: 319922

Comment 6

11 years ago
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-
(Assignee)

Comment 7

11 years ago
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.
Attachment #226914 - Attachment is obsolete: true
Attachment #226914 - Flags: review?(bugzilla)

Comment 8

11 years ago
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 9

11 years ago
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-
(Assignee)

Comment 10

11 years ago
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)
Attachment #226951 - Attachment is obsolete: true
Attachment #231478 - Flags: review?(stuart.morgan)
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

11 years ago
Attachment #231478 - Flags: review?(mozilla)

Comment 11

11 years ago
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
(Assignee)

Comment 13

11 years ago
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.
Attachment #231478 - Attachment is obsolete: true
Attachment #231478 - Flags: review?(mozilla)
(Assignee)

Updated

11 years ago
Attachment #232290 - Flags: review?(stuart.morgan)

Comment 14

11 years ago
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-
(Assignee)

Comment 15

11 years ago
Created attachment 232650 [details] [diff] [review]
Patch

Sorry 'bout that.
Attachment #232290 - Attachment is obsolete: true
Attachment #232650 - Flags: review?(stuart.morgan)

Comment 16

11 years ago
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+
Whiteboard: [needs checkin]

Comment 18

11 years ago
Checked in on the trunk and MOZILLA_1_8_BRANCH.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 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.