1045213 - [10.10] Contextmenus shadow and background color are incorrect

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[José Jeria]

Attachment: Firefox's context-menu

Build ID:Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:34.0) Gecko/20100101 Firefox/34.0

Firefox's context-menu looks a bit different compared to the native context-menues:
* The background color is darker compared to native context-menues
* Shadow seems to be stronger than Safari's 
* Distance between items seems to be smaller

See attached screenshots to compare

Comment 1

[José Jeria]

Attachment: Safari's context-menu

Comment 2

[:Gijs (he/him)]

(In reply to José Jeria from comment #0)
> Created attachment 8463583 [details]
> Firefox's context-menu
> 
> Build ID:Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:34.0)
> Gecko/20100101 Firefox/34.0
> 
> Firefox's context-menu looks a bit different compared to the native
> context-menues:
> * The background color is darker compared to native context-menues
> * Shadow seems to be stronger than Safari's 
> * Distance between items seems to be smaller
> 
> See attached screenshots to compare

Did you self-compile this copy of Firefox? Can you check with an official build and post a new screenshot? It looks like the font in your screenshot is broken because of bug 931040.

Comment 3

[José Jeria]

(In reply to :Gijs Kruitbosch from comment #2)
> Did you self-compile this copy of Firefox? Can you check with an official
> build and post a new screenshot? It looks like the font in your screenshot
> is broken because of bug 931040.

I did not self-compile Firefox, I grabbed the official nightly build. 

Correction of comment 0:
The context-menu in Firefox is _lighter_ than the native context-menues. The other points remain valid as well. Tested again with "Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:34.0) Gecko/20100101 Firefox/34.0"

Comment 4

[Markus Stange [:mstange]]

We need to reimplement the context menu background using behind-window vibrancy.

For the selected menu item, the dark blue background is achieved with a special undocumented "material" on its NSVisualEffectView ([view setMaterial:3] does it, I think). This blue vibrancy shows in the areas of the window that we leave transparent, so we need to paint the text directly onto a completely clear background. In order to get correct subpixel text antialiasing we'll need to use the private API CGContextSetFontSmoothingBackgroundColor which is present on 10.9 and up.

Comment 5

[:Gijs (he/him)]

(In reply to Markus Stange [:mstange] from comment #4)
> We need to reimplement the context menu background using behind-window
> vibrancy.

This is a little confusing to me - now that I finally managed to install 10.10 on a real machine, it seems like the context menus use the background of the page. For instance, if I open the context menu in safari on apple.com against the complex backgrounds the site uses, it's that background that "vibrates" through the context menu - not the desktop background. Are context menus "windows" as far as this is concerned, or something? :-)

Comment 6

[Markus Stange [:mstange]]

(In reply to :Gijs Kruitbosch from comment #5)
> Are
> context menus "windows" as far as this is concerned, or something? :-)

Correct.

Comment 7

[Markus Stange [:mstange]]

(In reply to Markus Stange [:mstange] from comment #4)
> For the selected menu item, the dark blue background is achieved with a
> special undocumented "material" on its NSVisualEffectView ([view
> setMaterial:3] does it, I think).

Update: This is now material 4, and additionally we need to call [view setEmphasized:YES]. (NO means inactive selection, which is gray)

Comment 8

[Markus Stange [:mstange]]

Attachment: part 1: make region methods chainable

Comment 9

[Markus Stange [:mstange]]

Attachment: part 2: use vibrancy for the menu background

Comment 10

[Markus Stange [:mstange]]

Part 2 doesn't make us use vibrancy for the highlighted menu item yet. Highlighted menu items are still drawn opaquely onto the vibrant menu. That's because the vibrancy type needs to be different based on whether an attribute is set on the menu item, but in nsChildView::UpdateThemeGeometries we don't have access to the original elements, we only know their -moz-appearance value, and that's not enough information to know whether the menu item is highlighted.

Comment 11

[Markus Stange [:mstange]]

Attachment: part 3 wip: let the theme influence the widget type passed to UpdateThemeGeometries

Comment 12

[Markus Stange [:mstange]]

Attachment: part 4 wip: use vibrancy for the highlighted menu item

Comment 13

[Markus Stange [:mstange]]

Unfortunately I haven't been able to get anti-aliasing on the menu corners. The strategy that we use for the main window didn't want to work in my tests. There's CGSSetWindowCornerMask which -[NSPopOverFrame shapeWindow], -[NSWindow _cornerMaskChanged] and _NSSetWindowCornerMask use... maybe we can override -[NSWindow _hasCornerMask] and -[NSWindow _cornerMask] for context menu windows?

Comment 14

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8503220 [details] [diff] [review]
part 2: use vibrancy for the menu background

Looks reasonable to me.

Comment 15

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8503218 [details] [diff] [review]
part 1: make region methods chainable

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

Would multiple argument versions of the operations serve your needs just as well? If they would I think I'd prefer that kind of API. That being said these additions are probably fine.

Comment 16

[Omar]

Any updates on this? 33.1 is out, and the visual style for Yosemite is still broken

Comment 17

[:Gijs (he/him)]

(In reply to Omar from comment #16)
> Any updates on this? 33.1 is out, and the visual style for Yosemite is still
> broken

This particular bug needs more work and won't be in 33 or 34, most likely - but you should try using 34 (current beta) as it looks and works a lot better on Yosemite than 33 does.

Comment 18

[Markus Stange [:mstange]]

Attachment: part 2: use vibrancy for the menu background

Comment 19

[Markus Stange [:mstange]]

Attachment: part 3: let the theme influence the widget type passed to UpdateThemeGeometries

In the next part I need nsChildView::UpdateThemeGeometries to do something different for NS_THEME_MENUITEM only when it's highlighted. But at the moment, ThemeGeometry doesn't contain any additional information beyond the widget type, and the widget type isn't enough to determine whether the item is highlighted. So I'd like to put one step of indirection in between: nsITheme::ThemeGeometryTypeForWidget will have access to the nsIFrame and will check whether it's highlighted, and translate that into an enum which is put in the ThemeGeometry.

Comment 20

[Markus Stange [:mstange]]

Attachment: part 4: use vibrancy for the highlighted menu item

Comment 21

[Markus Stange [:mstange]]

Attachment: part 5: use _cornerMask for correct rounded corner anti-aliasing

This gets us correct anti-aliasing in the rounded corners of vibrant context menus.

Since the vibrancy effect is implemented in the window server, the window server needs to apply the anti-aliasing. In order to be able to do that, it's not enough for it to know the vibrant rectangles; it also needs to have a mask image. Supplying such a mask image is done through _cornerMask.

Comment 22

[Markus Stange [:mstange]]

Attachment: part 6: add a MakeRegionsNonOverlapping function for less repetitive code

Comment 23

[Markus Stange [:mstange]]

Comment on attachment 8557226 [details] [diff] [review]
part 3: let the theme influence the widget type passed to UpdateThemeGeometries

I need to rename mWidgetType to mType and fix Windows.

Comment 24

[Botond Ballo [:botond]]

Comment on attachment 8557233 [details] [diff] [review]
part 6: add a MakeRegionsNonOverlapping function for less repetitive code

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

Nice!

I would replace the names 'T' and 'TT' with 'Region' and 'Regions'.

(Is this the first use of variadic templates in the codebase? If so, I'm proud to be its reviewer! :))

Comment 25

[Markus Stange [:mstange]]

Attachment: part 3: let the theme influence the widget type passed to UpdateThemeGeometries

Comment 26

[Markus Stange [:mstange]]

(In reply to Botond Ballo [:botond] from comment #24)
> Comment on attachment 8557233 [details] [diff] [review]
> part 6: add a MakeRegionsNonOverlapping function for less repetitive code
> 
> Review of attachment 8557233 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice!
> 
> I would replace the names 'T' and 'TT' with 'Region' and 'Regions'.

Good idea.

> (Is this the first use of variadic templates in the codebase? If so, I'm
> proud to be its reviewer! :))

Looks like we missed that opportunity: https://dxr.mozilla.org/mozilla-central/search?tree=mozilla-central&q=%22typename...%22 :-)

Comment 27

[Markus Stange [:mstange]]

Comment on attachment 8557252 [details] [diff] [review]
part 3: let the theme influence the widget type passed to UpdateThemeGeometries

Still more to fix...

Comment 28

[Markus Stange [:mstange]]

Attachment: part 3: let the theme influence the widget type passed to UpdateThemeGeometries

Comment 29

[Markus Stange [:mstange]]

Attachment: part 5: use _cornerMask for correct rounded corner anti-aliasing

Updated for the changed part 3.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8557325 [details] [diff] [review]
part 3: let the theme influence the widget type passed to UpdateThemeGeometries

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

::: gfx/src/nsITheme.h
@@ +146,5 @@
>    virtual bool WidgetProvidesFontSmoothingBackgroundColor(nsIFrame* aFrame,
>                                        uint8_t aWidgetType, nscolor* aColor)
>    { return false; }
>  
> +  typedef uint8_t ThemeGeometryType;

Please write some documentation for what these values mean.

Comment 31

[Markus Stange [:mstange]]

Attachment: part 7: use foreground vibrancy and a different material for menus

Using foreground vibrancy means that the opaquely painted menu separators blend with the background properly.

Comment 32

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8557228 [details] [diff] [review]
part 4: use vibrancy for the highlighted menu item

Looks fine to me.

Comment 33

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8557326 [details] [diff] [review]
part 5: use _cornerMask for correct rounded corner anti-aliasing

This looks fine to me, and I assume you've tested it (and found that it works).  But I do have one question:

+  NSImage* maskImage = [NSImage imageWithSize:maskSize flipped:YES drawingHandler:^BOOL(NSRect dstRect) {
+    NSBezierPath *path = [NSBezierPath bezierPathWithRoundedRect:dstRect xRadius:radius yRadius:radius];
+    [path fill];
+    return YES;
+  }];

Do you know that [path fill] always uses the correct "fill color"?  Can we guarantee it?

Comment 34

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8558591 [details] [diff] [review]
part 7: use foreground vibrancy and a different material for menus

Looks good to me.

Comment 35

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/657fc2d2da4a
https://hg.mozilla.org/integration/mozilla-inbound/rev/984a21c9423c
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e3291b79e57
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa950339690a
https://hg.mozilla.org/integration/mozilla-inbound/rev/c07194e580ce
https://hg.mozilla.org/integration/mozilla-inbound/rev/caac1a006b9d
https://hg.mozilla.org/integration/mozilla-inbound/rev/b46cb1ef16f8

Comment 36

[Markus Stange [:mstange]]

Comment on attachment 8503218 [details] [diff] [review]
part 1: make region methods chainable

Jeff mentioned that you might oppose this patch. I was using the chainable methods in the "part 2" patch in this bug, but "part 3" removes those uses again, so I'd be fine with backing out this patch again. How do you feel about this change?

Comment 37

[Markus Stange [:mstange]]

Oops, looks like I forgot to address Botond's review comment (renaming T to Region). I'll fix that tomorrow.

Comment 38

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8503218 [details] [diff] [review]
part 1: make region methods chainable

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

(In reply to Markus Stange [:mstange] from comment #36)
> Jeff mentioned that you might oppose this patch. I was using the chainable
> methods in the "part 2" patch in this bug, but "part 3" removes those uses
> again, so I'd be fine with backing out this patch again. How do you feel
> about this change?

My preference would be to see methods like these:

nsRegion Or(const nsRegion& aOther) const; // returns a new region, doesn't modify |this|
void OrWith(const nsRegion& aOther); // modifies |this|

but having OrWith return |this| isn't that bad either. I can live with it if it's already landed. Bug 1040986 documents my original gripe and rationale.

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/657fc2d2da4a
https://hg.mozilla.org/mozilla-central/rev/984a21c9423c
https://hg.mozilla.org/mozilla-central/rev/4e3291b79e57
https://hg.mozilla.org/mozilla-central/rev/fa950339690a
https://hg.mozilla.org/mozilla-central/rev/c07194e580ce
https://hg.mozilla.org/mozilla-central/rev/caac1a006b9d
https://hg.mozilla.org/mozilla-central/rev/b46cb1ef16f8

Comment 40

[Markus Stange [:mstange]]

Renamed the template parameters. https://hg.mozilla.org/integration/mozilla-inbound/rev/c0b65373dde0

Comment 41

[Ryan VanderMeulen [:RyanVM]]

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

Comment 42

[David Adler]

FYI, this change apparently created an incompatibility Thumbnail Zoom Plus add-on.  Starting in Firefox 38 and tracked to this change using mozregression, TZP's pop-ups on OS X Yosemite appear too dark or slightly transparent / translucent.

I found that it is worked around by turning on "System Preferences > Accessibility > Display > Reduce transparency".

I (developer of TZP) fixed it by adding "-moz-appearance: none" CSS to TZP's 'panel' xul element.  With that fix, it works with either setting of "Reduce transparency".  The fix is in TZP 3.5beta1.

Details and test case at https://github.com/dadler/thumbnail-zoom/issues/215