Fix RenderWidgetHostViewAura last_pointer_type_ for mouse events [chromium/src : main]
Olga Gerchikov (Gerrit)
Olga Gerchikov added 2 comments![]( "Open in Gerrit")
last_pointer_type_ = event->pointer_details().pointer_type;Can you please confirm that VK is not displayed if Shell Handwriting is enabled and user does handwriting?
last_pointer_type_ = event->pointer_details().pointer_type;Just to be safe, should this change be gated behind a feature flag?
Related details
- Gaston Rodriguez
- John An
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
John An (Gerrit)
John An added 2 comments![]( "Open in Gerrit")
last_pointer_type_ = event->pointer_details().pointer_type;Can you please confirm that VK is not displayed if Shell Handwriting is enabled and user does handwriting?
It won't be displayed for writing specifically. But for short strokes and taps it will. The VK is displayed for gesture short press and long press: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/frame/web_frame_widget_impl.cc;drc=ee3cb7b7f68c3076293570f3b74d7f978bb6b899;l=4274
Fixing this bug does mean the VK will be displayed more frequently especially for tablet/keyboardless devices. This can be disabled in OS settings. This brings us into alignment with Android, but may warrant a separate discussion about desired behavior.
last_pointer_type_ = event->pointer_details().pointer_type;Just to be safe, should this change be gated behind a feature flag?
- Gaston Rodriguez
- Olga Gerchikov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
John An (Gerrit)
John An added 1 comment![]( "Open in Gerrit")
Related details
- Gaston Rodriguez
- Olga Gerchikov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olga Gerchikov (Gerrit)
Olga Gerchikov voted Code-Review+1![]( "Open in Gerrit")
- Gaston Rodriguez
- John An
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gaston Rodriguez (Gerrit)
Gaston Rodriguez added 2 comments![]( "Open in Gerrit")
last_pointer_type_ =
base::FeatureList::IsEnabled(features::kPenHoverPointerType)
? event->pointer_details().pointer_type
: ui::EventPointerType::kMouse;I quickly tested on Canary on my personal laptop (linux mint), and it seems like pen hovers just straight up move your mouse (there is no other pointer type like when you hover with a pen on Windows and it shows a little dot).
With this in mind, I suggest making the feature flag visible to both Win and Linux. I don't think letting Linux see the feature flag will do any harm, it will make the code easier to read and if there is some Linux desktop environment that implements better pen handling, they'll get the fix too.
#endifnit (and features.h):
```suggestion
#endif // BUILDFLAG(IS_WIN)
```
Related details
- John An
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gaston Rodriguez (Gerrit)
Gaston Rodriguez voted and added 1 comment![]( "Open in Gerrit")
Votes added by Gaston Rodriguez
| Code-Review | +1 |
1 comment
last_pointer_type_ =
base::FeatureList::IsEnabled(features::kPenHoverPointerType)
? event->pointer_details().pointer_type
: ui::EventPointerType::kMouse;I quickly tested on Canary on my personal laptop (linux mint), and it seems like pen hovers just straight up move your mouse (there is no other pointer type like when you hover with a pen on Windows and it shows a little dot).
With this in mind, I suggest making the feature flag visible to both Win and Linux. I don't think letting Linux see the feature flag will do any harm, it will make the code easier to read and if there is some Linux desktop environment that implements better pen handling, they'll get the fix too.
I just double checked and it seems like VirtualKeyboard is not implemented on Linux (x11 or wayland), so this suggestion is moot. Perhaps consider adding a comment explaining why Windows differs from Linux.
Related details
- John An
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
John An (Gerrit)
John An added 2 comments![]( "Open in Gerrit")
last_pointer_type_ =
base::FeatureList::IsEnabled(features::kPenHoverPointerType)
? event->pointer_details().pointer_type
: ui::EventPointerType::kMouse;Gaston RodriguezI quickly tested on Canary on my personal laptop (linux mint), and it seems like pen hovers just straight up move your mouse (there is no other pointer type like when you hover with a pen on Windows and it shows a little dot).
With this in mind, I suggest making the feature flag visible to both Win and Linux. I don't think letting Linux see the feature flag will do any harm, it will make the code easier to read and if there is some Linux desktop environment that implements better pen handling, they'll get the fix too.
I just double checked and it seems like VirtualKeyboard is not implemented on Linux (x11 or wayland), so this suggestion is moot. Perhaps consider adding a comment explaining why Windows differs from Linux.
It's probably righteous to change this on Linux as well.
nit (and features.h):
```suggestion
#endif // BUILDFLAG(IS_WIN)
```
- Gaston Rodriguez
- Olga Gerchikov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
John An (Gerrit)
John An voted and added 2 comments![]( "Open in Gerrit")
Votes added by John An
| Commit-Queue | +1 |
2 comments
Hi folks, PTAL or suggest a different owner. Appreciated!
Please fix this WARNING reported by ClangTidy: warning: building this file or its dependencies failed; no diagnostics will be i...
warning: building this file or its dependencies failed; no diagnostics will be issued. When diagnosing, it's normal that the clang-tidy step is green. You need to click through the step output to find the actual error.
(Lint observed on `linux-clang-tidy-rel`, but not on `android-clang-tidy-rel`)
Related details
- Dave Tapuska
- Gaston Rodriguez
- Nasko Oskov
- Olga Gerchikov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave Tapuska (Gerrit)
Dave Tapuska added 1 comment![]( "Open in Gerrit")
// With kMouseEventPenPointerType enabled, genuine mouse events (not pen) mustSince these two tests are essentially the same. Can we not put them in an outher loop
ie:
```
for (ui::EventPointerType type : [ ui::EventPointerType::kPen, ui::EventType::kMouse] {
```
Related details
- Gaston Rodriguez
- John An
- Nasko Oskov
- Olga Gerchikov
Nasko Oskov (Gerrit)
Nasko Oskov added 1 comment![]( "Open in Gerrit")
dtapuska@ is an OWNER of all files already and better suited reviewer, so removing myself.
Related details
- Gaston Rodriguez
- John An
- Olga Gerchikov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
John An (Gerrit)
John An added 1 comment![]( "Open in Gerrit")
// With kMouseEventPenPointerType enabled, genuine mouse events (not pen) mustSince these two tests are essentially the same. Can we not put them in an outher loop
ie:
```
for (ui::EventPointerType type : [ ui::EventPointerType::kPen, ui::EventType::kMouse] {
```
- Dave Tapuska
- Gaston Rodriguez
- Olga Gerchikov
John An (Gerrit)
John An added 1 comment![]( "Open in Gerrit")
Please fix this WARNING reported by ClangTidy: warning: building this file or its dependencies failed; no diagnostics will be i...
warning: building this file or its dependencies failed; no diagnostics will be issued. When diagnosing, it's normal that the clang-tidy step is green. You need to click through the step output to find the actual error.
(Lint observed on `linux-clang-tidy-rel`, but not on `android-clang-tidy-rel`)
Ended up being a non issue
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave Tapuska (Gerrit)
Dave Tapuska voted Code-Review+1![]( "Open in Gerrit")
- John An
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
John An (Gerrit)
John An added 1 comment![]( "Open in Gerrit")
Could I get another sign off for the later changes? I have owner approval, I need one more.
Related details
- Gaston Rodriguez
- Olga Gerchikov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gaston Rodriguez (Gerrit)
Gaston Rodriguez voted and added 3 comments![]( "Open in Gerrit")
Votes added by Gaston Rodriguez
| Code-Review | +1 |
3 comments
ping me on teams if you do any of my suggested changes and need another review.
last_pointer_type_ to kMouse. However, on Windows, OnMouseEvent can benit:
```suggestion
last_pointer_type_ to kMouse. However, OnMouseEvent can be
```
for (ui::EventPointerType pointer_type :You may have already considered this, but I'd suggest breaking these loops into testing params.
Here we have 2 * 6 = 12 assertions. If one of them fails, we will lose coverage on all other eleven. I think the `SCOPED_TRACE` helps with messaging to see if any tests are failing, but I think that breaking all the assertions into their own tests would also help with reading logs.
You can do a cross product of the params with gtest, the test would look something like (writing from memory, not apt for copypasting):
```
class RenderWidgetHostViewAuraTestLastPointerType : testingWithParams<ui::EventPointerType, ui::EventType> {
void SetUp() override {
scoped_feature_list_.InitAndEnableFeature(features::kMouseEventPenPointerType);
}
base::test::ScopedFeatureList scoped_feature_list_;
}
TEST_P(RenderWidgetHostViewAuraTestLastPointerType , PenMouseEventsSetPointerType) {
auto pointer_type = GetParam<0>();
auto event_type = GetParam<1>();
ui::MouseEvent mouse_event(
type, gfx::Point(10, 10), gfx::Point(10, 10), ui::EventTimeForNow(),
ui::EF_LEFT_MOUSE_BUTTON, ui::EF_LEFT_MOUSE_BUTTON,
ui::PointerDetails(pointer_type, 0));
parent_view_->OnMouseEvent(&mouse_event);
EXPECT_EQ(parent_view_->GetLastPointerType(), pointer_type);
}
INSTANTIATE_TEST_SUITE_P("All",
RenderWidgetHostViewAuraTestLastPointerType,
testing::Combine(
testing::Values({ui::EventPointerType::kPen, ui::EventPointerType::kMouse},
testing::Values({ui::EventType::kMouseMoved, ui::EventType::kMousePressed,
ui::EventType::kMouseDragged, ui::EventType::kMouseReleased,
ui::EventType::kMouseEntered, ui::EventType::kMouseExited}))
)
);
```
Maybe it's more code than one simple test is worth. Up to you
Related details
- John An
- Olga Gerchikov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
John An (Gerrit)
John An added 2 comments![]( "Open in Gerrit")
last_pointer_type_ to kMouse. However, on Windows, OnMouseEvent can benit:
```suggestion
last_pointer_type_ to kMouse. However, OnMouseEvent can be
```
Done
This is not an issue because EXPECT_EQ does not abort the function upon failure, ASSERT_EQ would. https://google.github.io/googletest/primer.html
Related details
- Olga Gerchikov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
John An (Gerrit)
John An voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Unreviewed changes
6 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
Fix RenderWidgetHostViewAura last_pointer_type_ for mouse events
Currently RenderWidgetHostViewAura::OnMouseEvent unconditionally sets
last_pointer_type_ to kMouse. However OnMouseEvent can be triggered by
stylus hover. Namely, this can break showing virtual keyboard on
windows if last_pointer_type_ is kMouse when
VirtualKeyboardControllerWin::UpdateTextInputState is called. This
change makes it so last_pointer_type_ is recorded accurately for
non-mouse events.
- M content/browser/renderer_host/render_widget_host_view_aura.cc
- M content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
- M content/common/features.cc
- M content/common/features.h
Code-Review: +1 by Dave Tapuska, +1 by Gaston Rodriguez
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |