796452 - #1931 forces excessive invalidation, causes bad perf with keyboard

Status: VERIFIED FIXED

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

Description

[GH to BZ]

[GitHub issue by cgjones on 2012-07-31T23:25:47Z, https://github.com/mozilla-b2g/gaia/issues/3028]
#1931 puts the keyboard in -moz-element, which forces slow paths in gecko.  Specifically, @mattwoodrow found that it causes us to over-invalidate when keys are pressed; we invalidate the entire screen.  This slow path is extremely hard to "fix" in gecko.

@vingtetun the patch there landed without a description of the bug it was fixing, and it also landed without review.  Can you help us understand what problem you were trying to solve?  There's likely another fix that doesn't require putting the keyboard iframe in -moz-element.

Comment 1

[GH to BZ]

[GitHub comment by vingtetun on 2012-08-01T10:58:37Z]
On 01/08/2012 01:26, Chris Jones wrote:
> #1931 puts the keyboard in -moz-element, which forces slow paths in gecko.  Specifically, @mattwoodrow found that it causes us to over-invalidate when keys are pressed; we invalidate the entire screen.  This slow path is extremely hard to "fix" in gecko.
>
> @vingtetun the patch there landed without a description of the bug it was fixing, and it also landed without review.  Can you help us understand what problem you were trying to solve?  There's likely another fix that doesn't require putting the keyboard iframe in -moz-element.

The problem with the keyboard is about mouse/touch events. The keyboard 
want sometimes to display some popups above the content, for example 
when you long press on the [e] key to have accented characters.

Here the previous situation: (+ == something is drawn, _ == transparent)

[++++++++++_______________________] Keyboard layer
                                                [+++++++++++] 
Content layer

The issue with this solution is any mouse/touch events on the content, 
to reposition the cursor, or to pan, is caught by the keyboard layer.


The proposed solution using -moz-element is as if.

[______________________] Keyboard overlay with moz-element, 
pointer-events: none;
                                                [++++++++++++] 
Content layer
[++++++++++] Keyboard layer

This way events are dispatched correctly but you say it creates a 
performance issue.


>
> ---
> Reply to this email directly or view it on GitHub:
> https://github.com/mozilla-b2g/gaia/issues/3028

Comment 2

[GH to BZ]

[GitHub comment by cgjones on 2012-08-01T14:59:23Z]
Hm, I think I may not understand your diagram, or some of the ascii art was lost in translation :).

Why is -moz-element needed specifically?  Could we implement this solution with the keyboard frame as a descendent of the overlay?  I suspect if I understood your explanation I would already know the answer to that.

Comment 3

[GH to BZ]

[GitHub comment by vingtetun on 2012-08-01T15:02:08Z]
Grrr! github has reformat my comment :/

I will put it somewhere else...

Comment 4

[GH to BZ]

[GitHub comment by vingtetun on 2012-08-01T15:03:53Z]
On 01/08/2012 16:59, Chris Jones wrote:
> Hm, I think I may not understand your diagram, or some of the ascii art was lost in translation :).
>
> Why is -moz-element needed specifically?  Could we implement this solution with the keyboard frame as a descendent of the overlay?  I suspect if I understood your explanation I would already know the answer to that.

Is http://pastebin.mozilla.org/1729634 clearer?

> ---
> Reply to this email directly or view it on GitHub:
> https://github.com/mozilla-b2g/gaia/issues/3028#issuecomment-7429669

Comment 5

[GH to BZ]

[GitHub comment by cgjones on 2012-08-01T15:22:37Z]
Clearer, but I don't understand

> The issue with this solution is any mouse/touch events on the content, to reposition the cursor, or to pan, is caught by the keyboard layer.

Can you give an example?  I think I might understand the issue now, but I want to be sure.

Comment 6

[GH to BZ]

[GitHub comment by vingtetun on 2012-08-01T16:26:02Z]
On 01/08/2012 17:22, Chris Jones wrote:
> Clearer, but I don't understand
>
>> The issue with this solution is any mouse/touch events on the content, to reposition the cursor, or to pan, is caught by the keyboard layer.
> Can you give an example?  I think I might understand the issue now, but I want to be sure.
With the old approach:

1. Open the SMS app and hit the '+' button to create a new message.
2. tap on the field at the top to enter a phone number
3. the keyboard comes in
4. type '0123+456789'
5. Try to reposition the cursor to delete the '+'

