Closed Bug 942600 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect)

28 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 30
Tracking Status
firefox29 --- verified
firefox30 --- verified

People

(Reporter: nicolas, Assigned: mconley)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [Australis:P2])

Attachments

(2 files)

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.
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.
Whiteboard: [Australis:P2]
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.
Summary: Australis: Bookmarks Toolbar Items disappearing when drag&dropping if they're in a custom position → Australis: dropping new bookmarks on the Bookmarks Toolbar Items when in the navbar triggers overflow
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Hardware: x86 → All
Whiteboard: [Australis:P2] → [Australis:P2][investigate-fix-on-aurora]
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...
(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? :-)
I'll tentatively take this and see what I can pump out this weekend.
Assignee: nobody → 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.
Attached patch Patch v1Splinter Review
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 on attachment 8382686 [details] [diff] [review]
Patch v1

Any thoughts on this, Marco? Or is this more in Mano's domain?
Attachment #8382686 - Flags: review?(mak77)
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.
Attachment #8382686 - Flags: review?(mak77) → review+
(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!
remote:   https://hg.mozilla.org/integration/fx-team/rev/7a80f82f5ce5
Whiteboard: [Australis:P2][investigate-fix-on-aurora] → [Australis:P2][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/7a80f82f5ce5
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P2][fixed-in-fx-team] → [Australis:P2]
Target Milestone: --- → Firefox 30
Depends on: 979456
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.
Attachment #8382686 - Flags: approval-mozilla-aurora?
Attachment #8382686 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.