1097419 - [Calendar][Text Selection] Selection bubble doesn't appear on account login

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect, P2)

Description

[Makoto Kato [:m_kato]]

I think z-index seems to be too large for oauth2 screen.  Please adjust z-index for text selection bubble.

Also, z-index will be incremented by bug 1095320.


- Step
1. Launch Calendar
2. Open [Calendar Settings]
3. Tap [Accounts]
4. Tap [Google]
5. Tap [Email] field, then input 'aaa'
6. Long tap 'aaa' to launch selection bubble

- Result
Selection bubble disappears

- Expected Result
Selection bubble appears

Comment 1

[howie [:howie]]

This is a 2.2 feature.

Comment 2

[howie [:howie]]

correction, this is bug not feature.

Comment 3

[Peter Chang[:pchang]]

This issue is still existed on master.

Comment 4

[Peter Chang[:pchang]]

George, I think this is zorder issue becasue I saw gecko did send out the selectionstatechange to sys app.

Comment 5

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

In today's build, I put console.log in 
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/text_selection_dialog.js#L119
but I cannot get any msg, so, I assume gaia cannot get selectionstatechanged event from gecko too.

Comment 6

[Peter Chang[:pchang]]

(In reply to George Duan [:gduan] [:喬智] from comment #5)
> In today's build, I put console.log in 
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/
> text_selection_dialog.js#L119
> but I cannot get any msg, so, I assume gaia cannot get selectionstatechanged
> event from gecko too.

After checking, the event couldn't pass successfully from BrowserElementParent to Shell.js. That's why gaia couldn't receive the dom event changes.

Comment 8

[Miller Medeiros [:millermedeiros]]

doesn't look like this bug belongs to the calendar team (since it also happens on email). we should assign it to the correct team.

Comment 9

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

Attachment: Handle nested mozbrowser iframe for selectionstatechanged, touchcarettap and scrollviewchange.

I'll add a mochitest for this bug later.

Comment 10

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

Attachment: Part 1: Handle nested mozbrowser iframe for selectionstatechanged, touchcarettap and scrollviewchange v2.

Update commit message

Comment 11

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

Attachment: Part 2: Add mochitest for nested mozbrowser iframe case.

Comment 12

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

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=64d24b18e87e

Comment 13

[[:fabrice] Fabrice Desré]

Comment on attachment 8539152 [details] [diff] [review]
Part 1: Handle nested mozbrowser iframe for selectionstatechanged, touchcarettap and scrollviewchange v2.

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

Redirecting to Kan-Ru since he owns this code!

Comment 14

[[:fabrice] Fabrice Desré]

Comment on attachment 8539153 [details] [diff] [review]
Part 2: Add mochitest for nested mozbrowser iframe case.

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

Redirecting to Kan-Ru since he owns this code!

Comment 15

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

Comment on attachment 8539153 [details] [diff] [review]
Part 2: Add mochitest for nested mozbrowser iframe case.

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

::: dom/browser-element/mochitest/browserElement_CopyPaste.js
@@ +14,5 @@
> +// Give our origin permission to open browsers, and remove it when the test is complete.
> +var principal = SpecialPowers.wrap(document).nodePrincipal;
> +SpecialPowers.addPermission("browser", true, { url: SpecialPowers.wrap(principal.URI).spec,
> +                                               appId: principal.appId,
> +                                               isInBrowserElement: true });

Could you use SpecialPowers.pushPermissions?

Comment 16

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

Comment on attachment 8539152 [details] [diff] [review]
Part 1: Handle nested mozbrowser iframe for selectionstatechanged, touchcarettap and scrollviewchange v2.

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

Does that mean multiple BrowserParents could receive these events? Is that expected?

Comment 17

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

Attachment: Part 1: Handle nested mozbrowser iframe for selectionstatechanged, touchcarettap and scrollviewchange v2.

Set useCapture to ture, so nested mozbrowser iframe would send selectionstatechanged, touchcarettap and scrollviewchange event to top most mozbrowser iframe only.

Comment 18

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

Attachment: Part 2: Add mochitest for nested mozbrowser iframe case v2. (carry r+: kanru)

Addressed to reviewer's comment.

Comment 19

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

(In reply to Kan-Ru Chen [:kanru] from comment #16)
> Comment on attachment 8539152 [details] [diff] [review]
> Part 1: Handle nested mozbrowser iframe for selectionstatechanged,
> touchcarettap and scrollviewchange v2.
> 
> Review of attachment 8539152 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Does that mean multiple BrowserParents could receive these events? Is that
> expected?
No, this is not expected. I updated my patch to address this problem. Could you review it again? Thanks!

Comment 20

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

Comment on attachment 8540605 [details] [diff] [review]
Part 2: Add mochitest for nested mozbrowser iframe case v2. (carry r+: kanru)

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

::: dom/browser-element/mochitest/browserElement_CopyPaste.js
@@ +16,5 @@
> +var context = { 'url': SpecialPowers.wrap(principal.URI).spec,
> +                'appId': principal.appId,
> +                'isInBrowserElement': true };
> +SpecialPowers.pushPermissions([
> +  {'type': 'browser', 'allow': 1, 'context': context}]);

Sorry, this is not correct. SpecialPowers.pushPermissions needs a callback as second argument. Usually it's the runTest function.

In this test you could use

  addEventListener('testready', function() {
    SpecialPowers.pushPermissions([
      {'type': 'browser', 'allow': 1, 'context': context}
    ], runTest);
  });

Comment 21

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

Attachment: Part 2: Add mochitest for nested mozbrowser iframe case v3.

Use callback of pushPermissions to run test.

Comment 22

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

Thanks for review. \o/

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/28714f577de0
https://hg.mozilla.org/integration/mozilla-inbound/rev/e85722c30be3

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Backed out for B2G mochitest timeouts.
https://hg.mozilla.org/integration/mozilla-inbound/rev/088cfed82c74

https://treeherder.mozilla.org/logviewer.html#?job_id=4957550&repo=mozilla-inbound

08:44:37 INFO - 508 INFO TEST-PASS | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | A valid string reason is expected
08:44:37 INFO - 509 INFO TEST-PASS | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | Reason cannot be empty
08:44:37 INFO - 510 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | Test timed out. - expected PASS 

Same for OOP.

Comment 25

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

Attachment: Part 2: Add mochitest for nested mozbrowser iframe case v4.

Fix b2g emulator test fail.

Comment 26

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

Comment on attachment 8543867 [details] [diff] [review]
Part 2: Add mochitest for nested mozbrowser iframe case v4.

Kanru, Please review it again. Thanks

The main difference is adding a "isChildProcess" function and check it at testSelectAll function.

Comment 27

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

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6254d4f1c7e9

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e5920cdf87b
https://hg.mozilla.org/integration/mozilla-inbound/rev/496546af038e

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8e5920cdf87b
https://hg.mozilla.org/mozilla-central/rev/496546af038e