Closed Bug 1092437 Opened 10 years ago Closed 9 years ago

Messages app should follow text selection pattern

Categories

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

x86
macOS
defect
Not set
normal

Tracking

(feature-b2g:2.2+, ux-b2g:2.2)

RESOLVED FIXED
2.2 S2 (19dec)
feature-b2g 2.2+
ux-b2g 2.2

People

(Reporter: swilkes, Unassigned)

References

Details

(Whiteboard: [p=1])

Attachments

(7 files)

See Comms pages of attached spec.

User should be able to select all message text, but user should be able to select message text separately (see spec) and not be forced to select all messages that reside in the selectable area. This may need to be deferred until the Clipboard API is available (which will be 2.2 or later).

* Input fields (input areas, input dialogs and search fields) are the only Building Blocks with selectable text. 

* HTML elements like buttons or links, or events like ontouchstart or onclick, should trigger the action when tapped rather than triggering text selection.
Should be easy enough if we have the API.

Does it make sense to allow the user to select only part of a message?
Blocks: gaia-copy-paste
No longer blocks: CopyPasteLegacy
feature-b2g: --- → 2.2?
(In reply to Julien Wajsberg [:julienw] from comment #1)
> Should be easy enough if we have the API.
> 
> Does it make sense to allow the user to select only part of a message?
This feature seems quite practical to me :)

Maybe we can simply apply user-select: none to non-contenteditable element, since there in no actual input/textArea element in message app. Just a reminder we also have subject and recipient input here, but I don't think we can handle the attachment in compose field and contact in recipient field properly.
The Gecko work is close to completion, the remaining part is to fix test case failures then pref it on in m-c bug 1092888. After the bug landed, text selection and cut/copy/paste be triggered in apps on both editable and non-editable elements.

Gaia per app work in v2.2 is to follow UX guideline and leverage CSS -moz-user-select https://developer.mozilla.org/en-US/docs/Web/CSS/user-select to make certain elements non-selectable.

Before Gecko is pref on in m-c, Gaia can manually switch it on by the following two pref to test in advance:
pref("selectioncaret.enabled", on);
pref("selectioncaret.noneditable", on);
Just a heads up that we will need some special handling for message bubble copy. Unlike the spec p.11, bubble selection should not across to different bubbles. So the flow of bubble selection might be:

1) Bubbles' user-select set to none by default to make sure long press context menu will not trigger copy/paste at first
2) Long press to open context menu for one bubble, and add 'selection' option
3) Enable the bubble user-select, and use selection API selectAllChildren to select all content. Need to make if there is any racing issue between user-select css applied and timing of selection API called.
4) Disable user-select when bubble blur.

Unfortunately clipboard API is unlikely ready for 2.2, otherwise UX will prefer to copy all the content directly into clipboard. Morris(:mtseng) will keep negotiating with webapi about the possibility that simply open privilege app write access to clipboard.

Jenny will release another spec for the decision above, so let's revisit again when spec ready.
Hi team, please see the spec for Message text selection. Thanks!
Will provide WIP in this sprint.
Whiteboard: [p=1]
Attached file Link to github
Hi Julien, this WIP is for bubble selection, and the basic functionality is doable. However there are still some bugs that might need gecko support:

- No caret if the selection is triggered by selectAllChildren API(so the selection range is not changeable)
- copy/paste panel and selection will not dismiss when element got blur. Do we need to do some other programmable to dismiss it?
- Even other element is set to user-select : none, it's still selected when click select all
- Other known issue like bug 1098161

