916953 - Bookmarks button in overflow panel looks broken

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Justin Dolske [:Dolske]]

Attachment: Screenshot

The separator is missing, and the right-side icon is weirdly offset.

Comment 1

[Mike de Boer [:mikedeboer]]

I think it's good to 'wait' for bug 885086 to be fixed and start fixing other overflow panel styling issues from that baseline.

Comment 2

[:Gijs (he/him)]

It looks better now, but the button has a single hover style (at least on OS X), which makes it impossible to tell whether clicking is going to open the Library or star/edit-bookmark the current page.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

As this is a landing blocker, we should get an assignee. Gijs or Mike, could one of you take it?

Comment 4

[:Gijs (he/him)]

I think the current state is no longer a landing blocker. However, I can probably take it tomorrow if Mike doesn't take it before then.

Comment 5

[Mike de Boer [:mikedeboer]]

I'll take it. I'm afraid it'll need input from UX though.

Comment 6

[:Gijs (he/him)]

After a brief discussion on IRC: let's not block landing on this in its current state. It might not be optimal, but it's not nearly as bad as it was.

Comment 7

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1: introduce separator in overflow panel buttons

What this patch does is
1) add a separator for all OSs for menubuttons in the overflow panel (only applies for the Bookmark button)
2) Fixes the dropmarker icon dimensions on Windows
3) Does not show the hover state of the button inside the menubutton anymore on Windows and Linux (where this was an issue).

Comment 8

[:Gijs (he/him)]

Comment on attachment 821707 [details] [diff] [review]
Patch 1: introduce separator in overflow panel buttons

Clearing review pending investigation as per IRC discussion related to the double rule here:

+#widget-overflow-list > .toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker > .dropmarker-icon,
+#widget-overflow-list > #bookmarks-menu-button[cui-areatype="toolbar"] > .toolbarbutton-menubutton-dropmarker > .dropmarker-icon {
+  padding: 0 6px;
+}

and trying to make the browser.css rule that the second selector is overriding more generic so that that's not necessary.

Comment 10

[Peter Retzer (:pretzer)]

Attachment: screenshot (windows)

On Windows this looks significantly worse, especially when hovering the item. Also the icon on the right is not displayed completely.

Comment 11

[Alice0775 White]

attachment 8334288 [details]

The click-able area for list should be larger than area of bookmarking.

Comment 12

[Mike de Boer [:mikedeboer]]

Design decision: only add separator.
String change in overflow mode: Change to 'Bookmark This Page'

Comment 13

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.1: introduce separator in overflow panel buttons

Comment 14

[:Gijs (he/him)]

Comment on attachment 8363580 [details] [diff] [review]
Patch 1.1: introduce separator in overflow panel buttons

Review of attachment 8363580 [details] [diff] [review]:
-----------------------------------------------------------------

This patch looks excellent. As we're using generic styling, did you check that this fixes (or at least doesn't make things worse for) the overflow panel styling issues raised in bug 940307 ?

::: browser/themes/windows/browser.css
@@ +1374,5 @@
>  #bookmarks-menu-button.bookmark-item > .toolbarbutton-menubutton-button > .toolbarbutton-icon {
>    -moz-margin-start: 5px;
>  }
>  
> +#bookmarks-menu-button[cui-areatype="toolbar"]:not(.overflowedItem) > .toolbarbutton-menubutton-dropmarker > .dropmarker-icon {

This conflicts with the patch Marco just landed. I *think* you need both your :not clause and his, but please doublecheck.

Comment 15

[Mike de Boer [:mikedeboer]]

remote: https://hg.mozilla.org/integration/fx-team/rev/767ee9d0f0c1

Comment 16

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.2: introduce separator in overflow panel buttons

Comment 17

[:Gijs (he/him)]

Comment on attachment 8363636 [details] [diff] [review]
Patch 1.2: introduce separator in overflow panel buttons

Review of attachment 8363636 [details] [diff] [review]:
-----------------------------------------------------------------

This is fine, too, please post-commit the following nits:

::: browser/base/content/browser-places.js
@@ +1294,5 @@
> +      return;
> +
> +    if (!this._starButtonOverflowedLabel) {
> +      let browserBundle = Services.strings.createBundle(
> +                          "chrome://browser/locale/browser.properties");

Nit: gNavigatorBundle.getString("starButtonOverflowed.label");

@@ +1304,5 @@
> +    if (!this._starButtonLabel)
> +      this._starButtonLabel = button.label;
> +
> +    if (button && button.label == this._starButtonLabel)
> +      button.setAttribute("label", this._starButtonOverflowedLabel);

Nit: button.label = ...

(alternatively, convert the other usages to getAttribute)

@@ +1319,5 @@
> +      return;
> +
> +    let button = this.button;
> +    if (button && button.label == this._starButtonOverflowedLabel)
> +      button.setAttribute("label", this._starButtonLabel);

Nit: the same.

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +3233,5 @@
>        if (child.getAttribute("overflows") != "false") {
>          this._collapsed.set(child.id, this._target.clientWidth);
>          child.classList.add("overflowedItem");
>          child.setAttribute("cui-anchorid", this._chevron.id);
> +        CustomizableUIInternal.notifyListeners("onWidgetOverflow", child, this._target);

These should be documented with the other event listeners both in this file and on the MDN page for the module. There might even be a bug on file about this.

Comment 18

[Mike de Boer [:mikedeboer]]

Pushed follow-up as https://hg.mozilla.org/integration/fx-team/rev/c6aad335ea99

Updated docs at https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/CustomizableUI.jsm

Comment 19

[Marco Bonardo [:mak]]

let button = this.button;
if (!this._starButtonLabel)
  this._starButtonLabel = button.label;
if (button 

using button before null-checking it looks unhealthy

Comment 20

[:Gijs (he/him)]

(In reply to Marco Bonardo [:mak] from comment #19)
> let button = this.button;
> if (!this._starButtonLabel)
>   this._starButtonLabel = button.label;
> if (button 
> 
> using button before null-checking it looks unhealthy

This is a fair point. In fact, why isn't that code using aNode - isn't that guaranteed to be the node we care about anyway?

Comment 21

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs Kruitbosch from comment #20)
> This is a fair point. In fact, why isn't that code using aNode - isn't that
> guaranteed to be the node we care about anyway?

It is. I'll post a follow-up patch shortly.

Comment 22

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2: follow-up

Comment 23

[:Gijs (he/him)]

Comment on attachment 8363729 [details] [diff] [review]
Patch 2: follow-up

Review of attachment 8363729 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 24

[Mike de Boer [:mikedeboer]]

remote: https://hg.mozilla.org/integration/fx-team/rev/b96543b9edd5

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/767ee9d0f0c1
https://hg.mozilla.org/mozilla-central/rev/c6aad335ea99
https://hg.mozilla.org/mozilla-central/rev/b96543b9edd5