Actual result:
  - the touch is caught by the keyboard overlay, nothing happens or the 
keyboard is dismissed if you give focus to the overlay.

Expected result:
  - move the cursor.

this is what has been fixed with -moz-element.

I hope this is clearer. In the worst case I'm available on IRC.
>
> ---
> Reply to this email directly or view it on GitHub:
> https://github.com/mozilla-b2g/gaia/issues/3028#issuecomment-7430283

Comment 7

[GH to BZ]

[GitHub comment by cgjones on 2012-08-01T19:20:24Z]
Yes, very helpful.

>  - the touch is caught by the keyboard overlay, nothing happens or the

What exactly do you mean by "caught"?

Comment 8

[GH to BZ]

[GitHub comment by vingtetun on 2012-08-01T19:23:06Z]
On 01/08/2012 21:20, Chris Jones wrote:
> Yes, very helpful.
>
>>   - the touch is caught by the keyboard overlay, nothing happens or the
> What exactly do you mean by "caught"?

Instead of beeing dispatched to the content iframe, the event is always 
dispatched to the keyboard iframe since it is over the content and takes 
all the height.

> ---
> Reply to this email directly or view it on GitHub:
> https://github.com/mozilla-b2g/gaia/issues/3028#issuecomment-7437117

Comment 9

[GH to BZ]

[GitHub comment by cgjones on 2012-08-01T19:28:07Z]
Wouldn't the same problem apply to events that should target the extended keyboard (e.g. popup menu for long-press on "e")?  That is, the content frame would "catch" them?

Comment 10

[GH to BZ]

[GitHub comment by vingtetun on 2012-08-02T02:57:32Z]
On 01/08/2012 21:28, Chris Jones wrote:
> Wouldn't the same problem apply to events that should target the extended keyboard (e.g. popup menu for long-press on "e")?  That is, the content frame would "catch" them?

Since the long press is started on the keyboard, 
element.setCapture(false) can be used (soon on device 
https://bugzilla.mozilla.org/show_bug.cgi?id=774190). Until the finger 
is released the events are dispatched to the keyboard frame, even if the 
mouse/finger is above he content frame.

Comment 11

[GH to BZ]

[GitHub comment by cgjones on 2012-08-02T06:08:29Z]
So why can't we use that solution for the non-moz-element implementation?

Comment 12

[GH to BZ]

[GitHub comment by vingtetun on 2012-08-02T06:10:07Z]
On 02/08/2012 08:08, Chris Jones wrote:
> So why can't we use that solution for the non-moz-element implementation?

Because if the mouse/touch event start on the keyboard frame that 
overlays the content, it will stay on the keyboard frame but it should 
be dispatched to the content.
>
> ---
> Reply to this email directly or view it on GitHub:
> https://github.com/mozilla-b2g/gaia/issues/3028#issuecomment-7447880

Comment 13

[GH to BZ]

[GitHub comment by vingtetun on 2012-08-03T15:50:44Z]
@cgjones What do you think of adding support in gecko for |pointer-events: visiblePainted| ? That would let the keyboard frame lives on top of it without absorbing events?

Comment 14

[GH to BZ]

[GitHub comment by autonome on 2012-09-25T02:35:04Z]
Any update on a solution to this?

Comment 15

[GH to BZ]

[GitHub comment by cgjones on 2012-09-25T05:52:21Z]
Now that everything is out of process, this problem is much simpler.  Only content within the system app will be invalidated (and the browser UI).

I think @vingtetun's idea is good, but that's not feasible for v1 unless we conjure a minor miracle, and additionally the touch-event spec is changing in such a way that that approach may not work.  I recommend that we (i) re-triage perf impact of this; it hasn't been high on my list for a while, but the perf has seemed to fluctuate lately; (ii) wait for dlbi and explore a 95% solution that improves invalidation for the system app.

Comment 16

[GH to BZ]

[GitHub comment by cgjones on 2012-09-25T05:56:27Z]
> additionally the touch-event spec is changing in such a way that that approach may not work.

Actually, I take that back --- the spec is changing in such a way that this approach *should* work.

Comment 17

[GH to BZ]

[GitHub comment by autonome on 2012-09-25T06:15:45Z]
what's the best way to measure the performance impact of this change?

Comment 18

[GH to BZ]

[GitHub comment by cgjones on 2012-09-25T06:17:38Z]
Eideticker.

Comment 19

[Andrew Overholt [:overholt]]

Vivien, cjones, is this still a blocking issue?  Do we have eideticker for B2G yet?

Comment 20

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I think keyboard perf in OOP apps right now is acceptable for v1.  (There's some jank when the keyboard first loads, but we can fix that.)

Perf is noticeably worse in the browser app (in-process).  I think we should do we what we can to work around this in the browser, since it's the only in-process app for v1.

Comment 21

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Re: Eideticker: yes, we have b2g stood up on pandaboards, so eideticker (should) work out of the box.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Is the problem here that there is no way to have an <iframe> element not receive events on itself but allow its subdocument content to receive events?

If that's the problem, then we could introduce a new iframe attribute, let's say "mozpasspointerevents". When set, then regardless of the value of pointer-events on the <iframe>, we'd allow subdocument children to receive events if they have a value of pointer-events that's not 'none' (just like regular children of a pointer-events:none DOM element can receive events by changing their pointer-events to something other than none). I think this would be a couple of lines of code to implement.

Then I think you could put the keyboard frame on top, make it pointer-events:none, give it the mozpasspointerevents attribute, set pointer-events:none on the root element of the keyboard app, and set pointer-events:auto on the elements inside the keyboard frame that actually need to receive events.

How does that sound?

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22)
> I think this would be a couple of lines of code to implement.

For same-process subdocuments. For cross-process documents, it's far harder to implement, but fortunately the keyboard app is in-process.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: mozpasspointerevents

Comment 25

[Andreas Gal :gal]

++roc ++mats, awesome, lets see if this works

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Unfortunately, now that I got around to writing a proper test, the patch doesn't quite work yet. Working on it...

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The problem is that the CSS canvas of the subdocument catches all events. We need it to respect pointer-events:none set on the root element.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: mozpasspointerevents, v2, with mochitest

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Of course once this lands, someone will still need to modify Gaia so that the keyboard app is displayed on top without the -moz-element stuff, and add mozpasspointerevents, and sprinkle pointer-events:none and pointer-events:auto around in the keyboard app.

Comment 30

[Mats Palmgren (inactive)]

Comment on attachment 677287 [details] [diff] [review]
mozpasspointerevents, v2, with mochitest

> layout/style/nsStyleStructInlines.h

+nsStyleVisibility::GetEffectivePointerEvents(nsIFrame* aFrame) const
+{
+  if (!aFrame->GetContent()->GetParent()) {

Will null-pointer crash if aFrame is a viewport frame.  I suppose that can't
happen from the one call site you have now, but it seems nicer if the
methods in this file works for any valid frame.

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sure, I'll fix that.

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/954d162c91b6

We'll need this on Aurora I guess.

Comment 33

[Andreas Gal :gal]

Comment on attachment 677287 [details] [diff] [review]
mozpasspointerevents, v2, with mochitest

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 34

[Alex Keybl [:akeybl]]

(In reply to Andreas Gal :gal from comment #33)
> Risk to taking this patch (and alternatives if risky): 

If this patch has risk to desktop/mobile, please re-nominate with a risk evaluation. Otherwise, you're free to land with a=blocking-basecamp

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/954d162c91b6

Comment 36

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b43e0fcc9716

Comment 37

[Phil Ringnalda (:philor)]

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"

Comment 38

[Mats Palmgren (inactive)]

Vivien landed the wrong patch on Aurora.  The difference might be the cause of the
crashes actually.

Comment 39

[Mats Palmgren (inactive)]

... roc addressed my nit in comment 30 adding a null check when landing on m-i.

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Ah, right! I just spend ten minutes figuring that out :-)

Comment 41

[Tim Taubert [:ttaubert] (inactive)]

So we just found that for some reason this change is still on Aurora:

https://hg.mozilla.org/releases/mozilla-aurora/annotate/4e0a8f234a6d/content/base/src/nsGkAtomList.h#l573

It actually has been backed out but maybe that's a bad merge? It has the same revision id as the patch that landed on m-c (954d162c91b6). The change is not on beta.

Comment 42

[Tim Taubert [:ttaubert] (inactive)]

Please disregard my previous confused comment.

Comment 43

[Tim Taubert [:ttaubert] (inactive)]

Relanded on beta:

https://hg.mozilla.org/releases/mozilla-beta/rev/77e1ac74d715

Comment 44

[Chris Kreinbring [:CKreinbring]]

Verified in Unagi version 20130104070203.  Attempting to move the cursor to the middle of entered text in textboxes now places the cursor at the tapped location.