1147329 - [TextSelection] refactoring text_selection_dialog.js

Status: RESOLVED FIXED

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

Description

[(Inactive after June) George Duan [:gduan] [:喬智]]

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.

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] cctuan:1147329 > mozilla-b2g:master

Comment 2

[(Inactive after June) George Duan [:gduan] [:喬智]]

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

Hi Morris,
could I have your feedback on this patch?
Thanks.

Comment 3

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

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.

Comment 4

[(Inactive after June) George Duan [:gduan] [:喬智]]

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.

Comment 6

[(Inactive after June) George Duan [:gduan] [:喬智]]

Attachment: PR to master

Hi Morris,
need your feedback on this version which keeps original text_selection_dialog.js.
Thanks.

Comment 7

[(Inactive after June) George Duan [:gduan] [:喬智]]

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.

Comment 8

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

(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.

Comment 9

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

Attachment: PR to Gaia master

Comment 10

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

Comment on attachment 8623493 [details] [review]
PR to Gaia master

Cancel r? due to test failures.

Comment 11

[(Inactive after June) George Duan [: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.

Comment 12

[Jeremy Chen ([e-mail])]

(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.

Comment 13

[Jeremy Chen ([e-mail])]

Attachment: Part1: PR to Gaia master

The original patch was developed by Morris Tseng [e-mail],
and further refined by George Duan [e-mail].

Comment 14

[Jeremy Chen ([e-mail])]

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=11e8281c0503

Comment 15

[Jeremy Chen ([e-mail])]

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

Comment 16

[Jeremy Chen ([e-mail])]

Attachment: 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 :)

Comment 17

[Jeremy Chen ([e-mail])]

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 :)

Comment 18

[Jeremy Chen ([e-mail])]

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

>https://github.com/mozilla-b2g/gaia/pull/30748

Comment 19

[Jeremy Chen ([e-mail])]

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

Comment 20

[Jeremy Chen ([e-mail])]

push to try with full patch from Bug 1172382:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=444cf7820bed

Comment 21

[Kan-Ru Chen [:kanru] (UTC+9)]

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.

Comment 22

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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?

Comment 23

[Jeremy Chen ([e-mail])]

(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.

Comment 24

[Jeremy Chen ([e-mail])]

Attachment: Part1.5: Gaia_diff (diff file to explain what I've patched)

Upload this file for review purpose.

Comment 25

[Jeremy Chen ([e-mail])]

Attachment: 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.

Comment 26

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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.

Comment 27

[Jeremy Chen ([e-mail])]

Attachment: 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.

Comment 28

[Jeremy Chen ([e-mail])]

(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

Comment 29

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

(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 30

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

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.

Comment 31

[Kan-Ru Chen [:kanru] (UTC+9)]

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.

Comment 32

[Jeremy Chen ([e-mail])]

Attachment: Part2: Handle in-process case for Cut/Copy/Past feature. r=kanru (v3, carry r+)

Address reviewer's comments.

Comment 33

[Jeremy Chen ([e-mail])]

push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a144f20a2eef

Comment 34

[Jeremy Chen ([e-mail])]

(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

Comment 35

[Jeremy Chen ([e-mail])]

Try looks positive. Please check-in Part1 for Gaia and Part2 for Gecko, thanks.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/ec2199b324304d3678b6a98a08a31bdc13c9e984

Comment 37

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/a45190bd737c

Comment 38

[Ryan VanderMeulen [:RyanVM]]

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

Comment 39

[Jeremy Chen ([e-mail])]

Attachment: Part2: Handle in-process case for Cut/Copy/Past feature. r=kanru (v4, carry r+)

Set useCapture = true for EventListener to fix mochitest timeout.

Comment 40

[Jeremy Chen ([e-mail])]

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

Comment 41

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/201523ca9658

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/201523ca9658

Comment 43

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

Jeremy, thank you for working on this.