If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[TextSelection] refactoring text_selection_dialog.js

RESOLVED FIXED in Firefox 42

Status

Firefox OS
Gaia::System
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: gduan, Assigned: jeremychen)

Tracking

unspecified
FxOS-S4 (07Aug)
x86
Mac OS X
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(3 attachments, 7 obsolete attachments)

46 bytes, text/x-github-pull-request
timdream
: review+
Details | Review | Splinter Review
11.37 KB, patch
Details | Diff | Splinter Review
3.16 KB, patch
jeremychen
: review+
Details | Diff | Splinter Review
Gecko has been refactored and will only send caretstatechanged event to front by appWindow. This will help the bubble to remove a lot of conditional filter from text_selection_dialog.js and minify UI conflict from other component.

We also need to do some refactoring for gecko's change.
Assignee: nobody → gduan

Comment 1

3 years ago
Created attachment 8583009 [details] [review]
[gaia] cctuan:1147329 > mozilla-b2g:master
Comment on attachment 8583009 [details] [review]
[gaia] cctuan:1147329 > mozilla-b2g:master

Hi Morris,
could I have your feedback on this patch?
Thanks.
Attachment #8583009 - Flags: feedback?(mtseng)
Depends on: 1155493
Comment on attachment 8583009 [details] [review]
[gaia] cctuan:1147329 > mozilla-b2g:master

Hi George,
I think this patch is good!! But we decided that we want keep both version (with refactored and without refactored) of carets code on gecko side. We'll remove it once refactored code is stable. So I think gaia should support both version for now as well.
Attachment #8583009 - Flags: feedback?(mtseng) → feedback+
got it.
So we'll keep old version on master and let QA test the new version, once passing the tests, we'll use preference to switch the old one to new one.
Blocks: 1172382
Duplicate of this bug: 1118167
Attachment #8583009 - Attachment is obsolete: true
Created attachment 8620823 [details] [review]
PR to master

Hi Morris,
need your feedback on this version which keeps original text_selection_dialog.js.
Thanks.
Attachment #8620823 - Flags: feedback?(mtseng)
Morris told me that we can enable new text_selection_dialog of gecko by switching gfx.accessiblecaret.enabled preference to true. However, the keyboard would become larger. Not sure if it related to the flex css properties of keyboard or some other reason.
(In reply to (Inactive after June) George Duan [:gduan] [:喬智] from comment #7)
> Morris told me that we can enable new text_selection_dialog of gecko by
> switching gfx.accessiblecaret.enabled preference to true. However, the
> keyboard would become larger. Not sure if it related to the flex css
> properties of keyboard or some other reason.

Ah, I found that the pref I told you is wrong. The pref is layout.accessiblecaret.enabled, not gfx.accessiblecaret.enabled.
Attachment #8620823 - Flags: feedback?(mtseng) → feedback+
Created attachment 8623493 [details] [review]
PR to Gaia master
Attachment #8620823 - Attachment is obsolete: true
Attachment #8623493 - Flags: review?(gduan)
Comment on attachment 8623493 [details] [review]
PR to Gaia master

Cancel r? due to test failures.
Attachment #8623493 - Flags: review?(gduan)
Hi Ting yu or Jeremy,
would you mind to take it? I've updated my patch in attachment 8620823 [details] [review] and most of tests are fixed.
Assignee: georgeiscoming → nobody
(Assignee)

Comment 12

2 years ago
(In reply to (Inactive after June) George Duan [:gduan] [:喬智] from comment #11)
> Hi Ting yu or Jeremy,
> would you mind to take it? I've updated my patch in attachment 8620823 [details] [review]
> [details] and most of tests are fixed.

I'll take it from here. Thank you for your hard work.
(Assignee)

Updated

2 years ago
Assignee: nobody → jeremychen
(Assignee)

Comment 13

2 years ago
Created attachment 8627561 [details] [review]
Part1: PR to Gaia master

The original patch was developed by Morris Tseng mtseng@mozilla.com,
and further refined by George Duan gduan@mozilla.com.
Attachment #8623493 - Attachment is obsolete: true
Comment hidden (obsolete)
(Assignee)

Comment 15

2 years ago
Let's enable AccessibleCaret on B2G w/ refactored text_selection_dialog.js in Gaia.
push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3857e995f80
(Assignee)

Comment 16

2 years ago
Created attachment 8635259 [details] [diff] [review]
Part2: Handle in-process case for Cut/Copy/Past feature (v1)

In this bug, I try to make refactored app_text_selection_dialog.js work with AccessibleCaret in B2G. I found that event dispatching in SelectionCarets doesn't handle in-process cases well. This causes two problems: 1) cut/copy/paste bubble isn't shown on B2G desktop client (so we definitely can't pass Gij tests); 2) when selecting text in rocketbar on device, bubble isn't shown.

I make this patch to handle in-process cases, ex. rocketbar in system app, B2G desktop client. Hi Kan-Ru, could you review this? thx :)
Attachment #8635259 - Flags: review?(kchen)
(Assignee)

Comment 17

2 years ago
Comment on attachment 8627561 [details] [review]
Part1: PR to Gaia master

The original patch was developed by Morris Tseng, and further refined by George Duan. The changes I made are as follows:

1. Add null checks to make sure text_selection_dialog can always listen to either mozbrowsercaretchangedevent (normal app case) or mozChromeEvent (rocketbar case).
2. In refactored selectioncarets event loop, shortcut timeout may be longer than before. So, I extend wait time in longPress function to make sure we can fire long press on Gij as usual.
3. Skip 1 Gij test due to APZ is not enabled on B2G Desktop yet. I leave some comments, so we can enable the test once APZ is ready on B2G Desktop.

Hi Tim, could you review this? thx :)
Attachment #8627561 - Flags: review?(timdream)
Comment hidden (obsolete)
(Assignee)

Updated

2 years ago
Attachment #8635259 - Attachment description: Handle in-process case for Cut/Copy/Past feature (v1) → Part2: Handle in-process case for Cut/Copy/Past feature (v1)
(Assignee)

Comment 19

2 years ago
Enable AccessibleCaret and push to try with part1 and part2 patches:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7fea9260a00
(Assignee)

Comment 20

2 years ago
push to try with full patch from Bug 1172382:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=444cf7820bed
Comment on attachment 8635259 [details] [diff] [review]
Part2: Handle in-process case for Cut/Copy/Past feature (v1)

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

Please give the patch at least 8 lines of context.

I don't I understand the intention of the patch. Could you explain in more detail? Why do you want to load BrowserElementCopyPaste.js for every in-process browser-element when the old method should just work?

Morris, I need your feedback too.
Attachment #8635259 - Flags: review?(kchen)
Attachment #8635259 - Flags: review-
Attachment #8635259 - Flags: feedback?(mtseng)
Comment on attachment 8627561 [details] [review]
Part1: PR to Gaia master

LTGM but I wonder why the changes of purging the current working logic is not included?
Attachment #8627561 - Flags: review?(timdream) → review+
(Assignee)

Comment 23

2 years ago
(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #22)
> Comment on attachment 8627561 [details] [review]
> Part1: PR to Gaia master
> 
> LTGM but I wonder why the changes of purging the current working logic is
> not included?

Do you mean that you can't see the diff file between my patch and the original one from George? Please correct me if I misunderstand what you mean.

The reason why there is only one long patch is that I commit my patch by --amend. I thought it would be appropriate to leave only one commit message for merging this refactored text_selection_dialog.js into master. Another reason is that this patch hasn't been fully reviewed before, so a complete review would be better than just review my part of changes. If you think I should treat my part of changes as a separated commit, please let me know.
Flags: needinfo?(timdream)
Comment hidden (obsolete)
(Assignee)

Comment 25

2 years ago
Created attachment 8635920 [details] [diff] [review]
Part1.5: Gaia_diff (diff file to explain what I've patched) (v2)

Sorry for leaving so few context in the patch file uploaded before. Re-upload this patch with 8 lines context.
Attachment #8635913 - Attachment is obsolete: true
(In reply to Jeremy Chen [:jeremychen] from comment #23) 
> Do you mean that you can't see the diff file between my patch and the
> original one from George? Please correct me if I misunderstand what you mean.
> 
> The reason why there is only one long patch is that I commit my patch by
> --amend. I thought it would be appropriate to leave only one commit message
> for merging this refactored text_selection_dialog.js into master. Another
> reason is that this patch hasn't been fully reviewed before, so a complete
> review would be better than just review my part of changes. If you think I
> should treat my part of changes as a separated commit, please let me know.

Chat on IRC and being told there will be future patch purging the old impl when Gecko old impl is removed. Sounds good.
Flags: needinfo?(timdream)
(Assignee)

Comment 27

2 years ago
Created attachment 8635955 [details] [diff] [review]
Part2: Handle in-process case for Cut/Copy/Past feature (v2)

Sorry for leaving so few context in the patch file uploaded before. Re-upload this patch with 8 lines context.

In our original logic, BrowserElementCopyPaste.js is supposed to be loaded in every top browser-element. The original goal is letting every content app has its own selection/caret state change handler. (please see slide 1 in [1])

However, in B2G desktop, there is no OOP mode. On B2G desktop, the original logic would always load BrowserElementCopyPaste.js into system app, which is the top browser-element in in-process mode (see slide 3 in [1]). This could let shell.js always be the BrowserElementParent and fire mozChromeEvent instead of mozbrowsercaretstatechangedevent all the time (Note that mozChromeEvent is just for Rocketbar case, please see slide 2 in [1]). On the other side, in gaia, the app_text_selection_dialog.js is hoping to listen to mozbrowsercaretstatechangedevent for content app, which of course always fail.


Since B2G desktop is always run in chrome process, I use Utils.isContentProcess to tell that whether it is in content process or chrome process. To prevent from only system app can load BrowserElementCopyPaste.js, I skip the original logic and let every browser-element can load BrowserElementCopyPaste.js as expected.

In BrowserElementCopyPaste.js part, I set the condition to prevent over loop to out of app window, which helped to calculate the correct position of selection bubble.
Attachment #8635259 - Attachment is obsolete: true
Attachment #8635259 - Flags: feedback?(mtseng)
Attachment #8635955 - Flags: feedback?(mtseng)
(Assignee)

Comment 28

2 years ago
(In reply to Jeremy Chen [:jeremychen] from comment #27)
Missing ref link:
[1] https://docs.google.com/presentation/d/1O1fNaQ4AciIwZMEndFzgJw5wBPZKRdMnsaPZzAJyzow/edit#slide=id.ga3929ef5e_0_0
(In reply to Kan-Ru Chen [:kanru] from comment #21)
> Comment on attachment 8635259 [details] [diff] [review]
> Part2: Handle in-process case for Cut/Copy/Past feature (v1)
> 
> Review of attachment 8635259 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please give the patch at least 8 lines of context.
> 
> I don't I understand the intention of the patch. Could you explain in more
> detail? Why do you want to load BrowserElementCopyPaste.js for every
> in-process browser-element when the old method should just work?
> 
> Morris, I need your feedback too.
We want BrowserElementCopyPaste.js works on each app since each app know what and when data should be cut/copy/paste. The old method works because we route all copy/paste related event to shell.js and send mozChromeEvent to system app. But this method have many potential bugs because events from different apps may influence each other so that the behavior of bubble becomes weird.

After refactoring copy/paste code. We want to handle this event based on per app basis. That's why we need this change here. Does it make sense to you?
Comment on attachment 8635955 [details] [diff] [review]
Part2: Handle in-process case for Cut/Copy/Past feature (v2)

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

Passing review to :kanru based on comment 27 and 29.
Attachment #8635955 - Flags: feedback?(mtseng) → review?(kchen)
Blocks: 1186347
Comment on attachment 8635955 [details] [diff] [review]
Part2: Handle in-process case for Cut/Copy/Past feature (v2)

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

Thanks for the explanation, now it's more clear.

::: dom/browser-element/BrowserElementChild.js
@@ +48,2 @@
>  
> +  if(Utils.isContentProcess) {

Use Services.appinfo.processType directly then you could get rid of XPCOMUtils and Utils. Especially I don't want to pull in accessibility only for this.

@@ +54,5 @@
> +  } else {
> +    // rocketbar in system app and other in-process case (ex. B2G desktop client)
> +    if (docShell.isBrowserOrApp) {
> +      Services.scriptloader.loadSubScript("chrome://global/content/BrowserElementCopyPaste.js");
> +    }

docShell.isBrowserOrApp should always be true.
Attachment #8635955 - Flags: review?(kchen) → review+
(Assignee)

Comment 32

2 years ago
Created attachment 8637141 [details] [diff] [review]
Part2: Handle in-process case for Cut/Copy/Past feature. r=kanru (v3, carry r+)

Address reviewer's comments.
Attachment #8635955 - Attachment is obsolete: true
Attachment #8637141 - Flags: review+
(Assignee)

Comment 33

2 years ago
push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a144f20a2eef
(Assignee)

Comment 34

2 years ago
(In reply to Jeremy Chen [:jeremychen] from comment #33)
> push to try:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=a144f20a2eef

Bump into some temporary problems in Gij.
re-try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe723c5bdf87
The only fail is caused by Bug 1186219
So, let's rebase the patch and try again:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4243897d3110
(Assignee)

Comment 35

2 years ago
Try looks positive. Please check-in Part1 for Gaia and Part2 for Gecko, thanks.
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
Master: https://github.com/mozilla-b2g/gaia/commit/ec2199b324304d3678b6a98a08a31bdc13c9e984
status-b2g-master: --- → fixed
Target Milestone: --- → FxOS-S3 (24Jul)

Comment 37

2 years ago
https://hg.mozilla.org/integration/b2g-inbound/rev/a45190bd737c
Keywords: checkin-needed
Gecko patch backed out for test_browserElement_inproc_CopyPaste.html timeouts.
https://treeherder.mozilla.org/logviewer.html#?job_id=2367975&repo=b2g-inbound

https://hg.mozilla.org/integration/b2g-inbound/rev/7efc765695bb
status-b2g-master: fixed → ---
(Assignee)

Comment 39

2 years ago
Created attachment 8638581 [details] [diff] [review]
Part2: Handle in-process case for Cut/Copy/Past feature. r=kanru (v4, carry r+)

Set useCapture = true for EventListener to fix mochitest timeout.
Attachment #8637141 - Attachment is obsolete: true
Attachment #8638581 - Flags: review+
(Assignee)

Comment 40

2 years ago
Fix test_browserElement_inproc_CopyPaste.html timeouts and retry looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b96c35dd770d
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 41

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/201523ca9658
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/201523ca9658
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox42: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: FxOS-S3 (24Jul) → FxOS-S4 (07Aug)
Jeremy, thank you for working on this.

Updated

2 years ago
Depends on: 1231298
You need to log in before you can comment on or make changes to this bug.