Improve UI/UX of bookmark popup

[jkup]

• Submitted a ticket for my issue if one did not already exist.
• Used Github auto-closing keywords in the commit message.
• Added/updated tests for this change (for new code or code which already has tests).
• Ran git rebase -i to squash commits (if needed).

Fix #2894, #4854, #4864 and #3282 via @rohanthacker's changes!

Auditors @bsclifton @bradleyrichter

Test Plan:

Make sure of a few things:

• Bookmark is automatically created when you click the orange star
• Bookmark doorhanger aligns with orange star
• View all bookmarks page takes you to about:bookmarks

[bsclifton]

Checking it out right now, already noticed a few things (doh)

Launching w/ an empty tab, the icon is off

After you visit a site though, it fixes itself and looks OK. However, the star icon (when a site is favorited) moves up and down when you have the magic titlebar feature on and show/hide the URL bar.

Overlay

When you click it to trigger a bookmark, I can tell it's trying to put an overlay there, but it's in the wrong area. I ran into a similar issue when doing the titlebar (but mine was in main.js, where your overlay appears to be in addEditBookmark). Here's a pic:

This has a side effect of the modal not being dismissible by mouse click ☹️ In my situation, I was able to resolve it by moving the overlay in the DOM to a place where it worked

Style matching

Looking at the pic above and comparing to the mockup in #2894, I notice:

• the styles around the drop down and the input field don't seem to match
• the star is Brave orange as opposed to yellow
• the vertical spacing between the orange section title ("Bookmark added") and the first field's title ("Title") needs to be increased a bit
• the new default button style (grey button) looks great! 😄
• the original mockup had a X in the top right to dismiss, not sure if that's needed

[bsclifton]

@jkup notes left 😄 hopefully, I didn't do something wrong when testing (I grabbed latest from your branch bookmark-popup). Please let me know and I can retest

@bradleyrichter when you click the star when it's already bookmarked, the title still says "Bookmark added". If the site is bookmarked already and you click that, should we even show a title at all?

[jkup]

Wanted to run these fixes by you:

@bsclifton I don't think this technically needs to be a dialog anymore right? I think it can just be a div with some similar behaviors. I just removed the dialog class which fixes that shadow issue but I may be overlooking something.

@bbondy the positioning bug that @bsclifton found is because we are pushing an empty browserButton which has a width and a height but no content. Can I just return null here? https://github.com/brave/browser-laptop/pull/4726/files#diff-b9cf7ede0a812496eace3679483c2c40R129

[bbondy]

yes null is fine

[bradleyrichter]

wow. a breath of fresh UI air!

some initial problems I found:

1 - needs to Close on click-outside.
2 - pressing the star while open also closes (same as click-outside)
3 - with door hanger closed, clicking a marked star opens the hanger with "edit bookmark" as the title (not "bookmark added")
4 - ideally, the URL is stored in whatever state the URL is in when the hanger closes, not when it opens. This allows the user to actually edit the URL in the URL box to modify the bookmark info.
5 - the open hanger prevents the user from moving the browser window. It should close and allow dragging.

For styling issues, we can work on it in person.

[bsclifton]

Looking really good! 😄 All the same things Brad said above... plus the empty button issue you already mentioned (on about pages, including new tab)

@bradleyrichter the "View all bookmarks" seems a little hard to read, gray on gray.

[bradleyrichter]

no worries. We'll shine er' up on Thursday...

(spec image:)

[bsclifton]

I think this should also fix:
#2347

[bsclifton]

Tested on Mac and also Windows 10. Looks and works good- great job 😄