Put a bookmark-star button in the location bar

VERIFIED FIXED in Firefox 56

Status

()

Firefox
Theme
P1
normal
VERIFIED FIXED
5 months ago
14 days ago

People

(Reporter: jaws, Assigned: Gijs)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs)

52 Branch
Firefox 56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 verified)

Details

(Whiteboard: [photon-structure])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 1 obsolete attachment)

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.

Updated

5 months ago
Whiteboard: [photon]
(Assignee)

Updated

5 months ago
Blocks: 1349210
Depends on: 1354083
Blocks: 1354155

Updated

4 months ago
Flags: qe-verify+
Priority: -- → P2
QA Contact: gwimberly
Whiteboard: [photon] → [photon-structure]

Updated

3 months ago
Blocks: 1365421
(Assignee)

Comment 1

2 months ago
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)

Updated

2 months ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 56.1 - Jun 26
Priority: P2 → P1

Comment 2

2 months ago
Created attachment 8878080 [details]
bookmarksmenu-16.svg

Here's the icon you need.
Flags: needinfo?(bbell)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

2 months ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 8

2 months ago
mozreview-review
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.
Comment hidden (mozreview-request)
(Assignee)

Comment 10

2 months ago
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 11

2 months ago
mozreview-review
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)
(Assignee)

Comment 12

2 months ago
(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 13

2 months ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 18

2 months ago
mozreview-review
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+
(Assignee)

Comment 19

2 months ago
mozreview-review-reply
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.

Updated

2 months ago
Iteration: 56.1 - Jun 26 → 56.2 - Jul 10

Comment 20

2 months ago
mozreview-review
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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8880009 - Attachment is obsolete: true

Comment 23

2 months ago
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
(Assignee)

Updated

2 months ago
Depends on: 1376687

Comment 24

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ffcc7b9e7baa
https://hg.mozilla.org/mozilla-central/rev/8a35db6fffa6
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
(Assignee)

Updated

2 months ago
No longer depends on: 1376687

Comment 25

2 months ago
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]
(Assignee)

Updated

2 months ago
Depends on: 1377202
(Assignee)

Updated

2 months ago
Depends on: 1377165

Comment 27

2 months ago
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

Updated

2 months ago
Depends on: 1377463
(Assignee)

Updated

2 months ago
No longer depends on: 1377463
Verified on Windows, Mac, and Ubuntu.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
(Assignee)

Updated

2 months ago
Depends on: 1378161
(Assignee)

Updated

2 months ago
Depends on: 1378194
Screenshots:

https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=8f80d594c08d5c7a112e5d4b9eb44ffca717eb7b&newProject=mozilla-central&newRev=6190181ff4093756d3f8df754109ed16d132d215
(Assignee)

Updated

a month ago
Depends on: 1378560
(Assignee)

Updated

29 days ago
Depends on: 1382570
Blocks: 1387512
You need to log in before you can comment on or make changes to this bug.