Closed Bug 923439 Opened 8 years ago Closed 7 years ago

Extremely hard to drag a toolbar item next to the menu

Categories

(Firefox :: Toolbars and Customization, defect)

x86_64
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: jruderman, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P3][Australis:M9])

Attachments

(1 file, 1 obsolete file)

(UX nightly, 2013-10-03)

In a new profile,
1. Open toolbar customization mode
2. Try to drag "Character Encoding" between the Home button and Hamburger menu.

Result: The target for dropping is only a few pixels wide, and somehow only approachable from the left (not the right).  Overshoot by dragging directly over the Hamburger and the drag is considered to be "off the toolbar".
Whiteboard: [Australis:P3][Australis:M?]
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
So here's what I came up with. This removes the obsolete border styles for dropmarkers completely to prevent hidden items from suddenly becoming drop targets (e.g. the social toolbar item). More importantly, it enables directional dropping in the toolbars, based on the code that already did this for the menupanel (which should behave as it did before with this patch, I believe). I *have* switched the code to use getBoundingClientRect, because boxObject is very unreliable according to the discussion I had on IRC.
Attachment #816004 - Flags: review?(mdeboer)
Comment on attachment 816004 [details] [diff] [review]
make dragging items to the end of a toolbar easier

Review of attachment 816004 [details] [diff] [review]:
-----------------------------------------------------------------

This is going to the right place! Perhaps some day we can share (even) more code between toolbar and panel drag/ drop.

Did you test this thoroughly in RTL mode?

f+, because still a quite number of things to do.

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +793,5 @@
>  
>      // We need to determine the place that the widget is being dropped in
>      // the target.
>      let dragOverItem;
> +    let dragValue;

If it were up to me, I'd allow concatenated variable declarations for this case:
```js
let dragOverItem, dragValue, direction;
```

