Closed
Bug 172047
Opened 22 years ago
Closed 22 years ago
'Redo' item is missing in the context menu of text controls and urlbar
Categories
(Core :: Layout: Form Controls, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: MatsPalmgren_bugz, Assigned: timeless)
References
Details
Attachments
(1 file, 4 obsolete files)
7.18 KB,
patch
|
john
:
review+
bzbarsky
:
superreview+
asa
:
approval+
|
Details | Diff | Splinter Review |
'Redo' item is missing in the context menu of text controls. 'Redo' is the only one missing from those groups under the window Edit menu. I think it should be added for symmetri. Note: the keyboard shortcut (Ctrl+Y) already works. 2002-10-01-08 trunk Linux
Updated•22 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter | ||
Comment 1•22 years ago
|
||
Reporter | ||
Comment 2•22 years ago
|
||
The attached patch adds Redo to the text controls context menu, I also took the liberty of adding it to the urlbar context menu since it was missing there too. DISCLAIMER: I don't know this part of the code very well, I just copy-pasted the "undo" parts in these files and saw that the Redo item popped up in the context menus and seemed to work...
Comment 3•22 years ago
|
||
Ctrl+Y seems to be broken--it does the wrong redo, first off (if you type a long word quickly Undo gets rid of the entire word but redo seems to only redo a single letter and then you can't delete the letter that was redone. Probably the reason it wasn't simply enabled. I presume we're talking input type=text and textarea here?
Reporter | ||
Comment 4•22 years ago
|
||
> Probably the reason it wasn't simply enabled. So why is CTRL+Y enabled? ;-) > I presume we're talking input type=text and textarea here? Yes, and urlbar. I don't think the problem you found with the implementation of the Redo command should block this bug (I imagine that the kbd shortcuts are the more common method of invoking these commands anyway). BTW, did you report that bug or find a duplicate?
Comment 5•22 years ago
|
||
bug 171243 is the bug. All right, it's got a patch and everything, so I guess there's no need to make it block this. I'm perfectly fine with having a Redo menu in form controls. Someone who implemented menu stuff before and knows X[BU]L should review, though :)
Depends on: 171243
Cc brade@netscape.com for possible review.
undo and redo were never in the spec for the context menu of a webpage, so this patch prevents redo from landing and making the menu even longer than it should be. it also removes cut, paste and delete from webpages. So r=timeless for the original content and hopefully someone will r= my minor changes to nsContextMenu.js. If not then you can use my r= for attachment 101931 [details] [diff] [review] and just get that in (I won't be happy because I don't want undo/redo on the already overly long menus, but I'll live).
Comment 8•22 years ago
|
||
Dude, the menu isn't that big. It has 6 items on it.
um, wrong menu. open link in new window open link in new tab - bookmark this link save link target as... copy image location - save image as... send image... set as wallpaper - % undo * redo - + cut copy + paste + delete - select all - web search for "tinderbox" view selection source properties + items can never be enabled in this menu % same as +, but the spec clearly doesn't want it for this menu * same as %, but is new That's ok, only 24 items, I think it's possible to have more, ah yes, at the very least if the link was a mailto: i could have copy email address. The code does not remove undo or redo from places where they make sense. but it does remove them from places where they make no sense.
Comment 10•22 years ago
|
||
Comment on attachment 102233 [details] [diff] [review] same patch with more rules for readonly content I don't understand this change: + this.showItem( "context-selectall", this.isTextSelected ); Shouldn't SelectAll always be available?
Assignee | ||
Comment 11•22 years ago
|
||
redo + readonly content + upload fields upload fields don't actually work, but at least they don't look silly.
Attachment #101931 -
Attachment is obsolete: true
Attachment #102233 -
Attachment is obsolete: true
Comment 12•22 years ago
|
||
Comment on attachment 102286 [details] [diff] [review] redo + readonly content + upload fields (1) undo and redo do not depend on text being selected. It should just be this.onTextInput (2) Separators need to be shown iff the elements in the section are shown. This means context-sep-undo: this.onTextInput, context-sep-paste: true, context-sep-selectall: true Actually, WRT the selectors it seems best to me to get rid of context-sep-paste. It seems superfluous. But that is your call. An alternative would be to make context-sep-paste show only on this.onTextInput; otherwise, for non-text-inputs, we're going to have Copy and Select All both sitting in their very own sections of the menu. Other than that, r=me, though I'd like to see the final version (sorry about so many patches). So it looks like you found the file problem?
Attachment #102286 -
Attachment description: redo + readonly content + upload fields → redo + readonly content + upload fields
Comment 13•22 years ago
|
||
Comment on attachment 102286 [details] [diff] [review] redo + readonly content + upload fields (1) undo and redo do not depend on text being selected. It should just be this.onTextInput (2) Separators need to be shown iff the elements in the section are shown. This means context-sep-undo: this.onTextInput, context-sep-paste: true, context-sep-selectall: true Actually, WRT the selectors it seems best to me to get rid of context-sep-paste. It seems superfluous. But that is your call. An alternative would be to make context-sep-paste show only on this.onTextInput; otherwise, for non-text-inputs, we're going to have Copy and Select All both sitting in their very own sections of the menu. Other than that, r=me, though I'd like to see the final version (sorry about so many patches). So it looks like you found the file problem?
Assignee | ||
Comment 14•22 years ago
|
||
nope, i couldn't figure out how to get the file field to actually work, but at least we'll have the right menu items (always disabled). i think clipboard commands for file uploads are generally broken, the edit menu inconsistently updates.
Attachment #102286 -
Attachment is obsolete: true
Assignee | ||
Comment 15•22 years ago
|
||
ok, i think this is the best we're going to do. the remaining issue is that the context menu for file uploads will look right but won't really work. everything is much better with this patch than without it.
Attachment #102348 -
Attachment is obsolete: true
Comment 16•22 years ago
|
||
Comment on attachment 102372 [details] [diff] [review] now page selection things don't appear for text fields r=jkeiser
Attachment #102372 -
Flags: review+
Comment 17•22 years ago
|
||
> + this.showItem( "context-sep-selectall", this.isTextSelected &&
!this.onTextInput );
Why? Shouldn't this be on if and only if the "selectall" option is on?
Also, will "Copy" be properly disabled when no text is selected?
Other than that, looks good...
Comment 18•22 years ago
|
||
Comment on attachment 102372 [details] [diff] [review] now page selection things don't appear for text fields *timeless* copy will be disabled if there's nothing to copy but it will be visible, which is sort of traditional *timeless* the select all sep comes after select all *timeless* but in text areas, there's *nothing* after it *timeless* so we *don't* want a sep.. *timeless* if there's text selected then there's another group of items that appear *timeless* search for selection *timeless* view selection source *timeless* the sep goes before them sr=bzbarsky if you add a comment that explains the business with the separator.
Attachment #102372 -
Flags: superreview+
Comment 19•22 years ago
|
||
moa=jag
Comment 20•22 years ago
|
||
Comment on attachment 102372 [details] [diff] [review] now page selection things don't appear for text fields a=asa for checkin to 1.2 (on behalf of drivers)
Attachment #102372 -
Flags: approval+
Assignee | ||
Comment 22•22 years ago
|
||
checked in
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 23•22 years ago
|
||
It made no sense to "fix" this. Windows doesn't have Redo in textbox context menus, and for good reason -- it's hardly ever used. Blind consistency isn't always a good thing.
Comment 24•22 years ago
|
||
from http://bugzilla.mozilla.org/show_bug.cgi?id=192133#c5 jglick writes: "I would agree if we don't feel its a commonly used item, it shouldn't be in the context menu. In order to avoid huge context menus, we should limit them to the more frequently used actions."
Comment 25•22 years ago
|
||
this has moa from jag, so I guess I'm just beating a dead horse. see #192133 though, about a related issue for mailnews context menus.
You need to log in
before you can comment on or make changes to this bug.
Description
•