Last Comment Bug 693029 - Ctrl+F shouldn't work to open Find toolbar in about:permissions for realz this time.
: Ctrl+F shouldn't work to open Find toolbar in about:permissions for realz thi...
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Find Toolbar (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla18
Assigned To: Graeme McCutcheon [:graememcc]
:
Mentors:
Depends on: 661823 843432
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-07 23:25 PDT by Mardeg
Modified: 2013-02-20 18:38 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (3.80 KB, patch)
2012-10-04 09:29 PDT, Graeme McCutcheon [:graememcc]
gavin.sharp: review+
Details | Diff | Splinter Review

Description Mardeg 2011-10-07 23:25:20 PDT
This is "to address the remaining issues" bug 661823 didn't.
STR:
1. Launch Firefox
2. Go to about:permissions
3. Press CTRL+F

Result: Find toolbar opens.
Comment 1 Graeme McCutcheon [:graememcc] 2012-04-18 02:55:08 PDT
This isn't working - too early?
http://hg.mozilla.org/mozilla-central/file/0c7e2911be75/browser/base/content/browser.js#l4842

Dump debugging is telling me at this point, readystate is uninitialised, and though content.document is the XULDocument, it doesn't have any children or siblings at that point, and content.document.documentElement is null. Which of course, makes one of the if conditions fail, and leaves disableFind at false.

And it's broken in new tab, and about:config too.

Can the check go in endDocumentLoad, or is that too late?
Comment 2 Graeme McCutcheon [:graememcc] 2012-10-03 09:49:36 PDT
In fact, I think the only reason this even works for about:addons is because of the way addons loads content for the various panels after extensions.xul has loaded - we call onLocationChange a second time, and at this point we've loaded enough of extensions.xul to find the disableFastFind attribute.
Comment 3 Graeme McCutcheon [:graememcc] 2012-10-04 09:29:04 PDT
Created attachment 668021 [details] [diff] [review]
v1

> Can the check go in endDocumentLoad, or is that too late?
No it can't. These aren't network loads.

This seems to do the trick. I had to modify the test slightly for about:addons to wait for the correct load event.

Note that this doesn't solve the new tab page - at least not when it's launched from the new tab button (the patch works fine when manually typing about:newtab). Newtab currently fails (and continues to fail with this patch) for a different reason from the other pages - tabbrowser's mTabProgressListener suppresses onLocationChange calls for the new tab page, so we never get an opportunity to disable find. I can't see an easy fix for this.

With preload true, this seems to only be a problem the first time the new tab page is displayed - it works once the page is preloaded and swapped in.
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-05 03:11:22 PDT
Comment on attachment 668021 [details] [diff] [review]
v1

Thanks for cleaning this up - it's much better now!

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>   onLocationChange: function (aWebProgress, aRequest, aLocationURI, aFlags) {

>       // Disable find commands in documents that ask for them to be disabled.
>       if (aLocationURI &&
>           (aLocationURI.schemeIs("about") || aLocationURI.schemeIs("chrome"))) {
>+        // Ignore location changes for replaceStates in about:addons

nit: I don't like the phrasing of this comment, because it implies those are the only cases that matters (they may be the only known cases, but this code would like to ignore all SAME_DOCUMENT location changes). I would prefer:

// Don't need to re-show/hide find comments for same-document location changes
// (e.g. the replaceStates in about:addons)

>+  shouldDisableFind: function(aDocument) {

>+  disableFindCommands: function (aDisable) {

Can you just define these in onLocationChange, rather than putting them on XULBrowserWindow? That object is already kind of polluted with all sorts of unrelated stuff, I don't want to make that worse.
Comment 5 Graeme McCutcheon [:graememcc] 2012-10-05 04:06:53 PDT
> Can you just define these in onLocationChange, rather than putting them on XULBrowserWindow?
I took the opportunity to also move the event listener, to tidy up that code block.

https://hg.mozilla.org/integration/mozilla-inbound/rev/46581931a047
Comment 6 Graeme McCutcheon [:graememcc] 2012-10-05 04:20:24 PDT
And because I can be rely on to screw things up:
https://hg.mozilla.org/integration/mozilla-inbound/rev/34cc2b570666
Comment 8 Graeme McCutcheon [:graememcc] 2012-10-08 01:40:35 PDT
See comment 6:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4db6171d4fac
Comment 9 Ed Morley [:emorley] 2012-10-08 07:37:55 PDT
https://hg.mozilla.org/mozilla-central/rev/4db6171d4fac

Note You need to log in before you can comment on or make changes to this bug.