Closed Bug 1174036 Opened 9 years ago Closed 8 years ago

[e10s] Search on page breaks when it finds hidden <textarea> or <input>

Categories

(Toolkit :: Find Toolbar, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla48
Tracking Status
e10s + ---
firefox47 --- fixed
firefox48 --- verified

People

(Reporter: arni2033, Assigned: mrbkap)

References

Details

(Keywords: regression)

User Story

STR:
1. Open attachment 8704218 [details], make sure only 4 <textarea>s are visible on the page
2. Open Findbar (Ctrl+F), select all text there, type "e" instead of selected text
3. Make sure the first "e" on the page is highlighted
4. Press Shift+Enter
5. Press Enter

Result:
 After Steps 4 and 5   Nothing happens: the first "e" on page stays highlighted,
                       there's no informative text in findbar

Expectations:
 After Step 4   The last "e" on page should be highlighted; text labels in findbar should display:
                "4 of 4 matches   Reached top of page, continued from bottom"

 After Step 5   The first "e" on page should be highlighted; text labels in findbar should display:
                "1 of 4 matches   Reached end of page, continued from top"

Attachments

(5 files)

OS: Unspecified → Windows 7
Hardware: Unspecified → x86_64
Component: Untriaged → Find Backend
Product: Firefox → Core
Hello, Blake. I just saw that you were assigned to similar bug 1140512. Maybe these STR will help you somehow (I think it's the same bug)
If this is different bug, please at least mark it as NEW. Nobody seem to regarded it as valid one :(
Flags: needinfo?(mrbkap)
I don't think that this is a dupe of bug 1140512. That bug is specifically about the findbar not working after navigating the page, whereas this is probably just a bug in the remote findbar's handling of page mutations.

That said, I can reproduce this.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(mrbkap)
Assignee: nobody → mrbkap
I tried reproducing this in a recent Nightly and couldn't. Arni, can you still reproduce this?
Flags: needinfo?(arni2033)
Depends on: 1236972
The situation is the same on Nightly 46 (2016-01-04)
I don't like the way I reported this bug (sorry for #c0 and #c1), so I'll try to put a short description in "User story". (do you need a screencast?)
Also, this bug looks like yet another case when a thing slightly broken in single-process mode causes more serious problems in non-e10s. So I just filed bug 1236972 about that.
User Story: (updated)
No longer depends on: 1236972
Flags: needinfo?(arni2033)
Depends on: 1236972
User Agent 	Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:46.0) Gecko/20100101 Firefox/46.0
Build ID 	20160204004009

Tested on Windows 7 x64 with the Aurora 46.0a2 build. I can reproduce the problem.
Summary: [e10s] Search on page breaks when it founds hidden <textarea> or <input> → [e10s] Search on page breaks when it finds hidden <textarea> or <input>
Blake - should this block?
Flags: needinfo?(mrbkap)
Priority: -- → P1
I don't have any insight into how common this is. I don't think this should block, though. It seems more of a nuisance than a real bug. That being said, I wouldn't consider it a waste of time to work on this bug as it isn't simply a polish bug.
Flags: needinfo?(mrbkap)
(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #8)
> It seems more of a nuisance than a real bug.
I believe you're already aware of thousands of "edge cases" in e10s and Release...
So please just tell me the definition of "REAL BUG" so I could file only "REAL BUGS" from now on.

More about this bug. Detailed description:
 "Findbar loses ability to switch to the next/previous word and show info about matches if
  <input>/<textarea> element is focused on the page right after editing string in Findbar
  containing another hidden <input>/<textarea> before the visible one"
This breaks accessibility on sites like Bugzilla. I use many sites (e.g. forums) that use <textarea>
instead of [contenteditable] (which is too buggy), but sometimes there're hidden <input>/<textarea>,
so I encounter this bug each time I write a comment using info from previous comments.
I can freely use mouse, but it's really unclear what action will allow me to "fix" the Findbar.

Workarounds (all of them are NOT reliable)
 1) to click outside the textarea, select all text in Findbar and type it again.
 2) (access) to press (Shift+)Tab, then select all text in Findbar and type it again.
 3) to focus urlbar and type desired text there (pressing Escape each time you need to hide urlbar
    suggestions to see the text on page), copy text, then press Ctrl+F and paste text in Findbar
That will change scroll position and will probably show you a found string far away from <textarea>, but will still fix the issue. Though there's no guarantee that this workaround will work. If the first string after editing string in Findbar is found in the <textarea> - then Findbar still breaks and doesn't even show how much matches it found on the page. Sometimes it shows incorrect numbers, sometimes - nothing at all.
> There's no way to determine if Findbar is broken again [OR] if there's really only 1 such string
> on the page. And there's NO reliable series of steps that will help to handle this situation.

The Real Workaround is disabling e10s.  I use this workaround.
Attached patch FixSplinter Review
<textarea>s that were once initialized with an editor and subsequently hidden keep their editor, but it gets partially torn down. After that, either touching its selectionController or (sometimes) asking for the controller's NORMAL selection will throw. We should ignore the failure and keep on trucking.
Attachment #8733652 - Flags: review?(mdeboer)
Attached patch Fix for JS typesSplinter Review
I found this while writing a mochitest. Normally, the rect member of the data object passed to onFindResult is a Rect object (as defined in Geometry.jsm). In e10s, however, we lose the prototype over IPC and lose the nice API layered on top.
Attachment #8733653 - Flags: review?(mdeboer)
Attached patch Add a mochitestSplinter Review
Attachment #8733654 - Flags: review?(mdeboer)
(In reply to arni2033 from comment #9)
> I believe you're already aware of thousands of "edge cases" in e10s and
> Release...

My wording in comment 8 was a bit lax, I admit. I didn't mean to imply this wasn't a real bug. That being said, if this were the last bug sitting between e10s being released or not I would not block the release and I hold to that.
Comment on attachment 8733652 [details] [diff] [review]
Fix

Review of attachment 8733652 [details] [diff] [review]:
-----------------------------------------------------------------

Very reasonable! r=me.

::: toolkit/modules/Finder.jsm
@@ +4,5 @@
>  // file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  this.EXPORTED_SYMBOLS = ["Finder","GetClipboardSearchString"];
>  
> +const { interfaces: Ci, classes: Cc, utils: Cu, results: Cr } = Components;

nit: we're not using `Cr` in this file ;-)
Attachment #8733652 - Flags: review?(mdeboer) → review+
Comment on attachment 8733653 [details] [diff] [review]
Fix for JS types

Review of attachment 8733653 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, had to r- because of the failing tests, though.

::: toolkit/modules/RemoteFinder.jsm
@@ +7,5 @@
>  this.EXPORTED_SYMBOLS = ["RemoteFinder", "RemoteFinderListener"];
>  
>  const Ci = Components.interfaces;
>  const Cc = Components.classes;
>  const Cu = Components.utils;

While you're here, can you change this to use the shorter version as well?

@@ +10,5 @@
>  const Cc = Components.classes;
>  const Cu = Components.utils;
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Geometry.jsm");

Can you make this a lazy getter?

@@ +61,5 @@
>      switch (aMessage.name) {
>        case "Finder:Result":
>          this._searchString = aMessage.data.searchString;
> +        // The rect stops being a Geometry.jsm:Rect over IPC.
> +        aMessage.data.rect = Rect.fromRect(aMessage.data.rect);

I think you need to guard this, because the try run is yielding errors.
Attachment #8733653 - Flags: review?(mdeboer) → review-
Comment on attachment 8733654 [details] [diff] [review]
Add a mochitest

Review of attachment 8733654 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/modules/tests/browser/browser_Finder_hidden_textarea.js
@@ +1,1 @@
> +add_task(function* test_bug1174036() {

nit: please add a Creative Commons license block on top of the file.
Attachment #8733654 - Flags: review?(mdeboer) → review+
Component: Find Backend → Find Toolbar
Product: Core → Toolkit
Thanks for the fast review, Mike! I've fixed the items you mentioned here but I need to debug why my test is asserting and failing on non-e10s, only when run in the directory (it passes if run alone). I should have a new patch in a couple of hours.
I've addressed the other comments. This patch is necessary because
without it we might find the 'e' in the first textarea and flush layout
notifications afterwards, leading to a search result in an invisible
(and frameless) DOM node. The change to the patch was to help debugging.

The full patch set is on try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a3ca28c2675
Attachment #8734450 - Flags: review?(mdeboer)
Comment on attachment 8734450 [details] [diff] [review]
Fix intermittant orange

Review of attachment 8734450 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/typeaheadfind/nsTypeAheadFind.cpp
@@ +316,5 @@
>    }
>  
> +  // There could be unflushed notifications which hide textareas or other
> +  // elements that we don't want to find text in.
> +  presShell->FlushPendingNotifications(Flush_Layout);

This sounds perfectly reasonable, but I'm afraid I can't stamp this wholehartedly... Perhaps Ehsan is a good candidate for this one-liner?
Attachment #8734450 - Flags: review?(mdeboer) → review?(ehsan)
(In reply to Mike de Boer [:mikedeboer] from comment #20)
*wholeheartedly
Status: NEW → ASSIGNED
Will this also fix bug 1236972? Should that be duped here if so?
Flags: needinfo?(mrbkap)
(In reply to Justin Dolske [:Dolske] from comment #22)
> Will this also fix bug 1236972? Should that be duped here if so?

This does fix it and that bug should be duped here.
Flags: needinfo?(mrbkap)
Comment on attachment 8734450 [details] [diff] [review]
Fix intermittant orange

Looks like the callers already expect a possible flush, at least according to the comments!
Attachment #8734450 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/6a5f05ddfa83
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Fixed on:   Win7_64, Nightly 48, 32bit, ID 20160330030326

I still see minor bugs in those testcases, but nothing like bug 1174036 (this) and bug 1236972
Status: RESOLVED → VERIFIED
blake, should we uplift this to 47 or 48?
Flags: needinfo?(mrbkap)
Comment on attachment 8734450 [details] [diff] [review]
Fix intermittant orange

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: Sites that hide textareas could cause the find bar to not find strings later in the document.
[Describe test coverage new/current, TreeHerder]: This has been in the tree for a month and has an added mochitest testcase.
[Risks and why]: Low risk.
[String/UUID change made/needed]: n/a
Flags: needinfo?(mrbkap)
Attachment #8734450 - Flags: approval-mozilla-beta?
Attachment #8734450 - Flags: approval-mozilla-aurora?
Comment on attachment 8734450 [details] [diff] [review]
Fix intermittant orange

Nice fix for cases that break find in page. Adds a test case and arni did some extra manual testing. Let's uplift this to aurora at least.
Attachment #8734450 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8734450 [details] [diff] [review]
Fix intermittant orange

Fix was verified on 48, Beta47+
Attachment #8734450 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.