46485 - 3pane context menu: Text Selected should match spec

Status: RESOLVED WONTFIX

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Ninoschka Baca]

Build 2000-07-25-09M17: NT4
Haven't checked Mac or Linux yet.

Overview: In the Message Pane, within the body/content area, highlight specific 
text and its context menu should appear with one item:

- Copy

Currently the product shows many more context items than are specified in the 
spec. Let me know if you need a list of these extra items.

Comment 1

[jglick]

Yup, "Copy" was what was agreed upon in the context menu review meeting.

Comment 2

[lchiang]

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.

Comment 3

[timeless]

Stealing from putterman.

Comment 4

[timeless]

accepting.

Comment 5

[James Green]

Stealing from timeless... I have a fix.

Comment 6

[James Green]

OK the following patch is MY FIRST. Treat with extreeme CAUTION. You have been
warned. It works perfectly in my Linux debug build.

It adds one function to handle the changing of the Copy|Copy To menus, and
switches the other items off if we detect a selection has been made. And of
course it reverts back following clearing of the selection. Tiny whitespace
fixes included.

Comment 7

[James Green]

Attachment: Patch fixing bug

Comment 8

[James Green]

Attachment: Patch v2 (-uw)

Comment 9

[James Green]

Patch 38329 is taken -uw per hwaara's suggestion. He also suggested renaming the
variable |numSelected| in |fillMessagePaneContextMenu(contextMenu)| to
|msgIsLoaded| since that seems more appropriate - that change is restricted to
that one function though and has been tested.

Looking r/sr.

Comment 10

[Håkan Waara]

To add, that particular variable was used for two purposes: both as a boolean
(whether a msg was loaded) and an integer (how many selections in the threadpane).

Yes you addressed all my concerns. r=hwaara. Good work James!

CC bienvenu for super-review.

Comment 11

[David :Bienvenu]

msgIsLoaded seems like a boolean var, so we should be using true and false, not
1 and 0. And then when we're checking its value, we should just be saying
"msgIsLoaded", not msgIsLoaded == 1.

Comment 12

[James Green]

Yeah, I know :).

The reason why it's not a boolean is because the var |numSelected| is defined
elsewhere and given the number of messages selected (makes sense). The version I
changed to |msgIsSelected| was instead given the value 1 or 0 to indicate
whether any messages had been loaded. Either way, the |SetupXXXMenuItem()|
functions take the same parameter and check the value as the mumber of messages
selected (0, 1, or above).

So you effectively have the same parameter used is two slightly differing ways.
The |SetupXXXMenuItem()| function certainly test the parameter as a number, not
a boolean. So I guess the patch has to stand as it is, unless anyone has any
other bright ideas to make it clearer?

Comment 13

[David :Bienvenu]

Ah, then I think perhaps it would be clearer to leave it as numSelected, since
msgIsLoaded has a "boolean" name, but is used everywhere as an integer. Or maybe
it would make more sense to have two vars "numSelected" which you use when you
need to pass an integer value, and msgIsLoaded when you need a boolean value,
that you would then use as a boolean. 

And since I'm coming to this late, why are we no longer checking hideMailItems?
Is that intentional?

Comment 14

[James Green]

OK I've figured out what's happening.

To answer your queries about |numSelected| and |msgIsLoaded|, they do need to be
kept as integers since the functions get get passed them act on them as the
number of messages selected, be it 0, 1, more than one, or a range. I don't
think boolean will play very happily here. The fact that we have making our
version an integer 0 or 1 makes it play well with the functions, so I'll keep as
is unless I magically get time to test it as a boolean.

On the hideMailItems, yes it is intentional. hideMailItems is a boolean which
indicates whether any of the global menu items are visible, such as "Save Image"
or "Edit Link in Composer"; if any of those are visible, then we want to hide
the Mail menu items. Clearly, if we only want to show ONE item, "Copy", it is
natural to force the use of hideMenuItems to cut the lines of code down.

The downside is that now I know what hideMenuItems does, I have to figure out
how to switch off the browser general items. Soon as that's done, I'll attach a
new patch for review.

Comment 15

[James Green]

There is some confusion in the specs.

Situation: I have a paragraph of text selected which contains one or more links
or images. There are four possible ways to get a context menu:

1.  Right-click over plain text (only "Copy" should be present)
2.  Right-click over whitespace (only "Copy should be present)
3.  Right-click over one of the links within the selection (unclear)
4.  Right-click over one of the images within the selection (unclear)

From the specs, the situation is defined as:
    "Part or All of Context Area Selected
    "User specifically selects content in the message body,  which may or may
     not include Text, Links, Images, etc."
And then right-clicks over which of the four possibilities? Three of them could
have unique menu items.

From the specs, the menu result for the above quoted situation is:
    "Copy --- Ctrl+C
    "Add Copy to menu above below "Select All""
Problems with this:
a)  Clearly it should go above or below "Select All". But which?!
b)  I thought if we had a selection, only "Copy" should be present? Why then, do
    we describe _adding_ it to "Select All"?

Jennifer, you need to specify the context menus on selection more precisely when
right-clicking over a link or image.

Another problem, what do we do if we right-click over a link or an image which
is _not_ contained within the selection?

-  Ignore the fact that there is a selection or
-  Offer only "Copy" as if we have right-clicked over whitespace or plain text
   within the selection?

Comment 16

[James Green]

Assuming on right-clicking a link or image inside a selection, only the context
menu items specifically pertaining to the link or image ("Copy Link Location"
and not "Copy" or "Select All" etc.), then I have the fix in my tree, based on
my patch in bug 46484. Soon as that gets checked in I'll attach an updated patch
(unless jglick disagrees).

jglick: I'm available on irc.mozilla.org #mozilla much of the time if you can
get to irc and meet.

Comment 17

[jglick]

James, sorry for the confusion.  Attempt to clarify below. :-) For 3 and 4 
below, would like your opinion. Thanks.

>Situation: I have a paragraph of text selected which contains one or more links
>or images. There are four possible ways to get a context menu:

>1.  Right-click over plain text (only "Copy" should be present)
correct.

>2.  Right-click over whitespace (only "Copy should be present)
correct.

>3.  Right-click over one of the links within the selection (unclear)
I'm not sure. Either just copy should be present, since user specifically 
selected a chuck of content. Or should just items specific to a link be 
available? or both?  
  
>4.  Right-click over one of the images within the selection (unclear)
Again, this could go several ways like above. 

>Another problem, what do we do if we right-click over a link or an image which
>is _not_ contained within the selection?

I would prefer: Offer only "Copy" as if we have right-clicked over whitespace or 
plain text within the selection.

Comment 18

[jglick]

My inclination for items 3 and 4 would be to show:

Item 3: Copy plus Link menu shown in spec
Item 4 Copy plus Image menu shown in spec

Since the actions most users will probably want is to copy all the select text 
OR perform an action specific to the link or image hovered over.

Comment 19

[James Green]

I agree. I suggest the argument that if the user is presented with a paragraph
mostly containing links, then he would have to specfically right-click between
one of the links to get a Copy menu if we chose not to have Copy in the Image
selected menu. This would present the user with a problem.

I'll ensure the "Copy" menu is present no-matter what you click on, if you have
a selection.

Comment 20

[Blake Ross]

Doesn't look like this is getting fixed before the freeze tomorrow night.
Pushing out a milestone.  Please correct if I'm mistaken.

Comment 21

[chris hofmann]

doesn't look like this made the 0.9.4 train. have a look and get a good
milestone assigned.  thanks -chofmann

Comment 22

[Olga]

Just a note when verify to check Stand Alone window as well.

Comment 23

[Olga]

*** Bug 129149 has been marked as a duplicate of this bug. ***

Comment 24

[James Green]

Bouncing out, hope someone with more time than I catches.

Comment 25

[Philip Chee]

Spec is out of date.