Add special handling for keyboard triggered clicks [chromium/src : main]
Hao Liu (Gerrit)
Hao Liu voted and added 11 comments![]( "Open in Gerrit")
Votes added by Hao Liu
| Commit-Queue | +1 |
11 comments
Add special handling for keyboard simulated clickHao LiuNit: Maybe remove the term "simulated" here, and elsewhere in this patch.
Technically all clicks events are synthetic and created from mouse/pointer/keyboard hardware events, so these are just a different type of synthetic event, I think. There may be differences about where/how the gesture is created but "simulated" sounds almost like its just for lab tests.
Done
Currently if a checkbox is checked by a space key, the click event hasHao LiuNit: no longer just checkboxes. Perhaps just mention:
"input elements (such as buttons, checkboxes, radio), as well as links, etc"
Done
struct KeycodeInfo {Hao LiuCan this be private?
Also-- with your other patch to move data into PerformanceEventTiming, would we have this information if we just saved the whole last_keydown_event_timing_entry_ instead?
Or would we have flushed keydown and reset those values by the time we need them here?
discussed offline.
that moving data into PerformanceEventTiming is in WindowPerformance class. We eventually want to move logic to this ResponsivenessMetrics class. We'll do the merging of these data fields by that time.
RuntimeEnabledFeaturesBase::Hao LiuWhy are these two features detected differently?
I think the existing the `base::Features` that the existing `kEventTimingKeypressAndCompositionInteractionId` uses is needed when the feature is in both Chromium and Blink side.
But our feature is only in Blink side. So a `RuntimeEnaledFeatureBase` should suffice. see https://chromium.googlesource.com/chromium/src/third_party/+/HEAD/blink/renderer/platform/RuntimeEnabledFeatures.md#relationship-between-a-chromium-feature-and-a-blink-feature
entry->SetInteractionIdAndOffset(GetCurrentInteractionId(),Hao LiuI know this isn't part of your patch, but, is this expected to be equal to `last_keydown_keycode_info_->interactionId` here, or is this supposed to be a new interactionID?
If so, would it be better to just read that value directly?
Michal MocnyThis `current_interaction_id_for_event_timing_` (the value of GetCurrentInteractionId()) is also being updated by pointer event.
I think here it assumes that no pointer event occurs between the `keydown` event and `keypress` event, which probably is true.
But for keyboard simulated clicks, I'm not sure if that's true.
Hao LiuI think that we are trying to map the keypress to a keydown and re-use its interactionID, and we can just do that directly.
At first I thought this might differ for composition keypress events, but I see that this is only for the kNonComposition state.
Done
// be affected. But we still should report it.Hao LiuFor what its worth, I asked Google Gemini to list all the ways you might be able to simulate clicks and I was curious if ALL of them would be necessarily nested inside key events.
Here was the reply:
Interactive Elements:
- `<button>`: Buttons are designed for user interaction and respond to both Enter and Spacebar.
- `<a>` (with href attribute): Links with a destination (href) act like buttons and respond to Enter.
- `<input type="button">, <input type="submit">, <input type="reset">`: These input elements behave like buttons, responding to both Enter and Spacebar.
- `<input type="checkbox">, <input type="radio">`: These input elements respond to Spacebar to toggle their state.
- `<select>`: Dropdown menus respond to Enter and Spacebar to open/close the options list.
- `<details>/<summary>`: These elements allow for expandable content sections. The <summary> element responds to Enter and Spacebar to toggle the visibility of the <details> content.
Other Elements with Specific Attributes:
- Any element with the tabindex attribute: This attribute makes the element focusable. Once focused, elements with tabindex="0" respond to Enter, and elements with tabindex="-1" can be focused programmatically and respond to Enter when focused.
- Elements with the role="button" attribute: This ARIA role tells assistive technologies the element should be treated like a button, making it respond to Enter and Spacebar.
----If any of those DONT include duration nested within then the priority of this TODO changes. I still think its fine to followup.
I see. Created a bug for this. crbug.com/355605691
if (last_keydown_keycode_info_.has_value()) {Hao LiuNit: reverse these. If the false case happens, add to uma and `return false` early.
Then the true case just moves out of the if entirely as becomes the main body of the function.
updated.
HeapVector<Member<EventData>> events_data_;Hao LiuAfter all the changes to the algorithms, is it still necessary to switch to from deque to vector?
I think we can't do at a specific positition with `Deque`. That is, we can't insert EventData in order of processingStart with Deque.
processing_start >= events_data_.back()->GetProcessingStart()) {Hao LiuDo we need this special case?
The cost of `find_if_not()` and `InsertAt` when only the last item is replaced is O(1) and likely not so much more.
And, `find_if_not()` (and reverse iterator `base()`) should "just work" for an empty range, I think.
Initially I used this special handling as I was unsure of the behavior of the reverse iterator on an empty vector. But as you brought this up, it seems it works as no tests fail.
So I updated.
for (auto it = events_data_.begin(); it != iter; it = std::next(it)) {Hao LiuI don't mind this, but I think you could also use std::for_each() with a `[&]` lambda.
(In the future you should be able to do `for (auto item : std::ranges::subrange(start, end))` but I think not yet in chromium)
I see.
I initially thought the lambda object creation and invocation would create some overhead that might not be necessary in this case as we are calling the function synchronously. But I looked it up, it seems the compiler would optimize this so there's no performance issue.
I prefer to the for loop, for debugging purpose. We don't use gdb but still. I think we may leave it here for now if you also don't have a strong preference.
events.forEach(e => { target.addEventListener(e, eventListener, { once: true }); });Hao LiuWhy was this needed?
I can't remember exactly. I think it is because otherwise it could cause flakiness somehow. like those `pointermove` we are not testing against could trigger this multiple times.
But I removed this and it seems also passing the tests.
Related details
- Aoyuan Zuo
- Aoyuan Zuo
- Michal Mocny
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |