Bug 853711 (permission-dialog)

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

RESOLVED WONTFIX

Status

RESOLVED WONTFIX
6 years ago
8 months ago

People

(Reporter: alive, Assigned: alive)

Tracking

unspecified
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(tracking-b2g:+)

Details

Attachments

(2 attachments)

+++ 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
Blocks: 928433
Alias: permission-dialog
Blocks: 902766
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

5 years ago
take it
Assignee: alive → gasolin

Updated

5 years ago
Component: Gaia::System → Gaia::System::Window Mgmt

Comment 3

5 years ago
seems like need to attach permissionManager to appWindow, activityWindow, homescreenWindow.

try to keep current source structure if possible

Comment 4

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

Updated

4 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → 2.0 S3 (6june)

Updated

4 years ago
Blocks: 1007066

Updated

4 years ago
No longer blocks: 1007066

Updated

4 years ago
Target Milestone: 2.0 S3 (6june) → 2.0 S4 (20june)

Comment 5

4 years ago
not blocked bug 1007066 anymore
Status: ASSIGNED → NEW
Target Milestone: 2.0 S4 (20june) → ---

Comment 6

4 years ago
Created attachment 8441959 [details] [review]
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)

Updated

4 years ago
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: --- → +
Created attachment 8532529 [details] [review]
[PullReq] alivedise:bugzilla/853711/app-permission-dialog to mozilla-b2g:master
(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 → ---

Comment 17

8 months ago
Firefox OS is not being worked on
Status: NEW → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.