1101392 - [Keyboard][Text Selection] After Copy, the selection highlight and the carets should dismiss as well

Status: RESOLVED FIXED

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

Description

[Omega Feng [:Omega] [:馮於懋]]

[Steps To Reproduce]
1. Launch Messages app
2. Tap "New message" button on header
3. Type something in composer field
4. Long press on the text typed in step 3
5. Tap "Copy" button on the utility bubble

[Actual Result]
The bubble dismisses.

[Expected Result]
According to UX spec https://bug1023688.bugzilla.mozilla.org/attachment.cgi?id=8521957#7
The bubble should dismiss.
The selection highlight should dismiss.
The carets should dismiss.

Device: Flame
OS Version: 2.2.0.0-prerelease
Platform Version: 36.0a1
Build Identifier: 20141117160200
Git Commit Info: 2014-11-19 04:09:55 (e64428c5)

Comment 1

[Peter Chang[:pchang]]

Morris, please help on this bug.

Comment 2

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Working on this.

Comment 3

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Add a command that collapse selection to end after copying.

Add a new command so our copy behavior align to UX spec.

Comment 4

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8526595 [details] [diff] [review]
Add a command that collapse selection to end after copying.

Review of attachment 8526595 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/nsEditorCommands.cpp
@@ +380,5 @@
>    return aParams->SetBooleanValue(STATE_ENABLED,canUndo);
>  }
>  
>  NS_IMETHODIMP
> +nsCopyAndCollapseToEndCommand::IsCommandEnabled(const char * aCommandName,

Nit: Extra space between * and aCommandName.

@@ +425,5 @@
> +                                                     nsISupports *aCommandRefCon)
> +{
> +  bool canUndo;
> +  IsCommandEnabled(aCommandName, aCommandRefCon, &canUndo);
> +  return aParams->SetBooleanValue(STATE_ENABLED,canUndo);

Nit: Please add space before "canUndo".

Comment 5

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 1: Fix coding style in nsEditorCommands.cpp.

The coding style in nsEditorCommands is mess. Fix it in this patch.

Comment 6

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 2: Add a command that collapse selection to end after copying.

Update to addressed comment.

Comment 7

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

try run:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=02389d161e15

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8528152 [details] [diff] [review]
Part 2: Add a command that collapse selection to end after copying.

Review of attachment 8528152 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/nsEditorCommands.cpp
@@ +385,5 @@
> +    nsresult rv = editor->GetSelection(getter_AddRefs(selection));
> +    if (NS_SUCCEEDED(rv) && selection) {
> +      selection->CollapseToEnd();
> +    }
> +    return editor->Copy();

Shouldn't you collapse it *after* you've done the copy? Otherwise you won't copy anything.

Needs a test!

Comment 9

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #8)
> Comment on attachment 8528152 [details] [diff] [review]
> Part 2: Add a command that collapse selection to end after copying.
> 
> Review of attachment 8528152 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: editor/libeditor/nsEditorCommands.cpp
> @@ +385,5 @@
> > +    nsresult rv = editor->GetSelection(getter_AddRefs(selection));
> > +    if (NS_SUCCEEDED(rv) && selection) {
> > +      selection->CollapseToEnd();
> > +    }
> > +    return editor->Copy();
> 
> Shouldn't you collapse it *after* you've done the copy? Otherwise you won't
> copy anything.
Ah, you're right. Will fix in next patch.
> 
> Needs a test!
Will do.

Comment 10

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 2: Add a command that collapse selection to end after copying v2.

Address reviewer's comment.

Comment 11

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 3: Select all before paste since we collapse range after copying.

Fix try server fail.

Comment 12

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 4: Add mochitest.

Mochitest for this command.

Comment 13

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 2: Add a command that collapse selection to end after copying v3.

Fix build break on non-unified build.

Comment 14

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

try run:  https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=da204f071999

Comment 15

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 4: Add mochitest v2. (carry r+: roc)

Rebased.

Comment 16

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Try run on treeherder looks weird. Here is result on tbpl
https://tbpl.mozilla.org/?tree=Try&rev=da204f071999

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/5813ab25249c
https://hg.mozilla.org/integration/b2g-inbound/rev/f32ac1751455
https://hg.mozilla.org/integration/b2g-inbound/rev/d59fef58d1ac
https://hg.mozilla.org/integration/b2g-inbound/rev/1b10bab94f6e

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5813ab25249c
https://hg.mozilla.org/mozilla-central/rev/f32ac1751455
https://hg.mozilla.org/mozilla-central/rev/d59fef58d1ac
https://hg.mozilla.org/mozilla-central/rev/1b10bab94f6e