Closed Bug 1045213 Opened 5 years ago Closed 5 years ago

[10.10] Contextmenus shadow and background color are incorrect

Categories

(Core :: Widget: Cocoa, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: bugzilla, Assigned: mstange)

References

(Blocks 1 open bug)

Details

(Whiteboard: [yosemite])

Attachments

(9 files, 6 obsolete files)

21.80 KB, image/png
Details
23.15 KB, image/png
Details
4.50 KB, patch
jrmuizel
: review+
kats
: feedback+
Details | Diff | Splinter Review
13.01 KB, patch
Details | Diff | Splinter Review
8.72 KB, patch
smichaud
: review+
Details | Diff | Splinter Review
3.09 KB, patch
botond
: review+
Details | Diff | Splinter Review
29.59 KB, patch
roc
: review+
Details | Diff | Splinter Review
7.89 KB, patch
smichaud
: review+
Details | Diff | Splinter Review
4.78 KB, patch
smichaud
: review+
Details | Diff | Splinter Review
Attached image 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
Attached image Safari's context-menu
(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.
Flags: needinfo?(bugzilla)
(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"
Flags: needinfo?(bugzilla)
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.
(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? :-)
Flags: needinfo?(mstange)
(In reply to :Gijs Kruitbosch from comment #5)
> Are
> context menus "windows" as far as this is concerned, or something? :-)

Correct.
Flags: needinfo?(mstange)
Component: Menus → Widget: Cocoa
Product: Firefox → Core
(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)
Depends on: 1081160
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Attachment #8503218 - Flags: review?(jmuizelaar)
Attachment #8503220 - Flags: review?(smichaud)
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.
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 on attachment 8503220 [details] [diff] [review]
part 2: use vibrancy for the menu background

Looks reasonable to me.
Attachment #8503220 - Flags: review?(smichaud) → review+
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.
Attachment #8503218 - Flags: review?(jmuizelaar) → review+
Any updates on this? 33.1 is out, and the visual style for Yosemite is still broken
(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.
Depends on: 1101236
Attachment #8503220 - Attachment is obsolete: true
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.
Attachment #8557226 - Flags: review?(roc)
Attachment #8503227 - Attachment is obsolete: true
Attachment #8503228 - Attachment is obsolete: true
Attachment #8557228 - Flags: review?(smichaud)
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.
Attachment #8557230 - Flags: review?(smichaud)
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.
Attachment #8557226 - Flags: review?(roc)
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! :))
Attachment #8557233 - Flags: review?(botond) → review+
Attachment #8557226 - Attachment is obsolete: true
Attachment #8557252 - Flags: review?(roc)
(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 on attachment 8557252 [details] [diff] [review]
part 3: let the theme influence the widget type passed to UpdateThemeGeometries

Still more to fix...
Attachment #8557252 - Flags: review?(roc)
Updated for the changed part 3.
Attachment #8557326 - Flags: review?(smichaud)
Attachment #8557230 - Attachment is obsolete: true
Attachment #8557230 - Flags: review?(smichaud)
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.
Attachment #8557325 - Flags: review?(roc) → review+
Using foreground vibrancy means that the opaquely painted menu separators blend with the background properly.
Attachment #8558591 - Flags: review?(smichaud)
Comment on attachment 8557228 [details] [diff] [review]
part 4: use vibrancy for the highlighted menu item

Looks fine to me.
Attachment #8557228 - Flags: review?(smichaud) → review+
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?
Attachment #8557326 - Flags: review?(smichaud) → review+
Comment on attachment 8558591 [details] [diff] [review]
part 7: use foreground vibrancy and a different material for menus

Looks good to me.
Attachment #8558591 - Flags: review?(smichaud) → review+
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?
Attachment #8503218 - Flags: feedback?(bugmail.mozilla)
Oops, looks like I forgot to address Botond's review comment (renaming T to Region). I'll fix that tomorrow.
Flags: needinfo?(mstange)
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.
Attachment #8503218 - Flags: feedback?(bugmail.mozilla) → feedback+
Depends on: 1130892
Renamed the template parameters. https://hg.mozilla.org/integration/mozilla-inbound/rev/c0b65373dde0
Flags: needinfo?(mstange)
Depends on: 1142393
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
You need to log in before you can comment on or make changes to this bug.