Closed Bug 1352120 Opened 7 years ago Closed 7 years ago

Put a bookmark-star button in the location bar

Categories

(Firefox :: Theme, enhancement, P1)

Product:
Firefox
Component:
Theme
Version:
52 Branch
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 56
Iteration:
56.2 - Jul 10
Tracking Flags:
Tracking Status
firefox56 --- verified

People

(Reporter: jaws, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-structure])

Attachments

(3 files, 1 obsolete file)

bookmarksmenu-16.svg
823 bytes, image/svg+xml
Details
Bug 1352120 - move the bookmarks star into the url bar,
59 bytes, text/x-review-board-request
mak
: review+
Details
Bug 1352120 - fix theming for the star icon, fix theming dealing with empty string icon urls,
59 bytes, text/x-review-board-request
jaws
: review+
Details 
To complete the spec in bug 1352063, we will need to separate out the bookmark star from the bookmark menubutton and put the star in the location bar. This bug is to add the separate star button.
Whiteboard: [photon]
Blocks: photon-structure
Depends on: 1354083
Flags: qe-verify+
Priority: -- → P2
QA Contact: gwimberly
Whiteboard: [photon] → [photon-structure]
Blocks: 1365421
Is there an icon for the bookmarks menu button when it no longer has the star next to it? Just the dropdown will look a bit odd...
Flags: needinfo?(bbell)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 56.1 - Jun 26
Priority: P2 → P1
Attached image bookmarksmenu-16.svg — 
Here's the icon you need.
Flags: needinfo?(bbell)
Comment on attachment 8880009 [details]
Bug 1352120 - to fold: test changes,

https://reviewboard.mozilla.org/r/151346/#review156664

::: browser/components/places/tests/browser/browser_toolbarbutton_menu_context.js:34
(Diff revision 1)
>    await popupShownPromise;
>    let contextMenuShownPromise = onPopupEvent(contextMenu, "shown");
>    EventUtils.synthesizeMouseAtCenter(BMB_showAllBookmarks, {type: "contextmenu", button: 2 });
>    info("Waiting for context menu on bookmarks menu to be shown.");
>    await contextMenuShownPromise;
> +  await new Promise(r => setTimeout(r, 5000));

Oops, should have removed this, used it for debugging.
Comment on attachment 8880360 [details]
Bug 1352120 - fix theming for the star icon, fix theming dealing with empty string icon urls,

https://reviewboard.mozilla.org/r/151724/#review156694

::: commit-message-43970:1
(Diff revision 1)
> +Bug 1352120 - fix theming for the star icon, fix theming dealing with empty string keys, r?jaws

Self-nit: these were empty string *values*, not keys.

::: browser/base/content/theme-vars.inc.css:23
(Diff revision 1)
> +%ifdef MOZ_PHOTON_THEME
> +:root[lwthemeicons~="--bookmark_star-icon"] #star-button:-moz-lwtheme,
> +%endif
>  :root[lwthemeicons~="--bookmark_star-icon"] #bookmarks-menu-button:-moz-lwtheme {

Potential follow-up work: it seems add-ons can't define separate starred / unstarred icons? That seems like a bug we should fix, but I'd prefer not to do it here given it's pre-existing.
So my initial trypush was already mostly green (yay!):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=98b88a8b6489&selectedJob=109167116

besides the theming orange. I fixed the flake8 orange, and then fixed the theming orange, and I fired off another trypush but I'm only re-running mochitests this time (as marionette came back green and fx-functional-ui was green besides artifact-only known permaorange). New trypush: https://treeherder.mozilla.org/#/jobs?repo=try&revision=22bb1643aa73
Comment on attachment 8880008 [details]
Bug 1352120 - move the bookmarks star into the url bar,

https://reviewboard.mozilla.org/r/151344/#review156724

Overall it looks cleaner than expected!

When the new "menu" ends un in the overflow panel, it reads "Bookmrk This Page" but it's not what it does, instead it opens the Library.
The other confusing thing in this state, is that it shows a (wrongly rescaled) dropmarker, but it's not a menu anymore.
This happens if I move the button at the most-right position on the toolbar and shrink the window enough for it to enter the overflow.
If I instead put it in the overflow in customization, it shows the bookmarks menu...

There's another bug related to the urlbar icons when the window size becomes very small and they escape the frame, but that sounds like a photon bug unrelated to this change (though the addition of the star probably makes it easier to hit).

Note that I didn't check for eventual regressions related to overflow when photon is disabled, it would be nice to check that. I may check at the next pass.

::: browser/base/content/browser-places.js:1354
(Diff revision 1)
>        return;
>  
>      // Ideally this code would never be reached, but if you click the outer
>      // button's border, some cpp code for the menu button's so-called XBL binding
>      // decides to open the popup even though the dropmarker is invisible.
> -    if (this._currentAreaType == CustomizableUI.TYPE_MENU_PANEL) {
> +    if (this.button.getAttribute("cui-areatype") == CustomizableUI.TYPE_MENU_PANEL) {

Maybe we could retain the old code and just if (!AppConstants.MOZ_PHOTON_THEME && ...
The bug seems to only affect the menu-button binding, that in photon doesn't exist. Maybe worth checking?

::: browser/base/content/browser-places.js:1740
(Diff revision 1)
>  
>      if (this._itemGuids.size > 0) {
>        this.broadcaster.setAttribute("starred", "true");
>        this.broadcaster.setAttribute("buttontooltiptext", this._starredTooltip);
> -      if (this.button.getAttribute("overflowedItem") == "true") {
> +      this.broadcaster.setAttribute("tooltiptext", this._starredTooltip);
> +      if (!AppConstants.MOZ_PHOTON_THEME && this.button.getAttribute("overflowedItem") == "true") {

I noticed there may be a bug here, when the bookmarks menu-panel goes into the overflow panel, the dropdown icon becomes tiny.
Now, we may not care since photon will remove that, but please verify that when the new menu goes into the overflow it looks good now.

::: browser/base/content/browser-places.js:1788
(Diff revision 1)
>      }
>  
> +    // Disable the old animation in photon
> +    if (AppConstants.MOZ_PHOTON_THEME) {
> +      return;
> +    }

I see only one call to _showBookmarkedNotification, so it'd be better to just exclude that call.
Maybe you could even rename this method to something more meaningful, like _showMenuButtonStarringAnimation

::: browser/themes/shared/jar.inc.mn:115
(Diff revision 1)
>    skin/classic/browser/bookmark.svg                   (../shared/icons/bookmark.svg)
>    skin/classic/browser/bookmark-hollow.svg            (../shared/icons/bookmark-hollow.svg)
> +#ifndef MOZ_PHOTON_THEME
>    skin/classic/browser/bookmarksMenu.svg              (../shared/icons/bookmarksMenu.svg)
> +#else
> +  skin/classic/browser/bookmarksMenu.svg              (../shared/icons/bookmarksMenu-with-bar.svg)

I wondered what a menu with bar looked like... This is also confusing with the bookmarksMenu root favicon that is even different...
Maybe, for future clarity, we could start by naming the file bookmark-star-on-tray.svg

::: browser/themes/shared/toolbarbutton-icons.inc.css:75
(Diff revision 1)
>  #bookmarks-menu-button[cui-areatype="toolbar"] > .toolbarbutton-menubutton-dropmarker > .dropmarker-icon {
> +%endif
>    list-style-image: url("chrome://browser/skin/bookmarksMenu.svg");
>  }
>  
> +

why this double newline?
Attachment #8880008 - Flags: review?(mak77)
(In reply to Marco Bonardo [::mak] from comment #11)
> Comment on attachment 8880008 [details]
> Bug 1352120 - move the bookmarks star into the url bar,
> 
> https://reviewboard.mozilla.org/r/151344/#review156724
> 
> Overall it looks cleaner than expected!

\o/

> When the new "menu" ends un in the overflow panel, it reads "Bookmrk This
> Page" but it's not what it does, instead it opens the Library.
> The other confusing thing in this state, is that it shows a (wrongly
> rescaled) dropmarker, but it's not a menu anymore.
> This happens if I move the button at the most-right position on the toolbar
> and shrink the window enough for it to enter the overflow.
> If I instead put it in the overflow in customization, it shows the bookmarks
> menu...

Ah, right, we have specialcasing for the overflow menu based on the old structure of the button. I'll try to find where we do this and update it accordingly.

> There's another bug related to the urlbar icons when the window size becomes
> very small and they escape the frame, but that sounds like a photon bug
> unrelated to this change (though the addition of the star probably makes it
> easier to hit).

Yes, this is bug 1370401.

> ::: browser/base/content/browser-places.js:1740
> (Diff revision 1)
> >  
> >      if (this._itemGuids.size > 0) {
> >        this.broadcaster.setAttribute("starred", "true");
> >        this.broadcaster.setAttribute("buttontooltiptext", this._starredTooltip);
> > -      if (this.button.getAttribute("overflowedItem") == "true") {
> > +      this.broadcaster.setAttribute("tooltiptext", this._starredTooltip);
> > +      if (!AppConstants.MOZ_PHOTON_THEME && this.button.getAttribute("overflowedItem") == "true") {
> 
> I noticed there may be a bug here, when the bookmarks menu-panel goes into
> the overflow panel, the dropdown icon becomes tiny.

Yes, this is bug 1197624. It's a known issue. I don't want to fix that here.

> Now, we may not care since photon will remove that, but please verify that
> when the new menu goes into the overflow it looks good now.

OK.

> ::: browser/base/content/browser-places.js:1788
> (Diff revision 1)
> >      }
> >  
> > +    // Disable the old animation in photon
> > +    if (AppConstants.MOZ_PHOTON_THEME) {
> > +      return;
> > +    }
> 
> I see only one call to _showBookmarkedNotification, so it'd be better to
> just exclude that call.
> Maybe you could even rename this method to something more meaningful, like
> _showMenuButtonStarringAnimation

I did this deliberately because the photon animation team will want to start its own animation - it will just be different. Does that sound OK? (ni for this one)
Flags: needinfo?(mak77)
Comment on attachment 8880009 [details]
Bug 1352120 - to fold: test changes,

https://reviewboard.mozilla.org/r/151346/#review156760

::: browser/base/content/test/general/browser_bookmark_popup.js:14
(Diff revision 2)
>   */
>  
>  let bookmarkPanel = document.getElementById("editBookmarkPanel");
> -let bookmarkStar = document.getElementById("bookmarks-menu-button");
> +let bookmarkStar = AppConstants.MOZ_PHOTON_THEME ?
> +  document.getElementById("star-button") :
> +  document.getElementById("bookmarks-menu-button");

Maybe you could use the BookmarkingUI getters here (star or anchor?). The test seems to use this just for the starred attribute and a click
Attachment #8880009 - Flags: review?(mak77) → review+
(In reply to :Gijs from comment #12)
> I did this deliberately because the photon animation team will want to start
> its own animation - it will just be different. Does that sound OK? (ni for
> this one)

I as thinking at that point we could have a _showLegacyMenuButtonStarringAnimation and _showPhotonMenuButtonStarringAnimation and just call the right one.
Flags: needinfo?(mak77)
Comment on attachment 8880360 [details]
Bug 1352120 - fix theming for the star icon, fix theming dealing with empty string icon urls,

https://reviewboard.mozilla.org/r/151724/#review157306

Thanks for fixing up this code!
Attachment #8880360 - Flags: review?(jaws) → review+
Comment on attachment 8880008 [details]
Bug 1352120 - move the bookmarks star into the url bar,

https://reviewboard.mozilla.org/r/151344/#review156724

> Maybe we could retain the old code and just if (!AppConstants.MOZ_PHOTON_THEME && ...
> The bug seems to only affect the menu-button binding, that in photon doesn't exist. Maybe worth checking?

Yes, though we need to do the same thing to make this open the subview like it does in the permanent overflow area (ie if you move it in customize mode / use the context menu 'pin to overflow menu' option). So I kept it for now and expanded some of the comments.
Iteration: 56.1 - Jun 26 → 56.2 - Jul 10
Comment on attachment 8880008 [details]
Bug 1352120 - move the bookmarks star into the url bar,

https://reviewboard.mozilla.org/r/151344/#review157742

It looks great, thank you!
Attachment #8880008 - Flags: review?(mak77) → review+
Attachment #8880009 - Attachment is obsolete: true
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ffcc7b9e7baa
move the bookmarks star into the url bar, r=mak
https://hg.mozilla.org/integration/autoland/rev/8a35db6fffa6
fix theming for the star icon, fix theming dealing with empty string icon urls, r=jaws
Depends on: 1376687
https://hg.mozilla.org/mozilla-central/rev/ffcc7b9e7baa
https://hg.mozilla.org/mozilla-central/rev/8a35db6fffa6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
No longer depends on: 1376687
I have reproduced this bug with Nightly 55.0a1 (2017-03-30) on Windows 10 , 64 Bit ! 

This bug's fix is Verified with latest Nightly 56.0a1 !

Build   ID    20170629030206
User Agent    Mozilla/5.0 (Windows NT 10.0; WOW64; rv:56.0) Gecko/20100101 Firefox/56.0

[bugday-20170628]
I have reproduced this bug with Nightly 55.0a1 (2017-03-30) (64-bit) on Ubuntu 16.04 LTS!

This bug's fix is verified with latest Nightly!

Build   ID : 20170629100321
User Agent : Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0
QA Whiteboard: [bugday-20170628]
Depends on: 1377202
Depends on: 1377165
The issue is no longer reproducible on Firefox 56.0a1. 
Build ID: 20170629030206
Tests were performed under Windows 10x64.
[bugday-20170628]
status-firefox56: fixed → verified
Depends on: 1377463
No longer depends on: 1377463
Verified on Windows, Mac, and Ubuntu.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1378161
Depends on: 1378194
Depends on: 1378560
Depends on: 1382570
Blocks: 1387512
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: