870014 - Cannot insert customizable items to the left of the tabstrip

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

Latest UX nightly
If you move the home button from it's default location in the the upper left corner it can not be returned to that location. The restore defaults also does not return it to this location.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Hey Curtis - can you show us a screenshot for where you've moved the home button?

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Curtis, are you still experiencing this? Can you give me a screenshot?

Comment 3

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

Attachment: old home

Comment 4

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

Attachment: Aus home

Comment 5

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

Attached 2 screen shots both of the same profile one with current nightly and one with nightlyux
old home shows the home button where I've had it for ages
Aus home shows where it is now and I can't move it to the left side like I have it on all other FX browswer profiles I use

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Ok, I see the problem now. Thank you Curtis.

Comment 7

[:Gijs (he/him)]

Current guess: we're not wrapping the <tabs> element in a toolbarpaletteitem wrapper, so no droptarget, so no dropping it on (and therefore left) of the tabs.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

I think that's highly likely.

Comment 9

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) from comment #8)
> I think that's highly likely.

See, that's what I thought, and a couple of minutes debugging further and I'm confused. We're not even getting a drop event. Nor is the tabbrowser.xml code. Something's swallowing it, but I have no idea what it is. :s

Comment 10

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #9)
> (In reply to Mike Conley (:mconley) from comment #8)
> > I think that's highly likely.
> 
> See, that's what I thought, and a couple of minutes debugging further and
> I'm confused. We're not even getting a drop event. Nor is the tabbrowser.xml
> code. Something's swallowing it, but I have no idea what it is. :s

OK, using capturing event listeners fixes this. Probably a good idea anyway. However, now I have the problem I was originally afraid of - not a toolbarpaletteitem, things don't go well. :-)

Comment 11

[:Gijs (he/him)]

Attachment: Patch v1

This turned out to be trickier than I'd hoped. 3 things change with this patch:

1. All the event listeners go capturing. I think this is a saner choice generally, so I executed it everywhere.
1b. Use the fact that that gives us customizableArea === event.currentTarget (fewer parentNode walking to figure out which area we're in), move actual node figurings-out down in the functions so we do the more expensive stuff less often
1c. For the target node, don't just blindly take what the dnd bits give us, but look for something which is an actual child of the drop area, so the position-figuring-out code doesn't screw up.
2. unwrapToolbarItem... is nice, but shouldn't try to unwrap things which aren't a toolbarpaletteitem. Leads to force-closing browser windows (!?) when the <tabs id="tabbrowser-tabs"> element starts disappearing and stuff... yeah, not great.
3. Make the target (drop) node ID getting a bit more flexible. Not everything need be wrapped in a toolbarpaletteitem. If it's not, though, it should be a direct child of the toolbar (which we ensured in 1c).

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 754745 [details] [diff] [review]
Patch v1

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

Some behavioural problems with this patch:

1) Dragging items into the nav-bar breaks. The item doesn't show in the nav-bar, and the dragged item is unwrapped and left in its origin area
2) I dragged an item to the left of the tabstrip, and then the tabstrip was draggable. Not ideal.

Comment 13

[:Gijs (he/him)]

Attachment: Patch v1.1

This should fix those two issues, at least.

I've also added styling so you get a dropmarker for inserting before the tabstrip (woo)

I also experimented briefly with just using a <toolbaritem removable="false"> around the tabbrowser-tabs. That essentially broke the world in all manner of exciting ways (like the tabs disappearing except in customize mode?!), so I went back to this patch. The issues you identified were relatively trivial to fix anyhow, but I noticed that the tabs are essentially the only item which isn't considered customizable by the code...

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 755278 [details] [diff] [review]
Patch v1.1

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

I'm good with this - just one suggestion below. Thanks Gijs!

::: browser/themes/shared/customizableui/customizeMode.inc.css
@@ +108,5 @@
> +  border-left: 2px solid transparent;
> +  border-right: 2px solid transparent;
> +}
> +
> +toolbar[customizing="true"] > tabs[dragover="left"] {

Let's not put these in actually, since they'll need to go away anyways after bug 873056 lands.

Comment 15

[:Gijs (he/him)]

Pushed: https://hg.mozilla.org/projects/ux/rev/e66284b3e110

(without the style bits)

Comment 16

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #15)
> Pushed: https://hg.mozilla.org/projects/ux/rev/e66284b3e110
> 
> (without the style bits)

Except I screwed up and didn't resolve the conflicts caused by our fancy new drag placeholders correctly. Fixed by https://hg.mozilla.org/projects/ux/rev/3c4799dd127c .

Comment 17

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/e66284b3e110
https://hg.mozilla.org/mozilla-central/rev/3c4799dd127c