Hi Morris, please let us know if these questions above need gaia changes as well, thanks.
Attachment #8522727 - Flags: feedback?(mtseng)
Attachment #8522727 - Flags: feedback?(felash)
(In reply to Steve Chung [:steveck] from comment #7)
> Created attachment 8522727 [details] [review]
> Link to github
> 
> Hi Julien, this WIP is for bubble selection, and the basic functionality is
> doable. However there are still some bugs that might need gecko support:
> 
> - No caret if the selection is triggered by selectAllChildren API(so the
> selection range is not changeable)
> - copy/paste panel and selection will not dismiss when element got blur. Do
> we need to do some other programmable to dismiss it?
> - Even other element is set to user-select : none, it's still selected when
> click select all
> - Other known issue like bug 1098161
> 
> Hi Morris, please let us know if these questions above need gaia changes as
> well, thanks.
I think all above questions are gecko bug. So I don't think gaia needs change anything on this patch!
Attachment #8522727 - Flags: feedback?(mtseng) → feedback+
(In reply to Steve Chung [:steveck] from comment #7)
> Created attachment 8522727 [details] [review]
> Link to github
> 
> Hi Julien, this WIP is for bubble selection, and the basic functionality is
> doable. However there are still some bugs that might need gecko support:
> 
> - No caret if the selection is triggered by selectAllChildren API(so the
> selection range is not changeable)
I just try to debug this issue. I found that you first selectAllChildren() then call node.focus().
node.focus() will trigger blur so that carets would be hidden. So, you could change order for those 2 calls to avoid this problem. Thus, code looks like:

node.focus();
window.getSelection().selectAllChildren(node);

I tried in my local and look well.
(In reply to Morris Tseng [:mtseng] from comment #9)
> (In reply to Steve Chung [:steveck] from comment #7)
> > Created attachment 8522727 [details] [review]
> > Link to github
> > 
> > Hi Julien, this WIP is for bubble selection, and the basic functionality is
> > doable. However there are still some bugs that might need gecko support:
> > 
> > - No caret if the selection is triggered by selectAllChildren API(so the
> > selection range is not changeable)
> I just try to debug this issue. I found that you first selectAllChildren()
> then call node.focus().
> node.focus() will trigger blur so that carets would be hidden. So, you could
> change order for those 2 calls to avoid this problem. Thus, code looks like:
> 
> node.focus();
> window.getSelection().selectAllChildren(node);
> 
> I tried in my local and look well.

Changing the calling sequence seems not working :-/
(In reply to Morris Tseng [:mtseng] from comment #9)
> (In reply to Steve Chung [:steveck] from comment #7)
> > Created attachment 8522727 [details] [review]
> > Link to github
> > 
> > Hi Julien, this WIP is for bubble selection, and the basic functionality is
> > doable. However there are still some bugs that might need gecko support:
> > 
> > - No caret if the selection is triggered by selectAllChildren API(so the
> > selection range is not changeable)
> I just try to debug this issue. I found that you first selectAllChildren()
> then call node.focus().
> node.focus() will trigger blur so that carets would be hidden.

(commenting before looking at the code so sorry if I'm off topic).

* I don't get why we need to call focus
* I don't get why calling focus on the element where there is a selection would make the caret disappear.
Comment on attachment 8522727 [details] [review]
Link to github

My only concern is how we set the user-select properties.

I would expect to set "user-select: none" on body, and "user-select: all" (or text) on the element/elements that can be selected at that moment.
Attachment #8522727 - Flags: feedback?(felash)
This is a 2.2 feature.
feature-b2g: 2.2? → 2.2+
(In reply to Julien Wajsberg [:julienw] from comment #12)
> Comment on attachment 8522727 [details] [review]
> Link to github
> 
> My only concern is how we set the user-select properties.
> 
> I would expect to set "user-select: none" on body, and "user-select: all"
> (or text) on the element/elements that can be selected at that moment.

Hi Morris, I don't know why the "user-select: none" couldn't work on body set, since the firefox desktop seems applicable. Is it a bug or any limitation for copy paste?

BTW, when the selected bubble loses focus, the selection range is still there. Should I call removeAllRanges API to clear the range manually, or bug 1090008 will fixed it and we should let gecko to handle it automatically?
Flags: needinfo?(mtseng)
(In reply to Steve Chung [:steveck] from comment #14)
> (In reply to Julien Wajsberg [:julienw] from comment #12)
> > Comment on attachment 8522727 [details] [review]
> > Link to github
> > 
> > My only concern is how we set the user-select properties.
> > 
> > I would expect to set "user-select: none" on body, and "user-select: all"
> > (or text) on the element/elements that can be selected at that moment.
> 
> Hi Morris, I don't know why the "user-select: none" couldn't work on body
> set, since the firefox desktop seems applicable. Is it a bug or any
> limitation for copy paste?
I don't know why either. I think it is a bug. Maybe we should file a bug for this issue? 
> 
> BTW, when the selected bubble loses focus, the selection range is still
> there. Should I call removeAllRanges API to clear the range manually, or bug
> 1090008 will fixed it and we should let gecko to handle it automatically?
I don't think bug 1090008 would fix it. However, according to UX spec, we should clear selection range after text being copied. So in the future gecko will handle it automatically. We should file a new follow-up bug for this behavior.
Flags: needinfo?(mtseng)
(In reply to Morris Tseng [:mtseng] from comment #15)

> > BTW, when the selected bubble loses focus, the selection range is still
> > there. Should I call removeAllRanges API to clear the range manually, or bug
> > 1090008 will fixed it and we should let gecko to handle it automatically?
> I don't think bug 1090008 would fix it. However, according to UX spec, we
> should clear selection range after text being copied. So in the future gecko
> will handle it automatically. We should file a new follow-up bug for this
> behavior.

In this case, we want to remove the selection when the user taps elsewhere.
Depends on: 1101336
Depends on: 1101364
Depends on: 1101376
Comment on attachment 8522727 [details] [review]
Link to github

Some refinement for css, and I think this patch could provide better better use experience since the selection range is controllable now. I think there is only one major issue about the select all could block this one, and we could address others when we have time.
Attachment #8522727 - Flags: feedback?(felash)
Added some comments on github but didn't try the patch yet so keeping the feedback for now.
I've replied my explanation about the element focus and refine the edit mode css a little bit.
Comment on attachment 8522727 [details] [review]
Link to github

I left a comment on github (I think we don't move out of select mode when we should) but otherwise this looks good to me.
Attachment #8522727 - Flags: feedback?(felash) → feedback+
QA Contact: echang
Comment on attachment 8522727 [details] [review]
Link to github

Hi Oleg, I add more comments for the bubble selection functionality in the patch. Please enable the selection caret in prefs first:

adb shell "cd /data/b2g/mozilla/*.default/ && echo 'user_pref(\"selectioncaret.noneditable\", true);' >> prefs.js"

Have fun :)
Attachment #8522727 - Flags: review?(azasypkin)
No longer depends on: 1101364
Depends on: 1101364
Hey Steve,

Sorry for the delay, code part looks good, I've just left two nit questions at GitHub.

As I mentioned offline I can't test it on device currently due to bug 1090008 which changed signature of Gecko selection event and gaia should be updated too (appropriate patch is r+'ed, but not yet landed). So keeping r? for now.

Thanks!
Depends on: 1090008
Comment on attachment 8522727 [details] [review]
Link to github

Hey Steve,

Everything looks good to me (just tiny code style nit at Github). I've noticed few issues that look like related to Gecko\Selection API. You should know better if we already have appropriate bugs for them :)

1. If you drag left selection marker to the app top left corner then marker will go there (see attachment 8534211 [details]);

2. Maybe it's expected behaviour, but it's better to confirm: if you select text inside message and then scroll it out of viewport, then selection menu (Select All + Paste) is still visible, not sure how it should be (see attachment 8534212 [details]) :)

3. If selection range is too narrow, "Paste" item in selection menu is wrapped to the next line (see attachment 8534213 [details]);

4. If I select message content and then tap on other message bubble or message input, selection marker will go away, but selection menu will be still visible (see attachment 8534214 [details]).

Thanks a lot for the patch!
Attachment #8522727 - Flags: review?(azasypkin) → review+
Hi Oleg, thanks for the review! The Issues you mentions might need some help from gaia system's part(selection menu mispositioned or not dismissed correctly). ni? George for more details. Holding the patch until he could confirm the issues here all belongs to gaia system or selection api level.
Flags: needinfo?(gduan)
(In reply to Oleg Zasypkin [:azasypkin] from comment #27)
> Comment on attachment 8522727 [details] [review]
> Link to github
> 
> Hey Steve,
> 
> Everything looks good to me (just tiny code style nit at Github). I've
> noticed few issues that look like related to Gecko\Selection API. You should
> know better if we already have appropriate bugs for them :)
> 
> 1. If you drag left selection marker to the app top left corner then marker
> will go there (see attachment 8534211 [details]);
> 
currently, we don't have ability to detect when the selection is out of screen after scrolling, bug 1067728 is tracing it.
> 2. Maybe it's expected behaviour, but it's better to confirm: if you select
> text inside message and then scroll it out of viewport, then selection menu
> (Select All + Paste) is still visible, not sure how it should be (see
> attachment 8534212 [details]) :)
> 
file bug 1110040 for it.
> 3. If selection range is too narrow, "Paste" item in selection menu is
> wrapped to the next line (see attachment 8534213 [details]);
> 


Hi Peter,
do we already have bug for below issue?
> 4. If I select message content and then tap on other message bubble or
> message input, selection marker will go away, but selection menu will be
> still visible (see attachment 8534214 [details]).
> 
> Thanks a lot for the patch!
Flags: needinfo?(gduan) → needinfo?(pchang)
Please also try to trace this one, for some reason the left caret is gone and rect info of selection area seems not correct.
> 1. If you drag left selection marker to the app top left corner then marker
> will go there (see attachment 8534211 [details]);
>
In master: ad5580b8e6529e5eebf8517026286d0a8350d5fd

Although bubble content selection and copy/paste is enabled after this patch landed, please note we still has some follow up issues(bug 1101364, bug 1067728 and bug 1110040) from platform, and non-editable text selection functionality need these issues be addressed for completion.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
(In reply to George Duan [:gduan] [:喬智] from comment #29)
> (In reply to Oleg Zasypkin [:azasypkin] from comment #27)
> > Comment on attachment 8522727 [details] [review]
> > Link to github
> 
> Hi Peter,
> do we already have bug for below issue?
Create bug 1111433 to fix. 
> > 4. If I select message content and then tap on other message bubble or
> > message input, selection marker will go away, but selection menu will be
> > still visible (see attachment 8534214 [details]).
> > 
> > Thanks a lot for the patch!
Flags: needinfo?(pchang)
Target Milestone: --- → 2.2 S2 (19dec)
See Also: → 1230157
You need to log in before you can comment on or make changes to this bug.