693029 - Ctrl+F shouldn't work to open Find toolbar in about:permissions for realz this time.

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Mardeg]

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]]

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]]

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]]

Attachment: 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: [e-mail]]]

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]]

> 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]]

And because I can be rely on to screw things up:
https://hg.mozilla.org/integration/mozilla-inbound/rev/34cc2b570666

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/46581931a047
https://hg.mozilla.org/mozilla-central/rev/34cc2b570666

Comment 8

[Graeme McCutcheon [:graememcc]]

See comment 6:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4db6171d4fac

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4db6171d4fac