Trigger mouse events for mouse hover over inactive page [chromium/src : main]
Gaston Rodriguez (Gerrit)
Gaston Rodriguez added 6 comments![]( "Open in Gerrit")
mouse hovering over an inactive page that experiences scroll. This```suggestion
mouse hovering over an inactive page that experiences scrolling. This
```
I think.
the page is inactive. This affects anchor element interaction tracking
so this change also ensures pointer hover is only tracked for active
pages.Could you clarify how the anchor element is affected, and why the changes to it were needed?
TEST_F(EventHandlerSimTest, GestureTapHoverState) {
ResizeView(gfx::Size(800, 600));
if (!RuntimeEnabledFeatures::SyntheticMouseHoverOverInactivePageEnabled()) {
// RecomputeMouseHoverState() bails early if we are not focused.
GetPage().SetFocused(true);
}Would it be worth it to parametrize this test (and the others with a similar check) so that they enable or disable the feature, and test that the hovers are sent properly when the focus is false?
Something like:
```suggestion
TEST_P(EventHandlerSimTestBoolParam, GestureTapHoverState) {
ScopedSyntheticMouseHoverOverInactivePageForTest feature_flag_name(GetParam());
ResizeView(gfx::Size(800, 600));
// Having the synthetic hover feature flag enabled makes it so that we don't need the page to be focused to receive synthetic hover events. If the feature is disabled, we need the flag to avoid an early exit.
GetPage().SetFocused(!RuntimeEnabledFeatures::SyntheticMouseHoverOverInactivePageEnabled()); // or SetFocused(!GetParam())
```
This way we have more test coverage.
#include "third_party/blink/public/platform/web_runtime_features.h"Could this be why you were having trouble with the runtime feature flag? This is the `#includes` you should be using:
`#include "third_party/blink/renderer/platform/runtime_enabled_features.h"`
Same for all other instances of this `#includes`.
Page* page = GetDocument()->GetFrame()->GetPage();
if (event_type == event_type_names::kPointerover &&
(!RuntimeEnabledFeatures::SyntheticMouseHoverOverInactivePageEnabled() ||
(page && page->GetFocusController().IsActive()))) {```suggestion
if (event_type == event_type_names::kPointerover) {
if (!RuntimeEnabledFeatures::SyntheticMouseHoverOverInactivePageEnabled() ||
(Page* page = GetDocument()->GetFrame()->GetPage(); page && page->GetFocusController().IsActive())) {
```
Just a suggestion, feel free to ignore if you think it's uglier.
(page && page->GetFocusController().IsActive()))) {Here you're adding a new scenario where you're skipping this if. Previously, if the `== khover` passed then this was true, but now even if `kHover == true` but the feature is enabled and the page is not active then the message will not be sent. I'm not very clear why this was needed, could you add a comment? I'll also ask for clarification on the cl's description.
Related details
- Claire Chambers
- 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. |
Claire Chambers (Gerrit)
Claire Chambers added 2 comments![]( "Open in Gerrit")
TEST_F(EventHandlerSimTest, GestureTapHoverState) {
ResizeView(gfx::Size(800, 600));
if (!RuntimeEnabledFeatures::SyntheticMouseHoverOverInactivePageEnabled()) {
// RecomputeMouseHoverState() bails early if we are not focused.
GetPage().SetFocused(true);
}Would it be worth it to parametrize this test (and the others with a similar check) so that they enable or disable the feature, and test that the hovers are sent properly when the focus is false?
Something like:```suggestion
TEST_P(EventHandlerSimTestBoolParam, GestureTapHoverState) {
ScopedSyntheticMouseHoverOverInactivePageForTest feature_flag_name(GetParam());
ResizeView(gfx::Size(800, 600));// Having the synthetic hover feature flag enabled makes it so that we don't need the page to be focused to receive synthetic hover events. If the feature is disabled, we need the flag to avoid an early exit.
GetPage().SetFocused(!RuntimeEnabledFeatures::SyntheticMouseHoverOverInactivePageEnabled()); // or SetFocused(!GetParam())
```This way we have more test coverage.
+1 on this. If you have a test with feature-dependent expectations it should be tested with feature on and off, so that changing the feature state doesn't suddenly cause breaks.
// RecomputeMouseHoverState() bails early if we are not focused.Can this happen in the full browser? If so, does your change alter the behavior of focus events?
Related details
- John An
- Olga Gerchikov
Claire Chambers (Gerrit)
Claire Chambers added 1 comment![]( "Open in Gerrit")
// RecomputeMouseHoverState() bails early if we are not focused.Can this happen in the full browser? If so, does your change alter the behavior of focus events?
Closing per offline discussion.
Claire Chambers (Gerrit)
Claire Chambers added 1 comment![]( "Open in Gerrit")
(page && page->GetFocusController().IsActive()))) {Here you're adding a new scenario where you're skipping this if. Previously, if the `== khover` passed then this was true, but now even if `kHover == true` but the feature is enabled and the page is not active then the message will not be sent. I'm not very clear why this was needed, could you add a comment? I'll also ask for clarification on the cl's description.
Now that I understand this better - I think this should be a CRBug. It's fairly low-pri, basically as I understand it, the scenario here is preload heuristics don't/won't work if the user is mousing over and scrolling over an out-of-focus web page. This is not a functional issue, and it's not a regression (didn't work before this CL, still won't work after), so it shouldn't block this CL, but it's worth tracking so that the OWNERS of this feature may fix at some point.
Claire Chambers (Gerrit)
Claire Chambers added 1 comment![]( "Open in Gerrit")
if (!RuntimeEnabledFeatures::SyntheticMouseHoverOverInactivePageEnabled()) {
// RecomputeMouseHoverState() bails early if we are not focused.This is a bit awkward and hard to understand if you don't have this crrev up. I would change this comment to outline that the need for focus was experimentally fixed, link to the CRBug, and explain that this check only exists as a fallback in case this fix needs to be turned off.
John An (Gerrit)
John An added 7 comments![]( "Open in Gerrit")
mouse hovering over an inactive page that experiences scroll. This```suggestion
mouse hovering over an inactive page that experiences scrolling. This
```
I think.
Done
the page is inactive. This affects anchor element interaction tracking
so this change also ensures pointer hover is only tracked for active
pages.Could you clarify how the anchor element is affected, and why the changes to it were needed?
Done
TEST_F(EventHandlerSimTest, GestureTapHoverState) {
ResizeView(gfx::Size(800, 600));
if (!RuntimeEnabledFeatures::SyntheticMouseHoverOverInactivePageEnabled()) {
// RecomputeMouseHoverState() bails early if we are not focused.
GetPage().SetFocused(true);
}Claire ChambersWould it be worth it to parametrize this test (and the others with a similar check) so that they enable or disable the feature, and test that the hovers are sent properly when the focus is false?
Something like:```suggestion
TEST_P(EventHandlerSimTestBoolParam, GestureTapHoverState) {
ScopedSyntheticMouseHoverOverInactivePageForTest feature_flag_name(GetParam());
ResizeView(gfx::Size(800, 600));// Having the synthetic hover feature flag enabled makes it so that we don't need the page to be focused to receive synthetic hover events. If the feature is disabled, we need the flag to avoid an early exit.
GetPage().SetFocused(!RuntimeEnabledFeatures::SyntheticMouseHoverOverInactivePageEnabled()); // or SetFocused(!GetParam())
```This way we have more test coverage.
+1 on this. If you have a test with feature-dependent expectations it should be tested with feature on and off, so that changing the feature state doesn't suddenly cause breaks.
To me this is all calling out the fact that there is no explicit test for the new behavior. Mouse hover should fire for both active and inactive pages. I will add that and let that be the unit test signal instead of parameterizing every test that uses this feature flag.
if (!RuntimeEnabledFeatures::SyntheticMouseHoverOverInactivePageEnabled()) {
// RecomputeMouseHoverState() bails early if we are not focused.This is a bit awkward and hard to understand if you don't have this crrev up. I would change this comment to outline that the need for focus was experimentally fixed, link to the CRBug, and explain that this check only exists as a fallback in case this fix needs to be turned off.
Done
#include "third_party/blink/public/platform/web_runtime_features.h"Could this be why you were having trouble with the runtime feature flag? This is the `#includes` you should be using:
`#include "third_party/blink/renderer/platform/runtime_enabled_features.h"`
Same for all other instances of this `#includes`.
Ahh that's gotta be it. Thanks
Page* page = GetDocument()->GetFrame()->GetPage();
if (event_type == event_type_names::kPointerover &&
(!RuntimeEnabledFeatures::SyntheticMouseHoverOverInactivePageEnabled() ||
(page && page->GetFocusController().IsActive()))) {```suggestion
if (event_type == event_type_names::kPointerover) {
if (!RuntimeEnabledFeatures::SyntheticMouseHoverOverInactivePageEnabled() ||
(Page* page = GetDocument()->GetFrame()->GetPage(); page && page->GetFocusController().IsActive())) {
```Just a suggestion, feel free to ignore if you think it's uglier.
I thought about that approach but 1. I like having the conditions joined w && and 2. I think initializing page in the conditional is too ugly
(page && page->GetFocusController().IsActive()))) {Claire ChambersHere you're adding a new scenario where you're skipping this if. Previously, if the `== khover` passed then this was true, but now even if `kHover == true` but the feature is enabled and the page is not active then the message will not be sent. I'm not very clear why this was needed, could you add a comment? I'll also ask for clarification on the cl's description.
Now that I understand this better - I think this should be a CRBug. It's fairly low-pri, basically as I understand it, the scenario here is preload heuristics don't/won't work if the user is mousing over and scrolling over an out-of-focus web page. This is not a functional issue, and it's not a regression (didn't work before this CL, still won't work after), so it shouldn't block this CL, but it's worth tracking so that the OWNERS of this feature may fix at some point.
Described this in the CL description and in a new comment. Claire, I'm not convinced this is a bug. It may be the case that we only want to track interactions for anchor elements on an active page. I'd be curious to know what the owners think.
Related details
- Claire Chambers
- 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. |
Claire Chambers (Gerrit)
Claire Chambers added 6 comments![]( "Open in Gerrit")
TEST_F(EventHandlerSimTest, GestureTapHoverState) {
ResizeView(gfx::Size(800, 600));
if (!RuntimeEnabledFeatures::SyntheticMouseHoverOverInactivePageEnabled()) {
// RecomputeMouseHoverState() bails early if we are not focused.
GetPage().SetFocused(true);
}Claire ChambersWould it be worth it to parametrize this test (and the others with a similar check) so that they enable or disable the feature, and test that the hovers are sent properly when the focus is false?
Something like:```suggestion
TEST_P(EventHandlerSimTestBoolParam, GestureTapHoverState) {
ScopedSyntheticMouseHoverOverInactivePageForTest feature_flag_name(GetParam());
ResizeView(gfx::Size(800, 600));// Having the synthetic hover feature flag enabled makes it so that we don't need the page to be focused to receive synthetic hover events. If the feature is disabled, we need the flag to avoid an early exit.
GetPage().SetFocused(!RuntimeEnabledFeatures::SyntheticMouseHoverOverInactivePageEnabled()); // or SetFocused(!GetParam())
```This way we have more test coverage.
John An+1 on this. If you have a test with feature-dependent expectations it should be tested with feature on and off, so that changing the feature state doesn't suddenly cause breaks.
To me this is all calling out the fact that there is no explicit test for the new behavior. Mouse hover should fire for both active and inactive pages. I will add that and let that be the unit test signal instead of parameterizing every test that uses this feature flag.
Based on offline discussion and thinking it over, I reverse my +1. I think @jo...@microsoft.com is right and that a parameterized suite adds tech debt but doesn't guard against regression.
if (!frame_->GetPage() ||
(!RuntimeEnabledFeatures::SyntheticMouseHoverOverInactivePageEnabled() &&
!frame_->GetPage()->GetFocusController().IsActive())) {
return;
}
nit: triple negative is hard to understand. Here's a more readable version using De Morgan's law. Feel free to disregard.
```suggestion
const bool page_available_for_hover = frame_->GetPage() &&
(RuntimeEnabledFeatures::SyntheticMouseHoverOverInactivePageEnabled() ||
frame_->GetPage()->GetFocusController().IsActive())
if (!page_available_for_hover) {
return;
}
```
INSTANTIATE_TEST_SUITE_P(All, MouseEventManagerHoverTest, ::testing::Bool());The more I think about this, the more I think maybe I/we were wrong to harang you about this. I think I saw all those feature-conditional behaviors and went 'whoa!' and jumped to documenting / cleaning it up. I apologize for creating unnecessary work and hope you'll forgive my excessive conscientiousness.
You could probably remove this. If this change turns out to have major ramifications, we disable with experiment platforms (ecs in edge, finch for cr). For backdrop-filter work for example, we never wrote tests for a killswitch-off.
Worst case CL is setting this to `status: experimental` in code. Tests will still pass. This 'feature' will have to exist in some form because it's the CSS spec, and if it's creates a problem, it just means the problem needs to be fixed, rather than the 'feature' being abandoned wholesale. In this case *all* your feature-specific conditionals in test code could be removed, leaving only your production code intact.
If other reviewers disagree, or you do, just be sure to add a comment so this can be cleaned up later, as you did for the other files :)
```suggestion
// See crbug.com/385474535. We maintain coverage for both behaviors in
// case this experimental fix needs to be disabled. This will be un-
// -parameterized in a future CL.
INSTANTIATE_TEST_SUITE_P(All, MouseEventManagerHoverTest, ::testing::Bool());
```
EXPECT_EQ("rgb(255, 0, 0)", hover_color.SerializeAsCSSColor());```suggestion
// With the page focused, :hover pseudo-class should match when the
// pointer is over the element.
EXPECT_EQ("rgb(255, 0, 0)", hover_color.SerializeAsCSSColor());
```
EXPECT_EQ(GetParam() ? "rgb(255, 0, 0)" : "rgb(128, 128, 128)",
hover_color.SerializeAsCSSColor());```suggestion
// Same behavior is expected even when the page is not focused, Per
// W3C UI Events, § 3.4.5.6 “mouseenter”
EXPECT_EQ(GetParam() ? "rgb(255, 0, 0)" : "rgb(128, 128, 128)",
hover_color.SerializeAsCSSColor());
```
(page && page->GetFocusController().IsActive()))) {Claire ChambersHere you're adding a new scenario where you're skipping this if. Previously, if the `== khover` passed then this was true, but now even if `kHover == true` but the feature is enabled and the page is not active then the message will not be sent. I'm not very clear why this was needed, could you add a comment? I'll also ask for clarification on the cl's description.
John AnNow that I understand this better - I think this should be a CRBug. It's fairly low-pri, basically as I understand it, the scenario here is preload heuristics don't/won't work if the user is mousing over and scrolling over an out-of-focus web page. This is not a functional issue, and it's not a regression (didn't work before this CL, still won't work after), so it shouldn't block this CL, but it's worth tracking so that the OWNERS of this feature may fix at some point.
Described this in the CL description and in a new comment. Claire, I'm not convinced this is a bug. It may be the case that we only want to track interactions for anchor elements on an active page. I'd be curious to know what the owners think.
If you do not file CRBug, please add to review someone who actively works on the feature/area, not just an OWNERS (chromium OWNERS is intentionally broad). Neither of us know the answer, which is why it's important the question itself isn't lost.
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. |
Claire Chambers (Gerrit)
Claire Chambers added 1 comment![]( "Open in Gerrit")
(page && page->GetFocusController().IsActive()))) {Claire ChambersHere you're adding a new scenario where you're skipping this if. Previously, if the `== khover` passed then this was true, but now even if `kHover == true` but the feature is enabled and the page is not active then the message will not be sent. I'm not very clear why this was needed, could you add a comment? I'll also ask for clarification on the cl's description.
John AnNow that I understand this better - I think this should be a CRBug. It's fairly low-pri, basically as I understand it, the scenario here is preload heuristics don't/won't work if the user is mousing over and scrolling over an out-of-focus web page. This is not a functional issue, and it's not a regression (didn't work before this CL, still won't work after), so it shouldn't block this CL, but it's worth tracking so that the OWNERS of this feature may fix at some point.
Claire ChambersDescribed this in the CL description and in a new comment. Claire, I'm not convinced this is a bug. It may be the case that we only want to track interactions for anchor elements on an active page. I'd be curious to know what the owners think.
If you do not file CRBug, please add to review someone who actively works on the feature/area, not just an OWNERS (chromium OWNERS is intentionally broad). Neither of us know the answer, which is why it's important the question itself isn't lost.
Thinking about it more, I'm not sure how it couldn't be a bug.
I can test the anchor element interaction tracker right now and I get hits when the page isn't focused, using regular mouse hover (ie, non scrolls). Your change would regress this. The spec (https://html.spec.whatwg.org/multipage/speculative-loading.html#speculative-loading) is silent on the issue of focus, there's nothing indicating it shouldn't behave this way. Intuitively, it's not clear to me why tabbing out of my browser and hovering over things should have different prefetch, but that's just an opinion.
The reason why 4 unit tests break in this file isn't because all nonfocused pointerover events are unexpected, but rather because the GSE at the end of `DispatchPointerDownAndVerticalScroll` causes a single extra pointerover from your code that the tests do not expect. This to me seems more like a test expectation issue. Filtering ALL non-focused pointerover events is taking a sledgehammer to the problem.
wpt_internal/speculation-rules/prefetch/no-vary-search/prefetch-single-non-immediate-with-hint.https.html also fails with your change to the anchor interaction tracker.
My opinion:
- Fix the expectations (complicated), or..
- Add a scoped feature flag to disable your feature in the offending tests, then file an associated CRBug, or..
- Ask the people currently working on this feature to weigh in. I would recommend looking at this document: https://docs.google.com/document/d/1YPbtUPfZIDElzBZNx8IQMzRFvy8oauLG_i1XIr6jgTs/edit?tab=t.0#heading=h.ndgyftleqzya for collaborators, as they are all involved in the active improvement of this area and could likely answer any questions.