Closed Bug 1450055 Opened 6 years ago Closed 5 years ago

Stopping propagation of focus events prevents focusing properly on inputs.

Categories

(GeckoView :: IME, enhancement, P2)

Product:
GeckoView
Component:
IME
Platform:
All
Android
Type:
enhancement

Tracking

(geckoview64 wontfix, geckoview65 wontfix, geckoview66 fixed, firefox64 wontfix, firefox65 wontfix, firefox66 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla66
Tracking Flags:
Tracking Status
geckoview64 --- wontfix
geckoview65 --- wontfix
geckoview66 --- fixed
firefox64 --- wontfix
firefox65 --- wontfix
firefox66 --- fixed

People

(Reporter: wisniewskit, Assigned: m_kato)

References

Details

(Whiteboard: [webcompat:p2])

User Story

Important for GeckoView.

Attachments

(3 files)

Bug 1450055 - Part 1. Add focus event to system group to initialize editor. r?masayuki
47 bytes, text/x-phabricator-request
Details | Review
Bug 1450055 - Part 2. Add mochitest whether rendering caret or not. r?masayuki
47 bytes, text/x-phabricator-request
Details | Review
Bug 1450055 - Part 3. Update mochitests since focus event listener is system group. r?masayuki
47 bytes, text/x-phabricator-request
Details | Review 
Take this short test-case, for instance:

    <!doctype html>
    <html><body>           
      <input type="text" placeholder="type here"></input>
      <script>            
      document.querySelector("input").addEventListener("focus", e => {
        e.stopImmediatePropagation();
      });         
    </script>
  </body></html> 

On desktop Firefox and Chrome for Android, the input seems to properly get focused upon clicking/tapping it, so one can type in it.

However, on Fennec, the input never gets focused. It's almost as though one has called preventDefault on the event (though defaultPrevented is still set to false after calling stopImmediatePropagation). Note that calling stopPropagation does not cause this issue.

This implies that we may have our own internal focus event handler which is handled after the site's own handlers, and is therefore being stopped.

This issue is breaking the wishlist feature on fnac.com.
Flags: webcompat?
Seems like the kind of issue that would be good to fix for GeckoView.
Whiteboard: [webcompat] → [webcompat:p3]
Whiteboard: [webcompat:p3] → [webcompat:p2]
User Story: (updated)
Re-triaging per https://bugzilla.mozilla.org/show_bug.cgi?id=1473195

Needinfo :susheel if you think this bug should be re-triaged.
Priority: -- → P5
It looks like it's also possible to break the event in other, similar ways, for instance:

  <!doctype html>
  <html><body>           
    <input type="text" placeholder="type here"></input>
    <script>
      document.documentElement.addEventListener("focus", e => {
        e.stopPropagation();
      }, true);         
    </script>
  </body></html>
See Also: → https://webcompat.com/issues/18577
Summary: Calling stopImmediatePropagation on an input's focus event screws up the focus. → Stopping propagation of focus events prevents focusing properly on inputs.
Flags: webcompat? → webcompat+
(In reply to Firefox Product Integrity Bug Husbandry Bot (contact :emceeaich) from comment #2)
> Re-triaging per https://bugzilla.mozilla.org/show_bug.cgi?id=1473195
> 
> Needinfo :susheel if you think this bug should be re-triaged.

Susheel (note, you don't have :susheel in your Bugzilla profile nick, which makes this comment confusing), can we re-triage? This affects GeckoView.
Flags: needinfo?(sdaswani)
Sounds like a Gecko issue.
Flags: needinfo?(sdaswani) → needinfo?(snorp)
Moving this to GeckoView so it gets triaged appropriately.
Component: Keyboards and IME → GeckoView
Flags: needinfo?(snorp)
Is this GeckoView specific?
Flags: needinfo?(wisniewskit)
Both of the above testcases don't work for me in GeckoView or Fennec. GeckoView does at least call up the on-screen keyboard, but the keys being pressed do not end up in the text input.
Flags: needinfo?(wisniewskit)
Sorry, I was unclear. Is this mobile only?
Flags: needinfo?(twisniewski)
I'm afraid that I can't fully confirm that, as I have no touchscreen to test desktop Firefox with. But clicking with a mouse or using the responsive design devtool in touch-emulation mode is working fine.
Flags: needinfo?(twisniewski)
(In reply to Thomas Wisniewski [:twisniewski] from comment #10)
> I'm afraid that I can't fully confirm that, as I have no touchscreen to test
> desktop Firefox with. But clicking with a mouse or using the responsive
> design devtool in touch-emulation mode is working fine.

FWIW, I tested by Surface book. I couldn't reproduce this issue on the websites.
Removing priority so this gets re-triaged.
Priority: P5 → --
Product: Firefox for Android → GeckoView
https://searchfox.org/mozilla-central/rev/ecf61f8f3904549f5d65a8a511dbd7ea4fd1a51d/editor/libeditor/EditorEventListener.cpp#173-176

We don't receive focus event in editor, so selection and input context isn't initialized well...
Assignee: nobody → m_kato
(In reply to Makoto Kato [:m_kato] from comment #14)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=caffdab68d670ed011b64c7278b0f9138dde2c35

You need to touch around RemoveEventListenerByType() too.
(In reply to Masayuki Nakano [:masayuki] (JST, +0900)(sick) from comment #15)
> (In reply to Makoto Kato [:m_kato] from comment #14)
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=caffdab68d670ed011b64c7278b0f9138dde2c35
> 
> You need to touch around RemoveEventListenerByType() too.

Thanks.

Hmm, SystemGroup's event isn't same timing, so many tests in test_reftests_with_caret.html will be failure.  (Most failed tests use synthesizeKey on focus event.)
System group event propagation is started after:
- default event group propagation (i.e., all event listeners of web apps)
- event callbacks (e.g., nsIFrame::HandleEvent())

And also, such tests, synthesizeKey() is callsed by focus event listener, should be rewritten because it's non-realistic case. Any actual event event won't be fired while an event is being propagated.
Editor initializes selection and input context (via IMEStateManager) on focus
event. But if content script calls stopImmediatePropagation on focus event,
editor cannot initialize these since editor cannot receive focus event.

It means that Android widget doesn't open virtual keyboard since
GeckoEditableSupport::SetInputContext isn't called.  Also, Firefox desktop
doesn't show caret in this situation since selection isn't initialized in
editor.

So the event listener of focus and blur event should use system group.
The focus event listener in editor is system group, so synthesizeKey and/or synthesizeMouse doesn't work on focus event since editor doesn't initialize selection yet. So use setTimeout to work synthesizeKey and etc well.

Depends on D15924
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c4619332810
Part 1. Add focus event to system group to initialize editor. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/85fda059ef06
Part 2. Add mochitest whether rendering caret or not. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6cc9b822c1c
Part 3. Update mochitests since focus event listener is system group. r=masayuki

Makoto, do you think this change is safe to uplift to 65 Beta? There are only about 2 weeks left in the 65 Beta cycle.

Or we could wait until after 65 release and create a GeckoView 65 relbranch. Uplifting this fix to a GeckoView relbranch would remove any risk of destabilizing Firefox desktop or Fennec 65.

status-firefox64: --- → wontfix
status-firefox65: --- → ?
status-geckoview64: --- → wontfix
status-geckoview65: --- → ?
status-geckoview66: --- → fix-optional
Flags: needinfo?(m_kato)

(In reply to Chris Peterson [:cpeterson] from comment #23)

Makoto, do you think this change is safe to uplift to 65 Beta? There are only about 2 weeks left in the 65 Beta cycle.

This is risky because we change the timing of initializing editor at all case. So it isn't good to uplift this to beta.

Flags: needinfo?(m_kato)

(In reply to Makoto Kato [:m_kato] from comment #24)

This is risky because we change the timing of initializing editor at all case. So it isn't good to uplift this to beta.

Thanks! Good to know.

65=wontfix

status-firefox65: ? → wontfix
status-geckoview65: ? → wontfix
status-geckoview66: fix-optional → fixed

Moving some focus bugs to the new GeckoView::IME component.

Component: General → IME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: