Closed Bug 1101392 Opened 5 years ago Closed 5 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.2 S1 (5dec)

People

(Reporter: Omega, Assigned: mtseng)

References

Details

Attachments

(4 files, 4 obsolete files)

[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)
Flags: needinfo?(pchang)
Morris, please help on this bug.
Flags: needinfo?(pchang) → needinfo?(mtseng)
Working on this.
Assignee: nobody → mtseng
Status: NEW → ASSIGNED
Flags: needinfo?(mtseng)
Add a new command so our copy behavior align to UX spec.
Attachment #8526595 - Flags: feedback?(tlin)
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".
Attachment #8526595 - Flags: feedback?(tlin) → feedback+
The coding style in nsEditorCommands is mess. Fix it in this patch.
Attachment #8528151 - Flags: review?(roc)
Update to addressed comment.
Attachment #8526595 - Attachment is obsolete: true
Attachment #8528152 - Flags: review?(roc)
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!
Attachment #8528152 - Flags: review?(roc) → review+
(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.
Address reviewer's comment.
Attachment #8528152 - Attachment is obsolete: true
Attachment #8529543 - Flags: review?(roc)
Attached patch Part 4: Add mochitest. (obsolete) — Splinter Review
Mochitest for this command.
Attachment #8529546 - Flags: review?(roc)
Fix build break on non-unified build.
Attachment #8529543 - Attachment is obsolete: true
Attachment #8529543 - Flags: review?(roc)
Attachment #8529554 - Flags: review?(roc)
Rebased.
Attachment #8529546 - Attachment is obsolete: true
Try run on treeherder looks weird. Here is result on tbpl
https://tbpl.mozilla.org/?tree=Try&rev=da204f071999
Duplicate of this bug: 1101376
You need to log in before you can comment on or make changes to this bug.