chromiumembedded / cef

Chromium Embedded Framework (CEF). A simple framework for embedding Chromium-based browsers in other applications.
https://bitbucket.org/chromiumembedded/cef/
Other
3.21k stars 455 forks source link

Linux: Improve focus handling behavior #3304

Open magreenblatt opened 2 years ago

magreenblatt commented 2 years ago

Original report by Adam D. Ruppe (Bitbucket: adamdruppe, GitHub: adamdruppe).


I got quite frustrated by my input focus (seemingly) randomly jumping back to a CEF app while going somewhere else. Spent all day looking for bugs in my code, even my window manager, and turns out CEF does this deliberately, and I can’t understand why:

https://bitbucket.org/chromiumembedded/cef/src/a3b1dc01ea265e08f3e283270ed69df81fa48733/libcef/browser/native/window_x11.cc#lines-394

       CEF_POST_DELAYED_TASK(CEF_UIT,
                              base::BindOnce(&CefWindowX11::ContinueFocus,
                                             weak_ptr_factory_.GetWeakPtr()),
                              100);

But beyond that, calling XSetInputFocus in any case is questionable. It really should only be done upon direct request by the window manager so you respect the user.

https://bitbucket.org/chromiumembedded/cef/src/a3b1dc01ea265e08f3e283270ed69df81fa48733/libcef/browser/native/window_x11.cc#lines-238

 // Directly ask the X server to give focus to the window. Note that the call
  // would have raised an X error if the window is not mapped.
  connection_
      ->SetInputFocus(
          {x11::InputFocus::Parent, focus_target, x11::Time::CurrentTime})
      .IgnoreError();

I’d suggest simply deleting both these pieces of code unless there’s a very specific reason why they’re there (and even so, this isn’t the right way to do it - Chromium upstream handles these cases with a lot more finesse, It looks like the code here was copy/pasted from it in the past:

https://source.chromium.org/chromium/chromium/src/+/main:ui/ozone/platform/x11/x11_window.cc;l=743?q=SetInputFocus&ss=chromium%2Fchromium%2Fsrc:ui%2F

even there it is extremely user-hostile code (calling RaiseWindow is another crime against user-friendliness), but at least it is just a fallback after trying less bad options.

magreenblatt commented 2 years ago

A PR is welcome if you think that you can improve the current focus handling behavior.

The expected behavior is:

  1. Browser receives focus on navigation.
  2. Browser receives focus on mouse click or other event where the window manager normally assigns focus.
  3. Browser receives focus when requested by web content (via HTMLElement.focus() in JavaScript, for example).

The client can optionally cancel focus assignment by implementing CefFocusHandler::OnSetFocus in certain cases (navigation and system focus).

magreenblatt commented 2 years ago

Original comment by Adam D. Ruppe (Bitbucket: adamdruppe, GitHub: adamdruppe).


I think the easiest path right now might be to simply filter out the PointerNotify FocusIn/Out events. Chromium upstream tries to handle it comprehensively, but even their main thing is to ignore those: https://github.com/chromium/chromium/blob/main/ui/ozone/platform/x11/x11_window.cc#L1964 These events should be filtered out when changing internal state and calling OnSetFocus. If these were filtered, my OnSetFocus just always returning true to cancel the internal behavior is viable to work around the rest of the issues.

A nicer, but more involved, path would be to follow the XEmbed spec: https://specifications.freedesktop.org/xembed-spec/xembed-spec-latest.html

In the cases you outlined, CEF would send XEMBED_REQUEST_FOCUS to the containing window, and then instead of watching for the X FocusIn/FocusOut, it would watch for XEMBED_FOCUS_IN/OUT coming from its parent (if there is no parent passed in the WindowInfo when creating the browser, then it might assume it is top level, though again, ideally it’d still listen for the embedding lifecycle events specified there. This puts the parent window in control driving things, and notice how that spec includes cases like tabbing the focus through the whole application.

Generally speaking though, it is very rare that you should actually call XSetInputFocus, since the window manager is responsible for that 99% of the time. (see https://tronche.com/gui/x/icccm/sec-4.html section 4.1.7. Input Focus).

magreenblatt commented 2 years ago