The z-index work here is simple. Should be to remove the all [data-z-index-level=keyboard-frame].visible hacks, and then the z-index file could be more clear. \O/ The z-index of keyboard-frame should be 65536 always then.

And I think the keyboard-overlay element should be removed. We don't need it anymore.

(In reply to Alive Kuo [:alive] from comment #2) > And I think the keyboard-overlay element should be removed. We don't need it > anymore. Yep. It will be useless now :)

The Gaia patch is mostly based on the work of Alive. Thank you!

(In reply to Vivien Nicolas (:vingtetun) from comment #6) > Created attachment 678999 [details] [diff] [review] > Gaia Patch > > The Gaia patch is mostly based on the work of Alive. Thank you! Alive I can observe that the keyboard is sometimes on top of the lockscreen. Any ideas which change we need in zindex.css?

Comment on attachment 678999 [details] [diff] [review] Gaia Patch Review of attachment 678999 [details] [diff] [review]: ----------------------------------------------------------------- There is one serious problem in keyboard_manager.js, but the fix is easy, so I'm going to give r+ assuming you'll fix the bug. Note that I have not done the custom build of gecko that would be required for me to test this. I'm going to assume that you've tested and are confident it works. ::: apps/system/js/keyboard_manager.js @@ +44,5 @@ > + container.classList.remove('hide'); > + container.classList.add('visible'); > + var detail = { > + 'detail': { > + 'height': size size is undefined here. It looks like you dropped the line var size = parseInt(type[1]) @@ +46,5 @@ > + var detail = { > + 'detail': { > + 'height': size > + } > + }; I never understood why the transitionend handler was here before. I'm glad its gone, but since I don't know why it was there, I can't be sure that it is okay to take it away. Please confirm that you meant to remove it and that the keyboard open animation works without it. ::: apps/system/style/zindex.css @@ -118,5 @@ > -#screen.popup > [data-z-index-level="keyboard-frame"].visible, > -#screen.trustedui > [data-z-index-level="keyboard-frame"].visible, > -#screen.inline-activity > [data-z-index-level="keyboard-frame"].visible { > - z-index: 1023; /* Keep it just behind the modal dialog */ > -} I understand why the keyboard-overlay styles are being taken out, but I don't know enough about the zindex stuff to review the changes that modify the zindex of the keyboard-frame. I'm assuming that this is just a cleanup of unnecessary styles. Since I'm not qualified to reivew this file, please double-check that you're not accidentally deleting anything actually required by the keyboard.

(In reply to Vivien Nicolas (:vingtetun) from comment #7) > (In reply to Vivien Nicolas (:vingtetun) from comment #6) > > Created attachment 678999 [details] [diff] [review] > > Gaia Patch > > > > The Gaia patch is mostly based on the work of Alive. Thank you! > > Alive I can observe that the keyboard is sometimes on top of the lockscreen. > Any ideas which change we need in zindex.css? It's my bad to miss this. Add these below the z-index=2046(pinkeypadscreen) should resolve this. /* Keep keyboard frame under lockscreen when locked */ #screen.locked > [data-z-index-level="keyboard-frame"] { z-index: 2045; }

(In reply to David Flanagan [:djf] from comment #8) > Comment on attachment 678999 [details] [diff] [review] > Gaia Patch > > Review of attachment 678999 [details] [diff] [review]: > ----------------------------------------------------------------- > > There is one serious problem in keyboard_manager.js, but the fix is easy, so > I'm going to give r+ assuming you'll fix the bug. > > Note that I have not done the custom build of gecko that would be required > for me to test this. I'm going to assume that you've tested and are > confident it works. > > ::: apps/system/js/keyboard_manager.js > @@ +44,5 @@ > > + container.classList.remove('hide'); > > + container.classList.add('visible'); > > + var detail = { > > + 'detail': { > > + 'height': size > > size is undefined here. It looks like you dropped the line var size = > parseInt(type[1]) Oups. Thanks for catching it. > > @@ +46,5 @@ > > + var detail = { > > + 'detail': { > > + 'height': size > > + } > > + }; > > I never understood why the transitionend handler was here before. I'm glad > its gone, but since I don't know why it was there, I can't be sure that it > is okay to take it away. Please confirm that you meant to remove it and that > the keyboard open animation works without it. I meant to remove it. > > ::: apps/system/style/zindex.css > @@ -118,5 @@ > > -#screen.popup > [data-z-index-level="keyboard-frame"].visible, > > -#screen.trustedui > [data-z-index-level="keyboard-frame"].visible, > > -#screen.inline-activity > [data-z-index-level="keyboard-frame"].visible { > > - z-index: 1023; /* Keep it just behind the modal dialog */ > > -} > > I understand why the keyboard-overlay styles are being taken out, but I > don't know enough about the zindex stuff to review the changes that modify > the zindex of the keyboard-frame. I'm assuming that this is just a cleanup > of unnecessary styles. > > Since I'm not qualified to reivew this file, please double-check that you're > not accidentally deleting anything actually required by the keyboard. Yep. Alive is involved in the process, he is the zindex man.

What happens if we don't do that ?

Comment on attachment 678997 [details] [diff] [review] Gecko patch I don't think you want to remove the IsChrome() check; as written, only mozapp frames can have this attribute. This looks fine to me aside from that, but I've never even opened this file before, so I'd like a layout peer to look.

(In reply to dscravaglieri from comment #11) > What happens if we don't do that ? Half of the screen every time you hit a key. This results into a slower keyboard. This is a followup of a blocking basecamp+. (Bug 796452). Without this patch the work on this bug as no effects on Gaia.

Thanks Justin for the feedback. I add the isChrome check back. Roc wrote the code so I believe he is the right guy to review.

Is it really OK to allow any app to probe the status of each pixel of an IFRAME from any domain?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #15) > Is it really OK to allow any app to probe the status of each pixel of an > IFRAME from any domain? Hm, I thought we were granting a privilege to the embedder of the app, not to the app itself. But it sounds like that was wrong. We can predicate this on the app having a permission bit.

Comment on attachment 679453 [details] [diff] [review] Patch v2. Review of attachment 679453 [details] [diff] [review]: ----------------------------------------------------------------- OK, I'm expecting a new patch that restricts to only privileged apps.

This version restricts it only to application that have the embed-apps permission, i.e the permission to create mozapp iframes. In Gaia it means only the system application.

Comment on attachment 680162 [details] [diff] [review] Patch Review of attachment 680162 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsSubDocumentFrame.cpp @@ +176,5 @@ > + NS_ENSURE_TRUE(document, NS_NOINTERFACE); > + > + uint32_t permission = nsIPermissionManager::DENY_ACTION; > + permMgr->TestPermissionFromPrincipal(document->NodePrincipal(), > + "embed-apps", &permission); Just use GetContent()->NodePrincipal(). No null-checks needed. @@ +264,2 @@ > if (mContent->HasAttr(kNameSpaceID_None, nsGkAtoms::mozpasspointerevents) && > + (mHasEmbedAppsPerm || PresContext()->IsChrome())) { Why not just test for the permission here instead of caching? I assume the permission manager check is pretty fast? This code only runs once per paint or event.

I don't believe there is any good reasons.

Vivien, had you merged the gaia change you made? Some regression may happen if only gecko change applies....

(In reply to Alive Kuo [:alive] from comment #23) > Vivien, had you merged the gaia change you made? Some regression may happen > if only gecko change applies.... Not yet. I will land them synchronously once I land the Gecko changes in Aurora :)

> Not yet. I will land them synchronously once I land the Gecko changes in Aurora :) A note that the keyboard is broken in m-c might have been appreciated; I wasted a while screwing around with this yesterday before I tried Aurora.

(In reply to Justin Lebar [:jlebar] from comment #25) > > Not yet. I will land them synchronously once I land the Gecko changes in Aurora :) > > A note that the keyboard is broken in m-c might have been appreciated; I > wasted a while screwing around with this yesterday before I tried Aurora. I didn't know it is broken. And this patch should have no effect without the appropriate Gaia commit.

> I didn't know it is broken. Oh, okay! The keyboard was probably broken because of something else, then. :)

Backed out in https://hg.mozilla.org/releases/mozilla-aurora/rev/8b1cbefc1453 - Linux crashtests and reftests were crashing in nsDisplayList::HitTest, or nsINode::GetParent after asserting about "Primary child list can have at most one frame in it"

Vivien, my tree's in a bit of a mess right now, but try changing + if (!aFrame->GetContent()->GetParent()) { to + if (aFrame->GetContent() && !aFrame->GetContent()->GetParent()) {

Actually that's what I landed on trunk. See bug 796452 comment #39.

Re-landed on beta: https://hg.mozilla.org/releases/mozilla-beta/rev/66d68a6c5ae6