454534 - Some browser commands should be disabled for about:blank

Status: NEW

(Firefox :: General, defect)

Description

[Nochum Sossonko [:Natch]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1

It would probably be more correct to just disable the "Bookmark This Page" in the Bookmarks menu, but this is the reality when clicking "Bookmark this Page" on a new tab the panel is incorrectly positioned.

Reproducible: Always

Steps to Reproduce:
1.open a new tab/window
2.Select Bookmarks-->Bookmark this Page
Actual Results:  
Panel is off to the left.

Expected Results:  
Bookmark this page in about:blank should be grayed out?

Comment 1

[Nochum Sossonko [:Natch]]

Just a note if it is disabled, just think of the many context menus that would need tweaking for this (i.e. browser context, tabs context, etc.).

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

confirmed

Comment 3

[Nochum Sossonko [:Natch]]

The real question is, where to put it? There is no star icon in about:blank page, so what would be more appropriate? Note: I *really* don't think it makes sense to disable the option, as there are so many paths to cover, this should be handled, the question is how?

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

As you said, the correct fix would probably to disable bookmarking for that page, since we do that for the star we should be able to do that for the Bookmark This Page.

Alternatively if we want to allow that or that's too complex, we could use the New Bookmark dialog instead of the contextual popup when the star is not available for an uri, but the preferred way of fixing this is the first imo.

Comment 5

[Dão Gottwald [:dao]]

Looks like this will be a bigger cleanup patch...

Comment 6

[Dão Gottwald [:dao]]

Attachment: patch

Comment 7

[Nochum Sossonko [:Natch]]

Can't you use (content.document == ImageDocument)?

Comment 8

[Dão Gottwald [:dao]]

We want to exclude more than just images there, like videos. The use of mimeTypeIsTextBased seems fine to me.

Comment 9

[Nochum Sossonko [:Natch]]

That's interesting, maybe that can be used for bug 462892 and bug 466909... instead of adding the interface.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

We should add the interface either way, since we need to be able to detect videos reliably. Excluding text/[x]html is not sufficient for controlling whether video-related commands appear.

Comment 11

[Dão Gottwald [:dao]]

Attachment: updated to trunk

Comment 12

[Nochum Sossonko [:Natch]]

Would be nice if this made 1.9.2

Comment 13

[Asaf Romano (gone)]

Comment on attachment 375524 [details] [diff] [review]
updated to trunk

1. The "This Frame" menu isn't handled, leading to inconsitten behavior (at least for page info).
2. I'm not sure that it's a smart idea to disable page info for about:blank. 
3. I don't like the changes to the all-tabs handler. I would rather either merge it to PlacesCommandHook somehow, or leave it as is (and please do not remove the queryinterface method).
4. nit: spaces between methods

Also, In context menus we generally hide rather than disable items. Ask ux.