SiteNavBar on autoshow should stay visible if any current tab has links

RESOLVED EXPIRED

Status

enhancement
RESOLVED EXPIRED
18 years ago
9 years ago

People

(Reporter: sballard, Assigned: sballard)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

18 years ago
This bug is for a temporary solution until bug 102990 can be solved properly. I
propose that if the sitenavbar (link toolbar) is set to autoshow and *any*
current tab has links, the toolbar should be visible. This will eliminate the
movement of the tab bar when switching between tabs.

Cc'ing hyatt for tabbrowser insight, and gerv and choess for general linktoolbar
background :)

Would this be good to have as a temporary workaround?
Assignee

Updated

18 years ago
Blocks: 103053
Assignee

Comment 1

18 years ago
Hyatt, I tried looking at tabbrowser.xml but it's a little bit complex :)

I'm looking for the answer to three questions:

1) Is there an object which represents the current tab, that I can hang data
off, and how can I get that object from code in linkToolbarOverlay.js?

2) Is that object still available if the tabbrowser is disabled?

3) If the answer to 2 is "no", where can I hook some code in to find out when
the tabbrowser becomes enabled or disabled?

Given answers to these, I should be able to produce a patch for this bug pretty
easily.
I think this would be a wasted effort. 

The links toolbar (I will continue to call it that, no matter what its PR name)
is currently off by default, and I don't plan to push to turn it on until it's
part of the content area, loads before the content using special C++ events (or
whatever it was hyatt said) and is XBLified. People who turn it on meantime can
cope with the issues we still have to fix.

Let's not insert Band-aids, but get on with fixing issues the right way.

Gerv
->claudius, who tests toolbars.
QA Contact: sairuh → claudius

Comment 4

18 years ago
Browsing www.apple.com in a tab with the Site Navigation Bar set to "Show only
when needed" is a good way to see what this bug is talking about.
Assignee

Comment 5

18 years ago
This patch addresses this bug, bug 103097 and bug 102095. It incorporates the
patch from 103097 that is already in the process of being reviewed - at the
moment I'm having difficulty figuring out how to create a diff "on top" of that
one using patchmaker without including the changes from that patch too. As soon
as I can figure out how, I'll submit an updated patch for just this and bug
102095.

The changes for bug 102095 and this bug go together pretty well, so I'll keep
them as a single patch.

The trick I used here was essentially to allow the browser object to have a
"veto" over whether the toolbar would get hidden or not. This is done by
implementing a hasLinks property; the toolbar sets this property according to
the current page (ie the current tab), but the tabbrowser <getter> returns true
if *any* tab has links.

I'm attaching this patch for testing rather than for review as such - I'll wait
until bug 103097 is in before producing an incremental patch and getting review
on that. But anyone who uses tabs with links is welcome to try this patch - you
can use it on a binary build too, by using patch maker.
Assignee

Comment 6

18 years ago
Oops, all of those "102095"s should have been bug 102905, sorry!
Assignee

Updated

18 years ago
Attachment #63047 - Attachment description: uber-patch fixing this, bug 103097 and bug 102095 → uber-patch fixing this, bug 103097 and bug 102905
Attachment #63047 - Flags: needs-work+
Assignee

Comment 7

18 years ago
This is the same patch as attachment 63047 [details] [diff] [review], but re-diffed against a tree that
already had the patch to bug 103097 applied. This makes it clearer exactly what
changes are part of this patch. You must apply the patch from bug 103097 before
applying this patch.

Comments and testing of this patch are still appreciated :)
Assignee

Updated

18 years ago
Blocks: 119088
Assignee

Comment 8

18 years ago
Comment on attachment 63047 [details] [diff] [review]
uber-patch fixing this, bug 103097 and bug 102905

Obsolete because bug 103097 is now checked in; use subsequent patch instead.
Attachment #63047 - Attachment is obsolete: true
Assignee

Comment 9

18 years ago
Comment on attachment 63064 [details] [diff] [review]
Incremental patch - apply over bug 103097 patch

Should have had needs-work on this from the start - it needs just as much as
the previous patch, of course :)
Attachment #63064 - Flags: needs-work+
Assignee

Comment 10

17 years ago
Should be possible to implement this a slightly more "right" way now that bug
102905 has landed. The following things can be changed in the patch to make it
happen:

1) Delete the chunk that starts with "if (linkToolbarUI.isLinkToolbarEnabled())"
(second chunk). That's bug 102905 and therefore already fixed, I think.

2) Remove "linkToolbarUI.updateVisibility();" from the fourth chunk.

3) Make sure a "select" event (or some other event that the linkToolbarUI can
capture) is fired in both the places that updateVisibility() used to get called.
Make sure that whatever event it is, updateVisibility() gets triggered by it.

Note that by making these changes, there would be no references to the
linktoolbar in the tabbrowser. The only addition would be the hasLinks property,
which is a generic boolean property on the browser which, when set on the
browser object, applies to the tab, but when *got* from the browser object,
returns whether it's set on *any* tabs.

Note also that this code is designed in a way that wouldn't break if a <browser>
were used instead of <tabbrowser> for any reason: then hasLinks becomes a
non-magic property and you just get back what you set.
[Mozilla/5.0 (Windows; U; Win95; en-US; rv:1.4a) Gecko/20030401]

Bug still there.

What is the current status, since comment 10 dated 2002-10-17 ?
Assignee

Comment 12

16 years ago
As far as I know that comment still reflects the current status. I have zero
time available to work on Mozilla at the moment, but someone with basic
familiarity with Mozilla hacking should be able to make those changes to the patch.

The issue with the site nav bar right now seems to be that there's nobody with
both the time and motivation to be its owner (at least, I haven't seen any
significant activity on the critical bugs in a long, long time). This can only
mean bad things for the future of the site nav bar in general :(
Product: Core → Mozilla Application Suite

Updated

11 years ago
Component: XP Apps: GUI Features → UI Design
QA Contact: claudius → nobody

Comment 13

10 years ago
This bug report is registered in the SeaMonkey product, but has been without a comment since the inception of the SeaMonkey project. This means that it was logged against the old Mozilla suite and we cannot determine that it's still valid for the current SeaMonkey suite. Because of this, we are setting it to an UNCONFIRMED state.

If you can confirm that this report still applies to current SeaMonkey 2.x nightly builds, please set it back to the NEW state along with a comment on how you reproduced it on what Build ID, or if it's an enhancement request, why it's still worth implementing and in what way.
If you can confirm that the report doesn't apply to current SeaMonkey 2.x nightly builds, please set it to the appropriate RESOLVED state (WORKSFORME, INVALID, WONTFIX, or similar).
If no action happens within the next few months, we move this bug report to an EXPIRED state.

Query tag for this change: mass-UNCONFIRM-20090614
Status: NEW → UNCONFIRMED

Comment 14

9 years ago
MASS-CHANGE:
This bug report is registered in the SeaMonkey product, but still has no comment since the inception of the SeaMonkey project 5 years ago.

Because of this, we're resolving the bug as EXPIRED.

If you still can reproduce the bug on SeaMonkey 2 or otherwise think it's still valid, please REOPEN it and if it is a platform or toolkit issue, move it to the according component.

Query tag for this change: EXPIRED-20100420
Status: UNCONFIRMED → RESOLVED
Closed: 9 years ago
Resolution: --- → EXPIRED
You need to log in before you can comment on or make changes to this bug.