1678921 - "No bookmarks" message on empty bookmarks toolbar cannot be used as a drop target for new bookmarks, making bookmarks creation there error-prone

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Sören Hentzschel]

STR:

1. enable the bookmarks toolbar
2. make sure that the bookmarks toolbar is empty
3. drag an open tab to the bookmarks toolbar

Actual:

It depends on where you released the tab. With a German build and the display resolution of my MacBook Pro the new empty state text (introduced in bug 1674091) makes ~ 40 percent of the bookmarks toolbar and it's not possible to use this space for bookmarking a page. Instead of bookmarking the page the tab will be moved to a new window. This is a completely different result and therefore an unexpected usability regression.

Expected:

The page should be bookmarked no matter where I release the tab, even if I release the tab on the empty state text. This action should not move the tab to a new window.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1674091

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I'm not sure how the text was introduced, but it should ideally be transparent to drop operations, it sounds like it's just being introduced as a box without any drop handling.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Ah it's not part of personal-bookmarks, so it won't handle drops at all :(

Comment 4

[:Gijs (he/him)]

Attachment: Bug 1678921 - make empty toolbar disclaimer take up no horizontal space, so the entire toolbar functions as a drag target for bookmarks, r?jaws

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f4417c559c57
make empty toolbar disclaimer take up no horizontal space, so the entire toolbar functions as a drag target for bookmarks, r=jaws

Comment 6

[:Gijs (he/him)]

(In reply to Marco Bonardo [:mak] from comment #3)

Ah it's not part of personal-bookmarks, so it won't handle drops at all :(

I chatted with Jared, the reason for this (ie why not just make it part of the personal-bookmarks item?) is that we want to be able to show the item when the personal-bookmarks item is not on the toolbar, so that again, the toolbar is not completely empty without anything on it.

Comment 7

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/f4417c559c57

Comment 8

[:Gijs (he/him)]

Attachment: Bug 1678921 - make empty toolbar disclaimer take up no horizontal space, so the entire toolbar functions as a drag target for bookmarks (beta patch), r=jaws

Comment 9

[:Gijs (he/him)]

Comment on attachment 9191017 [details]
Bug 1678921 - make empty toolbar disclaimer take up no horizontal space, so the entire toolbar functions as a drag target for bookmarks (beta patch), r=jaws

Beta/Release Uplift Approval Request

• User impact if declined: Issues with dropping URLs/bookmarks on the places toolbar when it's empty
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See comment #0
• List of other uplifts needed: none
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a relatively minor JS + CSS change.
• String changes made/needed: none

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9191017 [details]
Bug 1678921 - make empty toolbar disclaimer take up no horizontal space, so the entire toolbar functions as a drag target for bookmarks (beta patch), r=jaws

Approved for 84.0b8.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9b2b15a13d6f

Comment 12

[Peter Magyari (Desktop QA)]

I've reproduced the issue on Firefox 85.0a1 (20201116210217) and verified the fix on Nightly 85.0a1 (20201204094005) and Beta 84.0b8 (20201203211213)

Comment 13

[Sören Hentzschel]

I can confirm that the reported issue is fixed but now the link in the placeholder message is no longer clickable. Should this ticket be reopened or do you prefer another ticket?

Comment 14

[:Gijs (he/him)]

(In reply to Sören Hentzschel from comment #13)

I can confirm that the reported issue is fixed but now the link in the placeholder message is no longer clickable. Should this ticket be reopened or do you prefer another ticket?

The answer to this question kind of depends on whether we can come up with a fix that is additive, or whether we need to back this fix out completely and fix the problem some other way.

We can make the link clickable again by giving it position: relative; and a z-index. But of course, that then means that if you drag something to the link, it stops creating a bookmark 🙃. Having that not work only for the link is better than not having it work for the entire message, but not a lot better / more understandable for users. We can't just give the empty bookmarks toolbar pointer-events: none to make the link work, either, because of course then the drag handling will go back to not working, but for the entire toolbar...

Still trying to figure out if there's a better way here.

Comment 15

[:Gijs (he/him)]

OK, per discussion with Jared and Ryan, we're going to at least back the fix from this bug out of beta. I'll file a follow-up for 85 where we'll try and actually fix both the original issue, the link clicks, and bug 1678951.

Ryan, needinfo for the backout as per our discussion.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Backed out for 84.0rc1.
https://hg.mozilla.org/releases/mozilla-beta/rev/38109b4b3b362f55d059569ddc060275be59ddf4