Closed Bug 734688 Opened 11 years ago Closed 11 years ago

OpenSearch broke session history for content tabs


(Thunderbird :: Search, defect)

10 Branch
Not set


(thunderbird12+ fixed, thunderbird13+ fixed)

Thunderbird 14.0
Tracking Status
thunderbird12 + fixed
thunderbird13 + fixed


(Reporter: mossop, Assigned: squib)



(Keywords: regression)


(1 file, 3 obsolete files)

Since Firefox 10 I've been seeing session history broken for content tabs created by my extension WebApp Tabs. I finally figured out the problem today, the browser elements created by specialTabs.js end up with the disablehistory attribute set on them. I have a workaround but it looks to me like this is a mistake caused by the landing of bug 677421.

That patch is too big for me to want to spend anytime trying to figure out exactly what the expectation is of these things, but here are two anomalies I can see that are likely bugs:

The overlay attempts to modify the dummycontentbrowser that all content browsers are cloned from to enable history by setting the disablehistory attribute to false. The problem is that that won't work, if the disablehistory attribute exists at all then history is disabled no matter its value.

As far as I can tell this should be unnecessary anyway as the specialTabs code removes the disablehistory attribute on startup before any new content tabs are created
(, this used to work fine but unfortunately bug 677421 also introduced a second element with the id "dummycontentbrowser" and it is this element that is now returned by getElementById("dummycontentbrowser") so it is that that gets its dummycontentbrowser attribute removed instead.

My best guess without really knowing what the rest of the OpenSearch code is up to is that renaming that new browser element to have a unique ID and removing the attempt to modify disablehistory from webSearchTab.xul should just make everything work.
Attached patch FIx this(?) (obsolete) — Splinter Review
It seems that somewhere along the lines, something went awry, and now the back and forward buttons don't work in opensearch. I think this is the same underlying issue as in the bug here, so let's fix it.

I'm not really sure this is the right way, but it fixes the back and forward buttons, at least.
Assignee: nobody → squibblyflabbetydoo
Attachment #606053 - Flags: review?(mbanner)
Comment on attachment 606053 [details] [diff] [review]
FIx this(?)

Review of attachment 606053 [details] [diff] [review]:

::: mail/base/content/webSearchTab.xul
@@ +67,3 @@
>              <button class="defaultButton" type="checkbox"/>
>            </vbox>
>            <browser id="dummycontentbrowser" type="content-targetable" flex="1"

Without renaming this ID you won't actually fix this bug as you'll still have two elements in the document with the same ID and the code to make session history work in content pages won't work because of it.
At the risk of turning this into an ever-expanding patch, here are fixes for the three issues I know of in OpenSearch:

1) Web history should work now (should we do things the specialTabs.js way[1]?)
2) The IDs are unique
3) The find bar shows up in the right spot (I'm surprised no one noticed this until now! Go ahead, try hitting Ctrl+F on 10.0+...)

Tests for #1 would be good, but I'm not sure how to override the engines to use a local page.

Attachment #606053 - Attachment is obsolete: true
Attachment #606116 - Flags: review?(mbanner)
Attachment #606053 - Flags: review?(mbanner)
Comment on attachment 606116 [details] [diff] [review]
Change IDs, remove disablehistory, and move the find bar

Review of attachment 606116 [details] [diff] [review]:

This looks reasonable, but we need to enable history the same way as content tabs - I don't want to impose loading global history too early in startup.
Attachment #606116 - Flags: review?(mbanner) → review-
Jim, any chance you could fix this up before next week?
Attached patch More fixes... (obsolete) — Splinter Review
Wow, some of these mistakes were really silly... everything should be working now, though. We probably ought to think about ways to test this so it doesn't regress again.
Attachment #606116 - Attachment is obsolete: true
Attachment #615204 - Flags: review?(mbanner)
Attached patch Alternate patchSplinter Review
Here's an alternate method for implementing the command controller. I think this is better, since it would have saved us from the switch statement fallthrough bug that the previous patch addresses.
Attachment #615205 - Flags: review?(mbanner)
Attachment #615205 - Flags: review?(mbanner) → review+
Comment on attachment 615204 [details] [diff] [review]
More fixes...

Yep, I prefer the other one.
Attachment #615204 - Attachment is obsolete: true
Attachment #615204 - Flags: review?(mbanner)
Comment on attachment 615205 [details] [diff] [review]
Alternate patch

[Triage Comment]
Attachment #615205 - Flags: approval-comm-beta+
Attachment #615205 - Flags: approval-comm-aurora+
Checked in:
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 14.0
btw for the tests, I think the search engine allows you to add in search engines via calls, investigation of the tests in toolkit/ might help with ideas.
Jim could you investigate the testing part further please ?
Flags: in-testsuite?
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.