Closed Bug 853711 (permission-dialog) Opened 7 years ago Closed 2 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(tracking-b2g:+)

RESOLVED WONTFIX
tracking-b2g +

People

(Reporter: alive, Assigned: alive)

References

Details

Attachments

(2 files)

+++ 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.
No longer blocks: 828278
Summary: Send PermissionRequestEvent from mozbrowser iframe, instead of mozChromeEvent → [Permission] Gaia fix of bug 852013: Move permission dialog into appWindow and bind to BrowserFrame
Alias: permission-dialog
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.
take it
Assignee: alive → gasolin
Component: Gaia::System → Gaia::System::Window Mgmt
seems like need to attach permissionManager to appWindow, activityWindow, homescreenWindow.

try to keep current source structure if possible
After some trial, will write new `PermissionDialog` instead of patch the current permission_manager
Status: NEW → ASSIGNED
Target Milestone: --- → 2.0 S3 (6june)
Blocks: 1007066
No longer blocks: 1007066
Target Milestone: 2.0 S3 (6june) → 2.0 S4 (20june)
not blocked bug 1007066 anymore
Status: ASSIGNED → NEW
Target Milestone: 2.0 S4 (20june) → ---
Attached file 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
Attachment #8441959 - Flags: review?(alive)
Depends on: 1015894
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.
Attachment #8441959 - Flags: review?(alive)
blocking-b2g: --- → backlog
Took too long time, taken
Assignee: gasolin → alive
tracking-b2g: --- → +
(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
(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.
(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.
Flags: needinfo?(kchen)
(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.
Flags: needinfo?(kchen)
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.
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?
Flags: needinfo?(kgrandon)
Depends on: 1122597
(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).
Flags: needinfo?(kgrandon)
blocking-b2g: backlog → ---
Firefox OS is not being worked on
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.