1398582 - [10.13] Tab bar is garbled on MacOS (native window title overlaid)

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect, P1)

Description

[whmountains]

Attachment: Screenshot of the bug

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170910100150

Steps to reproduce:

Open a new browser window.


Actual results:

Tab bar looks garbled.

* The corners look like they have notches in them
* The bar is half black and half gray.  (Upper part is black for me, but looks white when I take a screenshot.
* The title of the current tab is overlaid over all the tabs


Expected results:

The above-mentioned graphics artifacts should not be present.

Comment 2

[Marcia Knous [:marcia]]

I see this as well using 20170910100150 on 10.13. Adding regression/regression range wanted keyword.

Comment 4

[whmountains]

I started seeing this bug the morning of September 10 (yesterday)

I download all the nightly releases as soon as they come out, so that should give some information about what release first had it.

Comment 5

[Markus Stange [:mstange]]

This was caused by bug 1324892.

Comment 6

[Gregory Szorc [:gps]]

[Tracking Requested - why for this release]:

This is a regression on OS X 10.13. 10.13 will likely be released before or shortly after Firefox 57. We probably don't want to lose track of this bug.

Comment 7

[Stephen A Pohl [:spohl]]

This is "fixed" when I comment out [mWindow setTitle:title] in nsCocoaWindow::SetTitle. I haven't had a chance to figure out what we're doing differently with the 10.11 SDK yet, but I wanted to mention it here in case it's helpful.

Comment 8

[Marcia Knous [:marcia]]

Using 10.13 Beta 9 seed and latest nightly, I also wanted to comment that there is a usability issue - once the tabs get overlaid, it becomes difficult to both select and/or close them (in my case both the "x" and the "+" don't work on the two tabs I have open.

Comment 9

[Stephen A Pohl [:spohl]]

Spoke with mstange and I'll be taking a closer look at this.

Comment 10

[Markus Stange [:mstange]]

I wrote up some relevant information about our window setup at https://public.etherpad-mozilla.org/p/mac-window .

Comment 13

[Stephen A Pohl [:spohl]]

Attachment: Patch

This hides window titles by default. This seems to fix the reported issues with the title bar. We need to set the visibility of window titles as early as possible or the window title may appear visible for a split second.

Comment 15

[Markus Stange [:mstange]]

Comment on attachment 8908383 [details] [diff] [review]
Patch

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

r+ with another setTitleVisibility call in the other if branch, or whatever other solution you can come up with that works. (I'm away until Monday.)

::: widget/cocoa/nsCocoaWindow.mm
@@ +1845,5 @@
>      [mWindow disableSetNeedsDisplay];
>      [mWindow setTitle:title];
>      [mWindow enableSetNeedsDisplay];
>    } else {
> +    [mWindow setTitleVisibility:NSWindowTitleVisible];

You never set the title visibility back to hidden. I think this means that if you toggle the titlebar visibility during toolbar customization, you'll show the title and neven hide it again.

Comment 16

[Stephen A Pohl [:spohl]]

Attachment: Patch

Addressed feedback, carrying over r+.

(In reply to Markus Stange [:mstange] from comment #15)
> Comment on attachment 8908383 [details] [diff] [review]
> Patch
> 
> Review of attachment 8908383 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with another setTitleVisibility call in the other if branch, or whatever
> other solution you can come up with that works. (I'm away until Monday.)
> 
> ::: widget/cocoa/nsCocoaWindow.mm
> @@ +1845,5 @@
> >      [mWindow disableSetNeedsDisplay];
> >      [mWindow setTitle:title];
> >      [mWindow enableSetNeedsDisplay];
> >    } else {
> > +    [mWindow setTitleVisibility:NSWindowTitleVisible];
> 
> You never set the title visibility back to hidden. I think this means that
> if you toggle the titlebar visibility during toolbar customization, you'll
> show the title and neven hide it again.

Good point! I've added the setTitleVisibility call in the other if branch. Thank you!

Comment 17

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f89ae3c450ce33915e4d2fff0b11b942dec17f1a
Bug 1398582: Prevent drawing titles in title bars on macOS 10.13 when we don't want them. r=mstange

Comment 18

[Michał Gołębiowski-Owczarek [:mgol]]

(In reply to Gregory Szorc [:gps] from comment #6)
> [Tracking Requested - why for this release]:
> 
> This is a regression on OS X 10.13. 10.13 will likely be released before or
> shortly after Firefox 57. We probably don't want to lose track of this bug.

macOS 10.13 is going to be released on September 25 i.e. 3 days before Firefox 56, not 57.

Comment 19

[Stephen A Pohl [:spohl]]

(In reply to Michał Gołębiowski-Owczarek [:mgol] from comment #18)
> (In reply to Gregory Szorc [:gps] from comment #6)
> > [Tracking Requested - why for this release]:
> > 
> > This is a regression on OS X 10.13. 10.13 will likely be released before or
> > shortly after Firefox 57. We probably don't want to lose track of this bug.
> 
> macOS 10.13 is going to be released on September 25 i.e. 3 days before
> Firefox 56, not 57.

Firefox 56 is built with the 10.7 SDK, which doesn't run into this bug. See comment 5.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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

Comment 21

[Jorg K (CEST = GMT+2)]

This is busting Mac compiles for Thunderbird:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=2660f4337132dfcb6d9d4ccefd365f4353eff28c&selectedJob=131374693

/builds/slave/tb-c-cen-m64-00000000000000000/build/mozilla/widget/cocoa/nsCocoaWindow.mm:476:12: error: instance method '-setTitleVisibility:' not found (return type defaults to 'id') [-Werror,-Wobjc-method-access] [log…]
 
/builds/slave/tb-c-cen-m64-00000000000000000/build/mozilla/widget/cocoa/nsCocoaWindow.mm:476:31: error: use of undeclared identifier 'NSWindowTitleHidden'; did you mean 'kWindowTitleBarRgn'? [log…]
 
/builds/slave/tb-c-cen-m64-00000000000000000/build/mozilla/widget/cocoa/nsCocoaWindow.mm:1846:14: error: instance method '-setTitleVisibility:' not found (return type defaults to 'id') [-Werror,-Wobjc-method-access] [log…]
 
/builds/slave/tb-c-cen-m64-00000000000000000/build/mozilla/widget/cocoa/nsCocoaWindow.mm:1846:33: error: use of undeclared identifier 'NSWindowTitleHidden'; did you mean 'kWindowTitleBarRgn'? [log…]
 
/builds/slave/tb-c-cen-m64-00000000000000000/build/mozilla/widget/cocoa/nsCocoaWindow.mm:1850:14: error: instance method '-setTitleVisibility:' not found (return type defaults to 'id') [-Werror,-Wobjc-method-access] [log…]
 
/builds/slave/tb-c-cen-m64-00000000000000000/build/mozilla/widget/cocoa/nsCocoaWindow.mm:1850:33: error: use of undeclared identifier 'NSWindowTitleVisible' [log…] 

Are you missing some include file and haven't noticed it due to unified compilation? We've seen problems like this before.

Or maybe the Macs we use to build are missing the correct SDK?

Comment 22

[Stephen A Pohl [:spohl]]

This is available as of the 10.10 SDK. Has Thunderbird made the switch to the 10.11 SDK (bug 1324892)? That would be the proper way to fix it. Otherwise, we can declare this separately if needed.

Comment 23

[Stephen A Pohl [:spohl]]

(In reply to Stephen A Pohl [:spohl] from comment #22)
> This is available as of the 10.10 SDK. Has Thunderbird made the switch to
> the 10.11 SDK (bug 1324892)? That would be the proper way to fix it.
> Otherwise, we can declare this separately if needed.

I went ahead and backed this out[1] because we need to prevent users on 10.9 from calling this API. Also, while working on an unrelated 10.13 issue, I've noticed that we're still showing the title in some instances that we don't expect. I'll look into this as well.

[1] https://hg.mozilla.org/integration/mozilla-inbound/rev/0f15faa4f0fab46833bf9f8ca8c48c5f482f22b3

Comment 24

[Jorg K (CEST = GMT+2)]

Thanks for the quick answer that left me a little confused. So you backed out:
https://hg.mozilla.org/mozilla-central/rev/f89ae3c450ce with
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f15faa4f0fab46833bf9f8ca8c48c5f482f22b3
and reported it here and over in bug 1324892. I'm not sure how that is related.
Busting Thunderbird has never been a reason to back out stuff, but I hope it will be to the benefit of FF users as well.

Coming to your question: "Has Thunderbird made the switch to the 10.11 SDK (bug 1324892)?"

The honest answer is: I don't know. Looking at the three landings from bug 1324892, only the first one
https://hg.mozilla.org/mozilla-central/rev/643c70cb9158
relates to build configuration.

Since TB is not yet on Taskcluster I only ported one line, this one (bug 1324892 comment #74):
https://hg.mozilla.org/mozilla-central/rev/8a67124d36d9#l3.13
export CROSS_SYSROOT=$HOME_DIR/src/MacOSX10.11.sdk
I realise that this is about cross compilation (which we don't have) like the other changes, but I had to port it to keep our build files in sync (otherwise the build goes orange and then indexing of the C-C tree stops and hell breaks loose).

We don't have an equivalent to:
browser/config/tooltool-manifests/macosx64/cross-clang.manifest and
browser/config/tooltool-manifests/macosx64/cross-releng.manifest
We only have:
https://dxr.mozilla.org/comm-central/source/mail/config/tooltool-manifests/macosx64/clang.manifest
https://dxr.mozilla.org/comm-central/source/mail/config/tooltool-manifests/macosx64/releng.manifest

I can see it's becoming increasingly difficult to maintain Thunderbird on non-TaskCluster and our build/release engineer is working on the transition to TaskCluster. I'm just the poor jack-of-all-trades (master of none), here in the function of C-C sheriff. Also see another Mac bustage which got shadowed by this one, bug 1398133 comment #4.

Comment 25

[Sören Hentzschel]

reopened based on comment 23.

Comment 26

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/mozilla-central/rev/0f15faa4f0fa (Backout)

For the TB side, we're now back to another compile problem bug 1398133 comment #4 and bug 1400533.

Comment 28

[Stephen A Pohl [:spohl]]

Attachment: Patch

The reason why the title was still displayed very frequently was because it isn't guaranteed that setDrawsContentsIntoWindowFrame will have been called on the window before we call nsCocoaWindow::SetTitle. This also means that our attempt at avoiding invalidations in nsCocoaWindow::SetTitle didn't work most of the time. We now set the visibility of the title in setDrawsContentsIntoWindowFrame directly. I've also taken this opportunity to clean up nsCocoaWindow::SetTitle. Furthermore, we now only call setTitleVisibility on 10.10 and above and we also declare placeholders in order to compile with SDKs below 10.10 (Thunderbird). I have tested builds compiled with 10.7 and 10.11 on macOS 10.13 and things are working as expected now.

Comment 30

[Markus Stange [:mstange]]

Comment on attachment 8909786 [details] [diff] [review]
Patch

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

::: widget/cocoa/nsCocoaWindow.mm
@@ +3152,5 @@
>    mDrawsIntoWindowFrame = aState;
>    if (changed) {
>      [self updateContentViewSize];
>      [self reflowTitlebarElements];
> +    if (nsCocoaFeatures::OnYosemiteOrLater()) {

Can we make this [self respondsToSelector:@selector(setTitleVisibility:)]? I dislike explicit version assumptions, even if we know that this assumption is correct.

Comment 31

[Stephen A Pohl [:spohl]]

Attachment: Patch

Thank you for the fast review! Addressed feedback, carrying over r+.

Comment 32

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e4ac2e4268c75afeb6c3428bf0e7957287f2bd9f
Bug 1398582: Prevent drawing titles in title bars on macOS 10.13 when we don't want them. r=mstange

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 34

[Khary]

I'm still experiencing this bug as well as bug 1401297 in the latest Nightly (20170920100426) on macOS 10.13 (17A362a)

Comment 35

[wavded]

I also am experiencing this bug, but perhaps it didn't make it out in today's update?

Comment 36

[:Gijs (he/him)]

(In reply to wavded from comment #35)
> I also am experiencing this bug, but perhaps it didn't make it out in
> today's update?

The changeset looks like it made it in for today's nightly. Stephen?

Comment 37

[Stephen A Pohl [:spohl]]

on it

Comment 38

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b3637dfc01ba79a37c2e3a0c4b65eacd1bb0fa56
Bug 1398582: Followup to fix the selector checks in changeset e4ac2e4268c7. r=mstange

Comment 39

[Stephen A Pohl [:spohl]]

Attachment: Followup

The patch had a copy/paste error in the selector checks, which is fixed in this followup. Carrying over r+ since this was what was reviewed and should have landed in the first place.

Comment 40

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 41

[Marcia Knous [:marcia]]

Using 20170921100141, I am not seeing this issue both on my existing Nightly profile as well as a newly created profile.

Comment 42

[wavded]

Looks great today.  Thanks everybody.

Comment 43

[charlie.siegel]

That's odd.  I'm still seeing the problem on the latest Nightly.  I even tried nuking my profile and starting over.  Are you two on High Sierra?  I'm on the High Sierra GM (17A362a).

Comment 44

[whmountains]

It seems that this is partially fixed.  The window title is no longer displayed, but there are still rendering artifacts.  (See the attachment named "rendering-artifacts.png"

Comment 45

[whmountains]

Attachment: Screenshot from the latest Firefox Nightly as of Sep. 21. I'm using MacOS High Sierra GM Candidate.

Comment 46

[charlie.siegel]

I'm seeing the same thing.  Still have the grey bar and white notches in the corner.  I'm uploading a screenshot of this - "nightly-title-bar.png"

Comment 47

[charlie.siegel]

Attachment: nightly-title-bar.png

Comment 48

[Sören Hentzschel]

Confirmed, the patch only fixed a part of the problem. The problem already exists since 12 days and there is already a first Beta of Firefox 57 available. Since Firefox 57 is an so important release and should be tested as much as possible I wonder if bug 1324892 should not be backed out if it's so difficult / time-consuming to fix this big visual regression of the browser tabs…

Comment 49

[Stephen A Pohl [:spohl]]

This bug handled the case of native window titles being overlaid on top of tabs. The remaining issue is covered by bug 1391790. Unless native window titles are still overlaid on top of our tabs, this bug is fixed. If there are other visual bugs that only affect macOS 10.13, please file new bugs and have them block bug 1391790.

Comment 50

[Stephen A Pohl [:spohl]]

(In reply to Stephen A Pohl [:spohl] from comment #49)
> The remaining issue is covered by bug 1391790.

This should have been bug 1400057.

Comment 51

[whmountains]

Thank you Stephen for your hard work on this!

Comment 53

[charlie.siegel]

I just wanted to let you know, I am still seeing this in the beta version of 57 & the shipping version of High Sierra.

Is this slated to be fixed prior the release of 57?

Comment 54

[Stephen A Pohl [:spohl]]

(In reply to charlie.siegel from comment #53)
> I just wanted to let you know, I am still seeing this in the beta version of
> 57 & the shipping version of High Sierra.
> 
> Is this slated to be fixed prior the release of 57?

This bug only dealt with the native title being overlaid on top of tabs (see attachment 8906359 [details]). You're most likely seeing bug 1401957. For other bugs specific to macOS 10.13, see the list of dependent bugs in bug 1391790.