Closed Bug 102905 Opened 23 years ago Closed 22 years ago

Site navigation (links) toolbar doesn't update when tabs are switched

Categories

(SeaMonkey :: Tabbed Browser, defect)

Product:
SeaMonkey
Component:
Tabbed Browser
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: ian, Assigned: drbrain-bugzilla)

References

(
URL
)

Details

(Keywords: polish, Whiteboard: [Hixie-P0])

Attachments

(1 file, 3 obsolete files)

Up-to-date patch
1.46 KB, patch
sballard
: review+
bzbarsky
: superreview+
blizzard
: approval+
Details | Diff | Splinter Review
STEPS TO REPRODUCE Open the link toolbar (View | Show/Hide | Link Toolbar). Go to a web page with links, e.g.: http://www.bath.ac.uk/~py8ieh/internet/eviltests/link2.html Open a new tab (Ctrl+T). Go to a web page with different or no links (e.g. http://www.w3.org/). Switch to the other tab. ACTUAL RESULTS The link toolbar doesn't switch with the tabbed browser stuff. cc'ing usual suspects as per bug 102832 + hyatt.
Keywords: testcase
Whiteboard: [Hixie-P0]
I'm seeing this problem on 2001100303 in Windows 98SE, switching back and forth between this bug and the Mozilla start page. Reloading the tab fixes the links.
Please have a look at bug 102990. I posted an idea there that is closely related to this.
*** Bug 103007 has been marked as a duplicate of this bug. ***
The link toolbar can use the DOM to add a "select" event listener to the tabbrowser. It will then be notified whenever the tabs are switched. This listener should check the target of the event to make sure it's a tab, though, since select also fires for the deck section of the tabbrowser as well.
Blocks: 103053
Interesting comment by hyatt there - internally the tabbrowser looks for the select event on the deck section rather than the tabs...
Yeah, you could do that too. Listen for either one.
*** Bug 103486 has been marked as a duplicate of this bug. ***
*** Bug 103901 has been marked as a duplicate of this bug. ***
updating summary for new toolbar name
Summary: Link toolbar doesn't update when tabs are switched → Site navigation toolbar doesn't update when tabs are switched
Don't know how I missed getting cc'd to this one :) I can probably put together a fix for this but it would actually be a wonderful bug for anybody with win or linux, a nightly build, and patch maker to try out their hacking skills on :) All you need to do is modify the initHandlers function in content/navigator/linkToolbarOverlay.js to add a handler to the "select" event (as described by Hyatt) that does the same thing as "load". I might be able to have a go at this tonight if nobody else gets to it first, but I really can't think of a better bug for someone who's wanting to make their first steps into mozilla hacking, especially if you are a user of tabs who's hurt by this bug. Anybody out there fit that profile?
Who am I to resist such a challenge? Grabbing the tarball now.
Attached patch First cut at patch--doesn't work. (obsolete) — — Splinter Review
All right. The patch I just submitted seems to be doing what Stuart suggested--adding a event listener for the "select" event--but it definitely doesn't work. I'm not entirely sure why, but since this is my first attempt at messing with Moz's JS, that's no great surprise. Any ideas? Am I going about it entirely the wrong way?
I don't think the content area actually gets the select event. I think you need to add the event listener to the tabbrowser container. Look in tabbrowser.xml for a clue about how the <tabbrowser> event stuff is working. http://lxr.mozilla.org/mozilla/source/xpfe/global/resources/content/bindings/tabbrowser.xml
Meep. I'd be lying if I said that I understood much of what's going on in tabbrowser.xml, much less why the content area isn't getting the "select" event upon the listener receiving it. Looking at that XML file, I see a lot of work with the progress listener--I'd assume that's for the nifty little rotating arrow animation that appears whenever the page is loading--and some hooks for a title listener, which activates when the title changes and is (sensibly) only kicked off once you actually open a second tab. I'm not sure how to go about adding a listener for the SNToolbar there, if it's necessary.
OK, a few problems here: 1) the select event is fired on the tabbrowser's tabbox. Search for 'select' in tabbrowser.xml to see where the handler is set up. 2) the "refresh" method does: if (event.originalTarget != getBrowser().contentDocument) return; This will need to be changed slightly to handle the onselect being fired, since that's _not_ fired on the contentDocument.
Methinks this is beyond my tiny Moz-hacking skills. Someone else feel free to give it a whack.
Hm. I've just tried a slightly modified version of this, and it seems that the "select" event gets fired *before* the document is actually updated. Thus selecting a tab causes the toolbar to act as it should have acted for whichever tab was *previously* selected, rather than for the newly selected tab. Is there a "correct" way to avoid being called until after the select operation is complete, or will I just need to use setTimeout()? Also, I removed the target check completely on the tab-selection, because I couldn't figure out what to check for. Any tips? I'll attach my work-in-progress patch below.
Btw, Phil, sorry for underestimating the complexity of this. I forgot about the "originalTarget" check, and I didn't realize that the event was firing somewhere different than the others.
Maybe I'm missing something... but would it not make sense to hook into the updateCurrentBrowser() method in tabbrowser.xml? That's _already_ listening to the right event and doing target checking and all that. Stuart, with that patch is your event getting triggered before or after updateCurrentBrowser() is called?
Putting the code into updateCurrentBrowser would break the self-containedness of the linkToolbar, although I can see doing that if it makes sense. Also (pragmatic reason) PatchMaker is telling me I can't edit tabbrowser.xml because it can't find the original source, so I can't easily produce patches for it (I might be able to hack chromelist.txt to avoid this problem, though). It appears that selectTab is called (twice, no less) *before* updateCurrentBrowser gets called at all. Does that help? :)
No problem. When you've never submitted a patch, you can't do any worse than a non-working one. At least I tried. ;)
*** Bug 106368 has been marked as a duplicate of this bug. ***
*** Bug 104671 has been marked as a duplicate of this bug. ***
The fact that the select fires *before* the tab has finished being selected is a bug. The problem is this: The tabs element fires the select before switching the tabpanels display. This means the tabbrowser element has to catch the select on the tabpanels rather than the tabs element. However this doesn't work when deleting the first tab, and the tabbrowser element has a hack to work around this. If the patch to 106656 is checked in then the tabs element will fire the select late enough for the tabbrowser to be able to catch it instead of the current hack. Once this is done you will be able to catch it after the tab has switched. Questions?
Depends on: 106656
*** Bug 108278 has been marked as a duplicate of this bug. ***
*** Bug 110511 has been marked as a duplicate of this bug. ***
*** Bug 111620 has been marked as a duplicate of this bug. ***
throwing another keyword in the summary to make this easier to find. I almost made a dup bug because I always thought of it as the links toolbar instead of site navigation toolbar. -matt
Summary: Site navigation toolbar doesn't update when tabs are switched → Site navigation (links) toolbar doesn't update when tabs are switched
*** Bug 114319 has been marked as a duplicate of this bug. ***
There is not assocation between a tab and the associated navigation bar (when it's supposed to exist), so when I change from one tab A to tab B, the navigation bar of tab A stay in place. Now I am reading tab B and if I click on up in the navigation bar, the tab A page is loading in tab B! (Win NT 4 + Moz build 2001122703).
Attached patch Working, but hacky, patch (obsolete) — — Splinter Review
I don't *like* this patch, but it does solve the problem. In an ideal world, the linktoolbar would be self-contained and be able to rely on events being fired to update itself. Unfortunately, the tabbrowser isn't firing the select event at the right time, so that isn't possible right now (bug 106656 apparently). Instead, this patch just gets the tabbrowser to update the linktoolbar manually, if applicable. This makes tabs and the linktoolbar usable together, almost. Issues with this patch: - It conflicts with the patch for bug 103097, which will hopefully be checked in soon if I can get sr on it. I'll produce an updated patch when bug 103097 is finalized. - The toolbar pops in and out of existence as you switch between tabs that do and don't have links. This is a pain in the neck. Bug 103428 tracks one workaround to this; bug 102990 tracks another. I believe that this patch is still sufficiently better than the current situation that it might be worth checking in. If this beats bug 103097 into the tree, then I'll update that patch to cover the necessary changes to this code, too. Either way this bug should be kept open until bug 106656 is fixed, so this can be done in a more elegant way.
Attachment #52780 - Attachment is obsolete: true
Attachment #52904 - Attachment is obsolete: true
I've produced a big patch containing these changes and bug 103097's patch and attached it to bug 103428. If you want the ultimate tabs-and-links experience, I suggest applying that patch :) It's not ready to checkin yet, mostly because it's not available separately from the changes in bug 103097, but testing would be appreciated. Users of binary nightly builds can apply the patch using patch-maker.
Upping severity to major -- this is probably the worst Link Toolbar bug there is. It regularly interferes with my using the link toolbar on a daily basis.
Severity: normal → major
The sidebar has the same problem. For instance, the DOM Inspector sidebar pane is not updated when tabs are switched.
Blocks: 119088
The patch in bug 103428 works for this problem - the only issue is that it breaks the self-containedness of the linktoolbar by putting linktoolbar-specific code in the tabbrowser. I'll work on figuring out a way to do this with events that will avoid that problem. This and bug 103428 are legitimately separate problems, but I'm working on the two together and patches will probably keep appearing there rather than here.
*** Bug 119395 has been marked as a duplicate of this bug. ***
Does the proposed fix for this bug include the case when both the tabs have <link> tags ? In this case, switching between tabs and clicking on one of the site-navigation links should load the correct document. If not, then I think that closing bug 119395 as duplicate is not correct.
*** Bug 124115 has been marked as a duplicate of this bug. ***
I can confirm this bug in 2002022203/WinXP. The two sites are www.w3.org in second tab and richinstyle.com in first tab.
Is there any chance of getting this fixed by 1.0? (As this surely blocks lots of people from using the site navigation bar...)
The patch works *now*. The problem is that it's not solving the problem in a clean way. It's occurred to me that this bug might be suitable for being checked in on a 1.0 *branch*, if there is such a thing, so that it goes into the 1.0 codebase but isn't there in 1.1, so that a "real" solution can be worked out for the future of the main codebase. But I don't know if drivers would accept that. Anyone want to do the necessary politicking?
I have observed this in 0.9.8 release (20020213) on FreeBSD. Would somebody with sufficient privileges please update the OS field (I wish Mozilla trusted me!).
OS: Windows 2000 → All
Hardware: PC → All
*** Bug 135702 has been marked as a duplicate of this bug. ***
I'm very sorry to see that this bug is still present so close to the 1.0.0 release. I was very excited when the site navigation bar was introduced quite some time ago, and I was quick to add LINK tags to my site <http://www.autark.se/>, but I have gradually stopped using the bar because I cannot rely on it. Having to do a manual reload after switching tabs is IMO unacceptable. I have been patient while Mozilla was in a beta stage, but as I see it, the current state of the site navigation bar is unacceptable for release, so either the bug must be fixed or the bar disabled (or at least hidden), in order to prevent embarrassment when Mozilla is released to the general public. Tested in build 20020408-14.
Per, it seems likely the site navigation bar will be backed out completely for 1.0, so this bug is the least of our worries (please don't discuss the merits of that decision in this bug - I don't like it much either, but unless someone other than me can create a miracle patch for bug 102992 and get it reviewed, super-reviewed and approved to be checked in in the middle of a deep code freeze, it is probably going to happen). My hope was that we could check in this hacky patch and enable the toolbar by default for 1.0, but it isn't going to happen. I guess we have to hope for 1.1 or so. If you want *this* bug to get fixed, the way to go about it is to pester the tabbrowser people to fix the dependent bug 106656. I don't think it's possible to produce a clean-enough-to-get-approved patch for this bug without 106656 being fixed.
*** Bug 140745 has been marked as a duplicate of this bug. ***
*** Bug 148999 has been marked as a duplicate of this bug. ***
*** Bug 154984 has been marked as a duplicate of this bug. ***
I'm starting to think that if we don't fix this bug, we should just back out the links toolbar. As it is, it is causing me to get very confused navigating to the wrong page more often than it is helping me.
if this bug (which seems to depend on a fix for tabbed browsing), could we not just disable the link toolbar when there are multiple tabs? alternatively, how about backing out the tabbed browser? seems to have quite a few bugs in it that aren't getting fixed...
Backing out tabbed browsing? Please don't! For many people (including myself) this feature is the main reason to use Moz.
It seems to me everybody is picking this particular hammer up by the head rather than by the handle - why not move the link bar and side panel *into* the content area, so that when you switch tabs you automatically switch link bar and side panel? This also prevents the tabs from bouncing up and down if you have "show linkbar only when needed" active, which is visually distracting. It seems to be that both the link bar and the side panel are logically part of the page being viewed. And I second the other comments - DON'T YOU DARE TOUCH MY TABBED BROWSING!
FWIW, I agree with links being sensibly associated with page content, but I do not agree that sidebar contents should be associated with a specific page. I have many sidebars (including the default bookmarks, history, search sidebars) which have a much larger scope than one web page's contents. RE: link toolbar should be moved to just above the content, skip up eighty-five bugs to bug 102990 -matt
If we're going for solutions as drastic as backing out the linktoolbar, I'd suggest instead checking in the patch in bug 103428. From a user standpoint, that patch is a complete fix to the problem (unless it's bitrotted since being written, which is possible I guess - someone would have to check it). The only major issue with the existing patch (other than possible bitrot) is that it makes the tabbrowser specifically aware of the existence of the linktoolbar, which it shouldn't have to be because the tabbrowser could conceivably be used in a place where there isn't a linktoolbar at all. Fixing that would require a fix for bug 106656. I've begun the attempt to raise hell in that bug ;)
With "Mozilla/5.0 (Windows; U; Win95; en-US; rv:1.2a) Gecko/20020910" Same bug: I did not read all of the Additional Comments ... but the initial bug description is still valid.
Well the patch to bug 106656 has now been checked in...
Anyone feel like trying the "work in progress patch" (that's currently marked obsolete, but may not be any more) to see if it is now sufficient to fix the problem? I don't have the capability to hack Mozilla any longer since my monitor at home blew up... The patch might have bitrotted, but with bug 106656 in the tree, an approach along those lines should be enough to make this work without you needing to click each tab twice. The one other thing that would probably be worth doing is adding a check on event.originalTarget.something (tagName maybe?) in the selectTab function to make sure that selectTab only gets called once. Try putting a dump("selectTab: tagname=" + event.originalTarget.tagName) into selectTab and watch the console. You'll probably get two dumps every time you click a tab. Then you can put an if(event.originalTarget.tagName != "whatever") return; into selectTab to filter out one or the other of them, to avoid the penalty of doing a full refresh twice per tab. There are better ways to do this but I can't think of any that don't involve accessing the Tabbrowser object's anonymous content. I still think it would be nice if the Tabbrowser object did a preventBubble() on one or the other of the two select events it fires.
> Anyone feel like trying the "work in progress patch" Possibly on Thursday... > I still think it would be nice if the Tabbrowser object did a preventBubble() > on one or the other of the two select events it fires. So have you filed a bug (and CC'd me) on the double select event issue?
Attached patch Up-to-date patch — — Splinter Review
Attachment #62996 - Attachment is obsolete: true
Comment on attachment 103051 [details] [diff] [review] Up-to-date patch What about XUL-as-content that happens to include a tabbed ui in the XUL? If that's not an issue, sr=bzbarsky
> What about XUL-as-content that happens to include a tabbed ui in the XUL? Then there won't be any links anyway, will there? Also <tabbrowser> includes an <tabpanels onselect> so if any events bubble up from content they'll affect <tabbrowser> as well :-P
Comment on attachment 103051 [details] [diff] [review] Up-to-date patch > Then there won't be any links anyway, will there? Why not? XHTML-namespace meta-tags in a XUL document _may_ work.... (they _should_ work, but XUL is fucked up like that). In any case, that's a core tabbrowser issue; please file it.
Attachment #103051 - Flags: superreview+
I'm confused. Why would XUL-as-content be remotely problematic? All this patch does is get the linktoolbar to listen to select events and refresh its content when they happen. All that would happen in the XUL-as-content-that-contains-a-tabbrowser case is that the refresh would happen sometimes when it actually shouldn't. Since the refresh would just reload the same links as already exist, I don't see a problem, except for a potential slight slowdown on tab-switching in the nested UI, in the unlikely case that there *are* links in the document. Stuart.
Heh. True. Well, my sr stands; someone needs to review the patch, though.
r=sballard@netreach.net on the patch, with the caveat that I can't actually test it, so someone else will have to do that (please make sure somebody actually does before setting has-review). With this code in place it should be possible to update the patch in bug 103428 to work well also: I'll comment over there on exactly what it would take. I expect that this patch will become annoying quickly without that one.
OT: Creating the toolbar via XBL would be easy (content in XUL), but for bug 174614. Without it, the the buttons will need to be ordered manually, which will not give any speed benefit. (requires at least one call to document.getElementsByTagName('link')). In fact, much of the existing toolbar could probably be dropped into an XBL binding attached to <head>... but that's a different bug ;)
drbrain, we already don't call getElementsByTagName except for two situations: 1) when the toolbar is turned on for the first time, so we don't have any stored state. 2) tab switching. #2 could be eliminated by storing all the necessary data in the tab object, which would be great if you feel like rearchitecting the linktoolbar to make that possible, but right now is somewhat tricky. I suppose you could just store an array of HTMLLinkElement objects hanging off the tab object and iterate over them on tabswitch. #1 can't ever be eliminated because we haven't even got any code loaded at that point (in an ideal world) so we can't possibly know what links should be present. The conversion to XBL (bug 102992 I think) has nothing to do with page-load performance, but rather startup and new-window performance (by deferring the load of all linktoolbar code until it's actually needed). The majority of the difficulty there lies in bootstrapping: when you get a Link event, you want to call some code to add it to the toolbar, but the toolbar hasn't been created yet. So you load the XBL binding that creates the toolbar, but by the time that's done, you've forgotten about the Link event. Solvable problem, but it's a pain in the neck, which is why that patch was so hard to write (and then, with my monitor blowing up, it's stuck in perpetual limbo...)
I tested attachment #103151 [details] and it worked perfectly. I didn't find any problems during my extensive 10 minute test. I will use this patch during my normal browser use and will report here if I find any problems. The test was made using cvs trunk on Linux. In my opinion this patch is ready to be checked in. Thanks guys, great work!
Ok, we can do this the hard way, or take an easy path. |updateCurrentBrowser| in tabbrowser.xml already does important stuff for tab switching, like selecting content/content-primary, something you need. So I wonder why we don't add a little snap like this: + if (linkToolbarUI.isLinkToolbarEnabled()) { + linkToolbarHandler.clearAllItems(); + linkToolbarUI.fullSlowRefresh(); + } to |updateCurrentBrowser| in tabbrowser.xml? That will solve the real problem, without using a new set of |addEventListener/removeEventListener| This will also prevent the 'quirky ordering' in the patch for bug 173703.
Because the principles of "information hiding" also known as "non-spaghetti maintainable code" demand that tabbrowser know nothing of the existence of link toolbar (consider the case of something like phoenix that wants to use the <tabbrowser> widget but does not want a link toolbar. They should not have to fork the widget....).
I prefer attachment 103051 [details] [diff] [review]. See the previous comment for why. You'll want to listen for select events coming from <tabpanels> though, since then you'll know for sure you get the right <browser>.
Oh well, the "non-spaghetti maintainable code" argument sounds fair but one note, the Phoenix <tabbrowser> widget is already forked.
*** Bug 176032 has been marked as a duplicate of this bug. ***
HJ: All the more reason to not make the linktoolbar depend on code in the tabbrowser! Consider what happens when someone wants to write a linktoolbar that's available as a downloadable addon to Phoenix, where their tabbrowser doesn't have this code. Now think how much easier it would be with the patch as it stands: all the code is already in the linktoolbar so it can just be shipped as-is. (Just don't get me started on the Phoenix-guys claims that the linktoolbar is a "geek" feature and shouldn't be included...) Btw, does anyone have any comments on bug 103428 yet? (comments over in that bug, please...)
> You'll want to listen for select events coming from <tabpanels> though, > since then you'll know for sure you get the right <browser>. No you won't, becuase sometimes <tabpanels> won't fire the select event, which is why the hack to call UpdateCurrentBrowser by hand was included. In fact, this was one of the points of fixing the select events on the <tabs> element - the old select event always fired before the current browser updated, and was therefore completely useless. The new select event can be made to fire at the right time.
*** Bug 176921 has been marked as a duplicate of this bug. ***
FWIW, I've been using the patch in attachment #103051 [details] [diff] [review] in my regular browsing experience for about a week now. I haven't been rigorously testing every aspect of it, but I have been dogfooding it, and I've had no troubles. No noticeable performance changes, no problems with wrong links showing up, and nothing seems to have broken anywhere else. Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2b) Gecko/20021020
Comment on attachment 103051 [details] [diff] [review] Up-to-date patch Since several people seem to have tested this now and report success, and I've reviewed the patch from a code point of view and think it looks good, r=sballard
Attachment #103051 - Flags: review+
will the patch get into 1.2?
Has anyone even asked for approval for 1.2?
This bug requires the patch at 173703?
Er, no. What gives you that idea? It required bug 106656, but that went in already. From reading the comments in bug 173703 it sounds like early versions of that patch would have regressed bug 106656, but I can't find any indication that bug 106656 was backed out...
Blame me for confusing Asa.
Comment on attachment 103051 [details] [diff] [review] Up-to-date patch a=blizzard on behalf of drivers for 1.2 final. Please try to get this in before the tree closure on the morning of Nov 5, 2002 otherwise it's going to have to wait until after the branch has finished being cut.
Attachment #103051 - Flags: approval+
checked in
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
This bug is not fixed. I tested with 2002110504-trunk/WinXP. 1. Create two tabs(right tab has link elements). 2. Close left tab by middle click. All buttons on site navigation toolbar are disabled.
Related: bug 173703? (closing tab with middle-click).
That's probably because closing a tab does not fire on onselect event. I'm starting to think that tabbrowser should fire off "tabswitch" events when tabs are switched (added, removed, whatever). Then the link toolbar could catch them....
bz: the problem is that the tabbrowser doesn't consistently handle tab switch events generated during a tab close (bug 173703).
Yes, the behaviour in comment 88 should be fixed when the fix for bug 173703 is checked in (soon, hopefully). This bug (toolbar not updating on [normal] tab switch) is fixed.
*** Bug 179077 has been marked as a duplicate of this bug. ***
*** Bug 179465 has been marked as a duplicate of this bug. ***
This is indeed fixed.
Status: RESOLVED → VERIFIED
*** Bug 169114 has been marked as a duplicate of this bug. ***
*** Bug 179879 has been marked as a duplicate of this bug. ***
This bug (fix) appears to have regressed. The reporter in bug 192888 suggested trying b.m.o and Mozillazine. With 2003022308 W2K I see the site navigation toolbar auto hide while Mozillazine is loading, or the first time you switch to it if you let it load in the background. If you keep switching back and forth between the two tabs the toolbar will eventually get left on the Mozillazine tab. If you then reload Mozillazine it vanishes again. Could somebody else verify this please?
I can verify the behavior that Arunan Bala describes with Linux build 2003020408 (about 20 days old now, I should update). The site navigation toolbar does not autohide when you switch to a tab that does not use page navigation links, but it does autohide when that page is first loaded or refreshed.
Regarding comments 98 and 99: the behavior you are describing sounds correct. This bug is not about autohide, it is about the *content* of the toolbar changing when the tab is changed. If switching to a page with no link elements causes the bar to be grayed out, and switching back reactivates the items, it is behaving correctly. The issue you are describing sounds like the intended behavior of bug 103428 (i.e. don't autohide the toolbar when switching tabs because that would cause the tabs to jump around).
Apologies to the 50000 people on the cc list ... > This bug is not about autohide, I had the impression it was as bug 179465 which was about showing/hiding the bar when switching tabs and reloading was duped against this one. > If switching to a page with no link elements causes the bar to be grayed out, > and switching back reactivates the items, it is behaving correctly. I assume you mean it's populating the bar with the correct links (none); if you have "show as needed" then when there are no links it's not behaving correctly if showing a bar with no enabled links that then vanishes when you reload.
Product: Core → SeaMonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: