Closed Bug 693029 Opened 8 years ago Closed 7 years ago

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

Categories

(Toolkit :: Find Toolbar, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: mardeg, Assigned: graememcc)

References

Details

Attachments

(1 file)

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.
OS: Windows XP → All
Hardware: x86 → All
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?
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.
Attached patch v1Splinter Review
> 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.
Attachment #668021 - Flags: review?(gavin.sharp)
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.
Attachment #668021 - Flags: review?(gavin.sharp) → review+
> 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
Assignee: nobody → graememcc_firefox
Status: NEW → ASSIGNED
And because I can be rely on to screw things up:
https://hg.mozilla.org/integration/mozilla-inbound/rev/34cc2b570666
https://hg.mozilla.org/mozilla-central/rev/46581931a047
https://hg.mozilla.org/mozilla-central/rev/34cc2b570666
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.