Closed Bug 102832 Opened 23 years ago Closed 23 years ago

Implement autoshow feature for Links Toolbar

Categories

(SeaMonkey :: UI Design, defect)

Product:
SeaMonkey
Component:
UI Design
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.5

People

(Reporter: gerv, Assigned: sballard)

Details

(Whiteboard: fix-in-hand)

Attachments

(3 files)

Patch for autoshow functionality
8.29 KB, patch
Details | Diff | Splinter Review
Patch v.2
8.16 KB, patch
gerv
: review+
Details | Diff | Splinter Review
Patch v.3 - with super-review
8.55 KB, patch
choess
: review+
asa
: superreview+
asa
: approval+
Details | Diff | Splinter Review 
After the marathon that is bug 87428, we need autoshow for the links toolbar. If
we can get a clean and simple patch quickly, we might get it into 0.9.5 and
avoid possibly having to turn the toolbar off by default.

Stuart asked me to file this bug on him when I checked in - and I just have :-)

Gerv
CCing the usual suspects.

Gerv
Target Milestone: --- → mozilla0.9.5
This stuff should work for the wallet toolbar too shouldn't it?
If we implement this we have got to be sure that we don't _hide_ the toolbar until 
the onload handler of the next page has fired, so that a slow loading page doesn't 
cause jerking about. This is vital. If a site has LINK elements, I don't want to
switch page, watch my bar disappear, wait for the page to load, and then watch the
bar come back in again. It should just get grayed out while the status of the new
page is being determined.

Also, this will need to monitor the DOM for LINK elements being added and removed.

Finally, the hiding and showing should probably be implemented as a sliding type of
behaviour, as seen in MacIE. mpt?
Ian, there is already a patch in the original bug 84728 that does autohide/show
- what I'm working on is adapting that patch to cover the fact that the toolbar
is now part of the build process rather than an XPI, and also covering the
changes to the code since I originally wrote the patch. (btw, Gerv, Patch Maker
ROCKS, thank you!)

To address your points:

1) Correct; nothing happens until the new page has been loaded. (Well, the items
on the toolbar become disabled and then enabled again with the new links, but
the toolbar doesn't hide). This was very deliberate.

2) That's not easy right now because, as far as I know, there's no way to hook
into the creation of elements in the DOM. I had hoped to do this using XBL, but
currently XBL bound to an untrusted document is untrusted and can't modify the
toolbar. Besides, hardly anyone ever adds links to their documents dynamically,
and if they did, it wouldn't show up on the toolbar (as currently implemented)
anyway. I suggest filing a separate bug for this functionality.

3) Just try doing that with CSS ;) I'd love to, but until you can provide an
"visibility-change: animate" CSS property, I'll stick to "display: none" and
instant disappearance.
Couldn't you use a DOM mutation event to trap additions of link elements?
Um, probably, if I knew what a dom mutation event was ;)

Still, it's a separate bug from this one, because you still need to add new
links (and delete removed ones) on the toolbar whether the toolbar is autohiding
or not. Since people seem to be interested in this feature, might I suggest
filing an appropriate bug?

There is obviously an interaction with this feature, and if that gets added
before this gets checked in I'll make sure they interact properly. If it
doesn't, whoever fixes that bug will have the responsibility to ensure that the
"hasItems" flag (to be added by my patch when finished) gets set appropriately.
Seems like the best way to do this is to put a check at the end of doRefresh for
hasItems.  hide/show based on hasItems.  Add an event handler on the <head>
element listening for mutation events DOMNodeInserted, DOMNodeRemoved, and
DOMAttrModified.

Uh, Mozilla does support mutation events right?

You are right though, changing the toolbar based on DOM events is a separate bug
from autohide.
The wallet toolbar no longer exists AIUI.

My idea for the sliding appearance was to use JS to dynamically modify the
height of the containing <toolbar> from 0 to whatever the height actually is. If
you got the margins correct the buttons would slide into view from a couple of
pixels in, rather than the very edge, which would look nicer. Or, you could hide
the buttons until it was completed.

However, I think we should get the autohide code in, in as simple a form as
possible, and assess the annoyance factor. If people decide sliding is needed,
we'll try and do sliding.

Gerv
Moz has support for the mutation events, although there seems to be problems with anonymous content leaking out (see http://bugzilla.mozilla.org/showvotes.cgi?bug_id=97634). I'm not sure if it will effect this, but I thought I'd bring it to the attention of people who might know.
The attached patch provides autoshow functionality.

(again, Gerv, THANK YOU for patchmaker!!!)

This code is ready for r/sr to the best of my knowledge, but I don't have the
free time right now to commit to pushing for these things. If someone else would
volunteer to carry the torch from here, I'd be very grateful (feel free to take
the bug if you do so).

Thanks!
cool new feature!

->claudius, since he tests toolbars.
QA Contact: sairuh → claudius
That patch doesn't quite apply to the current version. I've fixed it up and
moved a CSS rule from one place to another. The current version, with r=gerv, is
coming right up.

As to its behaviour, using it on Bugzilla buglists is fine. It doesn't flicker
between lists, and it's not intrusive. We may also want smooth scrolling, but I
think this is great for now.

Gerv
Attached patch Patch v.2 — — Splinter Review 

    
    

    
  
Comment on attachment 51912 [details] [diff] [review]
Patch v.2

r=gerv.

        Attachment #51912 -
        Flags: review+

    
    

    
  
Just one suggestion, to move the CSS into
chrome://navigator/content/navigator.css instead of putting it in the theme. 
sr=hewitt

        Attachment #51920 -
        Flags: review+

    
    

    
  
Hmm, looks like only super-reviewers can add "has-super-review".  This also needs 
a= to be checked in now, yes?

Blocks: LinkUI
Keywords: patch
Whiteboard: fix-in-hand

    
    

    
  
Comment on attachment 51920 [details] [diff] [review]
Patch v.3 - with super-review

a=asa (on behalf of drivers) for checkin to 0.9.5
Also adding has-sr status since Joe forgot.

        Attachment #51920 -
        Flags: superreview+

        Attachment #51920 -
        Flags: approval+

    
    

    
  
Checked in.

Gerv
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED

    
  
No longer blocks: LinkUI

    
    

    
  
I thought that the wallet toolbar was removed because of the lack of this
feature, so it might be worthwhile running a flag up the pole.

    
    

    
  
Actually, the wallet toolbar was removed because of this feature; however, I
think that this will be a bit different, for several reasons. We'll have to see :-)

Gerv

    
    

    
  
mass-verifying claudius' Fixed bugs which haven't changed since 2001.12.31.

if you think this particular bug is not fixed, please make sure of the following
before reopening:

a. retest with a *recent* trunk build.
b. query bugzilla to see if there's an existing, open bug (new, reopened,
assigned) that covers your issue.
c. if this does need to be reopened, make sure there are specific steps to
reproduce (unless already provided and up-to-date).

thanks!

[set your search string in mail to "AmbassadorKoshNaranek" to filter out these
messages.]
Status: RESOLVED → VERIFIED
Product: Core → Mozilla Application Suite

    
  
Component: XP Apps: GUI Features → UI Design

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: