Closed Bug 975807 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Theme, defect)

x86_64
Linux
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 30
Tracking Status
firefox29 --- verified
firefox30 --- verified

People

(Reporter: sjakthol, Assigned: phlsa)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P3] [bugday-20140423])

Attachments

(3 files, 2 obsolete files)

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.
Component: Untriaged → Theme
Jared, was this by design?
Flags: needinfo?(jaws)
See Also: → 975808
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).
Flags: needinfo?(jaws)
Summary: Separator between 'Bookmark this site' and 'Bookmark menu' buttons disappear when hovering over 'Bookmark this site' button → 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
Whiteboard: [Australis:P3]
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch Patch v1 (obsolete) — Splinter Review
This patch gives the dropmarker a negative margin, moving the buttons closer together. That means that the disappearing line makes sense again now.
Attachment #8386448 - Flags: review?(jaws)
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.
Attachment #8386448 - Flags: review?(jaws) → review-
Attached patch Patch v1.1 (obsolete) — Splinter Review
(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.
Attachment #8386448 - Attachment is obsolete: true
Attachment #8386926 - Flags: review?(jaws)
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.
Attachment #8386926 - Flags: review?(jaws) → review+
Attached patch Patch v1.2Splinter Review
Changed attribute to -moz-margin-start. Carrying forward r+ from jaws.
Attachment #8386926 - Attachment is obsolete: true
Attachment #8387008 - Flags: review+
remote:   https://hg.mozilla.org/integration/fx-team/rev/41a9f753ab25
Status: NEW → ASSIGNED
Keywords: checkin-needed
Whiteboard: [Australis:P3] → [Australis:P3][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/41a9f753ab25
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
Target Milestone: --- → Firefox 30
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
Attachment #8387008 - Flags: approval-mozilla-aurora?
Attachment #8387008 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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)
Status: RESOLVED → VERIFIED
Whiteboard: [Australis:P3] → [Australis:P3] [bugday-20140423]
You need to log in before you can comment on or make changes to this bug.