1461635 - The bookmark vertical separators are invisible on Mac platform

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

[Affected versions]:
- 62.0a1 (2018-05-14)
- 61.0b5 (20180514150347)

[Affected platforms]:
- macOS 10.13.3
- Mac OS X 10.10.5

[Steps to reproduce]:
1. Launch Firefox
2. Enable the bookmarks toolbar
3. Try to add some separators (by using the "New Separator" context menu option)

[Expected result]:
- The separators are visible and successfully added 

[Actual result]:
- The separators are invisible (blank spaces are added instead) 

[Regression range]:
- Last good revision: 964dbe5e2afe0956a3f1b08ec1c0248dfdb33820
- First bad revision: 485b7f8c9d9a6a7cc23dc70803c8cd3f169634ef
- Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=964dbe5e2afe0956a3f1b08ec1c0248dfdb33820&tochange=485b7f8c9d9a6a7cc23dc70803c8cd3f169634ef
- bug 1452303 introduced the regression

Comment 1

[Mike Taylor [:miketaylr]]

hey Dao, is this worth blocking on?

Comment 2

[Dão Gottwald [:dao]]

(In reply to Mike Taylor [:miketaylr] from comment #1)
> hey Dao, is this worth blocking on?

Tracking yes. Blocking probably not, but let's try hard to fix this.

Comment 3

[Dão Gottwald [:dao]]

mstange, could this be a widget::cocoa bug? ntim tested -moz-appearance:separator with both orientations on Mac in attachment 8966788 [details], so I'm a bit confused. Maybe the separator is rendered but with the same color as the toolbar, making it invisible?

Comment 4

[Markus Stange [:mstange]]

The inspector shows that the width of the separator element is zero. If I set the width to 1px, for example, then the separator shows up, and its color is sufficiently different from the toolbar color.

It's probably worth setting a non-zero size for NS_THEME_SEPARATOR in nsNativeThemeCocoa::GetMinimumWidgetSize.

Comment 5

[Jim Mathies [:jimm]]

trying to determine how serious this is.

Comment 6

[Markus Stange [:mstange]]

It's a trivial fix, I'll do it.

Comment 7

[Markus Stange [:mstange]]

Attachment: Bug 1461635 - Enforce a minimum widget size of 1x1 for toolbar separators.

Review commit: https://reviewboard.mozilla.org/r/246176/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/246176/

Comment 8

[Stephen A Pohl [:spohl]]

Comment on attachment 8979991 [details]
Bug 1461635 - Enforce a minimum widget size of 1x1 for toolbar separators.

https://reviewboard.mozilla.org/r/246176/#review252310

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eedb9180bc3b
Enforce a minimum widget size of 1x1 for toolbar separators. r=spohl

Comment 10

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/eedb9180bc3b

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 12

[Markus Stange [:mstange]]

Comment on attachment 8979991 [details]
Bug 1461635 - Enforce a minimum widget size of 1x1 for toolbar separators.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1452303
[User impact if declined]: invisible separators in the bookmarks toolbar
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: it's working for me in today's nightly
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: only affects toolbar separators, which are rarely used, in an extremely simple way (enforces a minimum size of 1x1)
[String changes made/needed]: none

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8979991 [details]
Bug 1461635 - Enforce a minimum widget size of 1x1 for toolbar separators.

Simple fix for invisible bookmark separators. Approved for 61.0b9.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/37d3889a39a9

Comment 15

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

I can confirm that 61.0b9 build1 (20180528091514) and 62.0a1 (2018-05-28) are verified fixed using macOS 10.13.4 and Mac OS X 10.11.6.