Closed Bug 318052 Opened 19 years ago Closed 18 years ago

Drag and Drop Feedback for Personal Toolbar

Categories

(Firefox :: Bookmarks & History, defect, P2)

Product:
Firefox
Component:
Bookmarks & History
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 2 beta1

People

(Reporter: bugs, Assigned: annie.sullivan)

References

Details

(Keywords: fixed1.8.1)

Attachments

(1 file, 1 obsolete file)

Addresses Ben's comments
54.54 KB, patch
bugs
: review+
Details | Diff | Splinter Review 
There needs to be drag and drop feedback for the personal toolbar. This needs to work even in Luna on XP (in Firefox 1.0 and 1.5 it only worked in the classic theme in Windows, not Luna). Rather than drawing a nasty bar like in 1.x, I suggest using an arrow like for tab D&D, or coming up with something more interesting.
On Mac we really want Safari-like sliding.
Priority: -- → P1
Priority: P1 → P2
Target Milestone: --- → Firefox 2 beta1

        Attachment #212092 -
        Flags: review?(bugs)

    
    

    
  
Comment on attachment 212092 [details] [diff] [review]
Implements drag and drop for personal toolbar and menus

>Index: browser/components/places/content/menu.xml
>+          this.startMarker = null;
>+          this.endMarker = null;

Are these private? For safety, would it be better to initialize these to -1 since these are index values?

>       <method name="_rebuild">
>         <body><![CDATA[
>+          if (this._DNDObserver._overFolder.node)
>+            this._DNDObserver._clearOverFolder();
>+            

Add a comment why this is here? I remember you telling me, but you should make it explicit. 


>-                this.appendChild(element);
>+                if (this.endMarker)
>+                  this.insertBefore(element, this.childNodes[this.endMarker++]);
>+                else
>+                  this.appendChild(element);

How does this work? What are start/endMarker used for? Brief documentation?

>-            this.appendChild(element);
>+            if (this.endMarker)
>+              this.insertBefore(element, this.childNodes[this.endMarker++]);
>+            else
>+              this.appendChild(element);

See above.

>+      <method name="hidePopupAndChildPopups">
>+        <body><![CDATA[
>+          for (var i = 0; i < this.childNodes.length; i++) {
>+            if (this.childNodes[i].getAttribute("type") == "menu" &&
>+                this.childNodes[i].lastChild &&
>+                this.childNodes[i].lastChild.getAttribute("type") == "places")
>+              this.childNodes[i].lastChild.hidePopupAndChildPopups();
>+          }
>+          this.hidePopup();
>+        ]]></body>
>+      </method>

Explain why you need this method in a comment. 

>       <method name="getCopyableSelection">
>-        <body><![CDATA[ 
>+        <body><![CDATA[
>+          if (PlacesController.nodeIsReadOnly(this._resultNode))
>+            return null;

Really? Why can't a readonly node be copied? That doesn't seem right. 

>+      
>+      <field name="_DNDObserver"><![CDATA[({
>+        // XXXben ew.

You can remove this. I can get over my misgivings about nsDragAndDrop as an API for now. 

>+        _self: this,
>+        _overFolder: {node: null, openTimer: null, hoverTime: 350, closeTimer: null},
>+        _closeMenuTimer: null,

Documentation!

>+        notify: function TBV_DO_notify(timer) {

More comments in all of these functions!

>+        _draggingOverChildNode: function TBV_DO_draggingOverChildNode(node) {

Documentation!

>+        _closeParentMenus: function TBV_DO_closeParentMenus() {

Documentation!

>+        _clearOverFolder: function TBV_DO_clearOverFolder() {

Documentation!

(I'm beginning to sound like Steve Ballmer)

>+          this.removeAttribute("autoopened");

What is this attribute for? It doesn't look standard. D!

>+      <handler event="draggesture"><![CDATA[
>+        // XXXben ew.

Again, my disgust with myself isn't worth propagating at this point. 

>Index: browser/components/places/content/toolbar.xml
>+
>+      <field name="_dropIndicatorBar">document.getAnonymousNodes(this)[0].firstChild</field>
>+      <field name="_chevron">document.getAnonymousNodes(this)[0].childNodes[1].firstChild.firstChild</field>

You might want to add a comment <!-- --> in the <content> section explaining that these elements are located in this fashion, so that people don't break this behavior when editing the content. OR - use getAnonymousElementByAttribute or whatever it is to dynamically locate it based on anonid. 


>       <method name="_rebuild">
>         <body><![CDATA[
>+          // Clear out references to existing nodes, since we'll be deleting and re-adding.
>+          if (this._DNDObserver._overFolder.node)
>+            this._DNDObserver._clearOverFolder();
>+          this._openedMenuButton = null;

Maybe in your comment explain what can actually happen if you keep holding onto nodes after a rebuild. Say where these nodes (what member variables) are held from. 

Also more documentation in this file, just like Menu. 

Can you explain to me quickly what the subtle differences in behavior are between toolbar and menu? What functions? You said you were having a hard time figuring out how to organize this because of the subtle differences. Answer this one and then I'll give you my thoughts on what to do for v2 of this patch.

        Attachment #212092 -
        Flags: review?(bugs) → review-

    
    

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

    
    

    
  
> Can you explain to me quickly what the subtle differences in behavior are
> between toolbar and menu? What functions? You said you were having a hard time
> figuring out how to organize this because of the subtle differences. Answer
> this one and then I'll give you my thoughts on what to do for v2 of this patch. 

The functions in the _DNDObserver objects are really similar, but not quite the same.  Here are some differences:

notify()
When the close timer goes off, the menu should check if any of its parent menus are being dragged over, and close them if they're not.  The toolbarbutton doesn't have any parent menus, so it doesn't need this check.
Toolbars have an extra timer for cleaning up styles.
Menus have an extra timer for auto-closing themselves.

_getDropPoint()
Menus use their resultNode for the folder to drop into, and toolbars use their result.root.  Toolbars loop through all the child nodes, but menus loop through the children between startMarker and endMarker.  In the future, we may also want different calculations for where to drop--for example, hovering over the middle 50% of a toolbar folder results in a drop into the folder, but hovering over the middle 80% of a submenu drops into the submenu.

onDragStart()
"this._self._selection = event.target.node;" seems to only be necessary for menus.

onDragOver()
These functions are the most different, because menus and toolbars have different visual indications for drag and drop.  The toolbar one moves the drag marker, and the menu one sets dragover-top and dragover-bottom styles on menuitems.

onDragExit()
The toolbar and menu have different visual indicators to clean up here.  Also, the menu needs to check if it should close itself, and the toolbar doesn't.

The functions _setTimer(), _clearOverFolder(), canDrop(), onDrop(), and getSupportedFlavours(), and the _overFolder object are exactly the same.

    
    

    
  
> >       <method name="getCopyableSelection">
> >-        <body><![CDATA[ 
> >+        <body><![CDATA[
> >+          if (PlacesController.nodeIsReadOnly(this._resultNode))
> >+            return null;
> 
> Really? Why can't a readonly node be copied? That doesn't seem right. 

onDragStart() calls PlacesController.getTransferData(), which calls getCopyableSelection().  getCopyableSelection() is also called by PlacesController.copy().  It seems like the right thing to do here is make a second function for views and have getTransferData() call that.

> >+          this.removeAttribute("autoopened");
> 
> What is this attribute for? It doesn't look standard. D!

When a menu automatically opens because it was dragged over, it needs to remember that and close itself on dragexit.  What's the right way to do this?



    
    

    
  
(In reply to comment #6)
> onDragStart() calls PlacesController.getTransferData(), which calls
> getCopyableSelection().  getCopyableSelection() is also called by
> PlacesController.copy().  It seems like the right thing to do here is make a
> second function for views and have getTransferData() call that.

Don't forget a drag action can be a copy too, if the user has the ctrl key pressed during the drag. There may need to be better checking. 

> 
> > >+          this.removeAttribute("autoopened");
> > 
> > What is this attribute for? It doesn't look standard. D!
> 
> When a menu automatically opens because it was dragged over, it needs to
> remember that and close itself on dragexit.  What's the right way to do this?

This is fine, just make sure you note down what you're doing. 

As for the comments above, is it possible to specialize the rendering/computation etc of the drag feedback into functions specific to each view and share the rest? Or would that add just as much overhead? 

    
    

    
  
(In reply to comment #7)
> (In reply to comment #6)
> > onDragStart() calls PlacesController.getTransferData(), which calls
> > getCopyableSelection().  getCopyableSelection() is also called by
> > PlacesController.copy().  It seems like the right thing to do here is make a
> > second function for views and have getTransferData() call that.
> 
> Don't forget a drag action can be a copy too, if the user has the ctrl key
> pressed during the drag. There may need to be better checking.

I tried getting PlacesControllerDragHelper.getSession() and checking the session's dragAction in getTransferData(), but it turns out that getTransferData is only called from onDragStart(), and the session doesn't get invoked until after onDragStart() returns.  So I just added an argument to getTransferData() for the dragAction.

    
    

    
  


  

        Attachment #212092 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 212149 [details] [diff] [review]
Addresses Ben's comments

r=ben@mozilla.org

nit/note: you spelled hierarchy wrong.

        Attachment #212149 -
        Flags: review+

    
  
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED

    
    

    
  
shouldn't this one get kwd: fixed1.8.1 ?
Depends on: 327762

    
    

    
  
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: