Closed Bug 1376538 Opened 2 years ago Closed 2 years ago

APZ: Transmit focus updates more frequently

Categories

(Core :: Panning and Zooming, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: rhunt, Assigned: rhunt)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file)

Async keyboard scrolling can only happen when APZ has the current focus target. We currently transmit a focus update in nsDisplayList::PaintRoot, but we should be able to transmit more frequently than that.

(see Botond's comment in bug 1351783 comment 168)
Blocks: 1376525
The description of this bug made me realize that I actually made a mistake in bug 1351783 comment 168:

(In reply to Botond Ballo [:botond] from comment #168)
> (In reply to Botond Ballo [:botond] from comment #55)
> > > +Bug 1351783 part 10 - Create and sync the current FocusTarget on each layer transaction r?kats
> > > +
> > > +This commit modifies PresShell and nsDisplayList to send a FocusTarget update on
> > > +every layer transaction. Ideally we would like to send updates as often as possible,
> > > +but this seems like it works well. This can be iterated on later, if necessary.
> > 
> > As a lead for future iteration on this: look into "empty transactions" (bug
> > 1257641 is a good place to look), and make sure a focus update is sent with
> > empty transactions too.
> 
> It looks like empty transactions still go through PresShell::Paint(), so
> they will already pick up the focus update with the code as written.

The focus update is sent in nsDisplayList::PaintRoot(), not PresShell::Paint(), is nsDisplayList::PaintRoot() is *not* called for empty transactions (because for empty transactions we early-exit PresShell::Paint() here [1] or here [2], while PaintRoot() is called from PaintFrame() [3]).

So we will need two code changes in this bug:

  1) Sending the focus target as part of an empty transaction

  2) Scheduling an empty transaction when the focus target changes

[1] http://searchfox.org/mozilla-central/rev/b425854d9bbd49d5caf9baef3686e49ec91c17ec/layout/base/PresShell.cpp#6356
[2] http://searchfox.org/mozilla-central/rev/b425854d9bbd49d5caf9baef3686e49ec91c17ec/layout/base/PresShell.cpp#6401
[3] http://searchfox.org/mozilla-central/rev/b425854d9bbd49d5caf9baef3686e49ec91c17ec/layout/base/PresShell.cpp#6426
Attached patch focus-send.patchSplinter Review
I don't really know why we were calling layerManager->SetFocusTarget in nsDisplayList. There isn't any reason for it anymore (there was before), so this patch fixes that, which simplifies some things.

This patch then requests an empty transaction whenever we get a new focus sequence number so that we get focus updates quicker. This can potentially happen frequently, so it's important that we don't spam IPC with updates. I'm told by Botond that using an empty transaction could be a good fit for this.
Attachment #8882001 - Flags: review?(bugmail)
Comment on attachment 8882001 [details] [diff] [review]
focus-send.patch

Review of attachment 8882001 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. Might be good to amend the commit message to specifically mention empty transactions.
Attachment #8882001 - Flags: review?(bugmail) → review+
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ec5e890bcbd
Transmit APZ focus target updates on empty transactions. r=kats
https://hg.mozilla.org/mozilla-central/rev/8ec5e890bcbd
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Depends on: 1378006
Depends on: 1377814
You need to log in before you can comment on or make changes to this bug.