60151 - back button not working after applying theme

Status: VERIFIED FIXED

(SeaMonkey :: General, defect, P3)

Description

[kgw]

From Bugzilla Helper:
User-Agent: Mozilla/4.75 [en] (X11; U; Linux 2.2.16 i686)
BuildID:    2000111421

start up mozilla, then select "view"->"apply theme" and select any of the
available. Now navigate on the displayed website to discover that the back
button will stay disabled.

Reproducible: Always
Steps to Reproduce:
1.apply theme
2.navigate using links from the current page


Actual Results:  back button stays disabled

Expected Results:  back button enabled to go back to where you came from

Comment 1

[Jacob Steenhagen]

This WORKSFORME (2000111220 on Win2k)...
What URL were you at when you tried this?  If it was a framed site, this could
be related to bug 56062.

Comment 2

[kgw]

No. I wasn't using a framed site at that moment (it was the index page for the
qt documentation)

Comment 3

[Cormac F]

Confirmed build 2000111404 WindowsNT SP5
site: www.ireland.com
opened new browser window, applied classic theme over modern theme, clicked 
onto a random link on the page and the back button does indeed remain greyed out

Comment 4

[ohely]

I see this bug on build 2000121414 (Linux); I noticed some
other stuff about it.  Here's an example.

Open http://www.mozilla.org in a new window.
Click the link to Mozilla 0.6.
Change themes (or even just apply the one you're using)
via View - Apply theme.
Click the back button.
Click the link to Mozilla 0.6 again.
-- The back button is disabled, the forward button
   looks enabled but doesn't do anything.

OR: after changing themes as above:
Click on the "bugs" link.
Click the back button (at this time it works).
(Notice that both back and forward buttons are
active).
Click on the README link.
-- The forward button is still active, though
it doesn't do anything.
Now click the back button, then the forward button.
-- This time the forward button is inactive.

  It seems that clicking a link will not cause the
status of forward and back buttons to change.  These
properties seem to be shared by the Back and Forward
selections in either the Go menu or the right-click context
menu (they reflect the buttons' status in all cases above).

  Any chance this is related to bug 61991 (URL bar gets
stuck after a theme change)?

Comment 5

[Viswanath Ramachandran]

nav triage team: looked at this bug, it is not a beta stopper. bulk update of 
several such bugs. 

Comment 6

[Tim Powell]

My observations of this bug. When you apply a theme and the current window has
the back button disabled, the back button will remain disabled from then on.
This can happen if you have your start page set to a blank page, or if you open
a new window and immediately apply a theme. The back button would normally be
available after going forward a page (or two if you started with a blank
window). This problem occurs regardless of theme.

The Go menu continues to show the history, but for whatever reason the back
button stops responding.

Could this be related to bug 44558? Perhaps something needs to change with the
way themes are applied?

Comment 7

[Tim Powell]

Should have mentioned that I observed this with Build ID: 2001010504 on WinNT
with starting page set to blank. You can reproduce it every time.

Comment 8

[Radha on family leave (not reading bugmail)]

I see the problem. 

Comment 9

[Keyser Sose]

Possibly related/dupe of bug 58416. Thoughts?

Comment 10

[Tim Powell]

*** Bug 58416 has been marked as a duplicate of this bug. ***

Comment 11

[Tim Powell]

Adding to meta skin switching blocker bug and updating to OS ALL based on 
comments here.

Comment 12

[Stephen P. Morse]

It's unfortunate that bug 58416 got duped to this one and not vice versa since 
that bug had a comment on the cause of the problem.  Therefore repeating that 
comment here:

     Reason this is happening is that the ReinitializeContentVariables() method
     of nsBrowserInstance is not getting called when clicking on links but it
     does get called when you type in a new url.

Comment 13

[Boris Zbarsky [:bzbarsky]]

*** Bug 71095 has been marked as a duplicate of this bug. ***

Comment 14

[Boris Zbarsky [:bzbarsky]]

*** Bug 71279 has been marked as a duplicate of this bug. ***

Comment 15

[djk]

Related to bug 68662?

Comment 16

[Radha on family leave (not reading bugmail)]

I think the comment about ReinitializeContentVariables() getting called for link 
clicks is not valid anymore because nsBrowserInstance.cpp no more holds a 
reference to SessionHistory. I think the problem is in having the right 
nsIwebprogressListener in the Browser object, after a theme change whose 
callbacks update the back and forward buttons.  I think that part of code has 
undergone some recent changes.  Any fix to this problem has to happen in the 
browser area. Giving it to the browser team.  I have a build from 4/9/01 and I 
don't see this bug. However a latest build depicts the problem. 

Comment 17

[Viswanath Ramachandran]

reassigning to morse. 

Comment 18

[Viswanath Ramachandran]

Steve, can you investigate and comment on the priority of this bug and whether
it is a nsbeta1+?

Comment 19

[Stephen P. Morse]

Yes, it should be nsbeta1+ and priority should be at least P2 and maybe P1

Symptoms are much worse than just the back button.  Nothing in the navigator 
window (forward button, url bar, etc) are getting updated after a theme switch.

Radha was correct in his assessement.  When the theme switch occurs, all the 
listeners are removed from the old mListenerList (see nsDocLoader.cpp), a new 
mListenerList is created, and all the listeners are reinserted into that new 
list.  All listeners that is, except for one.  That one being the listeners in 
navigator.js.

Comment 20

[Stephen P. Morse]

Here's a status update on this bug report.

The following lines of code will fix the problem:

var interfaceRequestor = getBrowser().docShell.QueryInterface
   (Components.interfaces.nsIInterfaceRequestor);
var webProgress = interfaceRequestor.getInterface
   (Components.interfaces.nsIWebProgress);
webProgress.addProgressListener(window.XULBrowserWindow);

I can demonstrate this by putting these lines of code into the BrowserReload 
function of navigator.js and then clicking on the reload button after the theme 
switch.  Once I do that, alll updates are occuring correctly.

So now I need to figure out a way of having that code automatically executed 
after the theme switch.  Note that it will not work to simply drop that code 
into the applyTheme function of navigator.js because that is too early -- the 
code must be executed *after* the next initialization occurs in nsDocLoader.

Comment 21

[Stephen P. Morse]

OK, I have something that works.  Basically I set up onclick and onkeypress 
handlers when the theme-switch occurs so that I can get control after the theme 
switch.  And at that time I reinstall the progress listeners for the new doc 
loader.

Note that I can wait for the next keypress or mouseclick because nothing in the 
browser window will change state until that happens.  For example, the back 
button won't need to be updated until the user goes to another url and he needs 
to either type in the URL field for that (keypress), tab to a link and press 
enter (keypress), or click on a link (mouseclick).

Attaching patch that accomplishes this.

Comment 22

[Stephen P. Morse]

Attachment: patch to update progress listeners after theme switch

Comment 23

[Stephen P. Morse]

r=matt

Comment 24

[Stephen P. Morse]

cc'ing alecf for super-review

Comment 25

[Stephen P. Morse]

Actually let me make a slight modification to the patch posted.  The patch 
attaches the onclick and onkeypress handles to the content area.  But that 
doesn't include the urlbar area, so we won't get control when the user navigates 
by typing in a new url.

Therefore the correction to the patch is to replace:

   var content = document.getElementById("appcontent");

with

   var toolbox = document.getElementById("navigator-toolbox");

and of course the next lines that start with "content." would start with 
"toolbox." instead.

Comment 26

[David Hyatt]

I don't understand why you have to use onclick and onkeypress listeners. 
Couldn't you just set a timeout following the applytheme function and then just
rehook up the listener in the timeout callback?

Comment 27

[Stephen P. Morse]

BTW, this patch fixes all the following bugs as well:

  bug 61796: Progress meter not working after theme switch (ben)
  bug 61853: Can't enter text in url field after theme switch (hyatt)
  bug 61991: url bar not updated after theme switch (alecf)
  bug 68227: status bar not updated after theme switch (ben)
  bug 68230: throbber stops working after theme switch (pchen)

Comment 28

[Stephen P. Morse]

Isn't that kind of lame to use a timeout?  By attaching the handlers I get 
called at just the time that I need to make the adjustment.  What if the timeout 
occurs too quickly?  What if it takes too long and the user already clicked on a 
link?

Comment 29

[Ben Goodger (use ben at mozilla dot org for email)]

setAttribute("onclick", "foopy");
&
setAttribute("onclick", "");

is probably better as

addEventListener("click", foopy, false);
&
removeEventListener("click", foopy, false);

I think jag was meaning to look into something related to this to see if he
broke something elsewhere, or failed to fix it. If he comes up blank, sr=ben. 

Comment 30

[Stephen P. Morse]

Attaching my latest patch for reference.  It's my original patch together with 
the correction that I indicated.  It also has ben's suggested change in it but 
commented out for the time being.

Reason I am posting this is because suddenly this is no longer working for me.  
I'm sure it worked earlier when I originally tested it.  By having the patch 
here I can ask someone else (namely jag) to apply it and see if it works for 
him.

Comment 31

[Stephen P. Morse]

Attachment: corrected patch

Comment 32

[Peter "jag" Annema]

Attachment: [patch] Fix. Use js web progress listener, not old (dead) appCore one.

Comment 33

[Peter "jag" Annema]

This should fix most issues, though there still is an issue on the uri bar not
updating. It looks like I'm somehow sending the new values to an old textfield,
but that seems to be a different issue.

Comment 34

[Stephen P. Morse]

Patch works.  r=morse

Comment 35

[Stephen P. Morse]

jag: with your patch the uri bar is updating fine for me.

Comment 36

[Ben Goodger (use ben at mozilla dot org for email)]

ah!

sr=ben

Comment 37

[Stephen P. Morse]

Fix checked in.

Comment 38

[selmer (gone)]

*** Bug 68227 has been marked as a duplicate of this bug. ***

Comment 39

[Claudius Gayle]

VERIFIED Fixed with 2001053104 builds