+Blink>Input for focus/OSK interaction.

There's some related history in  issue 386827  and issue 371372.

When I tap an already focused input field in Chrome Android, it does re-open the OSK for me.  Presumably that's not due to the Element::focus OSK codepath but the RenderWidget::DidHandleGestureEvent codepath.  That sounds like it wouldn't be hit in Silk when in accessibility mode?  Test case: https://jsbin.com/jisofit

Having Element::focus run the ShowVirtualKeyboardOnElementFocus steps even when already focused sounds like it's probably fine to me (though it still needs the UserGesture check, so the code might be slightly non-trivial).  There could be some risk of the OSK opening when we don't want it to (eg. if a site has a bug where it calls focus over and over again), but I'm not sure off hand how we'd find such cases.  I think it would be OK to just try.

I think ideally blink would be in charge of when the OSK is shown, the logic in RenderWidget::DidHandleGestureEvent that retroactively checks to see what element has focus at the end of a tap seems like a hack to me given that we also have code in blink to request the OSK when a suitable input field is focused.  Perhaps we could remove the RenderWidget::DidHandleGestureEvent OSK code path entirely (and then this problem of tapping on an already focused field not opening the OSK would affect Chrome too).

There's not any bug in Chromium proper here.  So whether we would take a change is purely down to whether it seems to be cleaner/simpler than the current approach (i.e. this is a refactoring from our POV).

What I'd like to avoid is just adding extra code that does something in Silk but absolutely no effect in Chrome.  But if the codepath in RenderWidget::DidHandleGestureEvent is deleted as part of this change, this *does* sound like a cleanup we should do.

Sorry for the delayed response. Got occupied with something else. I took a look at why this issue does not arise when running Chrome on Android in TalkBack mode.

Chrome running on Android in TalkBack mode:
When tapping on an input field for the first time, the accessibility service sends two actions, viz. ACTION_CLICK and ACTION_FOCUS, to the AccessibilityNodeProvider (implemented in WebContentsAccessibility). This is followed by a call to RenderWidget::DidHandleGestureEvent (however, the call to show the OSK is already made before this call, due to the accessibility actions being received).

On subsequent taps to this already focused input field, the two accessibility actions are still received, but RenderWidget::DidHandleGestureEvent is NOT called. The reason why the OSK gets shown on subsequent taps is that ACTION_FOCUS results in a call to AXNodeObject::OnNativeFocusAction. In this method, the focus is cleared, if already focused (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp?sq=package:chromium&l=2527), before calling Element::focus.

Silk running on FireOS with accessibility turned on:
The only accessibility action that is sent is ACTION_CLICK (instead of ACTION_CLICK and ACTION_FOCUS when running Chrome on Android). This results in a call to AXObject::OnNativeClickAction, which further results in call to InputTypeView::AccessKeyAction. This method calls Element::focus. However, since the Element is already focused, the OSK is not shown.

An alternative (and perhaps less risky) fix for this is to clear focus in InputTypeView::AccessKeyAction (if the Element is already focused), just like is done in AXNodeObject::OnNativeFocusAction. How does that sound (I verified that that does fix the bug)?

I'm not as concerned with optimizing for short-term change risk here, I care more about long-term maintainability.  In general Chromium's code review culture is to encourage CL authors to go the extra mile to delete and simplify the code they touch even when that creates short-term risk, since that's the only way to prevent gradual degeneration into spaghetti.

The one-liner you propose sounds like it would just accrete even more illogical complexity to the system.  It's hard to explain from first principles why InputTypeView::AccessKeyAction should affect focus at all.  So I prefer Rick's idea.

I agree with you on aiming for maintainability over short-term risk.

When you say Rick's idea, are you referring to
1) removing the RenderWidget::DidHandleGestureEvent OSK code path, or
2) having Element::focus run ShowVirtualKeyboardOnElementFocus even when already focused?

The former does not really apply, since RenderWidget::DidHandleGestureEvent is not called on subsequent taps on an input field (when running Chrome on Android with TalkBack).

So, the two approaches that I have in mind are 2) and the one-liner that I suggested in my previous comment.

The one-liner that I proposed in my previous comment makes InputTypeView::AccessKeyAction conform with  AXNodeObject::OnNativeFocusAction, such that it would also check (and clear, if needed) focus, before calling Element::focus.

Maybe try 2) and also delete the AXNodeObject::OnNativeFocusAction check/clear code?

Feel free to upload candidate patches on Gerrit to illustrate the different ideas, as that might make it a bit easier to see what really winds up cleaner (perhaps I'm not visualizing the code changes properly).

Posted a CL as per your suggestion here: https://chromium-review.googlesource.com/c/chromium/src/+/676282

This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Reporter, is this issue still reproducible on your side? We don't have those devices handy at this point to do a local testing.

Yes, this issue is still reproducible as originally reported.

Thank you for providing more feedback. Adding the requester to the cc list.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

I mark this as available for now until we get one of those devices.