Closed Bug 46484 Opened 24 years ago Closed 15 years ago

3pane context menu: News Content Area selected should match spec

Categories

(SeaMonkey :: MailNews: Message Display, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: nbaca, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: polish, Whiteboard: need sr=)

Attachments

(3 files)

Build 2000-07-25-09M17: NT4 Haven't checked Mac or Linux yet. Overview: In a News message, go to the Message Body/Content Area. Right click onto any area in the body with nothing specific selected. The context menu that appears should match the spec and should appear as follows: Open in New Window Select All ------------------- Reply to Sender Reply to Newsgroup Reply to All Forward Edit As New ------------------- Move To > (I think this should be removed from the spec for a News message) Copy To > Save As... Print... Delete ------------------- Add Sender to Address Book Add All To Address Book For the product to match the spec, the following should happen: 1. Remove a. "Copy" from the first section 2. Change a. "?Reply To All" should remove the question mark and appear as "Reply To All"
Keywords: nsbeta3, polish
QA Contact: lchiang → nbaca
Looks good
per mail triage: remove nsbeta3 nomination and move to future milestone. For the short term, we will combine items here into three bugs (in 3pane) to address for nsbetat3: 1. delete, change text, move menuitems; 2. enable/disable and text changes on context (dynamic); 3. accelerator keys to work (ie. what is shown in the menus actually does something) When this bug gets revisited after the first release, we'll need to see what items remaining that need to be addressed.
Keywords: nsbeta3
Target Milestone: --- → Future
Taking.
Assignee: putterman → jg
Keywords: ui
OS: Windows NT → All
Priority: P3 → P1
Target Milestone: Future → mozilla0.9.2
Is "Ca_ncel" supposed to be here? Intertwines with bug 83203, jglick's say needed.
Attached patch Proposed fix (deleted) — Splinter Review
Patch 38505 does the following: * Removes the "Copy" menu entirely should no selection be found * Adds the "Copy" menu should a selection be found AND the user has clicked a link or image (since there's still a selection that can be copied) Does this on both Mail and News message bodies. r/sr wanted.
Status: NEW → ASSIGNED
Keywords: patch, review
Addressing the original comment: Open in New Window -- not in current spec, so left out Move To -- is in current spec, but not in current implementation and makes no sense, so left out Address Book items -- not in current implementation nor spec, so left out. Mnemonics clash in the spec (Re_ply to All | _Print). Address those in another bug.
+ if (top.frames['messagepane'].getSelection().toString().length < 1) That seems like more of a hack than a fix to me. You don't need to check the length of the string, instead check if toString() returns "" (nothing).
Attached patch Updated per hwaara's comments (deleted) — Splinter Review
>Is "Ca_ncel" supposed to be here? Intertwines with bug 83203, jglick's say needed. Yes, Cancel instead of Delete for News. >Removes the "Copy" menu entirely should no selection be found >Adds the "Copy" menu should a selection be found AND the user has clicked a >link or image (since there's still a selection that can be copied) Are you adding Copy as an additional item to the existing menu below "Select All"? Shouldn't this be when there is a selection OR when the user has clicked a link or image? >Open in New Window -- not in current spec, so left out Good. Thanks. >Move To -- is in current spec, but not in current implementation and makes no >sense, so left out. Correct, shouldn't be there. >Address Book items -- not in current implementation nor spec, so left out. Good. Thanks. >Mnemonics clash in the spec (Re_ply to All | _Print). Address those in another bug. D'oh. Thought i caught all those. Lets go with "Reply _to All". Will update spec to reflect your changes. Thanks for the great work. :-)
> >Removes the "Copy" menu entirely should no selection be found > >Adds the "Copy" menu should a selection be found AND the user has clicked a > >link or image (since there's still a selection that can be copied) > Are you adding Copy as an additional item to the existing menu below "Select > All"? Yes. > Shouldn't this be when there is a selection OR when the user has clicked a > link or image? Undefined. In the specs you deal with selection, and links, but not both at the same time :). For example, if you have a paragraph with a link in the middle, and you selected the entire paragraph, you can either right-click on the link or not. Either way, there is still a selection to be copied, and if you happen to have clicked over the link the "Copy Link Location" option is there for that link specifically. IMO, the specs should be changed to say that if you have a selection and you are right-clicking over a link or image, the "Copy" menu and "Select All" menus should still be present as it is quite possible/probable that those options are the ones the user wants. I can change this behaviour quickly if you really only want "Copy" to appear on selection and NOT on link/image.
What you said seems correct. :-) User specifically selects content in the message body, which may or may not include Text, Links, Images, etc. Then Copy is included in the context menu below "Select All". If user invokes content menu directly over a link or image, regardless of whether paragraph of text is selected or not, they should get "Copy Link" or "Copy Image" as appropriate.
please seriously consider |if (!top.frames['messagepane'].getSelection().toString())| and is there any way to avoid toString()? it would appear to be rather expensive given all you need to know is if there is a selection.
Attached patch Better fix (deleted) — Splinter Review
Patch 38741 does the following: * Removes "Copy" if no selection is present * Adds "Copy" is there is a selection and pointer is over plain text or whitespace. If you want "Copy" present if the pointer is over a link or image too, please state so in bug 46485. * "Can_cel" becomes "Ca_ncel" to avoid conflict * In Messages drop-down as well as context menu, "Re_ply to All" becomes "Reply _to All" to avoid conflict * Uses |.getSelection.isCollapsed| instead of a comparison per smfr's optimisation advice. I bow to his knowledge. Looking for r/sr again.
r=fabian great job jmkg!
CC bienvenu for sr.
Whiteboard: need sr, a
*** Bug 83203 has been marked as a duplicate of this bug. ***
*** Bug 46483 has been marked as a duplicate of this bug. ***
Darn, bienvenu went on vacation. CC Seth for the sr=.
I think the patch in bug 72611 should go in first, and an updated patch attached here.
Whiteboard: need sr, a → need sr=
Target Milestone: mozilla0.9.2 → mozilla0.9.3
*** Bug 88773 has been marked as a duplicate of this bug. ***
Note to check the Mail vs News specific items when verifying this bug.
Missed the 0.9.3 train.
Target Milestone: mozilla0.9.3 → mozilla0.9.4
looks like it didn't make the 0.9.4 train. have a look an get a good milestone assigned... thanks -chofmann
Target Milestone: mozilla0.9.4 → ---
QA Contact: nbaca → olgam
Should get to this next week.
Priority: P1 → P3
status?
Keywords: mozilla0.9.9
Bouncing out, hope someone with more time than I catches.
Assignee: jg → sspitzer
Status: ASSIGNED → NEW
Blocks: 158011
Product: Browser → Seamonkey
Assignee: sspitzer → mail
Assignee: mail → nobody
Priority: P3 → --
QA Contact: olgam → message-display
Only remaining issue is that the Copy menu item is disabled instead of hidden. Closing as FIXED. File a new bug if you think the menu item should be hidden instead.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: