Closed Bug 367542 Opened 18 years ago Closed 17 years ago

when dragging and dropping a tab, show "don't drop" indicator when drop position will not move the tab.

Categories

(Firefox :: Tabbed Browser, defect)

Product:
Firefox
Component:
Tabbed Browser
Version:
2.0 Branch
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: moco, Assigned: dao)

References

Details

Attachments

(1 file, 6 obsolete files)

fix nits
4.95 KB, patch
Details | Diff | Splinter Review 
when dragging and dropping a tab, show "don't drop" indicator when drop position will not move the tab.

right now, we show the "don't drop" indicator when the mouse is over the current tab, but we really need to extend that by 1/2 of the tab width in both directions.

in canDrop(), in tabbrowser.xml, we have http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/tabbrowser.xml#1705

1705       <method name="canDrop">
1706         <parameter name="aEvent"/>
1707         <parameter name="aDragSession"/>
1708         <body>
1709           <![CDATA[
1710             if (aDragSession.sourceNode &&
1711                 aDragSession.sourceNode.parentNode == this.mTabContainer &&
1712                 (aEvent.screenX >= aDragSession.sourceNode.boxObject.screenX &&
1713                  aEvent.screenX <= (aDragSession.sourceNode.boxObject.screenX +
1714                                     aDragSession.sourceNode.boxObject.width)))
1715                 return false;
1716             return true;
1717           ]]>
1718         </body>

given that all our tabs are the same width, we can just do:

(aEvent.screenX >= (aDragSession.sourceNode.boxObject.screenX -
                    aDragSession.sourceNode.boxObject.width/2) &&
 aEvent.screenX <= (aDragSession.sourceNode.boxObject.screenX +
                    aDragSession.sourceNode.boxObject.width + 
                    aDragSession.sourceNode.boxObject.width/2))

i'd want to double check if the this breaks grabbing a tab and scrolling right or left, when the source node is the is on the edge, next to the scroll buttons.

wayne, dao:  what do you think?
the original fix to not show the drop indicator over the source tab came from bug #345524.
Depends on: 345524
Attached patch patch (obsolete) — — Splinter Review 
> given that all our tabs are the same width

There are extensions that change that. I'd like to take care of this.

> i'd want to double check if the this breaks grabbing a tab and scrolling right
> or left, when the source node is the is on the edge, next to the scroll
> buttons.

works fine
Assignee: nobody → bugzilla
Status: NEW → ASSIGNED
Attachment #252108 - Flags: review?(sspitzer)
Comment on attachment 252108 [details] [diff] [review]
patch

thanks for fixing this dao!  

It's been bothering me for a while.

let's double check with beltzner (with ui-r=?) and since this is toolkit, we'd need a peer (like asaf) to review.
Attachment #252108 - Flags: ui-review?(beltzner)
Attachment #252108 - Flags: review?(sspitzer)
Attachment #252108 - Flags: review?(mano)
Attachment #252108 - Flags: review+
toolkit changes require test cases, but I don't think it makes sense for this patch.

robert, do you agree with my testcase-?
Flags: in-testsuite-
This will also turn off the d&d arrow, won't it? My cvs seems to be busted right today, so I can't check your patch on Mac.

The d&d indicator is intended to be on in adjacent tabs, as it's easier to follow dragging in the UI (see bug 333791 comment 44).

While I'll trust you that it looks better to have no arrow when there's a "no-drop" indicator instead, OS X doesn't have this. If so, this should be a platform-specific fix.
Comment on attachment 252108 [details] [diff] [review]
patch

Ugh, could you please separate this to few meaningfully named variables?
Attached patch more comprehensible patch (obsolete) — — Splinter Review 

        Attachment #252108 -
        Attachment is obsolete: true

        Attachment #252255 -
        Flags: ui-review?(beltzner)

        Attachment #252255 -
        Flags: review?(mano)

        Attachment #252108 -
        Flags: ui-review?(beltzner)

        Attachment #252108 -
        Flags: review?(mano)

    
    

    
  
Did you test RTL ui with these changes?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        rtl-sensitive patch (obsolete)
        — 
          — Splinter Review
      

    


  
nope, I didn't :(

        Attachment #252255 -
        Attachment is obsolete: true

        Attachment #252263 -
        Flags: ui-review?(beltzner)

        Attachment #252263 -
        Flags: review?(mano)

        Attachment #252255 -
        Flags: ui-review?(beltzner)

        Attachment #252255 -
        Flags: review?(mano)

    
    

    
  
Comment on attachment 252263 [details] [diff] [review]
rtl-sensitive patch

r=mano, thanks.

        Attachment #252263 -
        Flags: review?(mano) → review+

    
    

    
  
s/window.getComputedStyle/document.defaultView.getComputedStyle, I realize we already depend on window.getComputedStyle in this binding.
Target Milestone: --- → Firefox 3 alpha2

    
    

    
  
As outlined in my previous comment, I strongly feel this should be made Windows-specific, or at least NOT affect Mac. It is *intentional* that the dnd arrow is shown in adjacent tabs, even if the drop doesn't result in moving the tab. The Windows "no drop" marker might make it OK to now hide the arrow for that platform, but Mac doesn't have that (does Linux?).

    
    

    
  
Does Mac OS X have a drag cursor?

    
    

    
  
(In reply to comment #14)
> Does Mac OS X have a drag cursor?

The cursor, when dragging, is identical to when not dragging. So I guess not.

Looking at Apple's native apps, I can't see any consistency there. The cursor isn't modified in most instances of dragging that I've looked at.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        revert behaviour for Mac OS X (obsolete)
        — 
          — Splinter Review
      

    


  
untested

        Attachment #252263 -
        Attachment is obsolete: true

        Attachment #252269 -
        Attachment is obsolete: true

        Attachment #252302 -
        Flags: ui-review?(beltzner)

        Attachment #252302 -
        Flags: review?(mano)

        Attachment #252263 -
        Flags: ui-review?(beltzner)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        revert behaviour for Mac OS X (obsolete)
        — 
          — Splinter Review
      

    


  
sorry, the attached diff was not the newest I had.

        Attachment #252302 -
        Attachment is obsolete: true

        Attachment #252306 -
        Flags: ui-review?(beltzner)

        Attachment #252306 -
        Flags: review?(mano)

        Attachment #252302 -
        Flags: ui-review?(beltzner)

        Attachment #252302 -
        Flags: review?(mano)

    
    

    
  
Cool. Does any platform other than Windows use a no-drop marker? Does Linux? If not, maybe this should be made Windows-specific instead of Mac-exclusive.

    
    

    
  
Comment on attachment 252306 [details] [diff] [review]
revert behaviour for Mac OS X

r=mano code-wise... I actually find the no-marker-at-all more correct on Windows as well.

        Attachment #252306 -
        Flags: review?(mano) → review+

    
    

    
  
Comment on attachment 252306 [details] [diff] [review]
revert behaviour for Mac OS X

A couple of nits though:

>Index: tabbrowser.xml
>===================================================================

>+            var dir = document.defaultView.getComputedStyle(this.mTabContainer, null).direction;
>+            var sibling = dir == "ltr" ? sourceNode.previousSibling : sourceNode.nextSibling;
>+            if (sibling && aEvent.screenX < sourceBoxObject.screenX - sibling.boxObject.width/2)
>+              return true;
>+
>+            sibling = dir == "ltr" ? sourceNode.nextSibling : sourceNode.previousSibling;
>+            if (sibling && aEvent.screenX > sourceBoxObject.screenX + sourceBoxObject.width + sibling.boxObject.width/2)
>+              return true;
>+#endif


Please try to keep lines no longer than 80 characters.

>-              if (window.getComputedStyle(this.parentNode, null)
>+              if (document.defaultView.getComputedStyle(this.parentNode, null)
>                         .direction == "ltr") {


Fix the second-line indentation here.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        fix nits
        — 
          — Splinter Review
      

    


  
btw, GNOME does have a d&d cursor, as well as a "don't drop" cursor (although the difference is hardly visible).

    
    

    
  
> btw, GNOME does have a d&d cursor, as well as a "don't drop" cursor (although
> the difference is hardly visible).

Sounds good then :)



    
    

    
  
Comment on attachment 252504 [details] [diff] [review]
fix nits

given bug 333791 comment 44, it's probably better to ask mconnor for ui-review.

        Attachment #252504 -
        Flags: ui-review?(mconnor)

    
  

        Attachment #252306 -
        Attachment is obsolete: true

        Attachment #252306 -
        Flags: ui-review?(beltzner)

    
    

    
  
FWIW, the "do not drop" indicator is usually used to indicate "invalid location" or "illegal operation", not "no-op". I kinda agree with bug 333791 comment 44 here.

    
    

    
  
Yeah, I'm pretty much still there, this doesn't feel or look right, but open to rationale for changing this contrary to what it traditionally means.

    
  
Target Milestone: Firefox 3 alpha2 → ---

    
  

        Attachment #252504 -
        Flags: ui-review?(mconnor)

    
  
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → WONTFIX

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: