Closed Bug 1322385 Opened 3 years ago Closed 3 months ago

User input in forms triggers typeaheadfind when RDM is active

Categories

(DevTools :: Responsive Design Mode, defect, P1)

52 Branch
defect

Tracking

(firefox68 verified, firefox69 verified)

VERIFIED FIXED
Firefox 68
Tracking Status
firefox68 --- verified
firefox69 --- verified

People

(Reporter: fvsch, Assigned: bradwerth)

References

(Blocks 3 open bugs)

Details

(Whiteboard: [rdm-mvp] [dt-q])

Attachments

(2 files, 2 obsolete files)

Users with the "accessibility.typeaheadfind=true" preference cannot type in form fields when RDM is active. The user's input is used by the "Search as you type" feature and not by the form field (which remains empty).

Steps to reproduce:
1. Set accessibility.typeaheadfind=true in about:config (or Preferences>Advanced>General>Search for text when I start typing).
2. Open a page with a text input, e.g. https://devdocs.io/
3. Enable Responsive Design Mode
4. Focus the search field, and type a few letters

Expected result:
The typed characters appear in the page's text input.

Actual result:
The typed characters appear in the browser's "Quick Find" toolbar.

This bug makes it impossible to use the new RDM together with Firefox's typeaheadfind mode and use or test web forms (a common need as a web developer or designer). Of course this only affects the small minority of designers/devs who enabled this advanced feature.
Thanks for the report!  I think this is low priority since it's a non-standard configuration, but good to know about it for future tracking.
Priority: -- → P3
Whiteboard: [rdm-v2]
Duplicate of this bug: 1393016
I just did a clean install of FireFox Quantum and this option was by default set to true! So I suppose this is not a low priority anymore?
(In reply to tomdesmet from comment #3)
> I just did a clean install of FireFox Quantum and this option was by default
> set to true! So I suppose this is not a low priority anymore?

Hmm, are you on Linux where your distro may have customized this?  The Firefox default still appears to false as far as I can see:

https://searchfox.org/mozilla-central/rev/a70da6775d5341a9cca86bf1572a5e3534909153/browser/app/profile/firefox.js#699
Same issue here on (FF 60.0.1 64bit Ubuntu).

This makes it impossible to develop/debug responsive websites that include forms when typeaheadfind is active.
Disabling the feature is a possible workaround for me, but still quite annoying.

Can I help in any way to get this resolved?
Product: Firefox → DevTools
Firefox 62.0
Same bug there is (i have accessibility.typeaheadfind=true) and in RDM i cannot input to input fields :(
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [rdm-v2] → [rdm-v2][dt-q]
This preference can also be set by:

1. Going to about:preferences
2. Enable "Search for text when you start typing."

Then if you go into RDM and try to search in Google you will be completely unable to type in the Google input field.
Is bug 1317323 also a duplicate?
Duplicate of this bug: 1513785
Blocks: rdm-ux
Duplicate of this bug: 1481849
Duplicate of this bug: 1300105
Priority: P3 → P2
Whiteboard: [rdm-v2][dt-q] → [rdm-mvp] [dt-q]
Assignee: nobody → bwerth
See Also: → 1317323
Status: NEW → ASSIGNED
Priority: P2 → P1

This is occurring because the keypress event given to FindBardChild.onKeypress has a target that is the RDM pane iframe, not a specific element in the content. BrowserUtils.shouldFastFind looks at that element and decides that it's not an input element, so it triggers the find.

I'm looking for the event creation and propagation.

Got a bit further and found the point of divergence that eventually leads to the changed event target. In PresShell::EventHandler::MaybeHandleEventWithAnotherPresShell, we have one of two outcomes:

  1. In the normal case, GetFrameForHandlingEventWith returns aFrameForPresShell, which causes the function to early exit shortly afterwards.
  2. In the RDM case, GetFrameForHandlingEventWith returns the ViewportFrame, which means the function proceeds further and sends the event to the PresShell associated with the ViewportFrame.

So the challenge for this bug is for case 2 to either have an event target identical to case 1 (impossible? because of the process boundary), or for it to encode some information that indicates it came from an input field so that later processing can ignore it.

New thinking: the problem is not in the platform code, but in the RDM swap. My current theory is that RDM is swapping the actor event listeners, and we don't want it to.

Here's how it works without RDM:

  1. Key is pressed in an input element.
  2. The platform sends the keypress event to the parent process, which has no listener and sends the event to the child process.
  3. The child process assigns the event target correctly to the input element.
  4. The child process has the keypress listener, which realizes the event is targeted at an input element, and so ignores the event.
  5. Default event processing causes the event to put text into the input element.

With RDM:

  1. Key is pressed in an input element.
  2. The platform sends the keypress event to the parent process, which unexpectedly now has the keypress listener, and so the parent process attempts to handle the event.
  3. The parent process assigns the event target incorrectly to the iframe, which is the closest it can get to the input element in the child process.
  4. The parent process keypress listener treats the event as being outside an input element and triggers typeahead find.

I'll see if I can prevent the event listener swap from happening.

After more investigation, it seems that the Actors added by tab-content.js are the ones that need to be re-targeted when the browser swap happens. It's possible this bug is a regression caused by the landing of Bug 1472491. I'm going to do a mozregression to see if that is the case.

(In reply to Brad Werth [:bradwerth] from comment #16)

It's possible this bug is a regression caused by the landing of Bug 1472491. I'm going to do a mozregression to see if that is the case.

I'm being ridiculous; this bug predates Bug 1472491 by a considerable timespan. Understanding the refactoring done in Bug 1472491 does seem to be important to fixing this.

Blocks: 1317323

Some observations while testing this patch. Brad, I believe these are the JS errors you were talking about during our meeting and are specific to this patch:

  • Closing RDM will throw the following message:

    JavaScript error: resource:///actors/LinkHandlerChild.jsm, line 72: TypeError: this.content is null
    
  • While in RDM, typing whether an input field is focused or not throws this error:

    JavaScript error: resource://gre/actors/FindBarChild.jsm, line 65: TypeError: this.content is null
    
  • Similarly, clicking anywhere in the browser content will also show this:

    JavaScript error: resource:///actors/ClickHandlerChild.jsm, line 144: TypeError: content is null
    

Otherwise, this patch fixes typeahead find triggering the FindBar when the text input field isn't focused in RDM.

(In reply to Micah Tigley [:mtigley] from comment #21)

Some observations while testing this patch. Brad, I believe these are the JS errors you were talking about during our meeting and are specific to this patch:

Thank you for documenting this. These errors certainly point to deficiencies in the proposed patches that I should also address. I'll investigate further.

New thinking: After discussion with Mike Conley, it's intended that the Browser Actors should travel with the docshell and swap alongside them without special effort. What is likely happening here is that the Actors are duplicated for both the RDM UI content (in the parent process) and also for the RDM panel content (in the child process). We want the parent process version of the FindBarChild to ignore events targeting an iframe mozbrowser, and to allow the child process to pick it up instead. This will involve changes to the manifest in ActorManagerParent.jsm, and to BrowserUtils.jsm.

Attachment #9059144 - Attachment is obsolete: true
Attachment #9059145 - Attachment is obsolete: true

I am struggling to build an effective test for this. Attachment 9061139 [details] is the closest I've gotten so far, but it's not working yet. There are at least two problems with it:

  1. The method I'm using to simulate keypresses is not hitting the pathways that trigger the bug. When the test is run on a build WITHOUT Part 1 applied, the keypresses show up in the input field just fine. That's the whole point of the bug, and I can't figure out a way to simulate keypresses that behave the same as interactive keypresses.
  2. I can't figure out an effective method of detecting when find has been triggered. The test can't await a find result, because there might not be one. Currently the test attempts to detect when the findBar has been populated with text, but it's not detecting any text even when the findBar appears to have text in it. I assume this is due to confusion with the RDM pane.

I'll spend a bit more time on this test and if I can't get it working, I'll land Part 1 of the patch without test coverage.

Blocks: 1547783
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/feef45c56170
Part 1: Make BrowserUtils.shouldFastFind avoid triggering on iframes. r=mconley
https://hg.mozilla.org/integration/autoland/rev/31a0bd977b2f
Part 2: Add a test that RDM content triggers typeahead find correctly. r=gl

Backed out 2 changesets (Bug 1322385) for mochitest failure at: toolkit/content/tests/chrome/test_findbar_entireword.xul

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&searchStr=c3&revision=31a0bd977b2fc13c35cc4f54ab54a9887f8fdb50

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=243710016&repo=autoland&lineNumber=5607

[task 2019-04-30T18:53:24.565Z] 18:53:24     INFO - TEST-PASS | toolkit/content/tests/chrome/test_findbar_entireword.xul | should've found 1 match 
[task 2019-04-30T18:53:24.566Z] 18:53:24     INFO - Buffered messages finished
[task 2019-04-30T18:53:24.567Z] 18:53:24     INFO - TEST-UNEXPECTED-FAIL | toolkit/content/tests/chrome/test_findbar_entireword.xul | testQuickfind: failed to open findbar 
[task 2019-04-30T18:53:24.567Z] 18:53:24     INFO - SimpleTest.ok@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:275:18
[task 2019-04-30T18:53:24.568Z] 18:53:24     INFO - testQuickfind@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/findbar_entireword_window.xul:252:7
[task 2019-04-30T18:53:24.569Z] 18:53:24     INFO - async*onDocumentLoaded@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/findbar_entireword_window.xul:144:13
[task 2019-04-30T18:53:24.570Z] 18:53:24     INFO - async*startTestWithBrowser@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/findbar_entireword_window.xul:136:13
[task 2019-04-30T18:53:24.570Z] 18:53:24     INFO - async*onLoad/<@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/findbar_entireword_window.xul:113:17
[task 2019-04-30T18:53:24.571Z] 18:53:24     INFO - async*onLoad@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/findbar_entireword_window.xul:115:9
[task 2019-04-30T18:53:24.571Z] 18:53:24     INFO - onload@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/findbar_entireword_window.xul:1:1
[task 2019-04-30T18:53:24.572Z] 18:53:24     INFO - Not taking screenshot here: see the one that was previously logged
[task 2019-04-30T18:53:24.572Z] 18:53:24     INFO - TEST-UNEXPECTED-FAIL | toolkit/content/tests/chrome/test_findbar_entireword.xul | testQuickfind: find field is not focused 
[task 2019-04-30T18:53:24.572Z] 18:53:24     INFO - SimpleTest.ok@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:275:18
[task 2019-04-30T18:53:24.572Z] 18:53:24     INFO - testQuickfind@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/findbar_entireword_window.xul:253:7
[task 2019-04-30T18:53:24.572Z] 18:53:24     INFO - async*onDocumentLoaded@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/findbar_entireword_window.xul:144:13
[task 2019-04-30T18:53:24.572Z] 18:53:24     INFO - async*startTestWithBrowser@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/findbar_entireword_window.xul:136:13
[task 2019-04-30T18:53:24.572Z] 18:53:24     INFO - async*onLoad/<@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/findbar_entireword_window.xul:113:17
[task 2019-04-30T18:53:24.573Z] 18:53:24     INFO - async*onLoad@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/findbar_entireword_window.xul:115:9
[task 2019-04-30T18:53:24.573Z] 18:53:24     INFO - onload@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/findbar_entireword_window.xul:1:1
[task 2019-04-30T18:53:24.574Z] 18:53:24     INFO - Not taking screenshot here: see the one that was previously logged
[task 2019-04-30T18:53:24.574Z] 18:53:24     INFO - TEST-UNEXPECTED-FAIL | toolkit/content/tests/chrome/test_findbar_entireword.xul | testQuickfind: entire word mode status text should be visible 
[task 2019-04-30T18:53:24.575Z] 18:53:24     INFO - SimpleTest.ok@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:275:18
[task 2019-04-30T18:53:24.576Z] 18:53:24     INFO - testQuickfind@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/findbar_entireword_window.xul:255:7
[task 2019-04-30T18:53:24.576Z] 18:53:24     INFO - async*onDocumentLoaded@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/findbar_entireword_window.xul:144:13
[task 2019-04-30T18:53:24.576Z] 18:53:24     INFO - async*startTestWithBrowser@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/findbar_entireword_window.xul:136:13
[task 2019-04-30T18:53:24.576Z] 18:53:24     INFO - async*onLoad/<@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/findbar_entireword_window.xul:113:17
[task 2019-04-30T18:53:24.577Z] 18:53:24     INFO - async*onLoad@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/findbar_entireword_window.xul:115:9
[task 2019-04-30T18:53:24.577Z] 18:53:24     INFO - onload@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/findbar_entireword_window.xul:1:1
[task 2019-04-30T18:59:34.557Z] 18:59:34     INFO - Buffered messages finished
[task 2019-04-30T18:59:34.558Z] 18:59:34    ERROR - TEST-UNEXPECTED-TIMEOUT | toolkit/content/tests/chrome/test_findbar_entireword.xul | application timed out after 370 seconds with no output
[task 2019-04-30T18:59:34.559Z] 18:59:34    ERROR - Force-terminating active process(es).
[task 2019-04-30T18:59:34.560Z] 18:59:34     INFO - Determining child pids from psutil...
[task 2019-04-30T18:59:34.568Z] 18:59:34     INFO - [4810, 4948]
Flags: needinfo?(bwerth)
Backout by dvarga@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/79e333495c16
Backed out 2 changesets for mochitest failure at: toolkit/content/tests/chrome/test_findbar_entireword.xul

I'm narrowing in on the cause of the test failures, and I could use some help understanding what I'm seeing. Here's what appears to be going on:

  1. When the FindBar Actor is declared with "allFrames: true", it is added to the list of "ChildSingletonActors" eventually received by ActorManagerChild.init(), which creates a MozDocumentMatcher for it (and other singletons).
  2. As AMC.init creates MozDocumentObserver objects, that will trigger an attempted match against the singleton MDM created earlier. Those attempted matches will hit the platform code MozDocumentMatcher::Matches.
  3. As the test_findbar_entireword.xul test is loaded, the expected call to MDM::Matches occurs with the URL "chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_findbar_entireword.xul". That match fails at https://searchfox.org/mozilla-central/source/toolkit/components/extensions/WebExtensionPolicy.cpp#622.

Because this match fails, the FindBarChild is never attached to the MessageManager for the window, and the test times out waiting for the FindBar to respond to key input.

I'm still trying to figure out what this means. Are browser Actors not allowed to attach to chrome URLs?

Flags: needinfo?(mconley)

Referencing comment 31, after test_findbar_entireword.xul test is loaded, the MDM::Matches is then called on the URL "newtab" which is also rejected as a match. I'm not sure why the URL is "newtab", and not "findbar_entireword_window.xul" as I would expect looking at the relevant JS call at https://searchfox.org/mozilla-central/source/toolkit/content/tests/chrome/test_findbar_entireword.xul#32

(In reply to Brad Werth [:bradwerth] from comment #31)

  1. As the test_findbar_entireword.xul test is loaded, the expected call to MDM::Matches occurs with the URL "chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_findbar_entireword.xul". That match fails at https://searchfox.org/mozilla-central/source/toolkit/components/extensions/WebExtensionPolicy.cpp#622.

That's super odd - I believe how this is supposed to work is that we match all documents unless the matches: property is given some list of matching strings to compare against.

Am I correct on that, aswan?

Flags: needinfo?(mconley) → needinfo?(aswan)

(In reply to Brad Werth [:bradwerth] from comment #32)

Referencing comment 31, after test_findbar_entireword.xul test is loaded, the MDM::Matches is then called on the URL "newtab" which is also rejected as a match. I'm not sure why the URL is "newtab", and not "findbar_entireword_window.xul" as I would expect looking at the relevant JS call at https://searchfox.org/mozilla-central/source/toolkit/content/tests/chrome/test_findbar_entireword.xul#32

This might be due to the preloaded about:newtab that is loaded silently in the background pre-emptively. You can disable that preloading by setting browser.newtab.preload to false to confirm that.

I'm not very familiar with how Fission uses MatchPattern, I suspect Kris is.

Flags: needinfo?(aswan) → needinfo?(kmaglione+bmo)

In case Kris isn't able to answer promptly, the "matches" property is mandatory, it should be set to the magic string <all_urls> (blame the original Chrome extension designers, we didn't invent that) if you want to match everything.
It looks like that's getting set here, I don't know if there are other code paths that don't set it:
https://searchfox.org/mozilla-central/rev/116bd975c30746ddefc3d20e6947d1871469354f/toolkit/modules/ActorManagerParent.jsm#435

MatchPattern was also originally written specifically for extensions which deliberately don't have access to things like (most) about: and chrome: pages. The restrictSchemes parameter here is supposed to relax that limit:
https://searchfox.org/mozilla-central/rev/116bd975c30746ddefc3d20e6947d1871469354f/toolkit/modules/ActorManagerChild.jsm#282

(In reply to Mike Conley (:mconley) (:⚙️) from comment #33)

(In reply to Brad Werth [:bradwerth] from comment #31)

  1. As the test_findbar_entireword.xul test is loaded, the expected call to MDM::Matches occurs with the URL "chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_findbar_entireword.xul". That match fails at https://searchfox.org/mozilla-central/source/toolkit/components/extensions/WebExtensionPolicy.cpp#622.

That's super odd - I believe how this is supposed to work is that we match all documents unless the matches: property is given some list of matching strings to compare against.

Privileged match patterns are allowed to match chrome: URIs, but they have to ask for them explicitly. <all_urls> still only matches webby URLs. chrome://*/* will match all chrome URIs.

Flags: needinfo?(kmaglione+bmo)
Blocks: 1552279
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c3954692e846
Part 1: Make BrowserUtils.shouldFastFind avoid triggering on iframes. r=mconley
https://hg.mozilla.org/integration/autoland/rev/b38eabe18317
Part 2: Add a test that RDM content triggers typeahead find correctly. r=gl
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
Flags: needinfo?(bwerth)

Hi, This issue is verified as fixed in our latest Firefox Builds. Nightly 69.0a1 (2019-06-03) as well as Beta 68.0b6 .

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.