Closed Bug 808724 Opened 7 years ago Closed 7 years ago

Move the keyboard iframe above the application iframe and use the new property to ignore transparent background while resolving clicks

Categories

(Firefox OS Graveyard :: Gaia, defect, P1)

defect

Tracking

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed, firefox20 fixed)

RESOLVED FIXED
PreProduction
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed
firefox20 --- fixed

People

(Reporter: vingtetun, Assigned: vingtetun)

References

Details

(Keywords: smoketest, Whiteboard: QARegressExclude)

Attachments

(2 files, 4 obsolete files)

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.
Assignee: nobody → alive
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 :)
Attached patch Gecko patch (obsolete) — Splinter Review
Attachment #678997 - Flags: review?(justin.lebar+bug)
Attached patch Gaia PatchSplinter Review
The Gaia patch is mostly based on the work of Alive. Thank you!
Attachment #678941 - Attachment is obsolete: true
Attachment #678999 - Flags: review?(dflanagan)
(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.
Attachment #678999 - Flags: review?(dflanagan) → review+
(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 ?
blocking-basecamp: ? → -
Flags: needinfo?(21)
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.
Attachment #678997 - Flags: review?(justin.lebar+bug) → feedback+
(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.
blocking-basecamp: - → +
Depends on: 796452
Flags: needinfo?(21)
Attached patch Patch v2. (obsolete) — Splinter Review
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.
Assignee: alive → 21
Attachment #678997 - Attachment is obsolete: true
Attachment #679453 - Flags: review?(roc)
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.
Keywords: smoketest
Priority: -- → P1
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.
Attachment #679453 - Flags: review?(roc) → review-
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #679453 - Attachment is obsolete: true
Attachment #680162 - Flags: review?(roc)
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.
Attached patch Patch v3Splinter Review
I don't believe there is any good reasons.
Attachment #680162 - Attachment is obsolete: true
Attachment #680162 - Flags: review?(roc)
Attachment #680186 - Flags: review?(roc)
https://hg.mozilla.org/mozilla-central/rev/df73d3bca6e0
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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()) {
Whiteboard: QARegressExclude
Blocks: 1101029
You need to log in before you can comment on or make changes to this bug.