975807 - The separator between 'Bookmark this site' and 'Bookmark menu' should be placed at the same place as the right-side border of the hovered 'Bookmark this site' button

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Sami Jaktholm]

Attachment: Separator before hovering over button

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140223123707

Steps to reproduce:

Move mouse over 'Bookmark this site' button.


Actual results:

The separator next to it disappears.


Expected results:

The separator doesn't disappear.

Comment 1

[Sami Jaktholm]

Attachment: Missing separator while hovering

Comment 2

[:Gijs (he/him)]

Jared, was this by design?

Comment 3

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

Yes, it's by design because the separator and the right-side border are supposed to be overlapping and thus if we still showed the separator the right-side border may be darker than the rest of the borders. 

However the screenshot looks like the separator is not in the right place (it looks shifted to the right by about 3 or 4 pixels).

Comment 4

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: Patch v1

This patch gives the dropmarker a negative margin, moving the buttons closer together. That means that the disappearing line makes sense again now.

Comment 5

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

Comment on attachment 8386448 [details] [diff] [review]
Patch v1

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

::: browser/themes/linux/browser.css
@@ +123,5 @@
>  #personal-bookmarks[cui-areatype="menu-panel"] > #bookmarks-toolbar-placeholder {
>    list-style-image: url("chrome://browser/skin/places/bookmarksToolbar-menuPanel.png") !important;
>  }
>  
> +#bookmarks-menu-button > .toolbarbutton-menubutton-dropmarker {

This patch fixes the issue for the bookmarks-menu-button, but won't fix this issue for other type="menu-button" buttons. See http://mxr.mozilla.org/mozilla-central/source/browser/themes/linux/browser.css#637 for how that separator styling is done.

You should move this new rule to be closer to the previously referenced styling, and use
> :-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-1:not(:hover):not(:active):not([open]) > .toolbarbutton-menubutton-dropmarker
as the selector.

Comment 6

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: Patch v1.1

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> Comment on attachment 8386448 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 8386448 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/linux/browser.css
> @@ +123,5 @@
> >  #personal-bookmarks[cui-areatype="menu-panel"] > #bookmarks-toolbar-placeholder {
> >    list-style-image: url("chrome://browser/skin/places/bookmarksToolbar-menuPanel.png") !important;
> >  }
> >  
> > +#bookmarks-menu-button > .toolbarbutton-menubutton-dropmarker {
> 
> This patch fixes the issue for the bookmarks-menu-button, but won't fix this
> issue for other type="menu-button" buttons. See
> http://mxr.mozilla.org/mozilla-central/source/browser/themes/linux/browser.
> css#637 for how that separator styling is done.
> 
> You should move this new rule to be closer to the previously referenced
> styling, and use
> > :-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-1:not(:hover):not(:active):not([open]) > .toolbarbutton-menubutton-dropmarker
> as the selector.

This patch moves the code to the desired location. I had to remove tho :not(:hover)... parts of the selector because the item would jump around on hover/click otherwise.

Comment 7

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

Comment on attachment 8386926 [details] [diff] [review]
Patch v1.1

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

::: browser/themes/linux/browser.css
@@ +648,5 @@
>    box-shadow: 0 0 0 1px hsla(0,0%,100%,.2);
>  }
>  
> +:-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker {
> +  margin-left: -4px;

this should be -moz-margin-start. please upload a new patch with this changed and then mark the bug as checkin-needed.

Comment 8

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: Patch v1.2

Changed attribute to -moz-margin-start. Carrying forward r+ from jaws.

Comment 9

[:Gijs (he/him)]

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

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/41a9f753ab25

Comment 11

[:Gijs (he/him)]

Comment on attachment 8387008 [details] [diff] [review]
Patch v1.2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis Linux theming
User impact if declined: odd styling issues with menu buttons (e.g. the bookmarks menu button) on Linux
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 12

[:Gijs (he/him)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/b762e74fd0a7

Comment 13

[Francesca Ciceri [:madamezou]]

Hi,

I was able to reproduce the problem on Mozilla/5.0 (X11; Linux x86_64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140223004001 CSet: cff8055532f4.

I verified the fix in 

- latest Nightly (Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Firefox/31.0 ID:20140422030204 CSet: c962bde5ac0b) 

- latest Aurora (Mozilla/5.0 (X11; Linux x86_64; rv:30.0) Gecko/20100101 Firefox/30.0 ID:20140422004001 CSet: b3dddc937059)

- latest Beta (Mozilla/5.0 (X11; Linux x86_64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140417185217 CSet: da279b9f523a)