Closed Bug 1398582 Opened 2 years ago Closed 2 years ago

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

Categories

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

57 Branch
Unspecified
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 + fixed

People

(Reporter: whmountains, Assigned: spohl)

References

Details

(Keywords: regression)

Attachments

(3 files, 5 obsolete files)

Attached image 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.
Duplicate of this bug: 1398583
Component: Untriaged → Theme
OS: Unspecified → Mac OS X
I see this as well using 20170910100150 on 10.13. Adding regression/regression range wanted keyword.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Blocks: 1391790
Blocks: 1324892
Summary: Tab bar is garbled on MacOS (native window title overlaid) → [10.13] Tab bar is garbled on MacOS (native window title overlaid)
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Component: Theme → Widget: Cocoa
Product: Firefox → Core
Duplicate of this bug: 1398571
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.
This was caused by bug 1324892.
[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.
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.
Priority: -- → P1
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.
Spoke with mstange and I'll be taking a closer look at this.
Assignee: mstange → spohl.mozilla.bugs
I wrote up some relevant information about our window setup at https://public.etherpad-mozilla.org/p/mac-window .
Duplicate of this bug: 1399531
Duplicate of this bug: 1399716
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #8908383 - Flags: review?(mstange)
Duplicate of this bug: 1400055
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.
Attachment #8908383 - Flags: review?(mstange) → review+
Attached patch Patch (obsolete) — Splinter Review
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!
Attachment #8908383 - Attachment is obsolete: true
Attachment #8908650 - Flags: review+
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
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/f89ae3c450ce
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
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?
Flags: needinfo?(spohl.mozilla.bugs)
Flags: needinfo?(mstange)
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.
Flags: needinfo?(spohl.mozilla.bugs) → needinfo?(jorgk)
(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
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.
Flags: needinfo?(jorgk)
reopened based on comment 23.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Target Milestone: mozilla57 → ---
Duplicate of this bug: 1400616
Flags: needinfo?(mstange)
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #8908650 - Attachment is obsolete: true
Attachment #8909786 - Flags: review?(mstange)
Duplicate of this bug: 1401175
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.
Attachment #8909786 - Flags: review?(mstange) → review+
Attached patch PatchSplinter Review
Thank you for the fast review! Addressed feedback, carrying over r+.
Attachment #8909786 - Attachment is obsolete: true
Attachment #8909842 - Flags: review+
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
https://hg.mozilla.org/mozilla-central/rev/e4ac2e4268c7
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
I'm still experiencing this bug as well as bug 1401297 in the latest Nightly (20170920100426) on macOS 10.13 (17A362a)
I also am experiencing this bug, but perhaps it didn't make it out in today's update?
(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?
Flags: needinfo?(spohl.mozilla.bugs)
on it
Flags: needinfo?(spohl.mozilla.bugs)
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3637dfc01ba79a37c2e3a0c4b65eacd1bb0fa56
Bug 1398582: Followup to fix the selector checks in changeset e4ac2e4268c7. r=mstange
Attached patch FollowupSplinter Review
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.
Attachment #8910274 - Flags: review+
Depends on: 1401641
Using 20170921100141, I am not seeing this issue both on my existing Nightly profile as well as a newly created profile.
Looks great today.  Thanks everybody.
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).
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"
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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"
Attached image nightly-title-bar.png (obsolete) —
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…
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.
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
(In reply to Stephen A Pohl [:spohl] from comment #49)
> The remaining issue is covered by bug 1391790.

This should have been bug 1400057.
Thank you Stephen for your hard work on this!
Duplicate of this bug: 1401860
Depends on: 1402577
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?
Attachment #8910791 - Attachment is obsolete: true
Attachment #8910790 - Attachment is obsolete: true
(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.
Blocks: 1404036
No longer blocks: 1404036
Depends on: 1404036
Duplicate of this bug: 1400747
You need to log in before you can comment on or make changes to this bug.