@@ +796,5 @@
>      let dragOverItem;
> +    let dragValue;
> +    let window = targetNode.ownerDocument.defaultView;
> +    let getDragValueForDirection = function(aNode, aBefore) {
> +      let isLTR = window.getComputedStyle(aNode).direction == "ltr";

I'd like to suggest to cache the direction property, because it'll stay the same inside this scope. Let `getDragValueForDirection` cache this value.

@@ +824,4 @@
>            dragOverItem = position == -1 ? targetParent.firstChild : targetParent.children[position];
> +        } else {
> +          let existingDir = dragOverItem.getAttribute("dragover");
> +          console.error(aEvent.clientX, dropTargetCenter);

I assume you don't want to keep this around...

@@ +825,5 @@
> +        } else {
> +          let existingDir = dragOverItem.getAttribute("dragover");
> +          console.error(aEvent.clientX, dropTargetCenter);
> +          if (existingDir == "left") {
> +            dropTargetCenter += (parseInt(dragOverItem.style.borderLeftWidth) || 0) / 2;

don't forget to invoke parseInt with a radix: parseInt(x, 10);

@@ +827,5 @@
> +          console.error(aEvent.clientX, dropTargetCenter);
> +          if (existingDir == "left") {
> +            dropTargetCenter += (parseInt(dragOverItem.style.borderLeftWidth) || 0) / 2;
> +          } else {
> +            dropTargetCenter -= (parseInt(dragOverItem.style.borderRightWidth) || 0) / 2;

parseInt(x, 10)

@@ +829,5 @@
> +            dropTargetCenter += (parseInt(dragOverItem.style.borderLeftWidth) || 0) / 2;
> +          } else {
> +            dropTargetCenter -= (parseInt(dragOverItem.style.borderRightWidth) || 0) / 2;
> +          }
> +          console.error(aEvent.clientX, dropTargetCenter);

I assume you don't want to keep this around...

@@ +839,5 @@
>      if (this._dragOverItem && dragOverItem != this._dragOverItem) {
>        this._setDragActive(this._dragOverItem, false);
>      }
>  
> +    if (dragOverItem != this._dragOverItem || dragValue != dragOverItem.getAttribute("dragover")) {

Comparing a Boolean to a String looks fishy to me:
```js
true == "true"; // false
```

@@ +873,5 @@
>      }
> +    let targetNode = this._dragOverItem;
> +    let dir = window.getComputedStyle(targetNode).direction;
> +    let dropDir = targetNode.getAttribute("dragover");
> +    console.error(targetNode.id, dir, dropDir);

I assume you don't want to keep this around...

@@ +1082,2 @@
>            } else {
> +            let prop = aValue == "left" ? "borderLeftWidth" : "borderRightWidth";

comparing Boolean to String (see earlier comment)
Attachment #816004 - Flags: review?(mdeboer) → feedback+
(In reply to Mike de Boer [:mikedeboer] from comment #2)
> Comment on attachment 816004 [details] [diff] [review]
> make dragging items to the end of a toolbar easier
> 
> Review of attachment 816004 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is going to the right place! Perhaps some day we can share (even) more
> code between toolbar and panel drag/ drop.
> 
> Did you test this thoroughly in RTL mode?

I did now. Sorry about all the console.errors, I must have neglected to qref one last time. :S

> @@ +796,5 @@
> >      let dragOverItem;
> > +    let dragValue;
> > +    let window = targetNode.ownerDocument.defaultView;
> > +    let getDragValueForDirection = function(aNode, aBefore) {
> > +      let isLTR = window.getComputedStyle(aNode).direction == "ltr";
> 
> I'd like to suggest to cache the direction property, because it'll stay the
> same inside this scope. Let `getDragValueForDirection` cache this value.

There was only one call to this function for each branch, with various values for aNode, and because we can't cache outside this function (don't know when the node's direction would change), there's no point in caching it. However, I've just moved the direction checking code to the actual drag active setting and set an "after" or "before" attribute instead. Much cleaner without the poor man's macro.

> @@ +825,5 @@
> > +        } else {
> > +          let existingDir = dragOverItem.getAttribute("dragover");
> > +          console.error(aEvent.clientX, dropTargetCenter);
> > +          if (existingDir == "left") {
> > +            dropTargetCenter += (parseInt(dragOverItem.style.borderLeftWidth) || 0) / 2;
> 
> don't forget to invoke parseInt with a radix: parseInt(x, 10);

This isn't necessary, because we control that value and it won't ever be set to something that starts with 0 to weirdly produce octal or something. But even if it did:

[16:03:45.148] document.getElementById("clone_bug_menu").style.borderLeftWidth = "011px"
[16:03:45.150] "011px"
[16:03:48.012] document.getElementById("clone_bug_menu").style.borderLeftWidth
[16:03:48.015] "11px"

there's no way that the radix won't be 10.

> @@ +839,5 @@
> >      if (this._dragOverItem && dragOverItem != this._dragOverItem) {
> >        this._setDragActive(this._dragOverItem, false);
> >      }
> >  
> > +    if (dragOverItem != this._dragOverItem || dragValue != dragOverItem.getAttribute("dragover")) {
> 
> Comparing a Boolean to a String looks fishy to me:
> ```js
> true == "true"; // false
> ```

The boolean above (for the panel case) should have been a string; otherwise dragValue is always a string. Fixed in the upcoming version.
Ploink.
Attachment #817169 - Flags: review?(mdeboer)
Attachment #816004 - Attachment is obsolete: true
Comment on attachment 817169 [details] [diff] [review]
make dragging items to the end of a toolbar easier

Review of attachment 817169 [details] [diff] [review]:
-----------------------------------------------------------------

I like it! Also works nicely in RTL mode.

Superbe, thanks!
Attachment #817169 - Flags: review?(mdeboer) → review+
https://hg.mozilla.org/projects/ux/rev/1b4efccd9ddc
Whiteboard: [Australis:P3][Australis:M?] → [Australis:P3][Australis:M9][fixed-in-ux]
Pushed a followup bustage fix because I was a numpty when redoing my patch to reduce the getcomputedstyle impact: https://hg.mozilla.org/projects/ux/rev/ee074106b5b0
https://hg.mozilla.org/mozilla-central/rev/1b4efccd9ddc
https://hg.mozilla.org/mozilla-central/rev/ee074106b5b0
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3][Australis:M9][fixed-in-ux] → [Australis:P3][Australis:M9]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.