1351783 - (apz-keyboard) APZ: Add support for async scrolling using keyboard input

Status: RESOLVED FIXED

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

Description

[(no longer active)]

We'd like to try to see if we can add support for keyboard scrolling to APZ.  We had a meeting about this at the QF work week.  Kats and Botond can fill in the details of the discussion...

Comment 1

[Kartikaya Gupta (email:[e-mail])]

To do this, we would need to:

- Add a KeyboardInputType in widget/InputData.* alongside the other input types
- Add code to route keyboard inputs from widget code to APZ before it goes on to content, just like the other event types
- Add a key -> action mapping in APZ somewhere that determines that e.g. "up arrow button" corresponds to "scroll up one line".
- Propagate a "allow APZ to handle keyboard input" scrolling flag from the content to APZ, which is true if (a) the root scrolling element of the content has focus and (b) there are no key listeners on the document that can prevent-default the event.
- Add code in APZ that checks the aforementioned flag and, if allowed, processes key inputs to trigger the necessary APZ scrolling animations using the aforementioned action mapping. Note that this step should also skip processing key inputs if other input events have passed through APZ since the flag was last updated. This is needed for correctness because those other input events may result in focus changes that are deterministic in the non-APZ model.
- Test and bugfix

- Bonus: Eventually we might want to support keyboard scrolling even with interleaved mousemove events, provided that the mousemove is known to not trigger a mousemove listener. Doing this would require propagating additional state from content to APZ. We can propagate the state either as a per-layer flag or more complicated areas.

Before doing any of this we also want to add telemetry probes to measure how often pages have non-passive keyboard event listeners on the root content document/root scrollframe. We might also want to know how often pages have mousemove events on the root, as it will affect whether we want to do the bonus step above.

Something else that I thought of while writing the above is whether or not the parent process has a separate focus state from the content process. If so we might need to add a bit more machinery to handle that. Otherwise the content process might think it is APZ-keyboard-scrollable (because it has focus) but really some other part of the chrome has focus and keyboard input should go there rather than to the content document.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Addition
- APZ should not scroll root scrollable if it is contentEditable, document is in designMode or caret browsing is enabled.


And yes, chrome has its own focus manager.

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #1)
> ...
> 
> Before doing any of this we also want to add telemetry probes to measure how
> often pages have non-passive keyboard event listeners on the root content
> document/root scrollframe. We might also want to know how often pages have
> mousemove events on the root, as it will affect whether we want to do the
> bonus step above.

I'm not sure we're blocked on getting this telemetry, pretty sure that we'd implement keyboard APZ anyway, but either way, let's do the telemetry in the separate bug 1357880

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Telemetry data is starting to come in. On nightly the APZ_AWARE_KEY_LISTENERS flag is true ~14% of the time. So we can do basic APZ acceleration of keyboard scrolling on 86% of page loads.

APZ_AWARE_MOUSEMOVE_LISTENERS is true ~12% of the time. If we assume this is totally independent of APZ_AWARE_KEY_LISTENERS, then that means 88% * 86% = 75% of page loads will also be able to get the bonus behaviour of accelerating APZ keyboard scrolling even with interleaved mousemove events. However, assuming "totally independent events" is kind of a worst-case scenario. In practice I expect pages with one kind of listener to be highly correlated with pages with the other kind of listener. Which means that in practice I think we'll be able to do the bonus behaviour on more pages, probably at least 80%.

So yeah, nothing in this data makes me think we *shouldn't* do either keyboard APZ or the bonus behaviour.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

I very much agree.
(and I'm still not convinced we need to care about mousemove listeners at all. Might be good to have a pref for whether to care about mousemoves or not.)

Comment 6

[Ryan Hunt [:rhunt]]

I have set of patches now that implements this. It is not ready to be turned on by default, but it's at a stage where it works well for pages I can find, so I'd like to get it landed behind a pref and iterate before looking into enabling it by default.

With the pref disabled
https://treeherder.mozilla.org/#/jobs?repo=try&revision=efff839b0cc5a3933ab9c557d2322aecdafc738e

With the pref enabled (I see some related jetpack failures to investigate)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff6465bfcc93c972dfdda76a1b37ddd5a99ce0a3

Kats, can you flag proper reviewers for these patches if you are not the one?

Comment 7

[Ryan Hunt [:rhunt]]

Attachment: Bug 1351783 part 1 - Add includes for unified build issues.

Review commit: https://reviewboard.mozilla.org/r/146544/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146544/

Comment 8

[Ryan Hunt [:rhunt]]

Attachment: Bug 1351783 part 2 - Add a KeyboardInput type.

Every event type handled by APZ needs to have a InputData type. This commit
adds a new KeyboardInput type that stores the minimum fields needed to match
keyboard shortcuts.

Review commit: https://reviewboard.mozilla.org/r/146546/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146546/

Comment 9

[Ryan Hunt [:rhunt]]

Attachment: Bug 1351783 part 3 - Add a KeyboardScrollAction type.

The different types of keyboard scrolls are represented as command strings that
are dispatched to nsGlobalWindowCommands. This commit adds a class to represent
these command strings, along with a function to find the KeyboardAction corresponding
to a command string.

Review commit: https://reviewboard.mozilla.org/r/146548/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146548/

Comment 10

[Ryan Hunt [:rhunt]]

Attachment: Bug 1351783 part 4 - Add a KeyboardShortcut type.

Keyboard scrolling works by first dispatching a key event to the focused element
of the page. It is then caught by a XBL binding put on the chrome event handler of
every window. The XBL binding searches through all of its handlers to find one that
can handle the keyboard event. The matching binding has a command string which is
dispatched to the nsGlobalWindowCommands which dispatches to PresShell which
does the actual scrolling.

To do this asynchronously we need a representation of the XBL handlers that can
be applied to a KeyboardInput to get a KeyboardAction.

This commit adds KeyboardShortcut for this purpose. KeyboardShortcut is designed to
be compatible with nsXBLPrototypeHandler and to only handle the specific cases we care
about for keyboard scrolling. If a XBL handler runs javascript or does anything else we
cannot handle in an OMT situation, then we create a dispatch-to-content KeyboardShortcut.

Review commit: https://reviewboard.mozilla.org/r/146550/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146550/

Comment 11

[Ryan Hunt [:rhunt]]

Attachment: Bug 1351783 part 5 - Add a KeyboardMap type.

The XBL bindings used for scrolling are managed by a nsXBLWindowKeyHandler. This
class loads the handlers and has logic for searching through them to match a
keyboard event. This commit adds a KeyboardMap class for storing KeyboardShortcuts
and for mirroring the search logic of nsXBLWindowKeyHandler.

Review commit: https://reviewboard.mozilla.org/r/146552/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146552/

Comment 12

[Ryan Hunt [:rhunt]]

Attachment: Bug 1351783 part 6 - Create and send KeyboardMap to APZCTreeManager.

This commit makes it so we initialize, send, and store a KeyboardMap for every
APZCTreeManager for later keyboard event processing.

This is a naive approach so it may be worth improving.

Review commit: https://reviewboard.mozilla.org/r/146554/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146554/

Comment 13

[Ryan Hunt [:rhunt]]

Attachment: Bug 1351783 part 7 - Create FocusState and FocusTarget types.

This commit begins the work needed for tracking focus by creating two new classes,
FocusTarget and FocusState. FocusState is created and used by APZCTreeManager to
track the global focus information, while FocusTarget is created per layer tree and
sent to APZ with local focus information. Between the two we are able to figure out
what the correct scrollable layer is to use in response to a keyboard scroll.

See the comment in `FocusState.h` for more details on the architecture and things
needed in future patches to complete this.

Note, FocusState contains an internal set of focus targets it has received indexed
by their layer tree ID. This set only ever grows and that might not be optimal, it
also forms an implicit tree by starting at a specific node and following ref layers
to a leaf. This means that an incorrectly constructed focus target could cause apz
to spin in an infinite loop. A better solution may be needed for this.

Review commit: https://reviewboard.mozilla.org/r/146556/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146556/

Comment 14

[Ryan Hunt [:rhunt]]

Attachment: Bug 1351783 part 8 - Gather whether there are key listeners on the focused element.

This commit updates FocusTarget to collect whether there are key or scroll listeners
on the event target chain for the focused element. This is needed because we cannot
do async scrolling when this is the case.

Review commit: https://reviewboard.mozilla.org/r/146558/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146558/

Comment 15

[Ryan Hunt [:rhunt]]

Attachment: Bug 1351783 part 9 - Disable a FocusTarget for an editable element.

This commit updates FocusTarget to disable itself if the focused element is editable,
or is a part of an editable document. This is needed because we cannot do async
scrolling when this is the case.

Review commit: https://reviewboard.mozilla.org/r/146560/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146560/

Comment 16

[Ryan Hunt [:rhunt]]

Attachment: Bug 1351783 part 10 - Create and sync the current FocusTarget on each layer transaction.

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.

Review commit: https://reviewboard.mozilla.org/r/146562/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146562/

Comment 17

[Ryan Hunt [:rhunt]]

Attachment: Bug 1351783 part 12 - Create and sync focus sequence numbers.

Focus can change at any moment in a document. This causes non-determinism and
correctness problems for doing keyboard apz scrolling. To get around this, we
will maintain deterministic behavior for focus changes initiated by event listeners
and see if we can get away with more non-determinism for things like `setTimeout`

In order to do this, we disable async keyboard scrolling when an event is processed
that could have a event listener. We then attach a sequence number to that event
and dispatch it to content. In content, we record the highest sequence number that
we have processed from an event, and send that on each focus update. Using this, we
can determine in APZ if we have a current focus target or if we are still waiting
for an event to be processed and focus to be reconfirmed.

Review commit: https://reviewboard.mozilla.org/r/146564/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146564/

Comment 18

[Ryan Hunt [:rhunt]]

Attachment: Bug 1351783 part 13 - Add a function for determing if a ScrollSnapInfo has scroll snap points.

Review commit: https://reviewboard.mozilla.org/r/146566/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146566/

Comment 19

[Ryan Hunt [:rhunt]]

Attachment: Bug 1351783 part 15 - Hook up APZC for scrolling based on a KeyboardScrollAction.

This commit adds code for keyboard scroll animations and computing the delta
needed for a keyboard action. Keyboard scrolling also doesn't work well with
scroll snapping for some reason, so we ignore any scrolls when we have scroll
snap points.

Review commit: https://reviewboard.mozilla.org/r/146568/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146568/

Comment 20

[Ryan Hunt [:rhunt]]

Attachment: Bug 1351783 part 16 - Perform async scrolling for keyboard events when possible.

This commit ties it all together by dispatching keyboard actions to scroll targets
in response to keyboard inputs when we have current and valid focus state.

Review commit: https://reviewboard.mozilla.org/r/146570/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146570/

Comment 21

[Ryan Hunt [:rhunt]]

Attachment: Bug 1351783 part 11 - Sync FocusTarget with WebRenderLayerManager.

Invalidating the focus state on every mousemove event can cause situations where we sync
scroll because the user has moved their mouse recently and a repaint hasn't come soon enough.

As an optimization, we can track when we have mousemove event listeners and not invalidate
our focus state if we see that there are none.

Review commit: https://reviewboard.mozilla.org/r/146572/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146572/

Comment 22

[Ryan Hunt [:rhunt]]

Attachment: Bug 1351783 part 17 - Do less work when apz.keyboard.enabled is false.

When keyboard apz is disabled, we don't need to calculate focus targets and
we don't need to update focus state. It should be harmless even if it's done,
but I think it's good to not add something on this critical path that doesn't
do anything.

This commit also disable keyboard map generation in this case too for similar
reasoning. This has the side effect that you can't turn on keyboard apz without
doing a restart.

Review commit: https://reviewboard.mozilla.org/r/146574/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146574/

Comment 23

[Ryan Hunt [:rhunt]]

As a side note, I've ran into enough problems debugging why keyboard scrolling isn't async for page, that I think a logging system would be nice to add.

Comment 24

[Julien L.]

(In reply to Ryan Hunt [:rhunt] from comment #6)
> > With the pref enabled (I see some related jetpack failures to investigate)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=ff6465bfcc93c972dfdda76a1b37ddd5a99ce0a3
> 
> Kats, can you flag proper reviewers for these patches if you are not the one?

Hello, could you please explain me where I can download the build you made with this patch so I can test it? If it's already possible of course...
Thanks :)

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-ff6465bfcc93c972dfdda76a1b37ddd5a99ce0a3/
(Looks like no linux builds)

Comment 26

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8875157 [details]
Bug 1351783 part 1 - Add includes for unified build issues.

https://reviewboard.mozilla.org/r/146544/#review150706

Comment 27

[Kartikaya Gupta (email:[e-mail])]

Added some more reviewers for various bits. I didn't go through the patches in detail yet, but I noticed one thing missing - we should update the about:support code to also report if keyboard APZ is enabled or not. You'll have to tie that in at http://searchfox.org/mozilla-central/rev/20963d7269b1b14d455f47bc0260d0653015bf84/gfx/thebes/gfxPlatform.cpp#2541 or thereabouts.

Comment 28

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8875158 [details]
Bug 1351783 part 2 - Add a KeyboardInput type.

https://reviewboard.mozilla.org/r/146546/#review150890

::: gfx/ipc/GfxMessageUtils.h:1086
(Diff revision 1)
>    }
>  };
>  
>  template <>
> +struct ParamTraits<mozilla::KeyboardInput::KeyboardEventType>
> +  : public ContiguousEnumSerializer<

This should probably also go in nsGUIEventIPC.h along with the KeyboardInput type.

::: widget/InputData.cpp:811
(Diff revision 1)
> +    case eKeyDown: {
> +      mType = KeyboardInput::KEY_DOWN;
> +      break;
> +    }
> +    default:
> +      MOZ_ASSERT_UNREACHABLE("Did not assign a type to a KeyboardInput");

Do we want to assert here? There appear to be other message types like eKeyDownOnPlugin and such that would fall into this assertion and crash debug builds. Maybe we should just have a generic "KeyboardInput::OTHER" type to handle all the remaining messages?

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8875165 [details]
Bug 1351783 part 9 - Disable a FocusTarget for an editable element.

https://reviewboard.mozilla.org/r/146560/#review150900

::: gfx/layers/apz/src/FocusTarget.cpp:67
(Diff revision 1)
>    }
>    return false;
>  }
>  
> +static bool
> +IsEditableElement(nsIContent* aContent)

This could be IsEditableNode(nsINode* aNode)

::: gfx/layers/apz/src/FocusTarget.cpp:76
(Diff revision 1)
> +          aContent->IsHTMLElement(nsGkAtoms::input) ||
> +          aContent->IsHTMLElement(nsGkAtoms::textarea) ||
> +          aContent->IsXULElement(nsGkAtoms::editor));
> +}
> +
> +static bool

Then this wouldn't be needed

::: gfx/layers/apz/src/FocusTarget.cpp:147
(Diff revision 1)
>      mType = FocusTarget::NONE;
>      return;
>    }
>  
> +  // If the focus isn't on a remote browser then check for scrollable targets
> +  if (IsEditableElement(focusTarget) ||

And this could be then I guess just
if (IsEditableNode(focusTarget ? focusTarget : aDocument))

Comment 30

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8875160 [details]
Bug 1351783 part 4 - Add a KeyboardShortcut type.

https://reviewboard.mozilla.org/r/146550/#review150918

::: gfx/layers/apz/src/Keyboard.cpp:88
(Diff revision 1)
> +  , mDispatchToContent(true)
> +{
> +}
> +
> +bool
> +KeyboardShortcut::Matches(const KeyboardInput& aInput, const IgnoreModifierState& aIgnore, uint32_t mOverrideCharCode) const

s/mOverrideCharCode/aOverrideCharCode/

Comment 31

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8875159 [details]
Bug 1351783 part 3 - Add a KeyboardScrollAction type.

https://reviewboard.mozilla.org/r/146548/#review150920

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8875164 [details]
Bug 1351783 part 8 - Gather whether there are key listeners on the focused element.

https://reviewboard.mozilla.org/r/146558/#review150916

::: dom/events/EventTarget.cpp:68
(Diff revision 1)
> +  EventListenerManager* elm = GetExistingListenerManager();
> +  return elm && elm->HasKeyEventListeners();
> +}
> +
> +bool
> +EventTarget::HasScrollEventListeners() const

I don't know why we need to know about scroll event listeners.
I feel like I'm missing something.
scroll event listeners are just callbacks running around the same time as rAF callbacks. Why we care about scroll listeners but not rAF?
Scroll events are dispatched _after_ something has been scrolled.

I feel like I'm missing something here.

::: gfx/layers/apz/src/FocusTarget.cpp:21
(Diff revision 1)
>  
>  namespace mozilla {
>  namespace layers {
>  
> +static bool
> +HasListenersForInvalidEvents(nsIContent* aContent,

Invalid? What events are invalid?

::: gfx/layers/apz/src/FocusTarget.cpp:26
(Diff revision 1)
> +HasListenersForInvalidEvents(nsIContent* aContent,
> +                             mozilla::dom::EventTarget* aChromeEventHandler)
> +{
> +  using namespace mozilla::dom;
> +
> +  if (!aContent)

always {} with if

::: gfx/layers/apz/src/FocusTarget.cpp:53
(Diff revision 1)
> +      continue;
> +    }
> +
> +    // As above, xul:browser contains some bindings that effectively disable async
> +    // key scrolling. So we ignore xul:browser elements.
> +    nsCOMPtr<nsIContent> targetContent = do_QueryInterface(targets[i]);

This looks slow. QIing each target to nsIXULDocument and nsIContent. Could we do anything faster here? Perhaps add some virtual method to EventTarget?

Comment 33

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8875161 [details]
Bug 1351783 part 5 - Add a KeyboardMap type.

https://reviewboard.mozilla.org/r/146552/#review150940

Comment 34

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8875162 [details]
Bug 1351783 part 6 - Create and send KeyboardMap to APZCTreeManager.

https://reviewboard.mozilla.org/r/146554/#review150942

Comment 35

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8875159 [details]
Bug 1351783 part 3 - Add a KeyboardScrollAction type.

https://reviewboard.mozilla.org/r/146548/#review151104

::: dom/base/nsGlobalWindowCommands.h:31
(Diff revision 1)
> +class nsGlobalWindowCommands
> +{
> +public:
> +  typedef mozilla::layers::KeyboardAction KeyboardAction;
> +
> +  static bool FindScrollCommand(const char* aCommandName, KeyboardAction* aOutAction);

nit: too long line. Please break after |aCommandName,|.

::: dom/base/nsGlobalWindowCommands.cpp:275
(Diff revision 1)
>    return caretOn;
>  }
>  
>  static constexpr struct BrowseCommand {
>    const char *reverse, *forward;
> +  mozilla::layers::KeyboardAction::KeyboardActionType scrollAction;

At line # 44, |using namespace mozilla;|. So, I think that you should add |using namespace mozilla::layers;| to the next line. Then, you should remove |mozilla::layers::| from here and below.

::: dom/base/nsGlobalWindowCommands.cpp:349
(Diff revision 1)
>  // ScrollLine, etc., as if for horizontal-mode content, but this may need
>  // to be reconsidered once we have more experience with vertical content.
>  static const struct PhysicalBrowseCommand {
>    const char *command;
>    int16_t direction, amount;
> +  mozilla::layers::KeyboardAction::KeyboardActionType scrollAction;

Remove mozilla::layers:: from here and below.

::: dom/base/nsGlobalWindowCommands.cpp:1288
(Diff revision 1)
>  
>    return rv;
>  }
> +
> +/* static */ bool
> +nsGlobalWindowCommands::FindScrollCommand(const char* aCommandName, KeyboardAction* aOutAction)

nit: too long. Break after |aCommandName,|.

::: dom/base/nsGlobalWindowCommands.cpp:1290
(Diff revision 1)
> +  for (size_t i = 0; i < ArrayLength(physicalBrowseCommands); i++) {
> +    const PhysicalBrowseCommand& cmd = physicalBrowseCommands[i];
> +    if (!strcmp(aCommandName, cmd.command)) {
> +      int16_t dir = cmd.direction;
> +      bool forward = (dir == nsISelectionController::MOVE_RIGHT ||
> +                      dir == nsISelectionController::MOVE_DOWN);
> +
> +      *aOutAction = KeyboardAction(cmd.scrollAction, forward);
> +      return true;
> +    }
> +  }
> +
> +  for (size_t i = 0; i < ArrayLength(browseCommands); i++) {
> +    const BrowseCommand& cmd = browseCommands[i];
> +    bool forward = !strcmp(aCommandName, cmd.forward);
> +    bool reverse = !strcmp(aCommandName, cmd.reverse);
> +    if (forward || reverse) {
> +      *aOutAction = KeyboardAction(cmd.scrollAction, forward);
> +      return true;
> +    }
> +  }

Could you explain the reason why physicalBrowseCommands is preferred than browseCommands with comment in this method?

::: gfx/layers/apz/src/Keyboard.h:18
(Diff revision 1)
> +
> +
> +/**
> + * This class represents a scrolling action to be performed on a scrollable layer.
> + */
> +class KeyboardAction final

All of these members are public. So, why don't you use struct here? But up to you.

::: gfx/layers/apz/src/Keyboard.h:21
(Diff revision 1)
> + * This class represents a scrolling action to be performed on a scrollable layer.
> + */
> +class KeyboardAction final
> +{
> +public:
> +  enum KeyboardActionType {

nit: put |{| to the next line.

::: gfx/layers/apz/src/Keyboard.h:28
(Diff revision 1)
> +    SCROLL_LINE,
> +    SCROLL_PAGE,
> +    COMPLETE_SCROLL,
> +
> +    // Used as an upper bound for ContiguousEnumSerializer
> +    SENTINEL,

Is "sentinel" usually used for this purpose? If so, it's okay, but otherwise, perhaps, NUMBER_OF_KEYBOARD_ACTION_TYPE or KEYBOARD_ACTION_TYPE_COUNT is clearer to me.

::: gfx/layers/apz/src/Keyboard.h:30
(Diff revision 1)
> +    COMPLETE_SCROLL,
> +
> +    // Used as an upper bound for ContiguousEnumSerializer
> +    SENTINEL,
> +  };
> +  static nsIScrollableFrame::ScrollUnit GetScrollUnit(KeyboardActionType aDeltaType);

nit: over 80 characters. But this is under gfx/. so, it might be allowed up to 120 characters. (I'm not sure.)

::: gfx/layers/apz/src/Keyboard.h:36
(Diff revision 1)
> +  KeyboardActionType mType;
> +  bool mForward;

If you make KeyboardActionType as uint16_t or uint8_t, then, the instance becomes 4 bytes in x86, 8 bytes in x64. So, I think that you should declare KeyboardActionType as:

enum KeyboardActionType : uint8_t
{

::: gfx/layers/apz/src/Keyboard.cpp:21
(Diff revision 1)
> +    case KeyboardAction::COMPLETE_SCROLL:
> +      return nsIScrollableFrame::WHOLE;

Sigh... nsISelectionController uses the word "complete"? Should be replaced with "whole"...

::: gfx/layers/moz.build:102
(Diff revision 1)
>      # proper interface for the code there
>      'apz/src/APZCTreeManager.h',
>      'apz/src/APZUtils.h',
>      'apz/src/AsyncDragMetrics.h',
>      'apz/src/AsyncPanZoomAnimation.h',
> +    'apz/src/Keyboard.h',

Keyboard.h and Keyboard.cpp are not good names. Perhaps, KeyboardAction.h and KeyboardAction.cpp?

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8875159 [details]
Bug 1351783 part 3 - Add a KeyboardScrollAction type.

https://reviewboard.mozilla.org/r/146548/#review151114

::: gfx/layers/moz.build:102
(Diff revision 1)
>      # proper interface for the code there
>      'apz/src/APZCTreeManager.h',
>      'apz/src/APZUtils.h',
>      'apz/src/AsyncDragMetrics.h',
>      'apz/src/AsyncPanZoomAnimation.h',
> +    'apz/src/Keyboard.h',

Ah, but you add another class into these files. Although, I don't like Keyboard.(h|cpp), I have no idea of better name.

Comment 37

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8875160 [details]
Bug 1351783 part 4 - Add a KeyboardShortcut type.

https://reviewboard.mozilla.org/r/146550/#review151116

I'd like to check this patch again. And note that even if you agree with the file name issue, it's okay to append rename patch since changing file name causes breaking following patches and hard to merge.

::: dom/xbl/nsXBLPrototypeHandler.h:30
(Diff revision 1)
>  namespace dom {
>  class AutoJSAPI;
>  class EventTarget;
>  } // namespace dom
>  } // namespace mozilla
>  
> +namespace mozilla {
> +namespace layers {
> +class KeyboardShortcut;
> +} // namespace layers
> +} // namespace mozilla

Why don't you add |layers| into above |mozilla| namespace?

::: dom/xbl/nsXBLPrototypeHandler.h:75
(Diff revision 1)
>  class nsXBLPrototypeHandler
>  {
>    typedef mozilla::dom::IgnoreModifierState IgnoreModifierState;
> +  typedef mozilla::layers::KeyboardShortcut KeyboardShortcut;

Sigh, I should move these classes into mozilla namespace...

::: dom/xbl/nsXBLPrototypeHandler.h:100
(Diff revision 1)
>    // This constructor is used for handlers loaded from the cache
>    explicit nsXBLPrototypeHandler(nsXBLPrototypeBinding* aBinding);
>  
>    ~nsXBLPrototypeHandler();
>  
> +  bool TryConvertToKeyboardShortcut(KeyboardShortcut* aOut) const;

Could you add explanation for this method? Especially, aOut shouldn't be nullptr and meaning of the result.

::: dom/xbl/nsXBLPrototypeHandler.cpp:142
(Diff revision 1)
>    // We own the next handler in the chain, so delete it now.
>    NS_CONTENT_DELETE_LIST_MEMBER(nsXBLPrototypeHandler, this, mNextHandler);
>  }
>  
> +bool
> +nsXBLPrototypeHandler::TryConvertToKeyboardShortcut(KeyboardShortcut* aOut) const

Too long. Could you break after |(| and starts next line under "y" of "Try".

::: dom/xbl/nsXBLPrototypeHandler.cpp:144
(Diff revision 1)
>  }
>  
> +bool
> +nsXBLPrototypeHandler::TryConvertToKeyboardShortcut(KeyboardShortcut* aOut) const
> +{
> +  using namespace mozilla::layers;

Hmm, cannot move this to global scope of this file? If it causes bustage, it's okay. But I don't like to use |using namespace| in specific scope.

::: dom/xbl/nsXBLPrototypeHandler.cpp:163
(Diff revision 1)
> +  if (mKeyMask & cMetaMask) {
> +    modifiersMask |= MODIFIER_META;
> +    if (mKeyMask & cMeta) {
> +      modifiers |= MODIFIER_META;
> +    }
> +  }
> +  if (mKeyMask & cOSMask) {
> +    modifiersMask |= MODIFIER_OS;
> +    if (mKeyMask & cOS) {
> +      modifiers |= MODIFIER_OS;
> +    }
> +  }
> +  if (mKeyMask & cShiftMask) {
> +    modifiersMask |= MODIFIER_SHIFT;
> +    if (mKeyMask & cShift) {
> +      modifiers |= MODIFIER_SHIFT;
> +    }
> +  }
> +  if (mKeyMask & cAltMask) {
> +    modifiersMask |= MODIFIER_ALT;
> +    if (mKeyMask & cAlt) {
> +      modifiers |= MODIFIER_ALT;
> +    }
> +  }
> +  if (mKeyMask & cControlMask) {
> +    modifiersMask |= MODIFIER_CONTROL;
> +    if (mKeyMask & cControl) {
> +      modifiers |= MODIFIER_CONTROL;
> +    }
> +  }

Please move this into two methods, one is for modifiers and the other is for modifiersMask. Perhaps, nsXBLPrototypeHandler::GetModifiers() and nsXbLPrototypeHandler::GetModifiersMask()?

::: dom/xbl/nsXBLPrototypeHandler.cpp:206
(Diff revision 1)
> +  if ((mType & NS_HANDLER_TYPE_PREVENTDEFAULT) ||
> +      !(mType & NS_HANDLER_TYPE_XBL_COMMAND)) {
> +    // This handler specifies preventDefault or isn't an action
> +    // so we need to dispatch to content

I don't understand why we need to check mType & NS_HANDLER_TYPE_PREVENTDEFAULT and if it's true, it won't cause async handling. The flag means that when the command is performed, it should call preventDefault() of the event. (Although, this flag doesn't change anything actually, nsXBLWindowKeyHandler always call peventDefault(), IIRC.)

::: gfx/layers/apz/src/Keyboard.h:11
(Diff revision 1)
>  #ifndef mozilla_layers_Keyboard_h
>  #define mozilla_layers_Keyboard_h
>  
> +#include <stdint.h> // for int32_t, uint32_t
> +
> +#include "InputData.h"          // for KeyboardInput

Oh, I wonder, it might be better to name this KeyboardInput.h and move KeyboardInput to here? Then, we can avoid to use too generic name to the file.

::: gfx/layers/apz/src/Keyboard.h:57
(Diff revision 1)
> +  KeyboardShortcut(KeyboardInput::KeyboardEventType aEventType,
> +                   uint32_t aKeyCode,
> +                   uint32_t aCharCode,
> +                   Modifiers aModifiers,
> +                   Modifiers aModifiersMask,
> +                   KeyboardAction aAction);

Why isn't this |const KeyboardAction&|?

::: gfx/layers/apz/src/Keyboard.h:69
(Diff revision 1)
> +  bool Matches(const KeyboardInput& aInput,
> +               const IgnoreModifierState& aIgnore,
> +               uint32_t aOverrideCharCode = 0) const;
> +
> +private:
> +  bool MatchesKey(const KeyboardInput& aInput, uint32_t aOverrideCharCode) const;

Too long if the line should be 80 characters or less. If it's 120 characters, no problem.

::: gfx/layers/apz/src/Keyboard.h:70
(Diff revision 1)
> +               const IgnoreModifierState& aIgnore,
> +               uint32_t aOverrideCharCode = 0) const;
> +
> +private:
> +  bool MatchesKey(const KeyboardInput& aInput, uint32_t aOverrideCharCode) const;
> +  bool MatchesModifiers(const KeyboardInput& aInput, const IgnoreModifierState& aIgnore) const;

ditto.

::: gfx/layers/apz/src/Keyboard.h:73
(Diff revision 1)
> +  KeyboardInput::KeyboardEventType mEventType;
> +  // Only one of mKeyCode or mCharCode may be non-zero, whichever one is non-zero is the one to compare when matching
> +  uint32_t mKeyCode;
> +  uint32_t mCharCode;
> +  // The modifiers that must be active for this shortcut
> +  Modifiers mModifiers;
> +  // The modifiers to compare when matching this shortcut
> +  Modifiers mModifiersMask;
> +
> +  // The action to perform when this shortcut is matched: either a command or a dispatch-to-content
> +  bool mDispatchToContent;
> +  KeyboardAction mAction;

KeyboardAction mAction; shoudl be first member of this class. And then, uint32_t mKeyCode and mCharCode.

Then, if mEventType is samller than uint16_t (the size of Modifiers), next is Modifiers mModifiers and Modifiers mModifiersMask. And then, mEventType.

Finally, bool mDispatchToContent.

Then, the instance size will be minimized.

::: gfx/layers/apz/src/Keyboard.h:74
(Diff revision 1)
> +  bool MatchesKey(const KeyboardInput& aInput, uint32_t aOverrideCharCode) const;
> +  bool MatchesModifiers(const KeyboardInput& aInput, const IgnoreModifierState& aIgnore) const;
> +
> +public:
> +  KeyboardInput::KeyboardEventType mEventType;
> +  // Only one of mKeyCode or mCharCode may be non-zero, whichever one is non-zero is the one to compare when matching

ditto.

::: gfx/layers/apz/src/Keyboard.h:82
(Diff revision 1)
> +  // The modifiers that must be active for this shortcut
> +  Modifiers mModifiers;
> +  // The modifiers to compare when matching this shortcut
> +  Modifiers mModifiersMask;
> +
> +  // The action to perform when this shortcut is matched: either a command or a dispatch-to-content

ditto.

::: gfx/layers/apz/src/Keyboard.h:83
(Diff revision 1)
> +  Modifiers mModifiers;
> +  // The modifiers to compare when matching this shortcut
> +  Modifiers mModifiersMask;
> +
> +  // The action to perform when this shortcut is matched: either a command or a dispatch-to-content
> +  bool mDispatchToContent;

bool members should be last of class/struct definition.

::: gfx/layers/apz/src/Keyboard.cpp:11
(Diff revision 1)
> +struct IgnoreModifierState
> +{
> +  // When mShift is true, Shift key state will be ignored.
> +  bool mShift;
> +  // When mOS is true, OS key state will be ignored.
> +  bool mOS;
> +
> +  IgnoreModifierState()
> +    : mShift(false)
> +    , mOS(false)
> +  {
> +  }
> +};

Hmm, this is same as mozilla::dom::IgnoreModifierState. How about move it to widget/TextEvents.h? (I think mozilla::IgnoreModifierState is okay. dom namespace isn't necessary for this.)

::: gfx/layers/apz/src/Keyboard.cpp:57
(Diff revision 1)
> +}
> +KeyboardShortcut::KeyboardShortcut(KeyboardInput::KeyboardEventType aEventType,

Put empty line between them.

::: gfx/layers/apz/src/Keyboard.cpp:72
(Diff revision 1)
> +}
> +KeyboardShortcut::KeyboardShortcut(KeyboardInput::KeyboardEventType aEventType,

Put empty line between them.

::: gfx/layers/apz/src/Keyboard.cpp:88
(Diff revision 1)
> +  , mDispatchToContent(true)
> +{
> +}
> +
> +bool
> +KeyboardShortcut::Matches(const KeyboardInput& aInput, const IgnoreModifierState& aIgnore, uint32_t mOverrideCharCode) const

Even if this is allowed 120 characters per line, this is too long.

::: gfx/layers/apz/src/Keyboard.cpp:96
(Diff revision 1)
> +         MatchesKey(aInput, mOverrideCharCode) &&
> +         MatchesModifiers(aInput, aIgnore);
> +}
> +
> +bool
> +KeyboardShortcut::MatchesKey(const KeyboardInput& aInput, uint32_t aOverrideCharCode) const

Over 80 characters, but less than 120 characters.

::: gfx/layers/apz/src/Keyboard.cpp:100
(Diff revision 1)
> +bool
> +KeyboardShortcut::MatchesKey(const KeyboardInput& aInput, uint32_t aOverrideCharCode) const
> +{
> +  // We need to either compare the char code or the key code,
> +  // depending on which one is non-zero
> +  if (mCharCode != 0) {

Could you use early return style here? if |!mCharCode|, this does nothing actually.

::: gfx/layers/apz/src/Keyboard.cpp:113
(Diff revision 1)
> +      charCode = aInput.mCharCode;
> +    }
> +
> +    // Both char codes must be in lowercase to compare correctly
> +    if (IS_IN_BMP(charCode)) {
> +      charCode = ToLowerCase(char16_t(charCode));

please use static_cast.

::: gfx/layers/apz/src/Keyboard.cpp:123
(Diff revision 1)
> +
> +  return mKeyCode == aInput.mKeyCode;
> +}
> +
> +bool
> +KeyboardShortcut::MatchesModifiers(const KeyboardInput& aInput, const IgnoreModifierState& aIgnore) const

Over 80 characters but less than 120 characters.

Comment 38

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8875161 [details]
Bug 1351783 part 5 - Add a KeyboardMap type.

https://reviewboard.mozilla.org/r/146552/#review151124

I wonder, cannot use the new class in nsXBLWindowHandler? If so, we don't need to have duplicated code like KeyboardMap::FindMatch vs. nsXBLWindowKeyHandler::WalkHandlersAndExecute.

::: dom/xbl/nsXBLWindowKeyHandler.cpp:159
(Diff revision 1)
> +/* static */ void
> +nsXBLWindowKeyHandler::EnsureSpecialDocInfo()
> +{
> +  if (!sXBLSpecialDocInfo) {
> +    sXBLSpecialDocInfo = new nsXBLSpecialDocInfo();
> +    NS_ADDREF(sXBLSpecialDocInfo);

You should make sXBLSpecialDocInfo a StaticRefPtr.

::: dom/xbl/nsXBLWindowKeyHandler.cpp:411
(Diff revision 1)
>  }
>  
> +/* static */ mozilla::layers::KeyboardMap
> +nsXBLWindowKeyHandler::CollectKeyboardShortcuts()
> +{
> +  using namespace mozilla::layers;

I think it's no problem to move this into the global scope of this file.

::: dom/xbl/nsXBLWindowKeyHandler.cpp:418
(Diff revision 1)
> +  // Load the XBL handlers
> +  EnsureSpecialDocInfo();
> +
> +  nsXBLPrototypeHandler* handlers = nullptr;
> +  nsXBLPrototypeHandler* userHandlers = nullptr;
> +  sXBLSpecialDocInfo->GetAllHandlers("browser", &handlers, &userHandlers);

So, this isn't available when focused element is an editor, are you sure?  There are no callers of this method in this patch. So, I cannot check the callers...

::: dom/xbl/nsXBLWindowKeyHandler.cpp:421
(Diff revision 1)
> +  nsXBLPrototypeHandler* handlers = nullptr;
> +  nsXBLPrototypeHandler* userHandlers = nullptr;
> +  sXBLSpecialDocInfo->GetAllHandlers("browser", &handlers, &userHandlers);
> +
> +  // Convert the handlers into keyboard shortcuts
> +  nsTArray<KeyboardShortcut> shortcuts;

Use AutoTArray and investigate enough size with current build. Then, copy constructor of nsTArray will append only valid elements once. So, don't need to worry about the final size but we can omit a lot of allocations.

::: gfx/layers/apz/src/Keyboard.h:13
(Diff revision 1)
> +#include "nsTArray.h"           // for nsTArray
>  #include "nsIScrollableFrame.h" // for nsIScrollableFrame

Move nsTArray.h after nsIScrollableFrame.h (for sorting AtoZ).

::: gfx/layers/apz/src/Keyboard.cpp:153
(Diff revision 1)
> +}
> +
> +Maybe<KeyboardShortcut>
> +KeyboardMap::FindMatch(const KeyboardInput& aEvent) const
> +{
> +  // If there are no shortcut candidates, then just search with with the keyboard input

Over 80 characters, but less than 120 characters.

::: gfx/layers/apz/src/Keyboard.cpp:172
(Diff revision 1)
> +  }
> +  return Nothing();
> +}
> +
> +Maybe<KeyboardShortcut>
> +KeyboardMap::FindMatchInternal(const KeyboardInput& aEvent, const IgnoreModifierState& aIgnore, uint32_t aOverrideCharCode) const

Over 120 characters!

::: gfx/layers/apz/src/Keyboard.cpp:180
(Diff revision 1)
> +  // Windows native applications ignore Windows-Logo key state when checking
> +  // shortcut keys even if the key is pressed.  Therefore, if there is no
> +  // shortcut key which exactly matches current modifier state, we should
> +  // retry to look for a shortcut key without the Windows-Logo key press.
> +  if (!aIgnore.mOS && (aEvent.modifiers & MODIFIER_OS)) {
> +    IgnoreModifierState ignoreModifierState(aIgnore);
> +    ignoreModifierState.mOS = true;
> +    return FindMatchInternal(aEvent, ignoreModifierState, aOverrideCharCode);
> +  }

This block needs to be wrapped with #ifdef XP_WIN.
https://searchfox.org/mozilla-central/rev/1a054419976437d0778a2b89be1b00207a744e94/dom/xbl/nsXBLWindowKeyHandler.cpp#746,757

Comment 39

[Ryan Hunt [:rhunt]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #28)
> Comment on attachment 8875158 [details]
> Bug 1351783 part 2 - Add a KeyboardInput type
> 
> https://reviewboard.mozilla.org/r/146546/#review150890
> 
> ::: gfx/ipc/GfxMessageUtils.h:1086
> (Diff revision 1)
> >    }
> >  };
> >  
> >  template <>
> > +struct ParamTraits<mozilla::KeyboardInput::KeyboardEventType>
> > +  : public ContiguousEnumSerializer<
> 
> This should probably also go in nsGUIEventIPC.h along with the KeyboardInput
> type.

I think it should too. IIRC I ran a problem with IPDL and includes, but I'll try and fix it.

> ::: widget/InputData.cpp:811
> (Diff revision 1)
> > +    case eKeyDown: {
> > +      mType = KeyboardInput::KEY_DOWN;
> > +      break;
> > +    }
> > +    default:
> > +      MOZ_ASSERT_UNREACHABLE("Did not assign a type to a KeyboardInput");
> 
> Do we want to assert here? There appear to be other message types like
> eKeyDownOnPlugin and such that would fall into this assertion and crash
> debug builds. Maybe we should just have a generic "KeyboardInput::OTHER"
> type to handle all the remaining messages?

Most likely not. That sounds like a reasonable fix.

Comment 40

[Ryan Hunt [:rhunt]]

(In reply to Olli Pettay [:smaug] from comment #29)
> Comment on attachment 8875165 [details]
> Bug 1351783 part 9 - Disable a FocusTarget for an editable element
> 
> https://reviewboard.mozilla.org/r/146560/#review150900
> 
> ::: gfx/layers/apz/src/FocusTarget.cpp:67
> (Diff revision 1)
> >    }
> >    return false;
> >  }
> >  
> > +static bool
> > +IsEditableElement(nsIContent* aContent)
> 
> This could be IsEditableNode(nsINode* aNode)
> 
> ::: gfx/layers/apz/src/FocusTarget.cpp:76
> (Diff revision 1)
> > +          aContent->IsHTMLElement(nsGkAtoms::input) ||
> > +          aContent->IsHTMLElement(nsGkAtoms::textarea) ||
> > +          aContent->IsXULElement(nsGkAtoms::editor));
> > +}
> > +
> > +static bool
> 
> Then this wouldn't be needed
> 
> ::: gfx/layers/apz/src/FocusTarget.cpp:147
> (Diff revision 1)
> >      mType = FocusTarget::NONE;
> >      return;
> >    }
> >  
> > +  // If the focus isn't on a remote browser then check for scrollable targets
> > +  if (IsEditableElement(focusTarget) ||
> 
> And this could be then I guess just
> if (IsEditableNode(focusTarget ? focusTarget : aDocument))

Okay, that will simplify things.

Comment 41

[Ryan Hunt [:rhunt]]

(In reply to Olli Pettay [:smaug] from comment #32)
> Comment on attachment 8875164 [details]
> Bug 1351783 part 8 - Gather whether there are key or scroll listeners on the
> focused element
> 
> https://reviewboard.mozilla.org/r/146558/#review150916
> 
> ::: dom/events/EventTarget.cpp:68
> (Diff revision 1)
> > +  EventListenerManager* elm = GetExistingListenerManager();
> > +  return elm && elm->HasKeyEventListeners();
> > +}
> > +
> > +bool
> > +EventTarget::HasScrollEventListeners() const
> 
> I don't know why we need to know about scroll event listeners.
> I feel like I'm missing something.
> scroll event listeners are just callbacks running around the same time as
> rAF callbacks. Why we care about scroll listeners but not rAF?
> Scroll events are dispatched _after_ something has been scrolled.
> 
> I feel like I'm missing something here.
> 

From our discussion in #developers we agree that scroll listeners should be ignored. I will update with this change.

Comment 42

[Ryan Hunt [:rhunt]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #35)
> Comment on attachment 8875159 [details]
> Bug 1351783 part 3 - Add a KeyboardAction type
> 
> https://reviewboard.mozilla.org/r/146548/#review151104
> 
> ::: dom/base/nsGlobalWindowCommands.h:31
> (Diff revision 1)
> > +class nsGlobalWindowCommands
> > +{
> > +public:
> > +  typedef mozilla::layers::KeyboardAction KeyboardAction;
> > +
> > +  static bool FindScrollCommand(const char* aCommandName, KeyboardAction* aOutAction);
> 
> nit: too long line. Please break after |aCommandName,|.

Okay. My apologies for the many times this happened in these patches.

> ::: dom/base/nsGlobalWindowCommands.cpp:1290
> (Diff revision 1)
> > +  for (size_t i = 0; i < ArrayLength(physicalBrowseCommands); i++) {
> > +    const PhysicalBrowseCommand& cmd = physicalBrowseCommands[i];
> > +    if (!strcmp(aCommandName, cmd.command)) {
> > +      int16_t dir = cmd.direction;
> > +      bool forward = (dir == nsISelectionController::MOVE_RIGHT ||
> > +                      dir == nsISelectionController::MOVE_DOWN);
> > +
> > +      *aOutAction = KeyboardAction(cmd.scrollAction, forward);
> > +      return true;
> > +    }
> > +  }
> > +
> > +  for (size_t i = 0; i < ArrayLength(browseCommands); i++) {
> > +    const BrowseCommand& cmd = browseCommands[i];
> > +    bool forward = !strcmp(aCommandName, cmd.forward);
> > +    bool reverse = !strcmp(aCommandName, cmd.reverse);
> > +    if (forward || reverse) {
> > +      *aOutAction = KeyboardAction(cmd.scrollAction, forward);
> > +      return true;
> > +    }
> > +  }
> 
> Could you explain the reason why physicalBrowseCommands is preferred than
> browseCommands with comment in this method?

My understanding is that physicalBrowserCommands and browseCommands are disjoint, so there should be no preference because each command name is unique?

> ::: gfx/layers/apz/src/Keyboard.h:18
> (Diff revision 1)
> > +
> > +
> > +/**
> > + * This class represents a scrolling action to be performed on a scrollable layer.
> > + */
> > +class KeyboardAction final
> 
> All of these members are public. So, why don't you use struct here? But up
> to you.

I originally intended for the members of this class to be private, but for IPC serialization purposes that was difficult, so they were made public and I left it as a class. I will look into a better solution.

> ::: gfx/layers/apz/src/Keyboard.h:28
> (Diff revision 1)
> > +    SCROLL_LINE,
> > +    SCROLL_PAGE,
> > +    COMPLETE_SCROLL,
> > +
> > +    // Used as an upper bound for ContiguousEnumSerializer
> > +    SENTINEL,
> 
> Is "sentinel" usually used for this purpose? If so, it's okay, but
> otherwise, perhaps, NUMBER_OF_KEYBOARD_ACTION_TYPE or
> KEYBOARD_ACTION_TYPE_COUNT is clearer to me.

Sentinel is used for this purpose. I'm not sure if the IPDL code is in this patch or another. I used SENTINEL in this case because that's what is often used for enums being serialized over IPDL.

> enum KeyboardActionType : uint8_t
> {
> 
> ::: gfx/layers/apz/src/Keyboard.cpp:21
> (Diff revision 1)
> > +    case KeyboardAction::COMPLETE_SCROLL:
> > +      return nsIScrollableFrame::WHOLE;
> 
> Sigh... nsISelectionController uses the word "complete"? Should be replaced
> with "whole"...

I'm not sure if I understand, do you want me to update my patch and replace COMPLETE with WHOLE?

> ::: gfx/layers/moz.build:102
> (Diff revision 1)
> >      # proper interface for the code there
> >      'apz/src/APZCTreeManager.h',
> >      'apz/src/APZUtils.h',
> >      'apz/src/AsyncDragMetrics.h',
> >      'apz/src/AsyncPanZoomAnimation.h',
> > +    'apz/src/Keyboard.h',
> 
> Keyboard.h and Keyboard.cpp are not good names. Perhaps, KeyboardAction.h
> and KeyboardAction.cpp?

I agree, it's too generic. I'll try and think of a better name.

Comment 43

[Ryan Hunt [:rhunt]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #37)
> Comment on attachment 8875160 [details]

> ::: dom/xbl/nsXBLPrototypeHandler.h:30
> (Diff revision 1)
> >  namespace dom {
> >  class AutoJSAPI;
> >  class EventTarget;
> >  } // namespace dom
> >  } // namespace mozilla
> >  
> > +namespace mozilla {
> > +namespace layers {
> > +class KeyboardShortcut;
> > +} // namespace layers
> > +} // namespace mozilla
> 
> Why don't you add |layers| into above |mozilla| namespace?
> 

Not sure why I did that. I will do that.

> ::: dom/xbl/nsXBLPrototypeHandler.cpp:163
> (Diff revision 1)
> > +  if (mKeyMask & cMetaMask) {
> > +    modifiersMask |= MODIFIER_META;
> > +    if (mKeyMask & cMeta) {
> > +      modifiers |= MODIFIER_META;
> > +    }
> > +  }
> > +  if (mKeyMask & cOSMask) {
> > +    modifiersMask |= MODIFIER_OS;
> > +    if (mKeyMask & cOS) {
> > +      modifiers |= MODIFIER_OS;
> > +    }
> > +  }
> > +  if (mKeyMask & cShiftMask) {
> > +    modifiersMask |= MODIFIER_SHIFT;
> > +    if (mKeyMask & cShift) {
> > +      modifiers |= MODIFIER_SHIFT;
> > +    }
> > +  }
> > +  if (mKeyMask & cAltMask) {
> > +    modifiersMask |= MODIFIER_ALT;
> > +    if (mKeyMask & cAlt) {
> > +      modifiers |= MODIFIER_ALT;
> > +    }
> > +  }
> > +  if (mKeyMask & cControlMask) {
> > +    modifiersMask |= MODIFIER_CONTROL;
> > +    if (mKeyMask & cControl) {
> > +      modifiers |= MODIFIER_CONTROL;
> > +    }
> > +  }
> 
> Please move this into two methods, one is for modifiers and the other is for
> modifiersMask. Perhaps, nsXBLPrototypeHandler::GetModifiers() and
> nsXbLPrototypeHandler::GetModifiersMask()?
> 

That will be cleaner. I will try and do that.

> ::: dom/xbl/nsXBLPrototypeHandler.cpp:206
> (Diff revision 1)
> > +  if ((mType & NS_HANDLER_TYPE_PREVENTDEFAULT) ||
> > +      !(mType & NS_HANDLER_TYPE_XBL_COMMAND)) {
> > +    // This handler specifies preventDefault or isn't an action
> > +    // so we need to dispatch to content
> 
> I don't understand why we need to check mType &
> NS_HANDLER_TYPE_PREVENTDEFAULT and if it's true, it won't cause async
> handling. The flag means that when the command is performed, it should call
> preventDefault() of the event. (Although, this flag doesn't change anything
> actually, nsXBLWindowKeyHandler always call peventDefault(), IIRC.)

You're right, I had some reasoning why this is needed but it's invalid.

> ::: gfx/layers/apz/src/Keyboard.h:11
> (Diff revision 1)
> >  #ifndef mozilla_layers_Keyboard_h
> >  #define mozilla_layers_Keyboard_h
> >  
> > +#include <stdint.h> // for int32_t, uint32_t
> > +
> > +#include "InputData.h"          // for KeyboardInput
> 
> Oh, I wonder, it might be better to name this KeyboardInput.h and move
> KeyboardInput to here? Then, we can avoid to use too generic name to the
> file.

I think we'd like to keep KeyboardInput in InputData.h as that's where all the other InputData are. I'll try an think of a better name for Keyboard.h though.

> ::: gfx/layers/apz/src/Keyboard.h:57
> (Diff revision 1)
> > +  KeyboardShortcut(KeyboardInput::KeyboardEventType aEventType,
> > +                   uint32_t aKeyCode,
> > +                   uint32_t aCharCode,
> > +                   Modifiers aModifiers,
> > +                   Modifiers aModifiersMask,
> > +                   KeyboardAction aAction);
> 
> Why isn't this |const KeyboardAction&|?

No reason, I thought the size of the class would make it okay to pass on the stack. But it suppose it is a class, and that would be better style.

> ::: gfx/layers/apz/src/Keyboard.h:73
> (Diff revision 1)
> > +  KeyboardInput::KeyboardEventType mEventType;
> > +  // Only one of mKeyCode or mCharCode may be non-zero, whichever one is non-zero is the one to compare when matching
> > +  uint32_t mKeyCode;
> > +  uint32_t mCharCode;
> > +  // The modifiers that must be active for this shortcut
> > +  Modifiers mModifiers;
> > +  // The modifiers to compare when matching this shortcut
> > +  Modifiers mModifiersMask;
> > +
> > +  // The action to perform when this shortcut is matched: either a command or a dispatch-to-content
> > +  bool mDispatchToContent;
> > +  KeyboardAction mAction;
> 
> KeyboardAction mAction; shoudl be first member of this class. And then,
> uint32_t mKeyCode and mCharCode.
> 
> Then, if mEventType is samller than uint16_t (the size of Modifiers), next
> is Modifiers mModifiers and Modifiers mModifiersMask. And then, mEventType.
> 
> Finally, bool mDispatchToContent.
> 
> Then, the instance size will be minimized.

Okay, thank you! That will help.

> ::: gfx/layers/apz/src/Keyboard.cpp:11
> (Diff revision 1)
> > +struct IgnoreModifierState
> > +{
> > +  // When mShift is true, Shift key state will be ignored.
> > +  bool mShift;
> > +  // When mOS is true, OS key state will be ignored.
> > +  bool mOS;
> > +
> > +  IgnoreModifierState()
> > +    : mShift(false)
> > +    , mOS(false)
> > +  {
> > +  }
> > +};
> 
> Hmm, this is same as mozilla::dom::IgnoreModifierState. How about move it to
> widget/TextEvents.h? (I think mozilla::IgnoreModifierState is okay. dom
> namespace isn't necessary for this.)

Okay. I was attempting to keep my changes minimal, but it's best to remove duplication.

> ::: gfx/layers/apz/src/Keyboard.cpp:100
> (Diff revision 1)
> > +bool
> > +KeyboardShortcut::MatchesKey(const KeyboardInput& aInput, uint32_t aOverrideCharCode) const
> > +{
> > +  // We need to either compare the char code or the key code,
> > +  // depending on which one is non-zero
> > +  if (mCharCode != 0) {
> 
> Could you use early return style here? if |!mCharCode|, this does nothing
> actually.

Hmm, we should be doing an early return in this function a few lines down:

  if (mCharCode != 0) {
    ...
    return mCharCode == charCode;
  }
  ...
  return mKeyCode == aInput.mKeyCode

Or do you mean we should do nothing when !mCharCode? I intended to have either mCharCode or mKeyCode as non-null to signify which one is in this shortcut. So if !mCharCode we should compare mKeyCode.

Comment 44

[Ryan Hunt [:rhunt]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #38)
> Comment on attachment 8875161 [details]
> Bug 1351783 part 5 - Add a KeyboardMap type
> 
> https://reviewboard.mozilla.org/r/146552/#review151124
> 
> I wonder, cannot use the new class in nsXBLWindowHandler? If so, we don't
> need to have duplicated code like KeyboardMap::FindMatch vs.
> nsXBLWindowKeyHandler::WalkHandlersAndExecute.

Hmm. I thought about this. I don't know how well that would work because KeyboardMap, KeyboardShortcut, KeyboardAction are very slimmed down versions of XBL only for scrolling in the compositor. And it seems like we'd have to refactor XBL quite a bit to get that to work. I'd love to share code here for maintenance and bug sharing sake, but I'd need some guidance on how it's possible.

> ::: dom/xbl/nsXBLWindowKeyHandler.cpp:159
> (Diff revision 1)
> > +/* static */ void
> > +nsXBLWindowKeyHandler::EnsureSpecialDocInfo()
> > +{
> > +  if (!sXBLSpecialDocInfo) {
> > +    sXBLSpecialDocInfo = new nsXBLSpecialDocInfo();
> > +    NS_ADDREF(sXBLSpecialDocInfo);
> 
> You should make sXBLSpecialDocInfo a StaticRefPtr.

Okay, I can look into adding that change.

> ::: dom/xbl/nsXBLWindowKeyHandler.cpp:418
> (Diff revision 1)
> > +  // Load the XBL handlers
> > +  EnsureSpecialDocInfo();
> > +
> > +  nsXBLPrototypeHandler* handlers = nullptr;
> > +  nsXBLPrototypeHandler* userHandlers = nullptr;
> > +  sXBLSpecialDocInfo->GetAllHandlers("browser", &handlers, &userHandlers);
> 
> So, this isn't available when focused element is an editor, are you sure? 
> There are no callers of this method in this patch. So, I cannot check the
> callers...

Yes, we don't want to async scroll editors because we don't know how to move the caret around in the compositor. In a later patch we do work to ensure that when the focus is an editor we don't async scroll, so it should be okay if we only have the handlers for "browser".

> ::: dom/xbl/nsXBLWindowKeyHandler.cpp:421
> (Diff revision 1)
> > +  nsXBLPrototypeHandler* handlers = nullptr;
> > +  nsXBLPrototypeHandler* userHandlers = nullptr;
> > +  sXBLSpecialDocInfo->GetAllHandlers("browser", &handlers, &userHandlers);
> > +
> > +  // Convert the handlers into keyboard shortcuts
> > +  nsTArray<KeyboardShortcut> shortcuts;
> 
> Use AutoTArray and investigate enough size with current build. Then, copy
> constructor of nsTArray will append only valid elements once. So, don't need
> to worry about the final size but we can omit a lot of allocations.

Yes that's a good point. I will do that.

> ::: gfx/layers/apz/src/Keyboard.cpp:180
> (Diff revision 1)
> > +  // Windows native applications ignore Windows-Logo key state when checking
> > +  // shortcut keys even if the key is pressed.  Therefore, if there is no
> > +  // shortcut key which exactly matches current modifier state, we should
> > +  // retry to look for a shortcut key without the Windows-Logo key press.
> > +  if (!aIgnore.mOS && (aEvent.modifiers & MODIFIER_OS)) {
> > +    IgnoreModifierState ignoreModifierState(aIgnore);
> > +    ignoreModifierState.mOS = true;
> > +    return FindMatchInternal(aEvent, ignoreModifierState, aOverrideCharCode);
> > +  }
> 
> This block needs to be wrapped with #ifdef XP_WIN.
> https://searchfox.org/mozilla-central/rev/
> 1a054419976437d0778a2b89be1b00207a744e94/dom/xbl/nsXBLWindowKeyHandler.
> cpp#746,757

Oh yes, I forgot that.

Comment 45

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8875163 [details]
Bug 1351783 part 7 - Create FocusState and FocusTarget types.

https://reviewboard.mozilla.org/r/146556/#review151842

::: commit-message-863b7:13
(Diff revision 1)
> +
> +See the comment in `FocusState.h` for more details on the architecture and things
> +needed in future patches to complete this.
> +
> +Note, FocusState contains an internal set of focus targets it has received indexed
> +by their layer tree ID. This set only ever grows and that might not be optimal, it

We should be able to clear out stale entries as part of the UpdateHitTestingTree walk that we do in APZCTreeManager. I'll have to look at future patches to see more about how/when UpdateFocusState is invoked before I can suggest something more concrete, but I'll flag this as an issue for now. If I finish reviewing the patchset and forget to suggest something, remind me :)

::: gfx/layers/apz/src/FocusState.h:12
(Diff revision 1)
> +#define mozilla_layers_FocusState_h
> +
> +#include <unordered_map>    // for std::unordered_map
> +
> +#include "FrameMetrics.h"   // for FrameMetrics::ViewID
> +#include "InputData.h"      // for InputData

Doesn't seem like you need this include

::: gfx/layers/apz/src/FocusState.h:50
(Diff revision 1)
> + * others. This makes it impossible to always have the current focus information in
> + * APZ as it can be changed asynchronously at any moment. If we don't have the latest
> + * focus information, we may incorrectly scroll a target when we shouldn't.
> + *
> + * A tradeoff is designed here whereby we will maintain deterministic focus changes
> + * for event listeners, but not for other javascript code. The reasoning here is

I think it would be more correct to say "user input" rather than "event listeners" because "event listeners" is a very generic thing in the Web API, and includes things like XMLHttpRequest state change listeners which can be triggered by network activity.

::: gfx/layers/apz/src/FocusState.h:54
(Diff revision 1)
> + * To maintain deterministic focus changes for event listeners we invalidate our
> + * focus state in response to any event that may trigger an event listener. We

"To maintain deterministic focus changes for a given stream of user inputs, we invalidate our focus state whenever we receive a user input that may trigger event listeners"

::: gfx/layers/apz/src/FocusState.h:61
(Diff revision 1)
> + * Once we have received the latest focus sequence number from content, we know that
> + * all event listeners and focus changes have been processed and so we have a current
> + * target that we can use again.

"... we know that all event listeners triggered by user inputs, and their resulting focus changes, have been processed ..."

::: gfx/layers/apz/src/FocusState.cpp:45
(Diff revision 1)
> +        mFocusLayersId = target.mData.mRefLayerId;
> +        break;

Before this assignment, it's probably good to add this:

// guard against infinite loops
MOZ_ASSERT(mFocusLayersId != target.mData.mRefLayerId);
if (mFocusLayersId == target.mData.mRefLayerId) {
  return;
}

We should only ever enter the infinite loop case if there is a bug in the code, so this seems like appropriate handling for debug/release.

::: gfx/layers/apz/src/FocusTarget.h:20
(Diff revision 1)
> + * This class is used for communicating information about the currently focused
> + * element of a document and the scrollable frames to use when keyboard scrolling it.
> + */

Add something like "This class is created on the main thread at paint-time, but is then passed over IPC to the compositor/APZ code."

::: gfx/layers/apz/src/FocusTarget.h:42
(Diff revision 1)
> +    SENTINEL,
> +  };
> +  union FocusTargetData
> +  {
> +    uint64_t             mRefLayerId;
> +    FrameMetrics::ViewID mScrollTargets[2];

Looking at the way this is used in the patch it doesn't really seem like it's worth making it an array. Instead I'd prefer if you dropped the `enum Direction` and instead had a `struct ScrollTargets` with two fields, mHorizontalTarget and mVerticalTarget, and then used that struct inside the union.

Optionally, you could further look into using mozilla::Variant (see mfbt/Variant.h) as a tagged union type although I don't know if that's IPC-friendly or even if it will even result in more legible code (templates often don't).

::: gfx/layers/apz/src/FocusTarget.cpp:20
(Diff revision 1)
> +
> +namespace mozilla {
> +namespace layers {
> +
> +static FrameMetrics::ViewID
> +FindIDForScrollableFrame(nsIScrollableFrame* aScrollable)

Move this function into nsLayoutUtils, since it's basically the inverse of nsLayoutUtils::FindScrollableFrameFor(ViewID)

::: gfx/layers/apz/src/FocusTarget.cpp:42
(Diff revision 1)
> +FocusTarget::FocusTarget(nsIDocument* aDocument,
> +                         nsIScrollableFrame* aScrollHorizontalTarget,
> +                         nsIScrollableFrame* aScrollVerticalTarget)

Add a main-thread assertion at the start of this function, since this stuff can only run on the main thread.

Comment 46

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8875164 [details]
Bug 1351783 part 8 - Gather whether there are key listeners on the focused element.

https://reviewboard.mozilla.org/r/146558/#review151866

I don't have much to add here over what smaug said. I agree that we shouldn't treat scroll events as special here, just the key events.

Comment 47

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8875166 [details]
Bug 1351783 part 10 - Create and sync the current FocusTarget on each layer transaction.

https://reviewboard.mozilla.org/r/146562/#review151868

This patch looks fine, but please also add the plumbing for the WebRender codepath. So WebRenderLayerManager would keep a copy of the FocusTarget, and then in WebRenderLayerManager::EndTransactionInternal it would put it into the WebRenderScrollData object that it creates and ships over IPC. You can basically copy/paste what happens with the mPaintSequenceNumber. On the parent side the WebRenderBridgeParent::UpdateAPZ function will need to hand the FocusTarget to the APZCTreeManager. It should be fairly straightforward - as long as it compiles you should be good to go.

::: gfx/ipc/GfxMessageUtils.h:25
(Diff revision 1)
>  #include "ipc/IPCMessageUtils.h"
>  #include "mozilla/gfx/Matrix.h"
>  #include "mozilla/layers/AsyncDragMetrics.h"
>  #include "mozilla/layers/CompositorOptions.h"
>  #include "mozilla/layers/CompositorTypes.h"
> +#include "mozilla/layers/FocusState.h"

Shouldn't need to include FocusState here, right? It's only FocusTarget that goes over IPC

Comment 48

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8875166 [details]
Bug 1351783 part 10 - Create and sync the current FocusTarget on each layer transaction.

https://reviewboard.mozilla.org/r/146562/#review151886

::: gfx/layers/ipc/CompositorBridgeParent.cpp:1223
(Diff revision 1)
>      mApzcTreeManager->UpdateHitTestingTree(
>        mRootLayerTreeID, root, aInfo.isFirstPaint(),
>        mRootLayerTreeID, aInfo.paintSequenceNumber());

Ok, so in terms of clearing out stale FocusState entries from the APZCTreeManager, what you can do is this (don't need to do this exactly, but you can use it as a guide on the general strategy):

- In APZCTreeManager::TreeBuildingState, add a `std::unordered_set<uint64_t> mLayersIdsToDestroy` alongside the existing mNodesToDestroy item.
- When the TreeBuildingState object is created in UpdateHitTestingTreeImpl, copy all the keys from mFocusState.mFocusTree into that mLayersIdsToDestroy.
- Around where we do the call to PrepareNodeForLayer, remove the `layersId` from state.mLayersIdsToDestroy, because we know that layers id is still "active" in that there are APZCs for it in the tree.
- Near the end of UpdateHitTestingTreeImpl where we destroy the APZC nodes left in mNodesToDestroy, we can also clear out any entries in the FocusState that correspond to the remaining IDs in mLayersIdsToDestroy.

Comment 49

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8875167 [details]
Bug 1351783 part 12 - Create and sync focus sequence numbers.

https://reviewboard.mozilla.org/r/146564/#review152030

Overall seems pretty good. Some comments below.

::: gfx/layers/apz/public/IAPZCTreeManager.h:206
(Diff revision 1)
>  
>  protected:
>  
>    // Methods to help process WidgetInputEvents (or manage conversion to/from InputData)
>  
>    virtual void TransformEventRefPoint(

I think we should rename this function as well, to something like ProcessUnhandledEvent or the more awkward-sounding HandleUnhandledEvent. Or maybe HandleIgnoredEvent? Naming is hard

::: gfx/layers/apz/src/FocusState.h:13
(Diff revision 1)
>  
>  #include <unordered_map>    // for std::unordered_map
>  
>  #include "FrameMetrics.h"   // for FrameMetrics::ViewID
>  #include "InputData.h"      // for InputData
> +#include "mozilla/Maybe.h"  // for mozilla::Maybe

Unused include?

::: gfx/layers/apz/src/FocusState.h:85
(Diff revision 1)
> +
> +  /**
> +   * Whether the current focus state is known to be current or else if an event has been processed
> +   * that could change the focus but we have not received an update with a new confirmed target.
> +   */
> +  bool IsCurrent() const { return mLastContentProcessedEvent >= mLastAPZProcessedEvent; }

We should never get into a situation where mLastContentProcessedEvent is > mLastAPZProcessedEvent, right? It might be worth moving this function into the .cpp and adding a MOZ_ASSERT to that effect.

::: gfx/layers/apz/src/FocusState.h:90
(Diff revision 1)
> +  bool IsCurrent() const { return mLastContentProcessedEvent >= mLastAPZProcessedEvent; }
> +
> +  /**
> +   * Notify focus state that a potentially focus changing event has been processed by APZ.
> +   */
> +  void NotifyUnhandledEvent() { mLastAPZProcessedEvent += 1; }

Move this function implementation into the .cpp file. This seems like a useful function to add a log or a breakpoint and it's harder to do if it's in a header or getting inlined into places.

::: gfx/layers/apz/src/FocusState.h:97
(Diff revision 1)
> +  /**
> +   * Notify focus state of a potentially focus changing event.
> +   *
> +   * @param aEvent the event that could cause a focus change
> +   */
> +  void ReceiveUnhandledEvent(InputData& aEvent);

From an APZ-centric point of view (as opposed to keyboard-centric) I don't think this function should have "Unhandled" in the name, because the things we're passing to this function are actually handled by APZ code. I'd prefer calling it "HandleNonKeyboardInput" or something like that.

::: layout/base/nsIPresShell.h:1903
(Diff revision 1)
> +  // The focus sequence number of the last processed input event
> +  uint64_t                             mAPZFocusSequenceNumber;

I have a concern here because there could be multiple presShells that map to a single layer manager. For example if you have a page with iframes, each iframe has its own presShell. And I *believe* that when delivering events to content, the presShell that is most-focused or under the cursor (for mouse events) will handle the event. So you might end up in a state where one presShell has a more up-to-date sequence number but then when we send the sequence number to the compositor we're reading it from the topmost presShell and it's a stale value.

However I'm not totally sure about this, I think we'll need to check with smaug about whether or not the place you're updating mAPZFocusSequenceNumber will always get hit by the events, even if they are directed to some iframe or other sub-presShell.

Comment 50

[Botond Ballo [:botond]]

Comment on attachment 8875163 [details]
Bug 1351783 part 7 - Create FocusState and FocusTarget types.

https://reviewboard.mozilla.org/r/146556/#review152064

r+ with comments addressed.

::: gfx/layers/apz/src/FocusState.h:55
(Diff revision 1)
> + * for event listeners, but not for other javascript code. The reasoning here is
> + * that `setTimeout` and others are already non-deterministic and so it might not
> + * be as breaking to web content.
> + *
> + * To maintain deterministic focus changes for event listeners we invalidate our
> + * focus state in response to any event that may trigger an event listener. We

What about user inputs for which the default handling may trigger a focus change, like a "Tab" key that can advance the focus to a different element, or the hotkey that gives focus to the address bar?

Or are those actions still implemented using event listeners in chrome JS?

::: gfx/layers/apz/src/FocusState.h:79
(Diff revision 1)
> +   * @param aRootLayerTreeId the layer tree ID of the root layer for the parent APZCTreeManager
> +   * @param aOriginatingLayersId the layer tree ID that this focus target belongs to
> +   */
> +  void Update(uint64_t aRootLayerTreeId,
> +              uint64_t aOriginatingLayersId,
> +              FocusTarget aTarget);

Pass by reference-to-const.

::: gfx/layers/apz/src/FocusTarget.h:27
(Diff revision 1)
> + */
> +class FocusTarget final
> +{
> +public:
> +  enum Direction {
> +    HORIZONTAL = 0,

I think we established that for new enumerations, we're going to follow the recommended naming style of camel case with a "e" prefix for enumerators, as in eHorizontal, eVertical, eRefLayer, etc.

::: gfx/layers/apz/src/FocusTarget.h:42
(Diff revision 1)
> +    SENTINEL,
> +  };
> +  union FocusTargetData
> +  {
> +    uint64_t             mRefLayerId;
> +    FrameMetrics::ViewID mScrollTargets[2];

+1 for making this a struct instead of an array.

As for replacing the union with mozilla::Variant: Variant does not currently support IPC, but I filed a mentored bug for adding IPC support for it. (We can then refactor this to use Variant as a follow-up, probably in another mentored bug.)

::: gfx/layers/apz/src/FocusTarget.cpp:44
(Diff revision 1)
> +{
> +}
> +
> +FocusTarget::FocusTarget(nsIDocument* aDocument,
> +                         nsIScrollableFrame* aScrollHorizontalTarget,
> +                         nsIScrollableFrame* aScrollVerticalTarget)

I wonder if this is the best interface. Does the caller not need to also consult the focus manager to compute the nsIScrollableFrame arguments? If so, are we not duplicating work between the caller, and this method?

::: gfx/layers/apz/src/FocusTarget.cpp:52
(Diff revision 1)
> +  using namespace mozilla::layout;
> +
> +  // Find the currently focused content in this document
> +  nsCOMPtr<nsIContent> focusTarget;
> +  nsIFocusManager* fm = nsFocusManager::GetFocusManager();
> +  if (fm && aDocument) {

Do we ever pass in a null document? If not, it's better to assert that aDocument is not null.

Comment 51

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #50)
> What about user inputs for which the default handling may trigger a focus
> change, like a "Tab" key that can advance the focus to a different element,
> or the hotkey that gives focus to the address bar?
> 
> Or are those actions still implemented using event listeners in chrome JS?

This is probably a question for :smaug.

Comment 52

[Olli Pettay [:smaug][[e-mail]]]

Some are implemented as event listener, some as default action.
Tab being the latter
http://searchfox.org/mozilla-central/rev/12afa8cc9cea4a141494f767541b411845eef7a0/dom/events/EventStateManager.cpp#2821,2844-2845

Comment 53

[Botond Ballo [:botond]]

(In reply to Olli Pettay [:smaug] from comment #52)
> Some are implemented as event listener, some as default action.
> Tab being the latter
> http://searchfox.org/mozilla-central/rev/
> 12afa8cc9cea4a141494f767541b411845eef7a0/dom/events/EventStateManager.
> cpp#2821,2844-2845

Ok. So it sounds like we'd need a "blacklist" of keys whose default action can trigger a focus change, and consider key events involving those keys as being potentially focus-changing regardless of whether there are key event listeners registered?

Comment 54

[Olli Pettay [:smaug][[e-mail]]]

Hmm, why? I thought all the input events before up/down/left/right would stop apz scrolling or so.

Comment 55

[Botond Ballo [:botond]]

Comment on attachment 8875166 [details]
Bug 1351783 part 10 - Create and sync the current FocusTarget on each layer transaction.

https://reviewboard.mozilla.org/r/146562/#review152092

r+ with comments addressed (and in particular, with the <iframe> case tested)

::: commit-message-317ea:5
(Diff revision 1)
> +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.

::: gfx/layers/ipc/CompositorBridgeParent.cpp:857
(Diff revision 1)
>        mWaitForPluginsUntil = TimeStamp();
>        mHaveBlockedForPlugins = false;
>      }
>  #endif
>  
> +    if (mApzcTreeManager) {

Consider combining the two |if (mApzcTreeManager)| blocks into one (with a nested if for the additional conditions).

::: gfx/layers/ipc/CompositorBridgeParent.cpp:1215
(Diff revision 1)
>  
>    mCompositionManager->Updated(aInfo.isFirstPaint(), targetConfig);
>    Layer* root = aLayerTree->GetRoot();
>    mLayerManager->SetRoot(root);
>  
> +  if (mApzcTreeManager) {

Likewise.

::: gfx/layers/ipc/ShadowLayers.h:457
(Diff revision 1)
>    ClientLayerManager* mClientLayerManager;
>    Transaction* mTxn;
>    MessageLoop* mMessageLoop;
>    DiagnosticTypes mDiagnosticTypes;
>    bool mIsFirstPaint;
> +  FocusTarget mFocusTarget;

Should we null this out at the end of EndTransaction() (similarly to how we set mIsFirstPaint to false) to make sure we never send a stale focus target?

::: layout/base/PresShell.cpp:6338
(Diff revision 1)
>  
> +  // Update the focus target for async keyboard scrolling. This will be forwarded
> +  // to APZ by nsDisplayList::PaintRoot. We we need to to do this before we enter
> +  // the paint phase because dispatching eVoid events can cause layout to happen.
> +  mAPZFocusTarget = FocusTarget(GetDocument(),
> +                                GetFrameToScrollAsScrollable(nsIPresShell::eHorizontal),

This is the duplication of work I was referring to in the other patch: GetFrameToScrollAsScrollable() calls nsIFocusManager::GetFocusedElementForWindow() as well, so we're in fact calling GetFocusedElementForWindow() three times: once in either call to GetFrameToScrollAsScrollable(), and once in the FocusTarget constructor.

Here's what I would suggest doing instead:

  - Split GetFrameToScrollAsScrollable() into a function that gets
    the focused element, and a function that, given the focused
    element gets the frame to scroll in a given direction.
    
  - Call the first function once. Reuse its result as both the
    argument to TabParent::GetFrom() (to check for the ref layer),
    and the argument to the calls to the second function to get
    the scroll frames.
    
(This will obviously require some changes to the signature of the FocusTarget constructor. I'll leave it up to you exactly how to arrange it, but one idea would be to have the FocusTarget constructor take an nsIPresShell\*, and have it do all the work.)

::: layout/base/nsIPresShell.h:1526
(Diff revision 1)
>    uint32_t GetPresShellId() { return mPresShellId; }
>  
>    /**
> +   * Get the APZ FocusTarget used for async keyboard scrolling.
> +   */
> +  mozilla::layers::FocusTarget GetAPZFocusTarget() const { return mAPZFocusTarget; }

Return by reference-to-const. The caller can make a copy if necessary.

::: layout/base/nsIPresShell.h:1904
(Diff revision 1)
>    bool mNeedThrottledAnimationFlush : 1;
>  
>    uint32_t                  mPresShellId;
>  
> +  // The focus information needed for async keyboard scrolling
> +  mozilla::layers::FocusTarget         mAPZFocusTarget;

There are extra spaces between the type name and the variable name here, and yet I'm not seeing the variable name line up with anything.

::: layout/painting/nsDisplayList.cpp:2229
(Diff revision 1)
>                             root, FrameMetrics::NULL_SCROLL_ID, viewport, Nothing(),
>                             isRootContent, containerParameters));
>      }
>  
> +    // Send an updated focus target with this transaction
> +    layerManager->SetFocusTarget(presShell->GetAPZFocusTarget());

Here we are passing the focus target derived from a single pres shell (the one for the root document in the process), but we can potentially have a tree of documents and corresponding pres shells.

Have you tested what happens when there are child documents? For example, if there is an <iframe> with a scrollable element inside, and the scrollable element is focused, is the focus target reported by the root pres shell the scrollable element as opposed to the <iframe> (suggesting that the focus checking "descends" into the child document)?

Comment 56

[Ryan Hunt [:rhunt]]

(In reply to Olli Pettay [:smaug] from comment #54)
> Hmm, why? I thought all the input events before up/down/left/right would
> stop apz scrolling or so.

All other input events before up/down/left/right will stop apz key scrolling, except for key events where there is some more logic so we can keep the focus information we have longer.

The current assumption is that if we cannot find a scroll action to do for a key event in APZ and we have an up to date focus target which doesn't have key event listeners, then we assume that content won't change the focus target and we can keep it.

I added this because when hitting the up arrow, there is a eKeyDown, eKeyPress, and eKeyUp, dispatched. The action to scroll is attached to eKeyPress (or eKeyDown can't remember which), and so if we were to lose the focus target on every unrecognized key event, we would lose the focus target before being able to scroll.

So if tabbing is the default action for a key, then we either need to find a different solution to that problem or add tab to the KeyboardMap as a dispatch to content shortcut.

Comment 57

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8875169 [details]
Bug 1351783 part 15 - Hook up APZC for scrolling based on a KeyboardScrollAction.

https://reviewboard.mozilla.org/r/146568/#review152394

Overall this patch looks good but there's enough issues below that I'm minusing it for now.

::: commit-message-8a5cb:4
(Diff revision 1)
> +needed for a keyboard action. Keyboard scrolling also doesn't work well with
> +scroll snapping for some reason, so we ignore any scrolls when we have scroll

I'd be more specific here - "Keyboard scrolling behaviour is more complex with scroll snapping, so for this initial implementation we don't support async keyboard scrolling when we have scroll snap points"

::: gfx/layers/apz/src/AsyncPanZoomController.h:490
(Diff revision 1)
>     * Helper methods for handling scroll wheel events.
>     */
>    nsEventStatus OnScrollWheel(const ScrollWheelInput& aEvent);
>  
>    ParentLayerPoint GetScrollWheelDelta(const ScrollWheelInput& aEvent) const;
> +  ParentLayerPoint GetKeyboardActionDelta(const KeyboardAction& aAction) const;

Add a comment above this similar to the existing "Helper methods for ..." comments, to keep it separated from the wheel event functions.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1022
(Diff revision 1)
> +  if (ScrollSnapUtils::HasScrollSnapping(mScrollMetadata.GetSnapInfo())) {
> +    return nsEventStatus_eIgnore;
> +  }
> +
> +  // Calculate the delta for this keyboard command
> +  ParentLayerPoint delta = GetKeyboardActionDelta(aAction);

So this is a little confusing to me. In most cases (e.g. line/page/character scroll) this function will return a "context-free" delta. It's just an amount that you want to add to the current scroll offset. However in the "scroll to end" case it's a delta that has an assumption baked in about the current scroll offset (in that it checks the destination of any ongoing scroll animations).

Later in this function, we possibly clobber the existing scroll animation and start a new one from the current scroll offset. That basically invalidates the assumption that got baked in earlier. So I think in the case where we have an existing smooth scroll or wheel scroll, and we do a keyboard "scroll to end", the new animation might end at a place that's not where we want.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1029
(Diff revision 1)
> +  CSSPoint startPosition = mFrameMetrics.GetScrollOffset();
> +  // If we're already in a wheel scroll or smooth scroll animation,
> +  // the delta is applied to its destination, not to the current
> +  // scroll position.

I don't think this startPosition is used anywhere, and the comment doesn't apply here

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1116
(Diff revision 1)
>      case WHEEL_SCROLL:
>      case PAN_MOMENTUM:
>        MOZ_ASSERT(GetCurrentTouchBlock());
>        GetCurrentTouchBlock()->GetOverscrollHandoffChain()->CancelAnimations(ExcludeOverscroll);
>        MOZ_FALLTHROUGH;

Need to add the KEYBOARD_SCROLL case here as well. I filed bug 1372233 to remove the footgun.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1709
(Diff revision 1)
> +  ParentLayerSize scrollAmount;
> +  ParentLayerSize pageScrollSize;

Use more consistent names for these - maybe lineScrollAmount and pageScrollAmount. I prefer the first one having "line" in the name even though it's also used for "character" scrolling. (And yeah I realize you just copied this from elsewhere, I'd prefer to chose those too, but not in this bug)

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1719
(Diff revision 1)
> +    LayoutDeviceIntSize scrollAmountLD = mScrollMetadata.GetLineScrollAmount();
> +    LayoutDeviceIntSize pageScrollSizeLD = mScrollMetadata.GetPageScrollAmount();

ditto on the names. Or you could just inline these

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1727
(Diff revision 1)
> +    CSSRect scrollRectCSS = mScrollMetadata.GetMetrics().GetScrollableRect();
> +    CSSPoint scrollOffsetCSS = mScrollMetadata.GetMetrics().GetScrollOffset();

You can use mFrameMetrics instead of mScrollMetadata.GetMetrics(). The former is just a reference to the latter.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1759
(Diff revision 1)
> +      break;
> +    }
> +    case KeyboardAction::SCROLL_LINE: {
> +      int32_t lineCount = gfxPrefs::ToolkitVerticalScrollDistance();
> +      if (lineCount == 0) {
> +        lineCount = NS_DEFAULT_VERTICAL_SCROLL_DISTANCE;

There's some extra clamping code at http://searchfox.org/mozilla-central/rev/d840ebd5858a61dbc1622487c1fab74ecf235e03/layout/generic/nsGfxScrollFrame.cpp#1374 which looks important. I don't mind deferring it to a follow-up bug but it would be good to have a TODO or comment here.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1763
(Diff revision 1)
> +        delta.y = lineCount * scrollAmount.width;
> +      } else {
> +        delta.y = -lineCount * scrollAmount.width;

shouldn't this use the .height component of the scroll amount? Same for SCROLL_PAGE

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1779
(Diff revision 1)
> +      }
> +      break;
> +    }
> +    case KeyboardAction::COMPLETE_SCROLL: {
> +      if (aAction.mForward) {
> +        delta.y = scrollRect.BottomLeft().y - scrollOffset.y;

For the "scroll to end" case here, you actually don't want to use BottomLeft().y as the y value. What you really want is to subtract the composition bounds height from the scrollable rect's max y, because you can't actually get to scroll positions larger that than (or you'd end up in overscroll). I'm sure there's clamping elsewhere in the code that prevents that, but I'm not sure if it's before or after the animation. If it's after, then the animation might get a "truncated" effect because it was going to animate a greater delta than it actually did.

Anyway, look at http://searchfox.org/mozilla-central/rev/d840ebd5858a61dbc1622487c1fab74ecf235e03/gfx/layers/apz/src/AsyncPanZoomController.cpp#902 for an example of what to do.

Edit: after looking through the rest of the patch, it looks like the clamping happens in ClampOriginToScrollableRect call in KeyboardScrollAnimation::Update, so this should behave fine (i.e. the clamping is before the animation computation, so no truncation effect). However you might still want to make use of the GetAxisLength functions for convenience.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1785
(Diff revision 1)
> +      } else {
> +        delta.y = scrollRect.TopLeft().y - scrollOffset.y;
> +      }
> +      break;
> +    }
> +    default:

To get a jump on fixing bug 1372233, replace this "default" with "case KeyboardAction::SENTINEL".

::: gfx/layers/apz/src/KeyboardScrollAnimation.cpp:112
(Diff revision 1)
> +    case KeyboardAction::COMPLETE_SCROLL: {
> +      mOriginMaxMS = clamped(gfxPrefs::OtherSmoothScrollMaxDurationMs(), 0, 10000);
> +      mOriginMinMS = clamped(gfxPrefs::OtherSmoothScrollMinDurationMs(), 0, mOriginMaxMS);
> +      break;
> +    }
> +    default: {

ditto on "default"

::: gfx/thebes/gfxPrefs.h:662
(Diff revision 1)
>    DECL_GFX_PREF(Once, "slider.snapMultiplier",                 SliderSnapMultiplier, int32_t, 0);
>  
>    DECL_GFX_PREF(Live, "test.events.async.enabled",             TestEventsAsyncEnabled, bool, false);
>    DECL_GFX_PREF(Live, "test.mousescroll",                      MouseScrollTestingEnabled, bool, false);
>  
> +  DECL_GFX_PREF(Live, "toolkit.scrollbox.horizontalScrollDistance", ToolkitHorizontalScrollDistance, int32_t, 0);

In general we try to match the defaults in gfxPrefs.h with the values in all.js, so I would say to put 3 and 5 here rather 0 and 0. That way you also don't need the special handling for 0 in the code, since NS_DEFAULT_*_SCROLL_DISTANCE is #define'd to be the same values.

Comment 58

[Botond Ballo [:botond]]

Comment on attachment 8875167 [details]
Bug 1351783 part 12 - Create and sync focus sequence numbers.

https://reviewboard.mozilla.org/r/146564/#review152414

r+ with comments addressed (and in particular Kats' question about child pres shells).

There are some call sites that call IAPZCTreeManager::ReceiveInputEvent(InputData) directly, without going through IAPZCTreeManager::ReceiveInputEvent(WidgetInputEvent). For example, here [1]. With the code as written, the sequence number will never be transferred from the InputData version of the event to the WidgetEvent version at those call sites.

At the call sites in question, the event originates as an InputData, and it converted to a WidgetEvent using one of the ToWidget*Event() functions in InputData.h (like this one [2]). Transferring the sequence number in those functions as well, should therefore handle these call sites.

[1] http://searchfox.org/mozilla-central/rev/d840ebd5858a61dbc1622487c1fab74ecf235e03/widget/cocoa/nsChildView.mm#2909
[2] http://searchfox.org/mozilla-central/rev/61054508641ee76f9c49bcf7303ef3cfb6b410d2/widget/InputData.h#363

::: commit-message-1cbd9:5
(Diff revision 1)
> +Bug 1351783 part 11 - Create and sync focus sequence numbers r?kats
> +
> +Focus can change at any moment in a document. This causes non-determinism and
> +correctness problems for doing keyboard apz scrolling. To get around this, we
> +will maintain deterministic behavior for focus changes initiated by event listeners

Please tweak the terminology to refer to input events specifically, similar to what Kats suggested in his comments on part 7.

::: gfx/layers/apz/src/APZCTreeManager.cpp:911
(Diff revision 1)
> +      mFocusState.ReceiveUnhandledEvent(aEvent);
> +
>        MultiTouchInput& touchInput = aEvent.AsMultiTouchInput();
>        result = ProcessTouchInput(touchInput, aOutTargetGuid, aOutInputBlockId);
>        break;
>      } case MOUSE_INPUT: {

I find calling ReceiveUnhandledEvent() in each case a bit repetitive and error prone.

Suggested alternative:

  class AutoSequenceNumberSetter {
  public:
      AutoSequenceNumberSetter(FocusState& aFocusState, InputData& aEvent) 
        : mFocusState(aFocusState)
        , mEvent(aEvent)
        , mMayChangeFocus(true) 
      {}
      
      void MarkAsNonFocusChanging() { mMayChangeFocus = false; }
      
      ~AutoSequenceNumberSetter() {
        if (mMayChangeFocus) {
          mFocusState.ReceiveUnhandledEvent(mEvent);
        }
      }
  private:
      FocusState& mFocusState;
      InputData& mEvent;
      bool mMayChangeFocus;      
  };
  
  APZCTreeManager::ReceiveInputEvent(InputData& aEvent, ...) {
      // Place an AutoSequenceNumberSetter on the stack.
      AutoSequenceNumberSetter setter{mFocusState, aEvent};
      
      ...
      
      if (/* condition that tells us the event WON'T change focus */) {
          setter.MarkAsNonFocusChanging();
      }
      
      // If we haven't called MarkAsNonFocusChanging(), the
      // AutoSequenceNumberSetter destructor will call
      // ReceiveUnhandledEvent() for us.
  }
  
Update: it occurs to me that, if we add the assertion to PresShell::HandleEvent() that I suggested, then we'll still need to set a sequence number on non-focus-changing events, just not an _incremented_ sequence number. That should be straight-forward to do.

::: gfx/layers/apz/src/FocusState.h:97
(Diff revision 1)
> +  /**
> +   * Notify focus state of a potentially focus changing event.
> +   *
> +   * @param aEvent the event that could cause a focus change
> +   */
> +  void ReceiveUnhandledEvent(InputData& aEvent);

I would be happy for both NotifyUnhandledEvent() and ReceiveUnhandledEvent() to simply be called "NotifyEvent()", with a comment next to the zero-arg overload explaining that it's for cases where the event object itself isn't available at the call site, but we still want an incremented sequence number.

::: gfx/layers/apz/src/FocusState.cpp:12
(Diff revision 1)
>  
>  namespace mozilla {
>  namespace layers {
>  
>  FocusState::FocusState()
> -  : mFocusHasKeyOrScrollEventListeners(false)
> +  : mLastAPZProcessedEvent(0)

Can we initialize this to 1, and treat 0 as an "uninitialized" sequence number?

Specifically, I am proposing that we:

  1) Initialize this to 1 instead of 0 here (while
     continuing to initialize InputData and WidgetEvent's
     field to 0).

  2) Document that FocusState::LastAPZProcessedEvent()
     never returns 0, and that ReceiveUnhandledEvent()
     never sets InputData::mFocusSequenceNumber to 0.
     
  3) In PresShell::HandleEvent() where we check the
     sequence number, assert that it's not 0.
     
This way, if we ever introduce a codepath that bypasses the assignment of a sequence number to an event, the assertion will catch it.

::: gfx/layers/apz/src/FocusState.cpp:71
(Diff revision 1)
>          mFocusHorizontalTarget = target.mData.mScrollTargets[FocusTarget::HORIZONTAL];
>          mFocusVerticalTarget = target.mData.mScrollTargets[FocusTarget::VERTICAL];
> +
> +        // Mark what sequence number this target has so we can determine whether
> +        // it is stale or not
> +        mLastContentProcessedEvent = target.mSequenceNumber;

I think we should save the sequence number in the FocusTarget::NONE case as well. While it shouldn't make any functional difference, it should aid debugging, because we can more accurately diagnose whether we don't have a valid target because our focus information is stale, or because the focused element isn't scrollable.

::: layout/base/PresShell.cpp:7157
(Diff revision 1)
>  
>    NS_ASSERTION(aFrame, "aFrame should be not null");
>  
> +  // Update the latest focus sequence number with this new sequence number
> +  if (mAPZFocusSequenceNumber < aEvent->mFocusSequenceNumber) {
> +    // XXX should we push a new FocusTarget to APZ here

In a follow-up bug we should probably make sure an "empty transaction" (alluded to in an earlier comment) is sent in this case.

::: widget/BasicEvents.h:411
(Diff revision 1)
>    // The previous mRefPoint, if known, used to calculate mouse movement deltas.
>    LayoutDeviceIntPoint mLastRefPoint;
> +  // The sequence number of the last potentially focus changing event handled
> +  // by APZ. This is used to track when that event has been processed by content,
> +  // and focus can be reconfirmed for async keyboard scrolling.
> +  uint64_t mFocusSequenceNumber;

Would it be more appropriate to add this field to WidgetInputEvent rather than WidgetEvent? I don't think we need it for other types of events, and all the places that currently set it go through IAPZCTreeManager::ReceiveInputEvent() whose parameter type is WidgetInputEvent.

::: widget/InputData.h:92
(Diff revision 1)
>    // platform-specific event times (see bug 77992).
>    TimeStamp mTimeStamp;
> +  // The sequence number of the last potentially focus changing event handled
> +  // by APZ. This is used to track when that event has been processed by content,
> +  // and focus can be reconfirmed for async keyboard scrolling.
> +  uint64_t mFocusSequenceNumber;

I assume that this number wrapping around is something we're just not going to worry about, given that it would take an unrealistically large quantity of events for that to happen, even if a Firefox instance was left running for years. Still probably worth documenting?

Comment 59

[Botond Ballo [:botond]]

Comment on attachment 8875168 [details]
Bug 1351783 part 13 - Add a function for determing if a ScrollSnapInfo has scroll snap points.

https://reviewboard.mozilla.org/r/146566/#review152618

Please file a follow-up bug for getting scroll snapping to work with keyboard APZ.

::: layout/generic/ScrollSnap.h:41
(Diff revision 1)
>        const nsSize& aScrollPortSize,
>        const nsRect& aScrollRange,
>        const nsPoint& aStartPos,
>        const nsPoint& aDestination);
> +
> +  static bool HasScrollSnapping(const layers::ScrollSnapInfo& aSnapInfo);

Make this an instance method of ScrollSnapInfo instead.

Comment 60

[Botond Ballo [:botond]]

Comment on attachment 8875159 [details]
Bug 1351783 part 3 - Add a KeyboardScrollAction type.

https://reviewboard.mozilla.org/r/146548/#review152632

::: gfx/layers/apz/src/Keyboard.h:37
(Diff revision 1)
> +  KeyboardAction();
> +  KeyboardAction(KeyboardActionType aType, bool aForward);
> +
> +public:
> +  KeyboardActionType mType;
> +  bool mForward;

Please document the meaning of this field, as it's not obvious. (My first guess was "this keyboard action should be forwarded somewhere".)

Comment 61

[Botond Ballo [:botond]]

Comment on attachment 8875169 [details]
Bug 1351783 part 15 - Hook up APZC for scrolling based on a KeyboardScrollAction.

https://reviewboard.mozilla.org/r/146568/#review152634

I'm not done reviewing this, but I'm posting the comments I have so far.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1022
(Diff revision 1)
> +  if (ScrollSnapUtils::HasScrollSnapping(mScrollMetadata.GetSnapInfo())) {
> +    return nsEventStatus_eIgnore;
> +  }
> +
> +  // Calculate the delta for this keyboard command
> +  ParentLayerPoint delta = GetKeyboardActionDelta(aAction);

I think what might make sense here, is to have this return a destination rather than a delta, and later convert that destination to a delta based on the current scroll offset if necessary.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1053
(Diff revision 1)
> +  }
> +
> +  nsPoint deltaInAppUnits =
> +    CSSPoint::ToAppUnits(delta / mFrameMetrics.GetZoom());
> +
> +  // Cast velocity from ParentLayerPoints/ms to CSSPoints/ms then convert to

A cast like this without a corresponding conversion of the value by multiplying or dividing by a scale factor, always makes me suspicious.

In this case, there's a reason for casting, but it's buried in bugzilla history (bug 1022825 comment 35):

  "it makes more sense to use Screen coordinates
   when computing the scroll because you don't
   want the scroll duration to be affected by
   zoom, for example"

("ParentLayer" coordinates were called "Screen" coordinates at the time.)

Could you please mention in this comment that this is why it's OK to cast? For bonus points, also amend the same comment in OnScrollWheel() and SmoothScrollTo().

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1059
(Diff revision 1)
> +  // appunits/second
> +  nsPoint velocity =
> +    CSSPoint::ToAppUnits(CSSPoint(mX.GetVelocity(), mY.GetVelocity())) * 1000.0f;
> +
> +  KeyboardScrollAnimation* animation = mAnimation->AsKeyboardScrollAnimation();
> +  animation->Update(aEvent.mTimeStamp, deltaInAppUnits, nsSize(velocity.x, velocity.y));

MOZ_ASSERT(animation) for good measure.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1743
(Diff revision 1)
> +    scrollOffset = scrollOffsetCSS * mFrameMetrics.GetZoom();
> +  }
> +
> +  ParentLayerPoint delta;
> +  switch (aAction.mType) {
> +    case KeyboardAction::SCROLL_CHARACTER: {

The main thread code has some "if the scroll frame is so small that character or line scrolling would actually scroll by more than a page, just scroll by a page instead" logic [1]. We should probably replicate that here.

(Note that, in the case of a character scroll being converted to a page scroll, we would now use the _width_ of the page scroll amount.)

[1] https://dxr.mozilla.org/mozilla-central/rev/981da978f1f686ad024fa958c9d27d2f8acc5ad0/layout/generic/nsGfxScrollFrame.cpp#1373

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1773
(Diff revision 1)
> +    }
> +    case KeyboardAction::SCROLL_PAGE: {
> +      if (aAction.mForward) {
> +        delta.y = pageScrollSize.width;
> +      } else {
> +        delta.y = -pageScrollSize.width;

This should also use |.height|.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1779
(Diff revision 1)
> +      }
> +      break;
> +    }
> +    case KeyboardAction::COMPLETE_SCROLL: {
> +      if (aAction.mForward) {
> +        delta.y = scrollRect.BottomLeft().y - scrollOffset.y;

|.BottomLeft().y| is more simply written as |.YMost()|.

Similarly, |.TopLeft().y| is just |.y|.

Comment 62

[Botond Ballo [:botond]]

Comment on attachment 8875170 [details]
Bug 1351783 part 16 - Perform async scrolling for keyboard events when possible.

https://reviewboard.mozilla.org/r/146570/#review152534

In this patch I've only looked at gfxPrefs.h so far, but I wanted to post the comments I had about it:

Please add the pref to modules/libpref/init/all.js as well. This ensures it shows up in about:config.

::: gfx/thebes/gfxPrefs.h:315
(Diff revision 1)
>    DECL_GFX_PREF(Live, "apz.fling_friction",                    APZFlingFriction, float, 0.002f);
>    DECL_GFX_PREF(Live, "apz.fling_min_velocity_threshold",      APZFlingMinVelocityThreshold, float, 0.5f);
>    DECL_GFX_PREF(Live, "apz.fling_stop_on_tap_threshold",       APZFlingStopOnTapThreshold, float, 0.05f);
>    DECL_GFX_PREF(Live, "apz.fling_stopped_threshold",           APZFlingStoppedThreshold, float, 0.01f);
>    DECL_GFX_PREF(Live, "apz.highlight_checkerboarded_areas",    APZHighlightCheckerboardedAreas, bool, false);
> +  DECL_GFX_PREF(Live, "apz.keyboard.enabled",                  APZKeyboardEnabled, bool, false);

The keyboard map is initialized during compositor initialization, conditional on this pref.

That means this pref can't be Live, unless you write some code to also perform the keyboard map initialization in reaction to flipping this pref on.

Comment 63

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #61)
> ::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1743
> (Diff revision 1)
> > +    scrollOffset = scrollOffsetCSS * mFrameMetrics.GetZoom();
> > +  }
> > +
> > +  ParentLayerPoint delta;
> > +  switch (aAction.mType) {
> > +    case KeyboardAction::SCROLL_CHARACTER: {
> 
> The main thread code has some "if the scroll frame is so small that
> character or line scrolling would actually scroll by more than a page, just
> scroll by a page instead" logic [1]. We should probably replicate that here.
> 
> (Note that, in the case of a character scroll being converted to a page
> scroll, we would now use the _width_ of the page scroll amount.)
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/rev/
> 981da978f1f686ad024fa958c9d27d2f8acc5ad0/layout/generic/nsGfxScrollFrame.
> cpp#1373

After more careful examination, it appears that this codepath is not used during keyboard scrolling, only during a scrollbar button click. So, there is no need to replicate this logic in the APZ keyboard codepath.

Comment 64

[Botond Ballo [:botond]]

Comment on attachment 8875169 [details]
Bug 1351783 part 15 - Hook up APZC for scrolling based on a KeyboardScrollAction.

https://reviewboard.mozilla.org/r/146568/#review153556

::: gfx/layers/apz/src/KeyboardScrollAnimation.h:20
(Diff revision 1)
> +namespace mozilla {
> +namespace layers {
> +
> +class AsyncPanZoomController;
> +
> +class KeyboardScrollAnimation

This class shares the vast majority of its code with WheelScrollAnimation. Is it possible to reuse the shared code, perhaps by factoring out a common base class?

::: gfx/layers/apz/src/KeyboardScrollAnimation.cpp:73
(Diff revision 1)
> +
> +  // Note: we ignore overscroll for keyboard animations.
> +  ParentLayerPoint adjustedOffset, overscroll;
> +  mApzc.mX.AdjustDisplacement(displacement.x, adjustedOffset.x, overscroll.x);
> +  mApzc.mY.AdjustDisplacement(displacement.y, adjustedOffset.y, overscroll.y,
> +                              !mApzc.mScrollMetadata.AllowVerticalScrollWithWheel());

We shouldn't be calling AllowVerticalScrollWithWheel() for keyboard animations. We can just use the default argument of AdjustDisplacement(), as in the x-axis call.

::: gfx/thebes/gfxPrefs.h:384
(Diff revision 1)
>    DECL_GFX_PREF(Live, "general.smoothScroll.pixels",           PixelSmoothScrollEnabled, bool, true);
>    DECL_GFX_PREF(Live, "general.smoothScroll.pixels.durationMaxMS",
>                  PixelSmoothScrollMaxDurationMs, int32_t, 150);
>    DECL_GFX_PREF(Live, "general.smoothScroll.pixels.durationMinMS",
>                  PixelSmoothScrollMinDurationMs, int32_t, 150);
> +  DECL_GFX_PREF(Live, "general.smoothScroll.lines",            LineSmoothScrollEnabled, bool, true);

Who uses this pref? How about OtherSmoothScrollEnabled?

Comment 65

[Botond Ballo [:botond]]

Comment on attachment 8875170 [details]
Bug 1351783 part 16 - Perform async scrolling for keyboard events when possible.

https://reviewboard.mozilla.org/r/146570/#review153638

This generally looks really good!

I'm going to r- this patch because I have one important concern about it: I'm not seeing anything that prevents content from also scrolling in response to key events that APZ handles. We are setting mHandledByAPZ = true, but the places where main-thread code checks that flag are specific to input types, such as this check for wheel events [1]. I believe we need to add such a check in the main-thread key event handling code. (We should ask :smaug for guidance about where exactly that check should go. My guess would be somewhere in PresShell::HandleEventInternal(), but I'm not confident in that.)

In addition, please file two follow-up bugs:

  1) A bug to track enabling apz.keyboard.enabled by default. (The 
     Quantum Flow-related tracking information, such as blocking 
     bug 1370336, should then be transferred to that bug.)

  2) A bug, blocking the one from #1, to track correct handling of 
     "Tab" and other keys whose default actions can change focus 
     (i.e. the issue described in comment 56).
     
[1] http://searchfox.org/mozilla-central/rev/c49a70b53f67dd5550eec8a08793805f2aca8d42/dom/events/EventStateManager.cpp#3207

::: gfx/layers/apz/src/FocusState.h:118
(Diff revision 1)
> +   * No scrollable layer is returned if any of the following are true:
> +   *   1. We don't have a current focus target
> +   *   2. There are event listeners that could change the focus
> +   *   3. The target has not been layerized
> +   */
> +  Maybe<ScrollableLayerGuid> GetHorizontalTarget();

1) These methods can be const.
2) Please document that the mPresShellId of the returned ScrollableLayerGuid is not set and should not be used in comparisons.

::: gfx/layers/apz/src/FocusState.h:120
(Diff revision 1)
> +   *   2. There are event listeners that could change the focus
> +   *   3. The target has not been layerized
> +   */
> +  Maybe<ScrollableLayerGuid> GetHorizontalTarget();
> +  /**
> +   * Gets the scrollable layer that should be vertically scrolled for a key event, if any.

Rather than duplicating this comment, just say something like, "Similar to GetHorizontalTarget() but for vertical scrolling."

::: gfx/layers/apz/src/FocusState.h:130
(Diff revision 1)
> +   *   3. The target has not been layerized
> +   */
> +  Maybe<ScrollableLayerGuid> GetVerticalTarget();
> +
> +  /**
> +   * Gets whether it is safe to not increment the focus sequence number for a an unmatched keyboard event.

"for a an unmatched" --> "for an unmatched"

Comment 66

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #65)
> I believe we need to
> add such a check in the main-thread key event handling code. (We should ask
> :smaug for guidance about where exactly that check should go. My guess would
> be somewhere in PresShell::HandleEventInternal(), but I'm not confident in
> that.)

It's probably better to make this change in a new patch, rather than amending it into Part 14. Then :smaug can review it without having to look at the rest of Part 14.

Comment 67

[Botond Ballo [:botond]]

Comment on attachment 8875171 [details]
Bug 1351783 part 11 - Sync FocusTarget with WebRenderLayerManager.

https://reviewboard.mozilla.org/r/146572/#review153710

This looks good to me, but I would like :smaug to review the change to the FocusTarget constructor where we compute mHasMouseMoveListeners (i.e. to verify that |innerWindow->HasMouseMoveEventListeners()| is what we want).

::: gfx/layers/apz/src/FocusState.cpp:57
(Diff revision 1)
>  
>      const FocusTarget& target = currentNode->second;
>  
>      // Accumulate event listener flags on the path to the focus target
> +    if (target.mHasMouseMoveListeners) {
> +      mHasMouseMoveListeners = true;

This is a matter of taste (and thus up to you), but elsewhere we're written things like this as:

  mHasMouseMoveListeners |= target.mHasMouseMoveListeners.

Comment 68

[Botond Ballo [:botond]]

ni?smaug for the guidance requested in comment 65, and the review requested in comment 67.

Comment 69

[Botond Ballo [:botond]]

Ryan, thanks for a very well written and organized patch series!

Comment 70

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Botond Ballo [:botond] from comment #65)
> Comment on attachment 8875170 [details]
> Bug 1351783 part 14 - Perform async scrolling for keyboard events when
> possible
> 
> https://reviewboard.mozilla.org/r/146570/#review153638
> 
> This generally looks really good!
> 
> I'm going to r- this patch because I have one important concern about it:
> I'm not seeing anything that prevents content from also scrolling in
> response to key events that APZ handles. We are setting mHandledByAPZ =
> true, but the places where main-thread code checks that flag are specific to
> input types, such as this check for wheel events [1]. I believe we need to
> add such a check in the main-thread key event handling code. (We should ask
> :smaug for guidance about where exactly that check should go. My guess would
> be somewhere in PresShell::HandleEventInternal(), but I'm not confident in
> that.)

Well, arrow keys are handled by builtin XBL http://searchfox.org/mozilla-central/source/dom/xbl/builtin/browser-base.inc#4-7
And browser part of platformHTMLBindings.xml files too.

The listener is attached in http://searchfox.org/mozilla-central/rev/c49a70b53f67dd5550eec8a08793805f2aca8d42/dom/base/nsGlobalWindow.cpp#3048

Do we want some new attribute in those xbl files to indicate whether to skip processing if apz is enabled?
(but I think I saw in some of these patches some check for chrome event handler and skipping checking its listeners.... not sure why that was done)

Comment 71

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8875171 [details]
Bug 1351783 part 11 - Sync FocusTarget with WebRenderLayerManager.

https://reviewboard.mozilla.org/r/146572/#review153912

::: commit-message-5c40a:4
(Diff revision 1)
> +Bug 1351783 part 15 - Don't invalidate the FocusTarget if mousemove events happen without listeners r?kats
> +
> +Invalidating the focus state on every mousemove event can cause situations where we sync
> +scroll because the user has moved their mouse recently and a repaint hasn't come soon enough.

I thought we weren't going to check mousemoves. Apparently we do. I wish I understood why.
I assume some testing has been done indicating this is needed.

Comment 72

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8875171 [details]
Bug 1351783 part 11 - Sync FocusTarget with WebRenderLayerManager.

https://reviewboard.mozilla.org/r/146572/#review153914

::: gfx/layers/apz/src/FocusTarget.cpp:132
(Diff revision 1)
>      focusTarget = do_QueryInterface(focusedElement);
>    }
>  
>    // Collect event listener information so we can track what is potentially focus changing
>    if (nsPIDOMWindowInner* innerWindow = aDocument->GetInnerWindow()) {
> +    mHasMouseMoveListeners = innerWindow->HasMouseMoveEventListeners();

I think this is what we want, assuming we want this check at all.
But, remember, this doesn't check iframes.

Comment 73

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8875170 [details]
Bug 1351783 part 16 - Perform async scrolling for keyboard events when possible.

https://reviewboard.mozilla.org/r/146570/#review153998

Sorry for the delay here. One fairly major potential issue I see here is that the keyboard input is not routed through the InputQueue code, which means it "jump ahead" of the inputs waiting in the queue and get processed by the APZ code out-of-order. I'm not entirely sure if the generation counter thing will prevent that from happening, but I wanted to bring it up to see if you'd already considered that. I think it's probably good to route the input through the InputQueue anyway just for consistency and to ensure we don't have "special cases" in the machinery for processing input events. This patch will probably change significantly with that change so I'll do a more involved review later (or if we decide we don't want to make that change).

Comment 74

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8875172 [details]
Bug 1351783 part 17 - Do less work when apz.keyboard.enabled is false.

https://reviewboard.mozilla.org/r/146574/#review154010

This looks fine but as Botond pointed out earlier the keyboard-apz pref should be marked as "Once" rather than "Live" in gfxPrefs.h

Comment 75

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8875171 [details]
Bug 1351783 part 11 - Sync FocusTarget with WebRenderLayerManager.

https://reviewboard.mozilla.org/r/146572/#review153912

> I thought we weren't going to check mousemoves. Apparently we do. I wish I understood why.
> I assume some testing has been done indicating this is needed.

@smaug, the reason we want to special-case mousemoves is that we expect they happen frequently and can occur between keyboard events that would otherwise be contiguous. So if the user is scrolling with the down arrow repeatedly, and it's going through APZ, and then their optical mouse randomly decides to send a 1-pixel move, we don't want to disrupt that APZ keyboard scroll if there's no mouse move listeners on the page. If there *are* mousemove listeners then it's possible that the mousemove listener changes the focus and so we need to disrupt the APZ keyboard scroll until we can re-confirm the focus.

That being said, now that we are implementing this, it might be worth adding some telemetry to see how often this actually happens in practice (that a mouse move would have interrupted a keyboard-APZ sequence). I'm not sure how easy that is to do, but if we find that it's very low, then we can rip this code out.

Comment 76

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8875171 [details]
Bug 1351783 part 11 - Sync FocusTarget with WebRenderLayerManager.

https://reviewboard.mozilla.org/r/146572/#review154018

This also reminds me: we should file a follow-up bug to remove any code from bug 1357880 that's not needed any more. We got the data we needed and don't need that code sitting in the tree any more.

Comment 77

[Ryan Hunt [:rhunt]]

(In reply to Olli Pettay [:smaug] from comment #72)
> Comment on attachment 8875171 [details]
> Bug 1351783 part 15 - Don't invalidate the FocusTarget if mousemove events
> happen without listeners
> 
> https://reviewboard.mozilla.org/r/146572/#review153914
> 
> ::: gfx/layers/apz/src/FocusTarget.cpp:132
> (Diff revision 1)
> >      focusTarget = do_QueryInterface(focusedElement);
> >    }
> >  
> >    // Collect event listener information so we can track what is potentially focus changing
> >    if (nsPIDOMWindowInner* innerWindow = aDocument->GetInnerWindow()) {
> > +    mHasMouseMoveListeners = innerWindow->HasMouseMoveEventListeners();
> 
> I think this is what we want, assuming we want this check at all.
> But, remember, this doesn't check iframes.

Per this, that patch won't work. I also ran into another difficulty with the lossiness of this function making it ineffective so I'm going to remove this patch from the set for now. I'll add it to the list of follow up bugs.

Comment 78

[Ryan Hunt [:rhunt]]

(In reply to Botond Ballo [:botond] from comment #65)
> Comment on attachment 8875170 [details]
> Bug 1351783 part 14 - Perform async scrolling for keyboard events when
> possible
> 
> https://reviewboard.mozilla.org/r/146570/#review153638
> 
> This generally looks really good!
> 
> I'm going to r- this patch because I have one important concern about it:
> I'm not seeing anything that prevents content from also scrolling in
> response to key events that APZ handles. We are setting mHandledByAPZ =
> true, but the places where main-thread code checks that flag are specific to
> input types, such as this check for wheel events [1]. I believe we need to
> add such a check in the main-thread key event handling code. (We should ask
> :smaug for guidance about where exactly that check should go. My guess would
> be somewhere in PresShell::HandleEventInternal(), but I'm not confident in
> that.)
>

So in these patches I attempted to do this by returning nsEventStatus_eConsumeNoDefault for key scrolls that are handled by APZ. I'm not sure exactly what that entails, but the XBL key scroll handlers don't run if the event is preventDefaulted [1].

So if nsEventStatus_eConsumeNoDefault isn't good enough, it seems like we can set preventDefault on the event when mHandledByAPZ == true.

[1] http://searchfox.org/mozilla-central/rev/20d16dadd336e0c6b97e3e19dc4ff907744b5734/dom/xbl/nsXBLWindowKeyHandler.cpp#254

Comment 79

[Botond Ballo [:botond]]

(In reply to Ryan Hunt [:rhunt] from comment #78)
> (In reply to Botond Ballo [:botond] from comment #65)
> > I'm going to r- this patch because I have one important concern about it:
> > I'm not seeing anything that prevents content from also scrolling in
> > response to key events that APZ handles. We are setting mHandledByAPZ =
> > true, but the places where main-thread code checks that flag are specific to
> > input types, such as this check for wheel events [1]. I believe we need to
> > add such a check in the main-thread key event handling code. (We should ask
> > :smaug for guidance about where exactly that check should go. My guess would
> > be somewhere in PresShell::HandleEventInternal(), but I'm not confident in
> > that.)
> 
> So in these patches I attempted to do this by returning
> nsEventStatus_eConsumeNoDefault for key scrolls that are handled by APZ. 

Thanks, I had overlooked that.

Thinking about it a bit, it seems to me that returning nsEventStatus_eConsumeNoDefault and setting mHandledByAPZ are two complementary mechanisms for controlling the main thread's processing of events that APZ handles:

  - Returning nsEventStatus_eConsumeNoDefault causes the event not to
    reach the main thread at all.

  - Setting mHandledByAPZ allows the event to reach the main thread
    (assuming we're not also returning nsEventStatus_eConsumeNoDefault,
    of course), but allows the main thread to do something different
    when handling it.

For wheel events, we do the latter because e.g. the main thread can potentially preventDefault() them, so it still needs to receive the event.

If our approach for keyboard events that APZ can handle is to do the former, and that works, then that's fine (better in fact, since we're avoiding extra IPC traffic and processing!). In that case, though, there's no need to _also_ set mHandledByAPZ (since the event doesn't make it to the main thread at all), and in fact the field KeyboardInput::mHandledByAPZ probably doesn't need to exist at all.

However, I applied your patches locally, and during some brief testing that I did on Linux, I was seeting the main-thread keyboard scrolling codepath (PresShell::ScrollLine) being active even when keyboard events were going through APZ, suggesting that something in the intended mechanism is going wrong. Let me know if you can reproduce this - if not, I can investigate it further.

Comment 80

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #79)
> However, I applied your patches locally, and during some brief testing that
> I did on Linux, I was seeting the main-thread keyboard scrolling codepath
> (PresShell::ScrollLine) being active even when keyboard events were going
> through APZ, suggesting that something in the intended mechanism is going
> wrong. Let me know if you can reproduce this - if not, I can investigate it
> further.

I can't reproduce this any more. I suspect what I was seeing before, is PresShell::ScrollLine being called on a page where we can usually handle keyboard scrolls asynchronously, because for that particular keyboard event, the focus information was out of date (I see that happen if you e.g. click the page and then press an arrow key; that's expected for now).

Comment 81

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #65)
> I'm going to r- this patch because I have one important concern about it:
> I'm not seeing anything that prevents content from also scrolling in
> response to key events that APZ handles.

This concern has been addressed (per comment 79 and comment 80). 

I'd still like to review an updated version of the Part 14 patch, as the use of InputQueue (per comment 73) will involve some nontrivial changes to it.

Comment 82

[Ryan Hunt [:rhunt]]

Attachment: Bug 1351783 part 14 - Create a base class for WheelScrollAnimation.

Review commit: https://reviewboard.mozilla.org/r/150314/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150314/

Comment 83

[Ryan Hunt [:rhunt]]

Attachment: Bug 1351783 part 18 - Add async keyboard scrolling information to about:support.

Review commit: https://reviewboard.mozilla.org/r/150318/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150318/

Comment 84

[Ryan Hunt [:rhunt]]

Attachment: Bug 1351783 part 19 - Rename Keyboard.h to KeyboardMap.h.

This is a better name for the header that matches its main class.

Review commit: https://reviewboard.mozilla.org/r/150320/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150320/

Comment 85

[Ryan Hunt [:rhunt]]

Comment on attachment 8875157 [details]
Bug 1351783 part 1 - Add includes for unified build issues.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146544/diff/1-2/

Comment 86

[Ryan Hunt [:rhunt]]

Comment on attachment 8875158 [details]
Bug 1351783 part 2 - Add a KeyboardInput type.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146546/diff/1-2/

Comment 87

[Ryan Hunt [:rhunt]]

Comment on attachment 8875159 [details]
Bug 1351783 part 3 - Add a KeyboardScrollAction type.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146548/diff/1-2/

Comment 88

[Ryan Hunt [:rhunt]]

Comment on attachment 8875160 [details]
Bug 1351783 part 4 - Add a KeyboardShortcut type.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146550/diff/1-2/

Comment 89

[Ryan Hunt [:rhunt]]

Comment on attachment 8875161 [details]
Bug 1351783 part 5 - Add a KeyboardMap type.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146552/diff/1-2/

Comment 90

[Ryan Hunt [:rhunt]]

Comment on attachment 8875162 [details]
Bug 1351783 part 6 - Create and send KeyboardMap to APZCTreeManager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146554/diff/1-2/

Comment 91

[Ryan Hunt [:rhunt]]

Comment on attachment 8875163 [details]
Bug 1351783 part 7 - Create FocusState and FocusTarget types.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146556/diff/1-2/

Comment 92

[Ryan Hunt [:rhunt]]

Comment on attachment 8875164 [details]
Bug 1351783 part 8 - Gather whether there are key listeners on the focused element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146558/diff/1-2/

Comment 93

[Ryan Hunt [:rhunt]]

Comment on attachment 8875165 [details]
Bug 1351783 part 9 - Disable a FocusTarget for an editable element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146560/diff/1-2/

Comment 94

[Ryan Hunt [:rhunt]]

Comment on attachment 8875166 [details]
Bug 1351783 part 10 - Create and sync the current FocusTarget on each layer transaction.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146562/diff/1-2/

Comment 95

[Ryan Hunt [:rhunt]]

Comment on attachment 8875171 [details]
Bug 1351783 part 11 - Sync FocusTarget with WebRenderLayerManager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146572/diff/1-2/

Comment 96

[Ryan Hunt [:rhunt]]

Comment on attachment 8875167 [details]
Bug 1351783 part 12 - Create and sync focus sequence numbers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146564/diff/1-2/

Comment 97

[Ryan Hunt [:rhunt]]

Comment on attachment 8875168 [details]
Bug 1351783 part 13 - Add a function for determing if a ScrollSnapInfo has scroll snap points.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146566/diff/1-2/

Comment 98

[Ryan Hunt [:rhunt]]

Comment on attachment 8875169 [details]
Bug 1351783 part 15 - Hook up APZC for scrolling based on a KeyboardScrollAction.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146568/diff/1-2/

Comment 99

[Ryan Hunt [:rhunt]]

Comment on attachment 8875170 [details]
Bug 1351783 part 16 - Perform async scrolling for keyboard events when possible.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146570/diff/1-2/

Comment 100

[Ryan Hunt [:rhunt]]

Comment on attachment 8875172 [details]
Bug 1351783 part 17 - Do less work when apz.keyboard.enabled is false.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146574/diff/1-2/

Comment 101

[Ryan Hunt [:rhunt]]

I've posted a new patcheset that should address all the review comments for all of the patches except for part 16, where I still need to update it to use the APZ input queue.

Comment 102

[Ryan Hunt [:rhunt]]

(In reply to Botond Ballo [:botond] from comment #58)
> Comment on attachment 8875167 [details]
> Bug 1351783 part 11 - Create and sync focus sequence numbers
> 
> Would it be more appropriate to add this field to WidgetInputEvent rather
> than WidgetEvent? I don't think we need it for other types of events, and
> all the places that currently set it go through
> IAPZCTreeManager::ReceiveInputEvent() whose parameter type is
> WidgetInputEvent.
> 

I think WidgetCommandEvent might need this in the future, because it can trigger focus changes. Otherwise we definitely can limit this to WidgetInputEvent.

Comment 103

[Ryan Hunt [:rhunt]]

(In reply to Botond Ballo [:botond] from comment #58)
> Comment on attachment 8875167 [details]
> Bug 1351783 part 12 - Create and sync focus sequence numbers.
> 
> ::: layout/painting/nsDisplayList.cpp:2229
> (Diff revision 1)
> >                             root, FrameMetrics::NULL_SCROLL_ID, viewport, Nothing(),
> >                             isRootContent, containerParameters));
> >      }
> >  
> > +    // Send an updated focus target with this transaction
> > +    layerManager->SetFocusTarget(presShell->GetAPZFocusTarget());
> 
> Here we are passing the focus target derived from a single pres shell (the
> one for the root document in the process), but we can potentially have a
> tree of documents and corresponding pres shells.
> 
> Have you tested what happens when there are child documents? For example, if
> there is an <iframe> with a scrollable element inside, and the scrollable
> element is focused, is the focus target reported by the root pres shell the
> scrollable element as opposed to the <iframe> (suggesting that the focus
> checking "descends" into the child document)?

So the behavior from reading the code and testing this is that the event is dispatched to the top level or root PresShell (which in this case is the parent of the iframe), this PresShell then retargets the event to the child PresShell (the iframe) which handles 

So we should be getting the events that are being dispatched to the iframe in the top level PresShell. And we only paint for the top level PresShell, so focus updates will only happen for that one.

Comment 104

[Ryan Hunt [:rhunt]]

(In reply to Botond Ballo [:botond] from comment #58)
> Comment on attachment 8875167 [details]
> Bug 1351783 part 12 - Create and sync focus sequence numbers.
> 
> Can we initialize this to 1, and treat 0 as an "uninitialized" sequence
> number?
> 
> Specifically, I am proposing that we:
> 
>   1) Initialize this to 1 instead of 0 here (while
>      continuing to initialize InputData and WidgetEvent's
>      field to 0).
> 
>   2) Document that FocusState::LastAPZProcessedEvent()
>      never returns 0, and that ReceiveUnhandledEvent()
>      never sets InputData::mFocusSequenceNumber to 0.
>      
>   3) In PresShell::HandleEvent() where we check the
>      sequence number, assert that it's not 0.
>      
> This way, if we ever introduce a codepath that bypasses the assignment of a
> sequence number to an event, the assertion will catch it.

I like this idea and implemented it in the new patches, except for the assertion in PresShell. We don't currently send all widget events through APZ so this assertion fails. For example, there's a WidgetQueryContentEvent which isn't forwarded to APZ. I also mentioned that WidgetCommandEvent will probably need to get focus sequence numbers. I'd like to investigate getting this assertion working more in a follow up, if it's not a problem.

Comment 105

[Ryan Hunt [:rhunt]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3063fd558e9dd905b9da74baa60b64c5cbd78fd2

Comment 106

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8875164 [details]
Bug 1351783 part 8 - Gather whether there are key listeners on the focused element.

https://reviewboard.mozilla.org/r/146558/#review155046

::: dom/base/nsIContent.h:980
(Diff revision 2)
> +  {
> +    // xul:browser contains some bindings that effectively disable async key
> +    // scrolling. The event listeners appear to be okay to ignore, so as a
> +    // workaround, we pretend xul:browser doesn't have any key event listeners
> +    // ever.
> +    return !IsXULElement(nsGkAtoms::browser) &&

I don't understand this. Why special case xul:browser? What if one adds listener to xul:tabbrowser or so?
This doesn't seem to be too easily maintainable approach.

Do we need to know about the key event listeners in parent process? I'd rather take such approach than special case some particular element and XULDocument.

::: dom/events/EventListenerManager.cpp:1767
(Diff revision 2)
>      // JS implemented event listeners, and trickier to handle here.
>    }
>  }
>  
>  bool
> +EventListenerManager::HasKeyEventListeners()

I wonder... should this check only system group listeners?
Or, should this check only listeners which accept untrusted events.
The latter might work quite well, since Gecko is the only engine supporting adding listeners explicitly for trusted only events. In practice only chrome listeners are trusted-only listeners, so listeners added by web pages listen for untrusted events too.

If this checks only listeners which allow also untrusted events, the method name should be changed.

::: dom/ipc/TabChild.cpp:3508
(Diff revision 2)
>    NS_ENSURE_TRUE(mTabChild, nullptr);
>    return mTabChild->GetGlobal();
>  }
> +
> +bool
> +TabChildGlobal::HasKeyEventListeners() const

This looks hackish, and makes the method return a value which lies about the actually state.
But see the comment about EventListenerManager::HasKeyEventListeners()

::: dom/xul/XULDocument.cpp:3118
(Diff revision 2)
>  
>      return NS_OK;
>  }
>  
> +bool
> +XULDocument::HasKeyEventListeners() const

This looks hackish. What if someone adds an event listener to XULDocument which wants to handle arrow keys?

::: gfx/layers/apz/src/FocusTarget.cpp:58
(Diff revision 2)
> +  nsTArray<EventTarget*> targets;
> +  nsresult rv = EventDispatcher::Dispatch(aContent, nullptr, &event, nullptr,
> +      nullptr, nullptr, &targets);
> +  NS_ENSURE_SUCCESS(rv, false);
> +  for (size_t i = 0; i < targets.Length(); i++) {
> +    // Skip the chrome event handler target because it has the XBL key bindings

Also for this perhaps better to consider to check listeners which allow also untrusted events?
XBL key bindings require trusted events
http://searchfox.org/mozilla-central/rev/7cc377ce3f0a569c5c9b362d589705cd4fecc0ac/dom/xbl/nsXBLWindowKeyHandler.cpp#295-303,313-321

Comment 107

[Ryan Hunt [:rhunt]]

(In reply to Olli Pettay [:smaug] from comment #106)
> Comment on attachment 8875164 [details]
> Bug 1351783 part 8 - Gather whether there are key listeners on the focused
> element.
> 
> ::: dom/events/EventListenerManager.cpp:1767
> (Diff revision 2)
> >      // JS implemented event listeners, and trickier to handle here.
> >    }
> >  }
> >  
> >  bool
> > +EventListenerManager::HasKeyEventListeners()
> 
> I wonder... should this check only system group listeners?
> Or, should this check only listeners which accept untrusted events.
> The latter might work quite well, since Gecko is the only engine supporting
> adding listeners explicitly for trusted only events. In practice only chrome
> listeners are trusted-only listeners, so listeners added by web pages listen
> for untrusted events too.
> 
> If this checks only listeners which allow also untrusted events, the method
> name should be changed.
> 

Checking for listeners that accept only trusted events seems like a good idea, and would probably clean up these cases.

If this is used, then people adding chrome listeners will need to be careful to not change the focus in key event listeners, because they'll be treated differently than content event listeners which will disable apz key scrolling.

But that seems okay to me, and is basically what happens with the whitelist in this patch.

> ::: dom/xul/XULDocument.cpp:3118
> (Diff revision 2)
> >  
> >      return NS_OK;
> >  }
> >  
> > +bool
> > +XULDocument::HasKeyEventListeners() const
> 
> This looks hackish. What if someone adds an event listener to XULDocument
> which wants to handle arrow keys?
> 

Yes I agree, these are hacks and I was hoping to do better in a follow up. I'll try the above approach.

Comment 108

[Botond Ballo [:botond]]

Comment on attachment 8879029 [details]
Bug 1351783 part 14 - Create a base class for WheelScrollAnimation.

https://reviewboard.mozilla.org/r/150314/#review155216

Comment 109

[Botond Ballo [:botond]]

(In reply to Ryan Hunt [:rhunt] from comment #104)
> (In reply to Botond Ballo [:botond] from comment #58)
> > Comment on attachment 8875167 [details]
> > Bug 1351783 part 12 - Create and sync focus sequence numbers.
> > 
> > Can we initialize this to 1, and treat 0 as an "uninitialized" sequence
> > number?
> > 
> > Specifically, I am proposing that we:
> > 
> >   1) Initialize this to 1 instead of 0 here (while
> >      continuing to initialize InputData and WidgetEvent's
> >      field to 0).
> > 
> >   2) Document that FocusState::LastAPZProcessedEvent()
> >      never returns 0, and that ReceiveUnhandledEvent()
> >      never sets InputData::mFocusSequenceNumber to 0.
> >      
> >   3) In PresShell::HandleEvent() where we check the
> >      sequence number, assert that it's not 0.
> >      
> > This way, if we ever introduce a codepath that bypasses the assignment of a
> > sequence number to an event, the assertion will catch it.
> 
> I like this idea and implemented it in the new patches, except for the
> assertion in PresShell. We don't currently send all widget events through
> APZ so this assertion fails. For example, there's a WidgetQueryContentEvent
> which isn't forwarded to APZ. I also mentioned that WidgetCommandEvent will
> probably need to get focus sequence numbers. I'd like to investigate getting
> this assertion working more in a follow up, if it's not a problem.

Perhaps we could limit the assertion to the cases where WidgetEvent::AsInputEvent() returns non-null? But yeah, I'm happy to leave it for a follow-up if it's more involved than that.

Comment 110

[Botond Ballo [:botond]]

Comment on attachment 8875170 [details]
Bug 1351783 part 16 - Perform async scrolling for keyboard events when possible.

https://reviewboard.mozilla.org/r/146570/#review155236

LGTM, thanks!

I'm going to clear the review flag, as I'd still like to review the version of the patch that uses InputQueue.

::: gfx/layers/apz/src/APZCTreeManager.cpp:301
(Diff revisions 1 - 2)
>            aLayerMetrics.SetApzc(apzc);
>  
>            mApzcTreeLog << '\n';
>  
> +          // Mark that this layer tree is being used
> +          state.mLayersIdsToDestroy.erase(layersId);

We can avoid calling this once for every node, and just call it once for every layers id:

  - Call it once at the beginning, for |aRootLayerTreeId|
  - Call it here in the pre-action, but only if we have a new layers id

For example ...

::: gfx/layers/apz/src/APZCTreeManager.cpp:323
(Diff revisions 1 - 2)
>            // Note that |node| at this point will not have any children, otherwise we
>            // we would have to set next to node->GetFirstChild().
>            MOZ_ASSERT(!node->GetFirstChild());
>            parent = node;
>            next = nullptr;
>            layersId = aLayerMetrics.GetReferentId().valueOr(layersId);

... we could refactor this line as:

  if (Maybe<uint64_t> newLayersId = aLayerMetrics.GetReferentId()) {
    layersId = newLayersId;
    state.mLayersIdsToDestroy.erase(layersId);
  }

::: gfx/layers/apz/src/FocusState.h:186
(Diff revisions 1 - 2)
> +/**
> + * A RAII class used for setting the focus sequence number on input events
> + * as they are being processed. Any input event is assumed to be potentially
> + * focus changing unless explicitly marked otherwise.
> + */
> +class MOZ_RAII AutoFocusSequenceNumberSetter

As this class is only used in APZCTreeManager.cpp, could we just define it there? (That way, modifying this class only triggers recompilation of APZCTreeManager.cpp, not every cpp file that includes FocusState.h).

::: gfx/layers/apz/src/FocusState.cpp:110
(Diff revisions 1 - 2)
> +FocusState::GetFocusTargetLayerIds() const
> +{
> +  std::unordered_set<uint64_t> layersIds;
> +  layersIds.reserve(mFocusTree.size());
> +
> +  for (auto focusNode : mFocusTree) {

Use |const auto&| instead of |auto|, to avoid copying a pair on each iteration.

Comment 111

[Ryan Hunt [:rhunt]]

Comment on attachment 8875164 [details]
Bug 1351783 part 8 - Gather whether there are key listeners on the focused element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146558/diff/2-3/

Comment 112

[Ryan Hunt [:rhunt]]

Comment on attachment 8875165 [details]
Bug 1351783 part 9 - Disable a FocusTarget for an editable element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146560/diff/2-3/

Comment 113

[Ryan Hunt [:rhunt]]

Comment on attachment 8875166 [details]
Bug 1351783 part 10 - Create and sync the current FocusTarget on each layer transaction.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146562/diff/2-3/

Comment 114

[Ryan Hunt [:rhunt]]

Comment on attachment 8875171 [details]
Bug 1351783 part 11 - Sync FocusTarget with WebRenderLayerManager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146572/diff/2-3/

Comment 115

[Ryan Hunt [:rhunt]]

Comment on attachment 8875167 [details]
Bug 1351783 part 12 - Create and sync focus sequence numbers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146564/diff/2-3/

Comment 116

[Ryan Hunt [:rhunt]]

Comment on attachment 8875168 [details]
Bug 1351783 part 13 - Add a function for determing if a ScrollSnapInfo has scroll snap points.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146566/diff/2-3/

Comment 117

[Ryan Hunt [:rhunt]]

Comment on attachment 8879029 [details]
Bug 1351783 part 14 - Create a base class for WheelScrollAnimation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150314/diff/1-2/

Comment 118

[Ryan Hunt [:rhunt]]

Comment on attachment 8875169 [details]
Bug 1351783 part 15 - Hook up APZC for scrolling based on a KeyboardScrollAction.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146568/diff/2-3/

Comment 119

[Ryan Hunt [:rhunt]]

Comment on attachment 8875170 [details]
Bug 1351783 part 16 - Perform async scrolling for keyboard events when possible.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146570/diff/2-3/

Comment 120

[Ryan Hunt [:rhunt]]

Comment on attachment 8875172 [details]
Bug 1351783 part 17 - Do less work when apz.keyboard.enabled is false.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146574/diff/2-3/

Comment 121

[Ryan Hunt [:rhunt]]

Comment on attachment 8879030 [details]
Bug 1351783 part 18 - Add async keyboard scrolling information to about:support.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150318/diff/1-2/

Comment 122

[Ryan Hunt [:rhunt]]

Comment on attachment 8879031 [details]
Bug 1351783 part 19 - Rename Keyboard.h to KeyboardMap.h.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150320/diff/1-2/

Comment 123

[Ryan Hunt [:rhunt]]

I've updated the patch for checking for key event listeners. Per smaug's suggestion, we no longer special case for XULDocument, TabChildGlobal, etc.

We now disable async key scrolling on (non-system group || accepts untrusted events). This filters out all chrome event listeners.

Comment 124

[Botond Ballo [:botond]]

Comment on attachment 8875170 [details]
Bug 1351783 part 16 - Perform async scrolling for keyboard events when possible.

https://reviewboard.mozilla.org/r/146570/#review155298

The changes look good, thanks!

Comment 125

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8875164 [details]
Bug 1351783 part 8 - Gather whether there are key listeners on the focused element.

https://reviewboard.mozilla.org/r/146558/#review155336

Do we need still something do deal with XBL? I mean the builtin handlers. Or are those handled in some other patch?

::: dom/events/EventListenerManager.cpp:1769
(Diff revision 3)
>  }
>  
>  bool
> +EventListenerManager::HasUntrustedOrNonSystemGroupKeyEventListeners()
> +{
> +  uint32_t count = mListeners.Length();

Should we check non-passive only?

Comment 126

[Ryan Hunt [:rhunt]]

(In reply to Olli Pettay [:smaug] from comment #125)
> Comment on attachment 8875164 [details]
> Bug 1351783 part 8 - Gather whether there are key listeners on the focused
> element.
> 
> ::: dom/events/EventListenerManager.cpp:1769
> (Diff revision 3)
> >  }
> >  
> >  bool
> > +EventListenerManager::HasUntrustedOrNonSystemGroupKeyEventListeners()
> > +{
> > +  uint32_t count = mListeners.Length();
> 
> Should we check non-passive only?

We also need to check for passive listeners because IIRC passive only means it won't call preventDefault, not that it won't change focus. If we choose not to care about that, then we can check for only non-passive ones.

Comment 127

[Olli Pettay [:smaug][[e-mail]]]

And with XBL I meant that whether we need to do something to builtin XBL stuff in child process, or do we just not send key events at all to child process if there are no untrusted-default-group listeners?

Comment 128

[Ryan Hunt [:rhunt]]

(In reply to Olli Pettay [:smaug] from comment #127)
> And with XBL I meant that whether we need to do something to builtin XBL
> stuff in child process, or do we just not send key events at all to child
> process if there are no untrusted-default-group listeners?

The current approach doesn't send key events to the child process when APZ async keyboard scrolls.

If this proves to have issues, we can switch to sending events to the child process with mHandledByAPZ set and use that to not do the default action.

Comment 129

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8875160 [details]
Bug 1351783 part 4 - Add a KeyboardShortcut type.

https://reviewboard.mozilla.org/r/146550/#review155386

::: dom/xbl/nsXBLPrototypeHandler.cpp:54
(Diff revision 2)
>  #include "mozilla/Preferences.h"
>  #include "mozilla/dom/Element.h"
>  #include "mozilla/dom/EventHandlerBinding.h"
>  #include "mozilla/dom/ScriptSettings.h"
> +#include "mozilla/layers/Keyboard.h"
> +#include "mozilla/TextEvents.h"

nit: Could you move this line under "mozilla/Preferences.h"?

::: dom/xbl/nsXBLPrototypeHandler.cpp:144
(Diff revision 2)
>    // We own the next handler in the chain, so delete it now.
>    NS_CONTENT_DELETE_LIST_MEMBER(nsXBLPrototypeHandler, this, mNextHandler);
>  }
>  
> +bool
> +nsXBLPrototypeHandler::TryConvertToKeyboardShortcut(KeyboardShortcut* aOut) const

Still long line. Wrap after "(" and next line should start at below "y" of "Try".

::: dom/xbl/nsXBLPrototypeHandler.cpp:610
(Diff revision 2)
> +  if (mKeyMask & cMeta) {
> +    modifiers |= MODIFIER_META;
> +  }

Shouldn't this be:

if ((mKeyMask & cMeta) && (mKeyMask & cMeta)) {

? You wrote this in the previous patch as:

>   if (mKeyMask & cMetaMask) {
>     modifiersMask |= MODIFIER_META;
>     if (mKeyMask & cMeta) {
>       modifiers |= MODIFIER_META;
>     }
>   }

::: dom/xbl/nsXBLPrototypeHandler.cpp:613
(Diff revision 2)
> +  if (mKeyMask & cOS) {
> +    modifiers |= MODIFIER_OS;
> +  }
> +  if (mKeyMask & cShift) {
> +    modifiers |= MODIFIER_SHIFT;
> +  }
> +  if (mKeyMask & cAlt) {
> +    modifiers |= MODIFIER_ALT;
> +  }
> +  if (mKeyMask & cControl) {
> +    modifiers |= MODIFIER_CONTROL;
> +  }

And also they are so.

::: gfx/layers/apz/src/Keyboard.cpp:91
(Diff revision 2)
> +  // We need to either compare the char code or the key code,
> +  // depending on which one is non-zero
> +  if (mCharCode != 0) {
> +    uint32_t charCode;
> +
> +    // If we are comparing against a shortcut candidate then we might
> +    // have an override char code
> +    if (aOverrideCharCode) {
> +      charCode = aOverrideCharCode;
> +    } else {
> +      charCode = aInput.mCharCode;
> +    }
> +
> +    // Both char codes must be in lowercase to compare correctly
> +    if (IS_IN_BMP(charCode)) {
> +      charCode = ToLowerCase(static_cast<char16_t>(charCode));
> +    }
> +
> +    return mCharCode == charCode;
> +  }
> +
> +  return mKeyCode == aInput.mKeyCode;

I still think that this should be:

if (!mCharCode) {
  return mKeyCode == aInput.mKeyCode;
}

uint32_t charCode;
...
return mCharCode == charCode;

Anyway, even if you keep this style, you should do:

if (mCharCode), don't compare with 0.

Comment 130

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8879031 [details]
Bug 1351783 part 19 - Rename Keyboard.h to KeyboardMap.h.

https://reviewboard.mozilla.org/r/150320/#review155392

Comment 131

[Ryan Hunt [:rhunt]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #129)
> Comment on attachment 8875160 [details]
> Bug 1351783 part 4 - Add a KeyboardShortcut type.
> 
> https://reviewboard.mozilla.org/r/146550/#review155386
> 
> ::: dom/xbl/nsXBLPrototypeHandler.cpp:610
> (Diff revision 2)
> > +  if (mKeyMask & cMeta) {
> > +    modifiers |= MODIFIER_META;
> > +  }
> 
> Shouldn't this be:
> 
> if ((mKeyMask & cMeta) && (mKeyMask & cMeta)) {
> 
> ? You wrote this in the previous patch as:
> 
> >   if (mKeyMask & cMetaMask) {
> >     modifiersMask |= MODIFIER_META;
> >     if (mKeyMask & cMeta) {
> >       modifiers |= MODIFIER_META;
> >     }
> >   }
> 
> ::: dom/xbl/nsXBLPrototypeHandler.cpp:613
> (Diff revision 2)
> > +  if (mKeyMask & cOS) {
> > +    modifiers |= MODIFIER_OS;
> > +  }
> > +  if (mKeyMask & cShift) {
> > +    modifiers |= MODIFIER_SHIFT;
> > +  }
> > +  if (mKeyMask & cAlt) {
> > +    modifiers |= MODIFIER_ALT;
> > +  }
> > +  if (mKeyMask & cControl) {
> > +    modifiers |= MODIFIER_CONTROL;
> > +  }
> 
> And also they are so.
> 

I split apart the gathering of mModifiersMask and mModifiers into independent functions. Then after we calculate them both, we do a

> // Mask away any bits that won't be compared
> modifiers &= modifiersMask

And that gives us the same behavior as before.

> ::: gfx/layers/apz/src/Keyboard.cpp:91
> (Diff revision 2)
> > +  // We need to either compare the char code or the key code,
> > +  // depending on which one is non-zero
> > +  if (mCharCode != 0) {
> > +    uint32_t charCode;
> > +
> > +    // If we are comparing against a shortcut candidate then we might
> > +    // have an override char code
> > +    if (aOverrideCharCode) {
> > +      charCode = aOverrideCharCode;
> > +    } else {
> > +      charCode = aInput.mCharCode;
> > +    }
> > +
> > +    // Both char codes must be in lowercase to compare correctly
> > +    if (IS_IN_BMP(charCode)) {
> > +      charCode = ToLowerCase(static_cast<char16_t>(charCode));
> > +    }
> > +
> > +    return mCharCode == charCode;
> > +  }
> > +
> > +  return mKeyCode == aInput.mKeyCode;
> 
> I still think that this should be:
> 
> if (!mCharCode) {
>   return mKeyCode == aInput.mKeyCode;
> }
> 
> uint32_t charCode;
> ...
> return mCharCode == charCode;
> 
> Anyway, even if you keep this style, you should do:
> 
> if (mCharCode), don't compare with 0.

Ah, that's what you meant by early return. Yes, I think that's better and updated my patchset with it.

Comment 132

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8875157 [details]
Bug 1351783 part 1 - Add includes for unified build issues.

https://reviewboard.mozilla.org/r/146542/#review156304

This time I just reviewed the squashed diff to get a full-picture review, so all the comments are lumped together from all the parts. Sorry about that! Overall things are looking much better, the things below are just minor changes.

::: gfx/layers/apz/src/APZCTreeManager.cpp:187
(Diff revision 3)
> +      mFocusState.ReceiveFocusChangingEvent(mEvent);
> +    } else {
> +      mEvent.mFocusSequenceNumber = mFocusState.LastAPZProcessedEvent();

I feel like you can get rid of the version of ReceiveFocusChangingEvent that takes the input event. Instead you can just do

if (mMayChangeFocus) {
  mFocusState.ReceiveFocusChangingEvent();
}
mEvent.mFocusSequenceNumber = mFocusState.LastAPZProcessedEvent();

Since this is the only call site that uses that version of the function we can drop the function entirely, and remove the FocusState dependency on InputData.h.

::: gfx/layers/apz/src/AsyncPanZoomController.h
(Diff revision 3)
>     * Returns the same transform as GetCurrentAsyncTransform(), but includes
>     * any transform due to axis over-scroll.
>     */
>    AsyncTransformComponentMatrix GetCurrentAsyncTransformWithOverscroll(AsyncMode aMode) const;
>  
> -

nit: spurious line removal (the two blank lines was intentional, as it is the start of a "section" of variables)

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1188
(Diff revision 3)
> +    case KEYBOARD_SCROLL:
>      case OVERSCROLL_ANIMATION:
>        // Should not receive a touch-move in the OVERSCROLL_ANIMATION state
>        // as touch blocks that begin in an overscrolled state cancel the
>        // animation. The same is true for wheel scroll animations.
>        NS_WARNING("Received impossible touch in OnTouchMove");

Note to self: there might be a pre-existing bug here, if the user does a wheel scroll (or now a keyboard scroll) while in the middle of a touch-pan gesture. If my reading of the code is correct we'll switch from the TOUCHING or PANNING state to WHEEL_SCROLL (or KEYBOARD_SCROLL) and then if the user moves their finger, we might very well enter here while in that state and hit the NS_WARNING. This might have been impossible before the input block refactoring in bug 1289432 but I should check to see what happens now.

At any rate, don't worry about it in this bug.

::: gfx/layers/apz/src/FocusState.cpp:13
(Diff revision 3)
> +namespace mozilla {
> +namespace layers {
> +
> +FocusState::FocusState()
> +  : mLastAPZProcessedEvent(1)
> +  , mLastContentProcessedEvent(1)

I think mLastContentProcessedEvent should start at 0 since at the time this FocusState is created it should be considered "not current". Makes sense to start the APZ counter at 1.

::: gfx/layers/apz/src/FocusState.cpp:97
(Diff revision 3)
> +        // Mark what sequence number this target has for debugging purposes so
> +        // we can always accurately report on whether we are stale or not
> +        mLastContentProcessedEvent = target.mSequenceNumber;
> +        return;
> +      }
> +      default: {

s/default/FocusTarget::eSentinel/

::: gfx/layers/apz/src/FocusTarget.cpp:111
(Diff revision 3)
> +  if (IsEditableNode(scrollTarget ? static_cast<nsINode*>(scrollTarget.get())
> +                                  : static_cast<nsINode*>(presShell->GetDocument()))) {

I'd like to avoid the static_casts here. Since you're casting to a base class, I assume you need them for the two branches of the ternary operator to produce the same type. I'd rather use an if/else instead of ?: if it means we can avoid the static_cast.

::: gfx/layers/apz/src/KeyboardMap.cpp:25
(Diff revision 3)
> +      return nsIScrollableFrame::LINES;
> +    case KeyboardScrollAction::SCROLL_PAGE:
> +      return nsIScrollableFrame::PAGES;
> +    case KeyboardScrollAction::COMPLETE_SCROLL:
> +      return nsIScrollableFrame::WHOLE;
> +    default:

s/default/SENTINEL/

::: gfx/layers/ipc/CompositorBridgeParent.cpp:1219
(Diff revision 3)
> -  if (mApzcTreeManager && !aInfo.isRepeatTransaction() && aHitTestUpdate) {
> +  if (mApzcTreeManager) {
> +    mApzcTreeManager->UpdateFocusState(mRootLayerTreeID,
> +                                       mRootLayerTreeID,
> +                                       aInfo.focusTarget());
> +
> +    if (!aInfo.isRepeatTransaction() && aHitTestUpdate) {

I think you want the focus state update to only happen if this is not a repeat transaction. Repeat transactions basically call EndTransaction again without going through the layout/paint code [1]. The way you have the focus state variable passed around right now, the ShadowLayerForwarder clears its copy of the focus state when EndTransaction is called. For a repeat transaction it would then send the "empty" FocusState. So you should ignore it here if it's a repeat transaction otherwise you'll be clobbering a good focus state with a bad one.

[1] http://searchfox.org/mozilla-central/rev/714606a8145636d93b116943d3a65a6a49d2acf8/gfx/layers/client/ClientLayerManager.cpp#436-440

::: gfx/thebes/gfxPlatform.cpp:2612
(Diff revision 3)
> +  if (SupportsApzKeyboardInput()) {
> +    aObj.DefineProperty("ApzKeyboardInput", 1);
> +  }

Let's add && gfxPrefs::AccessibilityBrowseWithCaret() to this condition, since when that is set we always skip the apz keyboard handling.

Originally this code was set up to also report prefs that caused the feature to fail (see [1]) but it was removed in [2]. We could put it back. I don't care much either way, I'll leave it to you.

[1] https://hg.mozilla.org/mozilla-central/rev/16be901108a8
[2] https://hg.mozilla.org/mozilla-central/rev/608d89fa125b1fb9652774ca8192831775637402

::: gfx/thebes/gfxPrefs.h:387
(Diff revision 3)
> +  DECL_GFX_PREF(Live, "general.smoothScroll.lines",            LineSmoothScrollEnabled, bool, true);
> +  DECL_GFX_PREF(Live, "general.smoothScroll.lines.durationMaxMS",
> +                LineSmoothScrollMaxDurationMs, int32_t, 150);
> +  DECL_GFX_PREF(Live, "general.smoothScroll.lines.durationMinMS",
> +                LineSmoothScrollMinDurationMs, int32_t, 150);
> +  DECL_GFX_PREF(Live, "general.smoothScroll.other.durationMaxMS",
> +                OtherSmoothScrollMaxDurationMs, int32_t, 150);
> +  DECL_GFX_PREF(Live, "general.smoothScroll.other.durationMinMS",
> +                OtherSmoothScrollMinDurationMs, int32_t, 150);

Move these up to be in alphabetical order by pref name

::: layout/base/PresShell.h
(Diff revision 3)
> -  // This returns the focused DOM window under our top level window.
> -  //  I.e., when we are deactive, this returns the *last* focused DOM window.
> -  already_AddRefed<nsPIDOMWindowOuter> GetFocusedDOMWindowInOurWindow();

Why did you rename this function (and drop the comment)?

Comment 133

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8875164 [details]
Bug 1351783 part 8 - Gather whether there are key listeners on the focused element.

https://reviewboard.mozilla.org/r/146558/#review156332

Comment 134

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8875166 [details]
Bug 1351783 part 10 - Create and sync the current FocusTarget on each layer transaction.

https://reviewboard.mozilla.org/r/146562/#review156338

Comment 135

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8875169 [details]
Bug 1351783 part 15 - Hook up APZC for scrolling based on a KeyboardScrollAction.

https://reviewboard.mozilla.org/r/146568/#review156340

Comment 136

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8879030 [details]
Bug 1351783 part 18 - Add async keyboard scrolling information to about:support.

https://reviewboard.mozilla.org/r/150318/#review156342

Comment 137

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8875170 [details]
Bug 1351783 part 16 - Perform async scrolling for keyboard events when possible.

https://reviewboard.mozilla.org/r/146570/#review156346

Also waiting on InputQueue changes here, but otherwise looks good.

Comment 138

[Botond Ballo [:botond]]

Comment on attachment 8875159 [details]
Bug 1351783 part 3 - Add a KeyboardScrollAction type.

https://reviewboard.mozilla.org/r/146548/#review156356

::: gfx/layers/apz/src/Keyboard.h:22
(Diff revision 2)
> +struct KeyboardScrollAction final
> +{
> +public:
> +  enum KeyboardScrollActionType : uint8_t
> +  {
> +    SCROLL_CHARACTER,

Drive-by comment: this enum should also use |eScrollCharacter| naming.

Comment 139

[Ryan Hunt [:rhunt]]

Comment on attachment 8875157 [details]
Bug 1351783 part 1 - Add includes for unified build issues.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146544/diff/2-3/

Comment 140

[Ryan Hunt [:rhunt]]

Comment on attachment 8875158 [details]
Bug 1351783 part 2 - Add a KeyboardInput type.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146546/diff/2-3/

Comment 141

[Ryan Hunt [:rhunt]]

Comment on attachment 8875159 [details]
Bug 1351783 part 3 - Add a KeyboardScrollAction type.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146548/diff/2-3/

Comment 142

[Ryan Hunt [:rhunt]]

Comment on attachment 8875160 [details]
Bug 1351783 part 4 - Add a KeyboardShortcut type.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146550/diff/2-3/

Comment 143

[Ryan Hunt [:rhunt]]

Comment on attachment 8875161 [details]
Bug 1351783 part 5 - Add a KeyboardMap type.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146552/diff/2-3/

Comment 144

[Ryan Hunt [:rhunt]]

Comment on attachment 8875162 [details]
Bug 1351783 part 6 - Create and send KeyboardMap to APZCTreeManager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146554/diff/2-3/

Comment 145

[Ryan Hunt [:rhunt]]

Comment on attachment 8875163 [details]
Bug 1351783 part 7 - Create FocusState and FocusTarget types.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146556/diff/2-3/

Comment 146

[Ryan Hunt [:rhunt]]

Comment on attachment 8875164 [details]
Bug 1351783 part 8 - Gather whether there are key listeners on the focused element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146558/diff/3-4/

Comment 147

[Ryan Hunt [:rhunt]]

Comment on attachment 8875165 [details]
Bug 1351783 part 9 - Disable a FocusTarget for an editable element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146560/diff/3-4/

Comment 148

[Ryan Hunt [:rhunt]]

Comment on attachment 8875166 [details]
Bug 1351783 part 10 - Create and sync the current FocusTarget on each layer transaction.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146562/diff/3-4/

Comment 149

[Ryan Hunt [:rhunt]]

Comment on attachment 8875171 [details]
Bug 1351783 part 11 - Sync FocusTarget with WebRenderLayerManager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146572/diff/3-4/

Comment 150

[Ryan Hunt [:rhunt]]

Comment on attachment 8875167 [details]
Bug 1351783 part 12 - Create and sync focus sequence numbers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146564/diff/3-4/

Comment 151

[Ryan Hunt [:rhunt]]

Comment on attachment 8875168 [details]
Bug 1351783 part 13 - Add a function for determing if a ScrollSnapInfo has scroll snap points.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146566/diff/3-4/

Comment 152

[Ryan Hunt [:rhunt]]

Comment on attachment 8879029 [details]
Bug 1351783 part 14 - Create a base class for WheelScrollAnimation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150314/diff/2-3/

Comment 153

[Ryan Hunt [:rhunt]]

Comment on attachment 8875169 [details]
Bug 1351783 part 15 - Hook up APZC for scrolling based on a KeyboardScrollAction.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146568/diff/3-4/

Comment 154

[Ryan Hunt [:rhunt]]

Comment on attachment 8875170 [details]
Bug 1351783 part 16 - Perform async scrolling for keyboard events when possible.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146570/diff/3-4/

Comment 155

[Ryan Hunt [:rhunt]]

Comment on attachment 8875172 [details]
Bug 1351783 part 17 - Do less work when apz.keyboard.enabled is false.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146574/diff/3-4/

Comment 156

[Ryan Hunt [:rhunt]]

Comment on attachment 8879030 [details]
Bug 1351783 part 18 - Add async keyboard scrolling information to about:support.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150318/diff/2-3/

Comment 157

[Ryan Hunt [:rhunt]]

Comment on attachment 8879031 [details]
Bug 1351783 part 19 - Rename Keyboard.h to KeyboardMap.h.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150320/diff/2-3/

Comment 158

[Ryan Hunt [:rhunt]]

I've updated the patchset with a new version that uses input queue. I rebased in order to get kat's InputBlockState refactoring. I should have made a change for all the issues raised unless I missed one.

Comment 159

[Ryan Hunt [:rhunt]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #132)
> Comment on attachment 8875157 [details]
> Bug 1351783 part 1 - Add includes for unified build issues.
> 
> https://reviewboard.mozilla.org/r/146542/#review156304
> 
> ::: layout/base/PresShell.h
> (Diff revision 3)
> > -  // This returns the focused DOM window under our top level window.
> > -  //  I.e., when we are deactive, this returns the *last* focused DOM window.
> > -  already_AddRefed<nsPIDOMWindowOuter> GetFocusedDOMWindowInOurWindow();
> 
> Why did you rename this function (and drop the comment)?

I moved this function and it's comment from PresShell into nsIPresShell, and renamed it to match the function GetRootWindow(), which doesn't specify DOM. Part of the comment got lost on the way, which I fixed. I've also reverted the renaming of the function in this patch, just to make things simpler.

Comment 160

[Ryan Hunt [:rhunt]]

Oh, I just remembered that I forgot to add in InputMethod telemetry. I'll work on that.

Comment 161

[Ryan Hunt [:rhunt]]

Attachment: Bug 1351783 part 20 - Report async keyboard scrolling to telemetry.

Review commit: https://reviewboard.mozilla.org/r/151962/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151962/

Comment 162

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8875170 [details]
Bug 1351783 part 16 - Perform async scrolling for keyboard events when possible.

https://reviewboard.mozilla.org/r/146570/#review157208

Looks good!

Comment 163

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8880627 [details]
Bug 1351783 part 20 - Report async keyboard scrolling to telemetry.

https://reviewboard.mozilla.org/r/151962/#review157212

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1687
(Diff revision 1)
> +    case KeyboardScrollAction::eScrollComplete: {
> +      scrollMethod = ScrollInputMethod::ApzCompleteScroll;
> +      break;
> +    }
> +    case KeyboardScrollAction::eSentinel: {
> +      MOZ_ASSERT_UNREACHABLE("Invalid KeyboardScrollAction.");

add a return statement here

Comment 164

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #132)
> Note to self: there might be a pre-existing bug here, if the user does a
> wheel scroll (or now a keyboard scroll) while in the middle of a touch-pan
> gesture. If my reading of the code is correct we'll switch from the TOUCHING
> or PANNING state to WHEEL_SCROLL (or KEYBOARD_SCROLL) and then if the user
> moves their finger, we might very well enter here while in that state and
> hit the NS_WARNING. This might have been impossible before the input block
> refactoring in bug 1289432 but I should check to see what happens now.
> 
> At any rate, don't worry about it in this bug.

I filed bug 1375854 for this.

Comment 165

[Botond Ballo [:botond]]

Comment on attachment 8875170 [details]
Bug 1351783 part 16 - Perform async scrolling for keyboard events when possible.

https://reviewboard.mozilla.org/r/146570/#review157284

::: gfx/layers/apz/src/AsyncPanZoomController.h:344
(Diff revision 4)
>     * attribute.
>     */
>    bool HasScrollgrab() const { return mScrollMetadata.GetHasScrollgrab(); }
>  
>    /**
> +   * Returns whether this APZC has scroll snapping points.

s/snapping points/snap points

Comment 166

[Botond Ballo [:botond]]

Comment on attachment 8880627 [details]
Bug 1351783 part 20 - Report async keyboard scrolling to telemetry.

https://reviewboard.mozilla.org/r/151962/#review157286

Thanks!

Comment 167

[Botond Ballo [:botond]]

(In reply to Ryan Hunt [:rhunt] from comment #158)
> I should have
> made a change for all the issues raised unless I missed one.

Please don't forget to also file the follow-up bugs that I requested.

Recapping them for convenience:

  - Asserting for input events with an uninitialized sequence number in
    PresShell::HandleEvent

  - Getting scroll snapping to work with async keyboard scrolling

  - Handling keys, like Tab, whose default action can change focus

  - Consider scheduling an empty transaction when content has a new
    FocusTarget to send to APZ

  - Enabling "apz.keyboard.enabled" by default

Comment 168

[Botond Ballo [:botond]]

(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.

(In reply to Botond Ballo [:botond] from comment #58)
> >    NS_ASSERTION(aFrame, "aFrame should be not null");
> >  
> > +  // Update the latest focus sequence number with this new sequence number
> > +  if (mAPZFocusSequenceNumber < aEvent->mFocusSequenceNumber) {
> > +    // XXX should we push a new FocusTarget to APZ here
> 
> In a follow-up bug we should probably make sure an "empty transaction"
> (alluded to in an earlier comment) is sent in this case.

This would basically involve calling nsIFrame::SchedulePaint(nsIFrame::PAINT_COMPOSITE_ONLY), similarly to how the paint-skipping code does it [1]. This is what I was referring to by the fourth follow-up in the previous comment.

[1] http://searchfox.org/mozilla-central/rev/3291398f10dcbe192fb52e74974b172616c018aa/layout/generic/nsGfxScrollFrame.cpp#2876

Comment 169

[Ryan Hunt [:rhunt]]

Try run

https://treeherder.mozilla.org/#/jobs?repo=try&revision=130246dc991fb3aaeaca69dc177a55e53de4ad86

Try run with android build bustage fix

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b3c80453df5a7e56117ddf2a7b287f501f6a407

Comment 170

[Ryan Hunt [:rhunt]]

Flagging David for IPC peer approval for sync message changes.

The first is for a rename of an existing sync message, and the other is for adding a new sync message to PAPZCTreeManager for a new input type.

Comment 171

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8875170 [details]
Bug 1351783 part 16 - Perform async scrolling for keyboard events when possible.

https://reviewboard.mozilla.org/r/146570/#review157498

Comment 172

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8875167 [details]
Bug 1351783 part 12 - Create and sync focus sequence numbers.

https://reviewboard.mozilla.org/r/146564/#review157704

Comment 173

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/81716a06ec30
part 1 - Add includes for unified build issues. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/887a8e96eeda
part 2 - Add a KeyboardInput type. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/b95f62f11e87
part 3 - Add a KeyboardScrollAction type. r=kats,masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f051c7a8b08
part 4 - Add a KeyboardShortcut type. r=kats,masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9cc184d611a
part 5 - Add a KeyboardMap type. r=kats,masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8dfb8a2de00
part 6 - Create and send KeyboardMap to APZCTreeManager. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ad33598aad4
part 7 - Create FocusState and FocusTarget types. r=kats,botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/310bd86a94f5
part 8 - Gather whether there are key listeners on the focused element. r=kats,smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/965a72722c01
part 9 - Disable a FocusTarget for an editable element. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d3d222b4761
part 10 - Create and sync the current FocusTarget on each layer transaction. r=kats,botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/36bfbfdfa4a1
part 11 - Sync FocusTarget with WebRenderLayerManager. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/70136b7cb54b
part 12 - Create and sync focus sequence numbers. r=kats,botond,dvander
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4a493398777
part 13 - Add a function for determing if a ScrollSnapInfo has scroll snap points. r=botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/a63081b75178
part 14 - Create a base class for WheelScrollAnimation. r=botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/f163a2ba051b
part 15 - Hook up APZC for scrolling based on a KeyboardScrollAction. r=kats,botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3110a86cddf
part 16 - Perform async scrolling for keyboard events when possible. r=kats,botond,dvander
https://hg.mozilla.org/integration/mozilla-inbound/rev/e37625d5695e
part 17 - Do less work when apz.keyboard.enabled is false. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/70e30c042f38
part 18 - Add async keyboard scrolling information to about:support. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbe0888e6d97
part 19 - Rename Keyboard.h to KeyboardMap.h. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c3e7dff790a
part 20 - Report async keyboard scrolling to telemetry. r=botond

Comment 174

[Botond Ballo [:botond]]

Moved Quantum Flow tracking information over to bug 1376525.

Comment 175

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/81716a06ec30
https://hg.mozilla.org/mozilla-central/rev/887a8e96eeda
https://hg.mozilla.org/mozilla-central/rev/b95f62f11e87
https://hg.mozilla.org/mozilla-central/rev/2f051c7a8b08
https://hg.mozilla.org/mozilla-central/rev/f9cc184d611a
https://hg.mozilla.org/mozilla-central/rev/c8dfb8a2de00
https://hg.mozilla.org/mozilla-central/rev/5ad33598aad4
https://hg.mozilla.org/mozilla-central/rev/310bd86a94f5
https://hg.mozilla.org/mozilla-central/rev/965a72722c01
https://hg.mozilla.org/mozilla-central/rev/8d3d222b4761
https://hg.mozilla.org/mozilla-central/rev/36bfbfdfa4a1
https://hg.mozilla.org/mozilla-central/rev/70136b7cb54b
https://hg.mozilla.org/mozilla-central/rev/c4a493398777
https://hg.mozilla.org/mozilla-central/rev/a63081b75178
https://hg.mozilla.org/mozilla-central/rev/f163a2ba051b
https://hg.mozilla.org/mozilla-central/rev/c3110a86cddf
https://hg.mozilla.org/mozilla-central/rev/e37625d5695e
https://hg.mozilla.org/mozilla-central/rev/70e30c042f38
https://hg.mozilla.org/mozilla-central/rev/bbe0888e6d97
https://hg.mozilla.org/mozilla-central/rev/6c3e7dff790a

Comment 176

[avada]

(In reply to Ryan Hunt [:rhunt] from comment #128)
> 
> The current approach doesn't send key events to the child process when APZ
> async keyboard scrolls.
> 
> If this proves to have issues, we can switch to sending events to the child
> process with mHandledByAPZ set and use that to not do the default action.

Is this still the case with the landed patches? I foresee annoyances (eg: missing characters from typed text) for those who do things fast and like smooth scrolling.

Comment 177

[Botond Ballo [:botond]]

(In reply to avada from comment #176)
> (In reply to Ryan Hunt [:rhunt] from comment #128)
> > 
> > The current approach doesn't send key events to the child process when APZ
> > async keyboard scrolls.
> > 
> > If this proves to have issues, we can switch to sending events to the child
> > process with mHandledByAPZ set and use that to not do the default action.
> 
> Is this still the case with the landed patches? I foresee annoyances (eg:
> missing characters from typed text) for those who do things fast and like
> smooth scrolling.

It's only the key events that caused the scrolling (e.g. KeyDown) that we don't send to the web content. Other keys, like character keys, are still sent.

Comment 178

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #50)
> As for replacing the union with mozilla::Variant: Variant does not currently
> support IPC, but I filed a mentored bug for adding IPC support for it. 

That was bug 1371846, and it's close to being fixed.

> (We
> can then refactor this to use Variant as a follow-up, probably in another
> mentored bug.)

I filed bug 1383816 for this.

Comment 179

[Julien L.]

I have a problem with this "apz keyboard" : https://bugzilla.mozilla.org/show_bug.cgi?id=1365876  (see the last posts)
My question is : 
how long will this preference be present in about:config : apz.keyboard.enabled  
I must set to false to avoid the bug I mentionned above.
Thanks.

Comment 180

[Ryan Hunt [:rhunt]]

Currently we are trying to enable the pref by default so it won't be going away soon.

It looks like the problem you are experiencing is checker boarding and it applies to all APZ input methods. It's due to scrolling faster than Gecko can paint the web page, and isn't a bug.

There is some discussion on that bug on trying to improve the paint performance which should improve this situation.

Comment 181

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Julien L. from comment #179)
> ...
> apz.keyboard.enabled  
> I must set to false to avoid the bug I mentioned above.

If you set it to false and keep using the same profile, it will stay false through the Firefox updates, so you should be able to keep the original behavior.