942600 - Australis: dropping new bookmarks on the Bookmarks Toolbar Items when in the navbar triggers overflow

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[nicolas m]

Attachment: Screencast of the bug for documentation purpose

Australis is fantastic, but I encountered a bug.

I always customise the interface in the first place by moving the Bookmarks Toolbar Items in the main toolbar, right close to the Location Bar where the Search Box, removed, was sitting. It allows to gain some space and is quite a comfortable place to live for a Bookmarks Toolbar.

There's a folder in the Toolbar, and I add bookmarks in it by drag&dropping either the tab or the favicon of the appreciated webpage right into it.

However, with the current Nightly (number 28), the Bookmarks Toolbar Items have a tendency to fold on itself when doing so, becoming a not really useful sub-menu with a link to the Places window, and not really welcoming to incoming dropped bookmarks.

See attached video for an illustration.

Comment 1

[:Gijs (he/him)]

We should figure out how to make the navbar more robust wrt these kinds of changes. Not really sure how to just yet; there's an overflow listener on the navbar's customization target, and I'm not sure why overflow events happen in this case - clearly, the address field can contract more and the addition of a single item shouldn't actually trigger overflow.

Comment 2

[:Gijs (he/him)]

See also bug 895936 (which I recently couldn't reproduce, so closed already, but is likely a similar kind of issue) and bug 940027 and so on.

Comment 3

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

I had a small window of time to debug this - and it looks like it has to do with setting -moz-margin-start on the drop indicator. If we just translate the indicator and un-collapse it, the problem seems to go away...

Comment 4

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) from comment #3)
> I had a small window of time to debug this - and it looks like it has to do
> with setting -moz-margin-start on the drop indicator. If we just translate
> the indicator and un-collapse it, the problem seems to go away...

This is super-useful. We should do this. I poked at this and could never figure it out. Mike, want to work this out into a patch? :-)

Comment 5

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

I'll tentatively take this and see what I can pump out this weekend.

Comment 6

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

Wasn't able to get anything out for this last weekend, but I'm still looking at this in between customize mode transition smoothness bugs.

Comment 7

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

Attachment: Patch v1

Finally got some time between perf try pushes to work on this.

I think we're hitting layout funk what with the tricky hide-y show-y behaviour of the drop indicator. That funk is interacting poorly with our overflow toolbar. That's about as precise as I can get.

Anyhow, this patch seems to solve the problem. I just kinda splatted this one out, but I'll give it a thorough once-over tomorrow and then ask for review.

Comment 8

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

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

Any thoughts on this, Marco? Or is this more in Mano's domain?

Comment 9

[Marco Bonardo [:mak]]

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

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

this has a similar root cause as bug 970013, something enlarges or uncollapses and that causes an overflow that sucks up stuff into the overflow panel... surely figuring a solution to the main issue here would be better than having to workaround it everywhere, also cause we will never be able to workaround each single add-on.

Though in this case I like the fix, so no reason to not take it.  I tested it on Windows, please ensure it works as expected on Mac and Linux.

Comment 10

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

(In reply to Marco Bonardo [:mak] from comment #9)
> Comment on attachment 8382686 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 8382686 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> this has a similar root cause as bug 970013, something enlarges or
> uncollapses and that causes an overflow that sucks up stuff into the
> overflow panel... surely figuring a solution to the main issue here would be
> better than having to workaround it everywhere, also cause we will never be
> able to workaround each single add-on.

True. :/

> 
> Though in this case I like the fix, so no reason to not take it.  I tested
> it on Windows, please ensure it works as expected on Mac and Linux.

Tested on OS X and Linux, and it fixes it across the board.

Thanks Marco!

Comment 11

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

remote:   https://hg.mozilla.org/integration/fx-team/rev/7a80f82f5ce5

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/7a80f82f5ce5

Comment 13

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

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

Australis and the overflowing toolbar.


User impact if declined: 

Users who have moved the Bookmarks Toolbar Items into their nav-bar will have that item disappear into the overflow panel when attempting to drag a bookmark into the item.


Testing completed (on m-c, etc.): 

Lots of local testing, and several days baking on m-c.


Risk to taking this patch (and alternatives if risky): 

Low - this patch _does_ introduce bug 979456, but there's a patch in there that will be requested for uplift as well that fixes the problem.


String or IDL/UUID changes made by this patch:

None.

Comment 14

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/59a952699353