927783 - [Messaging][Forward] Per message context menu opens options/action menu (to incl. delete, forward)

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[Borja Salguero [:borjasalguero]]

We need to show an action menu with the options of deleting/forwarding (more options will be added in the near future!:) ).

Comment 1

[Rick Waldron [:rwaldron]]

There is a new action/option/whatever menu being introduced for setting subject, but I'm not sure that's the right place for this. A single menu is probably not a good mechanism for something that is per-message specific.

Comment 2

[Rick Waldron [:rwaldron]]

(Nevermind. https://bugzilla.mozilla.org/show_bug.cgi?id=927784#c1)

Borja, If you want help, please assign this to [e-mail], thanks.

Comment 3

[Borja Salguero [:borjasalguero]]

Attachment: Pull Request

Comment 4

[Borja Salguero [:borjasalguero]]

(In reply to Rick Waldron [:rwaldron][:rick][:rick waldron] from comment #1)
> There is a new action/option/whatever menu being introduced for setting
> subject, but I'm not sure that's the right place for this. A single menu is
> probably not a good mechanism for something that is per-message specific.

After talking with Ayman (WFs are coming :) ), the action in a single message will be a 'long press' one, and we will show an 'action-menu' (this name comes from UX specs) with some options: delete, forward... among others.
In this first patch we are adding 'delete' one, 'long press' event handling, and the logic needed for retrieving the messageID of the bubble. Next patch will be adding 'forwarding' feature. I hope it helps!

Comment 5

[Rick Waldron [:rwaldron]]

(In reply to Borja Salguero [:borjasalguero] from comment #3)
> Created attachment 818942 [details]
> Pull Request

schung, I had some time to review this and left several comments.

I'm wondering why this removes an existing capability where a contextmenu on "url-link" will behave as if it had been clicked. There is no reason to remove the capability, and certainly no reason to re-write the delegation handler (especially when that re-write appears to bring that code back to a form that we intentionally broke from).

There are also no tests include for this feature.

Comment 6

[Borja Salguero [:borjasalguero]]

Comment on attachment 818942 [details]
Pull Request

I've reverted the change made in `if/else` clause (although as Julien says, it's only a code styling issue!).
On the other hand the 'functionality' that you mentioned was a bug actually, due to a 'long-press' action in a url has *no action* due to there is no secondary options to be handled. We need to ensure to not confuse the user with unexpected behaviours, and this was one to be removed (actually I've added tests for ensuring that this is not going to happen in the future).
r? Thanks!

Comment 7

[Steve Chung [:steveck]]

Comment on attachment 818942 [details]
Pull Request

Hi Borja, sorry for the late. I left some comments in the patch. I have a major concern about the delete/update mechanism, The UI update will not take place immediately after delete action, but we don't apply any action to tell user what's goning on. Maybe we can improve the thread delete response time by reducing the threadList update and apply waitScreen when threadList updating, thanks.

Comment 8

[Rick Waldron [:rwaldron]]

Ayman, on the PR, Borja pointed out that there is no "long press" behaviour specified for URL links within messages:

> Page 27 of the WF. https://mozilla.app.box.com/s/wzgsb3lkqglv0dnfdgzs/1/864518456/7994811748/1 
> There is no secondary action in a link, so there is no long press action. That's why it's removed.

But Julien and I had agreed that long press on URL link should be the same as click on URL link (opens the link in browser) for accessibility. Can you advise? Thanks!

Comment 9

[Julien Wajsberg [:julienw]]

> But Julien and I had agreed that long press on URL link should be the same as click on URL
> link (opens the link in browser) for accessibility. Can you advise? Thanks!

Ayman, I also added that I'd agree with adding a "follow link" option to the context menu instead of using the "click" action.


(In reply to Steve Chung [:steveck] from comment #7)
> 
 Maybe we can improve the thread delete response time
> by reducing the threadList update and apply waitScreen when threadList
> updating, thanks.

I think we should not display the wait screen when thread list is updating (this was the conclusion of bug 893838). Here we're in the thread view, therefore the user does not see the thread list anyway.

Comment 10

[Borja Salguero [:borjasalguero]]

> I think we should not display the wait screen when thread list is updating
> (this was the conclusion of bug 893838). Here we're in the thread view,
> therefore the user does not see the thread list anyway.

Im currently working in a new approach, trying to remove all 're-rendering' stuff of the thread-list and only updating it. I was testing and it was working so good now (probably the profiling will show even a better result). When all tests will be working as expected I will update the patch.

Comment 11

[Julien Wajsberg [:julienw]]

Borja, this is really good, but can you do this in another patch in bug 854417 ?

Will make it easier to review :)

Comment 12

[Borja Salguero [:borjasalguero]]

At the end it's not a big change, and it was requested by the reviewer. The approach tries to avoid to call 'renderThreads', and instead we are updating the thread element in the list :). I've added tests and the old ones are updated with the new logic.

Comment 13

[Borja Salguero [:borjasalguero]]

Comment on attachment 818942 [details]
Pull Request

Steve, I've updated the patch with your suggestion. Now we are not 're-rendering' the entire list, only updating it! :) r?

Comment 14

[Rick Waldron [:rwaldron]]

(In reply to Borja Salguero [:borjasalguero] from comment #13)
> Comment on attachment 818942 [details]
> Pull Request
> 
> Steve, I've updated the patch with your suggestion. Now we are not
> 're-rendering' the entire list, only updating it! :) r?

I've removed the r=rwaldron from the PR because I haven't r+'ed this patch—please don't forge my r+. 

There are still no tests that support handling a click on a message bubble. Please add these tests and r? me. Thanks

Comment 15

[Borja Salguero [:borjasalguero]]

> I've removed the r=rwaldron from the PR because I haven't r+'ed this
> patch—please don't forge my r+. 

It was not my intention Rick. As you have more reviews regarding the Subject (Julien is so busy as well) that's why from the beginning I decided to move to other peer to have this review (check the story of this bug and you will realize about this), but I really appreciate your comments and suggestions. I'll add you as well as reviewer of this patch, and let's wait until having feedback from Ayman regarding the 'click' action in the url. Thanks again and sorry for the misunderstanding.

Comment 16

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #9)
> > But Julien and I had agreed that long press on URL link should be the same as click on URL
> > link (opens the link in browser) for accessibility. Can you advise? Thanks!
> 
> Ayman, I also added that I'd agree with adding a "follow link" option to the
> context menu instead of using the "click" action.
> 
> 
> (In reply to Steve Chung [:steveck] from comment #7)
> > 
>  Maybe we can improve the thread delete response time
> > by reducing the threadList update and apply waitScreen when threadList
> > updating, thanks.
> 
> I think we should not display the wait screen when thread list is updating
> (this was the conclusion of bug 893838). Here we're in the thread view,
> therefore the user does not see the thread list anyway.

I agree that we should remove waitScreen in the end for better use experience, but just like you said in comment 11, I would prefer the new thread list update mechanism in bug 854417, and just improve the thread delete performance if there is no need to update threadlist. But since the patch is right here, it's still a great job to fix the performance issue, I will take some time for reviewing this. Thanks Borja!

Comment 17

[Steve Chung [:steveck]]

(In reply to Borja Salguero [:borjasalguero] from comment #13)
> Comment on attachment 818942 [details]
> Pull Request
> 
> Steve, I've updated the patch with your suggestion. Now we are not
> 're-rendering' the entire list, only updating it! :) r?

You've done more than I expected! That works really good with huge performance improvement. Maybe you can also close bug 854417 since you also fixed other threadlist rendering issue in this patch. I've left some comments in github but I haven't found any serious issue yet. Let's wait for Ayman's comment now :)

Comment 18

[ayman maat :maat]

> Ayman, on the PR, Borja pointed out that there is no "long press" behaviour
> specified for URL links within messages:
> 
> > Page 27 of the WF. https://mozilla.app.box.com/s/wzgsb3lkqglv0dnfdgzs/1/864518456/7994811748/1 
> > There is no secondary action in a link, so there is no long press action. That's why it's removed.
> 
> But Julien and I had agreed that long press on URL link should be the same
> as click on URL link (opens the link in browser) for accessibility. Can you
> advise? Thanks!

Hey Rick, yes i am aware of the discussion on 12935 and am totally on board with what you have implemented in relation to accessibility, it was a good move. However there is a wider context that we need to consider than i have seen already discussed here or in 12935. 

Primarily we need to provide each individual message with its own menu of secondary activities (to forward, view delivery/read receipts, view message details etc etc) in order to fulfil must have requirements for 1.3.  'on tap' is clearly reserved for primary activities like opening attachments, actioning urls, phone numbers and email addresses that are contained within the given message module…So we cannot use that action. Long press, therefore, would be the next most pragmatic and findable action to use to initiate the menu and that contains the aforementioned secondary activities. 

I am of the opinion however that the implementation of long press for the secondary activity menu will not only mean sacrificing long press for directly opening links, but it will also mean that we have to sacrifice the implementation of long press on email addresses and phone numbers contained in the given message module. The reason for this is that, from a usability and affordance perspective, having the whole module 'long pressable' to initiate the secondary menu is far more accessible and user friendly than attempting to maintain the long press behaviour on urls, phone numbers and email addresses and trying to make long press anywhere else in the message module trigger the modules secondary menu. Following that path would lead to something of a fiddly experience (consider the situation where a module contains only links and no text that increases the target area for the modules secondary menu long press).

What i propose we do is this.

1) remove all long press behaviour for urls, emails and phone numbers and move this behaviour to on tap (see point 4 below)
2) make long press on the whole message module and long press all urls, emails and phone numbers (but not for the attachments) trigger the modules secondary menu containing activities to forward, view delivery/read receipts, view message details etc etc
3) tap on url opens url in browser
4) tap on phone number:
	if the phone number is contained in contact list
		opens menu that offers the CTAs:
			'call'
			'message' (this is a new CTA, but a pragmatic extension of the existing functionally)
	if the phone number is not contained in the contact list
		open a menu that offers the CTAs: 
			'call', 
			'message' (this is a new CTA, but a pragmatic extension of the existing functionally)
			'create new contact', 
			'add to existing contact'
5) tap on email address:
	if the email address is contained in the contact list
		directly open up a compose new email dialogue with the email address in the 'to field'
	if the email address is not contained in the contact list
		open a menu that offers the  CTAs: 
			'email', 
			'message' (this is a new CTA and an extension of the V1.3 requirement to allow messages to be sent to email addresses), 
			'create new contact', 
			'add to existing contact'

How does that feel for you?

Comment 19

[Borja Salguero [:borjasalguero]]

> How does that feel for you?

For me it's ok, Im going to adapt current code to new requirements. Thanks Ayman for your feedback!

Comment 20

[Julien Wajsberg [:julienw]]

Ayman,

You didn't comment about my proposal to add the "onclick" default behaviour in the "longpress" context menu. Eg, when the user longpresses an url link, it would have the normal longpress menu + a "follow the link" item. When the user longpresses on an email link, it would have the normal longpress menu + the items he would have had when tapping it, etc.

Comment 21

[Rick Waldron [:rwaldron]]

The current version of this patch breaks unrelated functionality during the creation of new threads when a message is received, specifically the contact lookup and matching operation.

This is what it should do (on master) http://www.youtube.com/watch?v=LOrVWs4Uab0

This is the bug (on uk_v1.2_sprint3_forward) http://www.youtube.com/watch?v=LOrVWs4Uab0

Comment 22

[Borja Salguero [:borjasalguero]]

> This is the bug (on uk_v1.2_sprint3_forward)
> http://www.youtube.com/watch?v=LOrVWs4Uab0

This is Desktop mockup, have you tried a real device? In my device is working as expected. I will upload a video with the STR you mentioned.

Comment 23

[ayman maat :maat]

(In reply to Julien Wajsberg [:julienw] from comment #20)
> Ayman,
> 
> You didn't comment about my proposal to add the "onclick" default behaviour
> in the "longpress" context menu. Eg, when the user longpresses an url link,
> it would have the normal longpress menu + a "follow the link" item. When the
> user longpresses on an email link, it would have the normal longpress menu +
> the items he would have had when tapping it, etc.

Hey Julien

Sorry for not being explicit in addressing your proposal. It was given quite some consideration as it certainly offered a valid path towards solution. However there were two reasons it was not followed i the end: 

Firstly we need to introduce options when the user acts on content in the instance that content is either a phone number (dial or message) or email address (email or message). This means that the user will no longer instantly go straight to dialler or email app.

Secondly, in concatenation with the above as there is now already a menu in place, for the benefit of discoverability and learnability and to reduce complexity a design decision was made to make a clear interaction division between functions that act on the object itself and functions that act on the object's content. It was decided that this interaction distinction should be defined by the users primary (tap) and secondary (long press) interaction with the interface during exploration. therefore: 

- 'Tap' was designated to act on content as content is in all probability the most important item on the interface to the user and tap is their primary action when foraging.

- 'Long press' was designated to act on the object (message module itself) as the object is in all probability of secondary importance to the end user and long press is their a secondary action when foraging.

Following this architecture we deliver a much cleaner and defined user experience. If we then start to surface functions dedicated to content along with functions dedicated to the object via the action defined for the object we start to duplicate CTAs and muddy the waters again.

Comment 24

[Rick Waldron [:rwaldron]]

(In reply to Borja Salguero [:borjasalguero] from comment #22)
> > This is the bug (on uk_v1.2_sprint3_forward)
> > http://www.youtube.com/watch?v=LOrVWs4Uab0
> 
> This is Desktop mockup, have you tried a real device? In my device is
> working as expected. I will upload a video with the STR you mentioned.

No, it's broken the device as well. http://www.youtube.com/watch?v=s3-t_jLv9d4

Comment 25

[Rick Waldron [:rwaldron]]

Attachment: This is the view after pressing send

Comment 26

[Rick Waldron [:rwaldron]]

Attachment: This is the thread list after pressing "Back" from the new conversation view

Comment 27

[Borja Salguero [:borjasalguero]]

> No, it's broken the device as well.
> http://www.youtube.com/watch?v=s3-t_jLv9d4

Are u testing the final patch? Here is the result in my device http://youtu.be/H22_1V38aVM , and as you can see it's working. Which BUILD are u using for testing?

Comment 28

[Julien Wajsberg [:julienw]]

(In reply to ayman maat :maat from comment #23)
> (In reply to Julien Wajsberg [:julienw] from comment #20)
> > Ayman,
> > 
> > You didn't comment about my proposal to add the "onclick" default behaviour
> > in the "longpress" context menu. Eg, when the user longpresses an url link,
> > it would have the normal longpress menu + a "follow the link" item. When the
> > user longpresses on an email link, it would have the normal longpress menu +
> > the items he would have had when tapping it, etc.
> 
> Hey Julien
> 
> Sorry for not being explicit in addressing your proposal. It was given quite
> some consideration as it certainly offered a valid path towards solution.
> However there were two reasons it was not followed i the end: 
> 
> Firstly we need to introduce options when the user acts on content in the
> instance that content is either a phone number (dial or message) or email
> address (email or message). This means that the user will no longer
> instantly go straight to dialler or email app.

I agree. (and to be honest, I welcome this, as we might tap on this accidentally, it's good to be able to cancel it, especially because we don't have (currently ?) an easy way to go back once the activities are triggered)

> 
> Secondly, in concatenation with the above as there is now already a menu in
> place, for the benefit of discoverability and learnability and to reduce
> complexity a design decision was made to make a clear interaction division
> between functions that act on the object itself and functions that act on
> the object's content. It was decided that this interaction distinction
> should be defined by the users primary (tap) and secondary (long press)
> interaction with the interface during exploration. therefore: 
> 
> - 'Tap' was designated to act on content as content is in all probability
> the most important item on the interface to the user and tap is their
> primary action when foraging.
> 
> - 'Long press' was designated to act on the object (message module itself)
> as the object is in all probability of secondary importance to the end user
> and long press is their a secondary action when foraging.
> 
> Following this architecture we deliver a much cleaner and defined user
> experience. If we then start to surface functions dedicated to content along
> with functions dedicated to the object via the action defined for the object
> we start to duplicate CTAs and muddy the waters again.

I understand the rationale, thanks.

My concern here is about accessibility (again): some people with disabilities (could be also simply people that are not used to technology) might press longer than expected for a simple click, and get the "object actions" menu instead of the "content actions" menu.

I agree that we can't just display all actions in a 6-item (or more) menu, this will clutter the menu. I have ideas but none looks good enough to me (eg: have an item that would "go to the content menu").

Asking a final NI for a final feedback, and then we'll go on with the proposed solution.
Thanks !

Comment 29

[Rick Waldron [:rwaldron]]

(In reply to Borja Salguero [:borjasalguero] from comment #27)
> > No, it's broken the device as well.
> > http://www.youtube.com/watch?v=s3-t_jLv9d4
> 
> Are u testing the final patch? Here is the result in my device
> http://youtu.be/H22_1V38aVM , and as you can see it's working. Which BUILD
> are u using for testing?

I was using your latest that was on github, at the time I replied.

Comment 30

[Borja Salguero [:borjasalguero]]

> I was using your latest that was on github, at the time I replied.

I would like to check the same STR as you, with your same conditions, because I can not reproduce with Today's build, with my Gaia branch applied. Could you specify the steps to reproduce & Gecko version? Thanks!

Comment 31

[ayman maat :maat]

(In reply to Julien Wajsberg [:julienw] from comment #28)
 
> My concern here is about accessibility (again): some people with
> disabilities (could be also simply people that are not used to technology)
> might press longer than expected for a simple click, and get the "object
> actions" menu instead of the "content actions" menu.
> 
> I agree that we can't just display all actions in a 6-item (or more) menu,
> this will clutter the menu. I have ideas but none looks good enough to me
> (eg: have an item that would "go to the content menu").
> 
> Asking a final NI for a final feedback, and then we'll go on with the
> proposed solution.
> Thanks !


Hey Julien, 

I am well pleased that you consistently have an eye on accessibility. I notice accessibility is one of those areas that sadly is frequently overlooked in products these days in favour of something novel or 'cool'. But its important. Very much so.

Anyway, I too have accessibility concerns. But i am viewing them from a slightly different angle to you. Your concern is about the speed with which a user can achieve the tap action, that they are too slow and therefore activate long press in error. That is a valid concern, but i think it is something we should handle system wide, within the settings app, because if the user has interaction problems here differentiating between tap and long press, they have problems everywhere on the phone. 

What i would like to see is a dedicated accessibility area within the phones settings that handles global accessibility settings such as: length of time to trigger long press, adjustable font size, external button to end call, magnification gestures, voice control …and such of the like. This is the approach we should take to your accessibility concern rather than trying to make fixes on a per instance basis. 

…Maybe we should open a bug to raise a request for such an area in settings?

The accessibility concern that i have is more about selectability within clusters of an objects content when users have motor restrictions. My main concern is that when there are several selectable content items in close proximity (urls, phone numbers, email address) users with motor restrictions might aim for one and get another in error. If this is the case it something that we cannot handle at a system level and have to handle on a per instance basis. 

In consideration of this: Firstly the ability for users to select one content item from a cluster of selectable content items has been around since 1.2, and to date i have not been made aware of any usability issues. Secondly with this in mind I also recently commissioned one of the testers to run some tests for me to give an objective opinion about ease of selectability of clustered content items within a message container, and they reported no concerns. Thirdly a majority of our benchmarks do not handle this situation (although this is not something i put much weight on). So these three items coupled together with an eye on the fact that addressing my accessibility concern would require quite a bit of design rework/thinking both in IxD and Patters, time we don't seem to have, I took a punt and shelved pursuing incorporating it now in favour of the design solution i have described in comment 23 so we can meet our deadlines. Its a risk i know, and i am not saying that the accessibility issue is absolutely not there but i would prefer to go with what I have prescribed so we meet our deadlines rather than burn project time folding something into the design that i cannot conclusively say is also an issue….

In the mean time what i prose to do is keep an eye out for such accessibility issues being raised and also in my down time pursue proposals towards what a solution to address what these could look like. (the last thing is something i am going to be doing anyway, because i am a designer and i cannot stop myself from thinking about it)

Comment 32

[Steve Chung [:steveck]]

I could not reproduce this issue with same STR, the thread title will update after thread update(send/delete). Another known issue but might not be related to this issue is:
1 ) Entering a thread without contact name
2 ) Tap on the number and add the number into contact list
3 ) Back to threadlist page without any thread update. Thread title still shows number in the threadlist page

I think this problem is not related to this patch, so I still wondering Rick's reproduce step. Hi Rick, would you mind sharing more detail about your step between opening the app and sending the message?

Comment 33

[Joe Cheng [:jcheng] (please needinfo)]

blocking a committed 1.3 user story. 1.3+

Comment 34

[Rick Waldron [:rwaldron]]

(In reply to Steve Chung [:steveck] from comment #32)
> so I still wondering
> Rick's reproduce step. Hi Rick, would you mind sharing more detail about
> your step between opening the app and sending the message?

There is nothing missing from the video. I selected a known contact, typed "test" and hit "send". Once the message is sent and I'm at the conversation view, I tap "back" and the thread that was created hasn't been updated with contact name yet (because the app has always done that in two steps).

Comment 35

[Julien Wajsberg [:julienw]]

Reply to comment 31: Ayman, thanks for the lengthy answer :) Let's move forward with your proposed solution now!

We do have a dedicated team for FxOS accessibility and it could be a good idea to talk with them.

Comment 36

[Borja Salguero [:borjasalguero]]

(In reply to Rick Waldron [:rwaldron] from comment #34)

> There is nothing missing from the video. I selected a known contact, typed
> "test" and hit "send". Once the message is sent and I'm at the conversation
> view, I tap "back" and the thread that was created hasn't been updated with
> contact name yet (because the app has always done that in two steps).

Rick, I've tested exactly your STR

Comment 37

[Borja Salguero [:borjasalguero]]

(In reply to Borja Salguero [:borjasalguero] from comment #36)
> (In reply to Rick Waldron [:rwaldron] from comment #34)
> 
> > There is nothing missing from the video. I selected a known contact, typed
> > "test" and hit "send". Once the message is sent and I'm at the conversation
> > view, I tap "back" and the thread that was created hasn't been updated with
> > contact name yet (because the app has always done that in two steps).

Rick, I've tested exactly your STR, and as you can see here http://www.youtube.com/watch?v=nDEa_zm0v_A it's working as expected... Could you try again? This is my patch + Today's B2G. Steve was testing the same and everything was working as well. Could you re-test this please? Thanks.

Comment 38

[Rick Waldron [:rwaldron]]

(In reply to Borja Salguero [:borjasalguero] from comment #37)
> (In reply to Borja Salguero [:borjasalguero] from comment #36)
> > (In reply to Rick Waldron [:rwaldron] from comment #34)
> > 
> > > There is nothing missing from the video. I selected a known contact, typed
> > > "test" and hit "send". Once the message is sent and I'm at the conversation
> > > view, I tap "back" and the thread that was created hasn't been updated with
> > > contact name yet (because the app has always done that in two steps).
> 
> Rick, I've tested exactly your STR, and as you can see here
> http://www.youtube.com/watch?v=nDEa_zm0v_A it's working as expected... Could
> you try again? This is my patch + Today's B2G. Steve was testing the same
> and everything was working as well. Could you re-test this please? Thanks.

Tested with latest gaia and latest version of this patch and the issue no longer exists. Thanks for your patience and follow-up

Comment 39

[Rick Waldron [:rwaldron]]

Comment on attachment 818942 [details]
Pull Request

r+ me after a rebase

Comment 40

[Borja Salguero [:borjasalguero]]

Comment on attachment 818942 [details]
Pull Request

Steve, I've addressed all your comments in the PR, and Rick reviewed all changes together and R+ this code. Im gonna merge this for delivering the second patch. Thanks a lot for your support and your suggestions! :)

Comment 41

[Steve Chung [:steveck]]

(In reply to Borja Salguero [:borjasalguero] from comment #40)
> Comment on attachment 818942 [details]
> Pull Request
> 
> Steve, I've addressed all your comments in the PR, and Rick reviewed all
> changes together and R+ this code. Im gonna merge this for delivering the
> second patch. Thanks a lot for your support and your suggestions! :)

Good to see the patch landed. It's ok for me since the issues I pointed out were not serious blocking problem. Great job Borja!