Closed Bug 172047 Opened 20 years ago Closed 19 years ago

'Redo' item is missing in the context menu of text controls and urlbar

Categories

(Core :: Layout: Form Controls, defect)

x86
Linux
defect
Not set
trivial

Tracking

()

RESOLVED FIXED

People

(Reporter: mats, Assigned: timeless)

References

Details

Attachments

(1 file, 4 obsolete files)

'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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch Patch (obsolete) — Splinter Review
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...
Keywords: patch, review
Summary: 'Redo' item is missing in the context menu of text controls → 'Redo' item is missing in the context menu of text controls and urlbar
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?
> 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?
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).
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 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?
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 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 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?
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
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 on attachment 102372 [details] [diff] [review]
now page selection things don't appear for text fields

r=jkeiser
Attachment #102372 - Flags: review+
> +        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 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+
Depends on: 106354
Keywords: review
moa=jag
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: jkeiser → timeless
checked in
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
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.
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."
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.