Closed
Bug 1101392
Opened 10 years ago
Closed 9 years ago
[Keyboard][Text Selection] After Copy, the selection highlight and the carets should dismiss as well
Categories
(Firefox OS Graveyard :: Gaia::System, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
2.2 S1 (5dec)
People
(Reporter: Omega, Assigned: mtseng)
References
Details
Attachments
(4 files, 4 obsolete files)
29.41 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
1.01 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
10.07 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
3.31 KB,
patch
|
Details | Diff | Splinter Review |
[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)
Reporter | ||
Updated•10 years ago
|
Flags: needinfo?(pchang)
Comment 1•10 years ago
|
||
Morris, please help on this bug.
Flags: needinfo?(pchang) → needinfo?(mtseng)
Assignee | ||
Comment 2•10 years ago
|
||
Working on this.
Assignee: nobody → mtseng
Status: NEW → ASSIGNED
Flags: needinfo?(mtseng)
Assignee | ||
Comment 3•10 years ago
|
||
Add a new command so our copy behavior align to UX spec.
Attachment #8526595 -
Flags: feedback?(tlin)
Comment 4•9 years ago
|
||
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+
Assignee | ||
Comment 5•9 years ago
|
||
The coding style in nsEditorCommands is mess. Fix it in this patch.
Attachment #8528151 -
Flags: review?(roc)
Assignee | ||
Comment 6•9 years ago
|
||
Update to addressed comment.
Attachment #8526595 -
Attachment is obsolete: true
Attachment #8528152 -
Flags: review?(roc)
Assignee | ||
Comment 7•9 years ago
|
||
try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=02389d161e15
Attachment #8528151 -
Flags: review?(roc) → review+
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+
Attachment #8528152 -
Flags: review+ → review-
Assignee | ||
Comment 9•9 years ago
|
||
(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.
Assignee | ||
Comment 10•9 years ago
|
||
Address reviewer's comment.
Attachment #8528152 -
Attachment is obsolete: true
Attachment #8529543 -
Flags: review?(roc)
Assignee | ||
Comment 12•9 years ago
|
||
Mochitest for this command.
Attachment #8529546 -
Flags: review?(roc)
Assignee | ||
Comment 13•9 years ago
|
||
Fix build break on non-unified build.
Attachment #8529543 -
Attachment is obsolete: true
Attachment #8529543 -
Flags: review?(roc)
Attachment #8529554 -
Flags: review?(roc)
Assignee | ||
Comment 14•9 years ago
|
||
try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=da204f071999
Attachment #8529544 -
Flags: review?(roc) → review+
Attachment #8529546 -
Flags: review?(roc) → review+
Attachment #8529554 -
Flags: review?(roc) → review+
Assignee | ||
Comment 16•9 years ago
|
||
Try run on treeherder looks weird. Here is result on tbpl https://tbpl.mozilla.org/?tree=Try&rev=da204f071999
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 17•9 years ago
|
||
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
Keywords: checkin-needed
Comment 18•9 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S1 (5dec)
You need to log in
before you can comment on or make changes to this bug.
Description
•