853711 - (permission-dialog) [Permission] Gaia fix of bug 852013: Move permission dialog into appWindow and bind to BrowserFrame

Status: RESOLVED WONTFIX

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

Description

[(inactive after 6/18) Alive Kuo [:alive]]

+++ This bug was initially created as a clone of Bug #852013 +++

... in order to properly fix bug #828278.

Read bug 828278 comment 15, bug 828278 comment 16 for detail.
And https://bugzilla.mozilla.org/show_bug.cgi?id=828278#c25

This is gaia side fix once gecko bug 852013 is done, to embed permission UI in browserAPI.

What we should do is:
* Remove permission_dialog
* Create a new Object under appWindow, and rending any UI under appWindow.
  For example, see error_dialog implementation.

Comment 1

[(inactive after 6/18) Alive Kuo [:alive]]

Acceptable:
Even the mozChromeEvent is not changed into mozBrowser API, we could know which appWindow the permission dialog belongs to by check manifestURL/pageURL. So we could start implementation now. Reference: AppModalDialog.

Note: Have better to inherit BaseUI in system. See bug 907013.

Comment 2

[[e-mail]]

take it

Comment 3

[[e-mail]]

seems like need to attach permissionManager to appWindow, activityWindow, homescreenWindow.

try to keep current source structure if possible

Comment 4

[[e-mail]]

After some trial, will write new `PermissionDialog` instead of patch the current permission_manager

Comment 5

[[e-mail]]

not blocked bug 1007066 anymore

Comment 6

[[e-mail]]

Attachment: PART 1

Here's part 1 of migration, 

It mainly focus on refactor permission manager to use app_modal_dialog-like element query approach and passed current test cases.

    turn single prototype to per prototype.method
    add _fetchElements method to query elements
    turn id query into class query

Comment 7

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8441959 [details] [review]
PART 1

Please read app-modal-dialog and do
(1) render element inside appWindow.
(2) remove all static #permission-* rules.
(3) rename to permission_dialog.
(4) register in appWindow sub components.

Comment 8

[(inactive after 6/18) Alive Kuo [:alive]]

Took too long time, taken

Comment 9

[GitHub Autolander Bot]

Attachment: [PullReq] alivedise:bugzilla/853711/app-permission-dialog to mozilla-b2g:master

Comment 10

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Autolander from comment #9)
> Created attachment 8532529 [details] [review]
> [PullReq] alivedise:bugzilla/853711/app-permission-dialog to
> mozilla-b2g:master

WIP, known issue:
mozbrowserfullscreen-origin-change is not caught.
http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementChildPreload.js#264

Comment 11

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Alive Kuo [:alive][NEEDINFO!][PDXww until 12/5] from comment #10)
> (In reply to Autolander from comment #9)
> > Created attachment 8532529 [details] [review]
> > [PullReq] alivedise:bugzilla/853711/app-permission-dialog to
> > mozilla-b2g:master
> 
> WIP, known issue:
> mozbrowserfullscreen-origin-change is not caught.
> http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/
> BrowserElementChildPreload.js#264

Another issue with rocketbar:
Keyboard will overlay on top of permssion dialog of the search window. It worked before because permission dialog was higher than keyboard but now it's different. Don't have good solution right now.

Comment 12

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Alive Kuo [:alive][NEEDINFO!][PDXww until 12/5] from comment #10)
> (In reply to Autolander from comment #9)
> > Created attachment 8532529 [details] [review]
> > [PullReq] alivedise:bugzilla/853711/app-permission-dialog to
> > mozilla-b2g:master
> 
> WIP, known issue:
> mozbrowserfullscreen-origin-change is not caught.
> http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/
> BrowserElementChildPreload.js#264

https://github.com/mozilla-b2g/gaia/pull/26637/files#diff-dfab6f985b6be2e2edf1a8df020b7793R14

Kanru, could you help to check why the event is not caught? Or there is something wrong in gaia side.

Comment 13

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

(In reply to Alive Kuo [:alive][NEEDINFO!][PDXww until 12/5] from comment #12)
> (In reply to Alive Kuo [:alive][NEEDINFO!][PDXww until 12/5] from comment
> #10)
> > (In reply to Autolander from comment #9)
> > > Created attachment 8532529 [details] [review]
> > > [PullReq] alivedise:bugzilla/853711/app-permission-dialog to
> > > mozilla-b2g:master
> > 
> > WIP, known issue:
> > mozbrowserfullscreen-origin-change is not caught.
> > http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/
> > BrowserElementChildPreload.js#264
> 
> https://github.com/mozilla-b2g/gaia/pull/26637/files#diff-
> dfab6f985b6be2e2edf1a8df020b7793R14
> 
> Kanru, could you help to check why the event is not caught? Or there is
> something wrong in gaia side.

The event is not fired as a mozbrowser event. It only files a mozChromeEvent to the system app.

Comment 14

[(inactive after 6/18) Alive Kuo [:alive]]

Status update:
Bug 1034001 is suspended so this issue is not its blocker.
The problem of the patch is:
* fullscreen event is not dispatched from the browser iframe
* searchWindow + permission case will show keyboard behind the search window's permission dialog at first time launch, not sure how to fix this elegantly still.

Comment 15

[(inactive after 6/18) Alive Kuo [:alive]]

Kevin, I saw https://github.com/mozilla-b2g/gaia/blob/master/apps/search/js/search.js#L100
My question is: is the permission necessary? Can we remove it?

Comment 16

[Kevin Grandon :kgrandon]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #15)
> Kevin, I saw
> https://github.com/mozilla-b2g/gaia/blob/master/apps/search/js/search.js#L100
> My question is: is the permission necessary? Can we remove it?

I've filed bug 1122597 to remove it for now, but if this patch is going to break permission prompts in the search window that might be bad. (I imagine we will need some type of permission prompt for the search window in the future).

Comment 17

[BMO Automation]

Firefox OS is not being worked on