[Text Selection] [cut/copy/paste] cut/copy options are always shown on CCP bubble

RESOLVED FIXED

Status

()

Core
Selection
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jeremychen, Assigned: jeremychen)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---

Firefox Tracking Flags

(firefox42 affected)

Details

(URL)

Attachments

(4 attachments, 2 obsolete attachments)

I just flashed my flame with the latest mozilla-central pvt-build and found that CCP short cut mode is not working.

##### STR
1. Open sms app and press the pan icon to create a new message.
(you can open any app to help you get into a text editor)
2. Input some text, select them, and copy them
3. Tap on any empty space in an editor

##### Expect
CCP get into short cut mode with only paste command available in pop-up bubble

##### Actual
cut/copy commands are shown in pop-up bubble as well

##### PVT Version info.
Build ID               20150707160207
Gaia Revision          86af137c7f4c41c1185e609339002ab47fd6c640
Gaia Date              2015-07-07 17:02:07
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/9340658848d1
Gecko Version          42.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150707.193318
Firmware Date          Tue Jul  7 19:33:30 EDT 2015
Bootloader             L1TC10011880
I flashed w/ mozilla-b2g37_v2_2 pvt-build and the short cut mode works as usual. Looks like v2.2 is not affected.

##### PVT Version info.
Build ID               20150707162503
Gaia Revision          ea11f422b687a982f0a961c9aea7858066561707
Gaia Date              2015-07-02 23:37:50
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/52d19a805d35
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150707.202802
Firmware Date          Tue Jul  7 20:28:14 EDT 2015
Bootloader             L1TC10011880
Not sure if this is related to Bug 1180872.
Summary: [Text Selection] [cut/copy/paste] CCP short cut mode is not working after copy some text → [Text Selection] [cut/copy/paste] cut/copy options are always shown on CCP bubble
Created attachment 8635963 [details] [diff] [review]
Disable Cut/Copy options on selection bubble when in collapsed mode (v1)

For now, our UI looks a bit weird since we always have cut/copy options in collapsed mode (in shortcut mode too). Main reason could be that we will query isCommandEnabled for cut/copy and these two values are always true. (see Bug 1162952). In Bug 1180872, it seems like we're not going to disable cut option when selecting on Non-Editable text for now. But, to my knowledge, it seems that we're not firing cut/copy/paste event as spec said when clicking cut/copy/paste buttons on bubble. Instead, we send msg to content and let gecko handle cut/copy/paste (see [1]).

I think maybe we can disable cut/copy options in collapsed mode to improve UX for now. In the future, if clipboard API in FireFox can support all three cut/copy/paste events, we can then come up with a plan for a new UI spec or something.

[1] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsIDocShell.idl#990
Assignee: nobody → jeremychen
Attachment #8635963 - Flags: feedback?(tlin)
Attachment #8635963 - Flags: feedback?(mtseng)
Comment on attachment 8635963 [details] [diff] [review]
Disable Cut/Copy options on selection bubble when in collapsed mode (v1)

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

Looks good.
Attachment #8635963 - Flags: feedback?(mtseng) → feedback+
Attachment #8635963 - Flags: feedback?(tlin) → feedback+
Created attachment 8641515 [details] [diff] [review]
Part1: Send selectionEditable info to app_text_selection_dialog (v1)

From gecko, we can aware whether the selected frame is editable or not. Let's send this info to app_text_selection_dialog in Gaia system app, so selection bubble could only show functional command options.

TY, can you take a look at this? Thank you. :)
Attachment #8641515 - Flags: feedback?(tlin)
Created attachment 8641516 [details] [review]
Part2: [gaia] chenpighead:1181418 > mozilla-b2g:master
Attachment #8641516 - Attachment description: [gaia] chenpighead:1181418 > mozilla-b2g:master → Part2: [gaia] chenpighead:1181418 > mozilla-b2g:master
Comment on attachment 8641515 [details] [diff] [review]
Part1: Send selectionEditable info to app_text_selection_dialog (v1)

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

::: layout/base/AccessibleCaretManager.cpp
@@ +941,5 @@
>      init.mSelectionVisible = true;
>    }
>  
> +  // Send isEditable info w/ event detail. This info can help determine
> +  // whether to show cut command on selection dialog or not

Please add a period at the end of the sentence.

@@ +946,5 @@
> +  Element* editingHost = nullptr;
> +  if (commonAncestorFrame) {
> +    editingHost = commonAncestorFrame->GetContent()->GetEditingHost();
> +  }
> +  init.mSelectionEditable = editingHost ? true : false;

Nit: init.mSelectionEditable = commonAncestorFrame && commonAncestorFrame->GetContent()->GetEditingHost();
Attachment #8641515 - Flags: feedback?(tlin) → feedback+
Created attachment 8641626 [details] [diff] [review]
Part1: Send selectionEditable info to app_text_selection_dialog (v2)

Address :TYLin's comments.

:kanru, could you review BrowserElement related files?
:smaug, could you review CaretStateChangedEvent.webidl part?
Thank you.
Attachment #8635963 - Attachment is obsolete: true
Attachment #8641515 - Attachment is obsolete: true
Attachment #8641626 - Flags: superreview?(bugs)
Attachment #8641626 - Flags: review?(tlin)
Attachment #8641626 - Flags: review?(kchen)
Attachment #8641516 - Flags: review?(timdream)
Comment on attachment 8641626 [details] [diff] [review]
Part1: Send selectionEditable info to app_text_selection_dialog (v2)

r+ (or sr+) for the .webidl
Attachment #8641626 - Flags: superreview?(bugs) → superreview+
Comment on attachment 8641626 [details] [diff] [review]
Part1: Send selectionEditable info to app_text_selection_dialog (v2)

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

r=me for AccessibleCaretManager.
Attachment #8641626 - Flags: review?(tlin) → review+
Attachment #8641626 - Flags: review?(kchen) → review+
Hi Tim, could you take a look at the Gaia patch? Thank you.
Flags: needinfo?(timdream)
Comment on attachment 8641516 [details] [review]
Part2: [gaia] chenpighead:1181418 > mozilla-b2g:master

I am very sorry for the delayed review.
Flags: needinfo?(timdream)
Attachment #8641516 - Flags: review?(timdream) → review+
Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e1aa6a5cedf
Please check-in Part1 for Gecko and Part2 for Gaia, thanks.
Keywords: checkin-needed
Keywords: leave-open
Please leave this bug open until the gaia part patch is landed.
Try looks positive. Please check-in Part2 for Gaia, thanks.
Keywords: leave-open → checkin-needed
landed on gaia https://github.com/mozilla-b2g/gaia/commit/474784fc494519a0fab866407c0215aacf984ed9
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.