Closed Bug 893349 Opened 7 years ago Closed 7 years ago

Dispatch event when find bar is initialized

Categories

(Firefox :: Extension Compatibility, defect)

25 Branch
x86_64
All
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 26
Tracking Status
firefox25 --- verified
firefox26 --- verified

People

(Reporter: quicksaver, Assigned: hobophobe)

References

Details

Attachments

(1 file, 2 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release)
Build ID: 20130618035212




Expected results:

This is a request. Ever since landing of bug 537013, the find bar is lazily added. This means that, for add-ons like FindBar Tweak that rely on the find bar being initialized, it becomes a much harder job to follow this state.

Listening to "TabSelect" events to know if the new tab has an initialized find bar, only partly resolves the issue because, when switching to a new tab, the find bar won't be initialized. When you open the find bar on a tab for the first time, thus initializing it, we have no easy means to know this happened. We know that the find bar was opened only after it happened, and in this case if we modify it at this point, there will be a brief "jump" between the original find bar state and whatever state we want to induce in the newly initialized find bar. Plus, modifying the open routine in whatever way becomes impossible, because it would always open in the normal default behavior at least once.

Modifying every single shortcut and command that opens the find bar so we know it's going to be initialized is, in my opinion, very poor coding practice, not to mention it can easily fail if other add-ons do the same.

A simple "FindBarInitialized" event, dispatched at the end of gBrowser.getFindBar(), right before the return statement, with e.target defined as either the newly created find bar element or the tab element where it was created, would easily fix this and remove the need of so many extra listeners and handlers to work around the current situation.

I realize this kind of event could be sent from an add-on, by modifying the gBrowser.getFindBar() method itself. But in my opinion this would be a much better fix done by firefox itself, for the sake of global compatibility.
Blocks: 537013
OS: Windows 7 → All
Component: Untriaged → Extension Compatibility
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch Add findinitialized event (obsolete) — Splinter Review
This adds the event and a test.

One alternative for addons is MutationObserver (https://developer.mozilla.org/en-US/docs/Web/API/MutationObserver) to observe findbar elements added.
Assignee: nobody → unusualtears
Status: NEW → ASSIGNED
Attachment #777493 - Flags: review?(dao)
(In reply to Adam [:hobophobe] from comment #1)
> One alternative for addons is MutationObserver
> (https://developer.mozilla.org/en-US/docs/Web/API/MutationObserver) to
> observe findbar elements added.

Using MutationObserver to track when a findbar is added has the same limitations as I've mentioned above. Because the MutationObserver only performs the callback when the DOM is done changing, it also has an asynchronous aspect (even if it's not actually a true async, I'm not sure about this point but it's irrelevant).

When I tested them, I would open the find bar through Ctrl+F and defined the callback to log the hidden state of the findbar. In the cases where the findbar element was added at that point, it logged its hidden state as false, meaning the full gFindBar.open() command had already been performed (the findbar was already unhidden) when the MutationObserver callback was called.

Modifying the gFindBar.open() command through a MutationObserver to log a message when it is called, won't log that message in these cases either, it will only log the message after the find bar is opened a second time in that tab. This confirms that the gFindBar.open() method comes before the MutationObserver's callback.

I didn't test for it, but I'm assuming any gFindBar.find() methods or others would also be done before the MutationObserver's callback if the find bar is initialized when they are called.

Your patch introduces the only method through which we can track the findbar initialization at a point where we are sure none of its methods have been called called yet (other than modifying the gBrowser.getFindBar() ourselves to do the same).
Comment on attachment 777493 [details] [diff] [review]
Add findinitialized event

Hm, I think if we dispatch this on the tab rather than on the find bar, it should probably follow the TabFoo naming convention for tab events.
Attachment #777493 - Flags: review?(dao) → review-
Could I suggest 'TabFindInitialize' perhaps then?
Attachment #777493 - Attachment is obsolete: true
Attachment #785314 - Flags: review?(dao)
Comment on attachment 785314 [details] [diff] [review]
Renamed event to TabFindInitialized

>   gBrowser.selectedTab = tabs[1];
>+  gBrowser.selectedTab.addEventListener("TabFindInitialized", continueTests1,
>+                                        true);
>+  // Initialize the findbar
>+  gFindBar;
>+}
>+function continueTests1() {
>+  gBrowser.selectedTab.removeEventListener("TabFindInitialized", continueTests1,
>+                                           true);

You can drop the third argument for both addEventListener and removeEventListener.
Attachment #785314 - Flags: review?(dao) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/52951be1b880
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 26
Is there a reason why this cannot make it into Firefox 25 still, together with the new find bar? Or am I misunderstanding the "Target Milestone" field completely and it will land there anyway?
It won't land there unless we request Aurora approval and uplift it, but I see no reason not to do that.

Adam, want to make the request?
Comment on attachment 785542 [details] [diff] [review]
Let test event listeners use default capture type; prepare for checkin

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 537013
User impact if declined: Add-ons targeting the per-tab findbar will have a difficult time supporting Firefox 25.
Testing completed (on m-c, etc.): On m-c since 6 August 2013.
Risk to taking this patch (and alternatives if risky): None.
String or IDL/UUID changes made by this patch: None.
Attachment #785542 - Flags: approval-mozilla-aurora?
Thank you for this patch, I for one do appreciate it!
Comment on attachment 785542 [details] [diff] [review]
Let test event listeners use default capture type; prepare for checkin

Thanks Adam
Attachment #785542 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
While testing for the pre-beta sign-off of the Find Bar Redesign feature, I tried verifying this issue on Mac OS X 10.9, Win 7 64 bit and Ubuntu 13.04 64bit, with the latest Aurora (build ID: 20130903004001) and this is what I found:

- I was able to install FindBar Tweak 1.3.1 only on Mac (I think it doesn't support other platforms) and the searches made through the find bar seem to work as expected

Is there anything else that QA can do here? Thanks!
Flags: needinfo?
I am able to install FindBar Tweak in all three major platforms (Windows XP/7/8, Linux and Mac), in the latest Aurora and Nightly builds, and correctly run searches in all platforms with or without FBT. In my opinion, in respect to the patch in this bug, everything seems great to me.

If you meant you tried to install FBT in Windows/Linux and failed, could you please e-mail me about it? (if anything appears in the error console, if installing in a clean profile, the usual suspects...) That is definitely some kind of bug as the add-on supports all three platforms.
> If you meant you tried to install FBT in Windows/Linux and failed, could you
> please e-mail me about it? (if anything appears in the error console, if
> installing in a clean profile, the usual suspects...) That is definitely
> some kind of bug as the add-on supports all three platforms.

I've retried to install and run searches with FBT on Win 7 and Ubuntu 13.04 (all on 64bit) and latest Aurora, and everything works ok now. (false alarm)

Thanks for the info. Marking this verified, per comment 16, comment 17 and comment 18.
Status: RESOLVED → VERIFIED
Flags: needinfo?
You need to log in before you can comment on or make changes to this bug.