Closed Bug 940455 Opened 6 years ago Closed 6 years ago

Aero glass fog and lightweight themes cover minimize/maximize buttons during tab drag on Windows 7

Categories

(Core :: Graphics: Layers, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30
Tracking Status
firefox29 + wontfix
firefox30 + fixed

People

(Reporter: daniel.nr01, Assigned: mattwoodrow)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [Australis:P2][see bug 983638 for 29])

Attachments

(3 files, 2 obsolete files)

Attached video screencast
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20131118094134

Steps to reproduce:

Drag a tab in the tab strip over another tab.


Actual results:

The aero glass fog covers minimize/maximize buttons during drag (see screencast)
Component: Untriaged → Theme
Blocks: australis-tabs-win
No longer blocks: australis-tabs
Yeah I definitely see this.
Status: UNCONFIRMED → NEW
Ever confirmed: true
It also happens if you toggle the menus by pressing "Alt" twice.
Whiteboard: [Australis:P3]
It could be the same problem of Bug 816733
(In reply to Daniel from comment #2)
> It also happens if you toggle the menus by pressing "Alt" twice.

It happens even if you select an item from the menu after pressing ALT as well.
Attached image fog.PNG
It also happens, when you resize the window while in customization mode. If you decrease the width of the window, then the fog slides on top of the window buttons as soon as you hit the min-width of the customization content. (see the screenshot)
Jim, do you have any idea what's up here and/or what we can do about this?
Flags: needinfo?(jmathies)
Interesting bug. We clear the background for the button box down in theme code here - 

http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsNativeThemeWin.cpp#1903

This doesn't reproduce outside of customize mode. Australis bug probably? I think we should be scrunching/clipping content in the titlebar to make room for the button box.
Flags: needinfo?(jmathies)
(In reply to Jim Mathies [:jimm] from comment #7)
> This doesn't reproduce outside of customize mode. Australis bug probably? I
> think we should be scrunching/clipping content in the titlebar to make room
> for the button box.

It does reproduce outside of customize mode, if you just drag tabs around - during the dragging, sometimes the fog overlaps the titlebar buttons. I don't know why that is, but it seems weird that that would be a frontend thing...
(In reply to :Gijs Kruitbosch from comment #8)
> (In reply to Jim Mathies [:jimm] from comment #7)
> > This doesn't reproduce outside of customize mode. Australis bug probably? I
> > think we should be scrunching/clipping content in the titlebar to make room
> > for the button box.
> 
> It does reproduce outside of customize mode, if you just drag tabs around -
> during the dragging, sometimes the fog overlaps the titlebar buttons. I
> don't know why that is, but it seems weird that that would be a frontend
> thing...

Hmm, yeah I can reproduce that resizing the window, appears to be intermittent.  Something messed up with the dims of the button box maybe?
(In reply to Jim Mathies [:jimm] from comment #9)
> (In reply to :Gijs Kruitbosch from comment #8)
> > (In reply to Jim Mathies [:jimm] from comment #7)
> > > This doesn't reproduce outside of customize mode. Australis bug probably? I
> > > think we should be scrunching/clipping content in the titlebar to make room
> > > for the button box.
> > 
> > It does reproduce outside of customize mode, if you just drag tabs around -
> > during the dragging, sometimes the fog overlaps the titlebar buttons. I
> > don't know why that is, but it seems weird that that would be a frontend
> > thing...
> 
> Hmm, yeah I can reproduce that resizing the window, appears to be
> intermittent.  Something messed up with the dims of the button box maybe?

The fog is meant to be below the buttons and clipped by the code you mentioned in comment #7.

We're heavily focusing on shipping in 29. This bug, however, doesn't seem like something we can fix on the frontend side. Is there a chance platform can investigate and fix this in time for a 29 release?
Flags: needinfo?(jmathies)
(In reply to :Gijs Kruitbosch from comment #10)
> (In reply to Jim Mathies [:jimm] from comment #9)
> > (In reply to :Gijs Kruitbosch from comment #8)
> > > (In reply to Jim Mathies [:jimm] from comment #7)
> > > > This doesn't reproduce outside of customize mode. Australis bug probably? I
> > > > think we should be scrunching/clipping content in the titlebar to make room
> > > > for the button box.
> > > 
> > > It does reproduce outside of customize mode, if you just drag tabs around -
> > > during the dragging, sometimes the fog overlaps the titlebar buttons. I
> > > don't know why that is, but it seems weird that that would be a frontend
> > > thing...
> > 
> > Hmm, yeah I can reproduce that resizing the window, appears to be
> > intermittent.  Something messed up with the dims of the button box maybe?
> 
> The fog is meant to be below the buttons and clipped by the code you
> mentioned in comment #7.
> 
> We're heavily focusing on shipping in 29. This bug, however, doesn't seem
> like something we can fix on the frontend side. Is there a chance platform
> can investigate and fix this in time for a 29 release?

All our win devs are concentrating on other things. tabraldes and myself are on the metro release, bbondy is on sandboxing. That's all we've got. I don't think this needs to block though, it's polish - barely noticeable and intermittent. But should be kept on the radar somehow.
Flags: needinfo?(jmathies)
Actually, for 29 one of us might have the time. Just not before the 28 merge in three weeks.
Hey Jim,

> Just not before the 28 merge in three weeks.

By this, did you mean the 29 merge in ~3 weeks? So, for clarity, nobody could be spared until 30 hits Nightly?

-Mike
Flags: needinfo?(jmathies)
(In reply to Mike Conley (:mconley) from comment #13)
> Hey Jim,
> 
> > Just not before the 28 merge in three weeks.
> 
> By this, did you mean the 29 merge in ~3 weeks? So, for clarity, nobody
> could be spared until 30 hits Nightly?
> 
> -Mike

yes, that's right.
Flags: needinfo?(jmathies)
(In reply to Jim Mathies [:jimm] from comment #12)
> Actually, for 29 one of us might have the time. Just not before the 28 merge
> in three weeks.

Yeah, sorry, I was off in my version numbers here. For 30 we might have some time, but not for 29. We're uplifting everything in metrofx to 28, which threw me off.
Whiteboard: [Australis:P3] → [Australis:P3][investigate-fix-on-aurora]
Duplicate of this bug: 966779
This also affects lightweight theme backgrounds (ie, they cover the buttons, too), per bug 966779. And dragging tabs while there's regular aero fog, and opening/closing menus - basically (it seems) anything that invalidates that bit of the screen sometimes results in this bug manifesting. I'm raising the priority of this issue.

I'm suspicious of the perf optimizations we made for bug 898126. They're the only Australis-related change I think we made in core windows widget code that I can think of. Is there any chance that's related? (an easy way for someone to test would be to test UX nightlies before/after those patches landed)

Of course, it's also possible there was a non-Australis change that happens to affect this, but we should be able to see that on holly... I've not checked if this happens there.
Summary: Aero glass fog covers minimize/maximize buttons during tab drag on Windows 7 → Aero glass fog and lightweight themes cover minimize/maximize buttons during tab drag on Windows 7
Whiteboard: [Australis:P3][investigate-fix-on-aurora] → [Australis:P2][investigate-fix-on-aurora]
Version: 28 Branch → Trunk
Bisecting this. I'm up to builds from october which were still good, so my suspicions in comment #17 seem to be unfounded. Quite curious what *is* causing this, though...
Last good revision: 59963be887e7 (2013-11-08)
First bad revision: bc9083bc3023 (2013-11-09)

http://hg.mozilla.org/projects/ux/pushloghtml?fromchange=59963be887e7&tochange=bc9083bc3023

Doing some local builds to narrow this down further. Note that this range only has a single merge cset, so it's unlikely to be a frontend styling issue of sorts, although as far as the fog is concerned there's the obvious workaround of making the fog stay further from the end of the tabstrip than it is from the beginning.
Merging from 545887140a1b works.

Merging from a751d2c77ec4 works, too.

Now building a merge with http://hg.mozilla.org/projects/ux/rev/a6e7f0663458, which I'd expect to fail because it contains all of the merge except for b2g and fx-team changes.

Between a751d2c77ec4 and a6e7f0663458 I'd suspect bug 880031 based on summaries etc... will keep looking at this.
(In reply to :Gijs Kruitbosch from comment #20)
> Merging from 545887140a1b works.
> 
> Merging from a751d2c77ec4 works, too.
> 
> Now building a merge with
> http://hg.mozilla.org/projects/ux/rev/a6e7f0663458, which I'd expect to fail
> because it contains all of the merge except for b2g and fx-team changes.
> 
> Between a751d2c77ec4 and a6e7f0663458 I'd suspect bug 880031 based on
> summaries etc... will keep looking at this.

Surprisingly, I was right in one, for a change. The last merge failed, backing out bug 880031 fixed it, qpopping the backout broke it again.

Matt, this is a pretty serious issue. Do you think you have time to look at how this could be fixed? The link in comment #7 might help elucidate what's going on here.
Blocks: 880031
Flags: needinfo?(matt.woodrow)
Considering the lightweight theme as well as the aero fog being affected by this, I think we should track this for 29.
I'm pretty sure this is a bug in the native drawing code.

The code for drawing the buttons doesn't actually draw anything, it just clears the pixels out so the DWM can fill them in later.

This assumes that there is only one layer covering that screen area.

What previously happened was we were drawing the fog into that region, then we draw the button item on top, which clears everything and get us back to blank.

With my patch, the fog and the button items end up in different layers (because there's an active transform layer - the dragged tab - between them). So we draw the fog into the pixels of one layer, and clear some pixels in a different one. Then we composite all the layers together onto the screen, and drawing the clear pixels on top of the fog doesn't result in fully clear (since layer compositing is OVER, not SOURCE).
Flags: needinfo?(matt.woodrow)
I'm not really sure what the best way to fix this is.

I don't think we can guarantee that there won't ever be layers behind a layer that contains this sort of native drawing.

Two ideas:

- Change the XUL code so that we no longer have overlapping content for those pixels. I'm not sure how practical that is.

- Add blend modes to layers (which is already partially implemented for css blend mode), mark the native themed display item as requiring OPERATOR_SOURCE, make sure it gets into a layer with only other OPERATOR_SOURCE content.

Neither of these seem great, adding ni?roc in case he has a better idea.
Flags: needinfo?(roc)
We could also eliminate the system drawn command buttons and draw our own. The downside to this is we'll loose hover glow effects that fall outside the window frame on Vista and Win7.
(In reply to Jim Mathies [:jimm] from comment #25)
> We could also eliminate the system drawn command buttons and draw our own.
> The downside to this is we'll loose hover glow effects that fall outside the
> window frame on Vista and Win7.

And the maintenance burden, given the number of Windows versions and configurations we support.
(In reply to Matt Woodrow (:mattwoodrow) from comment #23)
> I'm pretty sure this is a bug in the native drawing code.
> 
> The code for drawing the buttons doesn't actually draw anything, it just
> clears the pixels out so the DWM can fill them in later.
> 
> This assumes that there is only one layer covering that screen area.

Dumb question: can't it just clear out all the pixels from all the layers?
See Also: → 966936
(In reply to Matt Woodrow (:mattwoodrow) from comment #24)
> I'm not really sure what the best way to fix this is.
> 
> I don't think we can guarantee that there won't ever be layers behind a
> layer that contains this sort of native drawing.
> 
> Two ideas:
> 
> - Change the XUL code so that we no longer have overlapping content for
> those pixels. I'm not sure how practical that is.
> 
> - Add blend modes to layers (which is already partially implemented for css
> blend mode), mark the native themed display item as requiring
> OPERATOR_SOURCE, make sure it gets into a layer with only other
> OPERATOR_SOURCE content.
> 
> Neither of these seem great, adding ni?roc in case he has a better idea.

How about just tell the compositor enough of the window style that it can punch out the right area of the finished frame without us doing anything to the layer tree? I assume we have no need to ever draw anything over the top of those buttons.
Flags: needinfo?(roc)
This is polish. Once you have a potential uplift, we will consider it.
I don't think this is just polish, this is a rather serious visual glitch.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #28)

> How about just tell the compositor enough of the window style that it can
> punch out the right area of the finished frame without us doing anything to
> the layer tree? I assume we have no need to ever draw anything over the top
> of those buttons.

We actually have blend modes on layers already, and we will need to implement them for all compositor backends regardless.

I think then it's probably easier to create a ColorLayer with OP_SOURCE blend mode for this display item, and finish the blending code in the compositor, rather than creating something new.
I think we need to fix this before adding blend modes to the compositor, which seems to me like a nontrivial project. But if you think you can do blend-modes-in-compositor in a few days, I definitely won't stand in your way!
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #30)
> I don't think this is just polish, this is a rather serious visual glitch.

OK
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #32)
> I think we need to fix this before adding blend modes to the compositor,
> which seems to me like a nontrivial project. But if you think you can do
> blend-modes-in-compositor in a few days, I definitely won't stand in your
> way!

I do think that :)

But I've realized it's not actually useful here. We need OPERATOR_CLEAR, and the region isn't a simple rectangle. It's not really obvious how to build layers that would do what we want.
Duplicate of this bug: 973328
I'd also like to add that double clicking on the top bar of the window to change between small and full maximized state also results in this issue.
Matt, are you still working on this?
Flags: needinfo?(matt.woodrow)
Slowly! I don't have my windows machine with me this week, should have a patch up next week.
Flags: needinfo?(matt.woodrow)
Assignee: nobody → matt.woodrow
Status: NEW → ASSIGNED
Attached patch region-to-clear (obsolete) — Splinter Review
What do you think roc? It's pretty hideous, but it works.
Attachment #8374412 - Attachment is obsolete: true
Attachment #8381793 - Flags: feedback?(roc)
Comment on attachment 8381793 [details] [diff] [review]
region-to-clear

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

::: gfx/layers/basic/BasicLayerManager.cpp
@@ +625,5 @@
>      }
>  
>      PaintLayer(mTarget, mRoot, aCallback, aCallbackData, nullptr);
> +    if (!mRegionToClear.IsEmpty()) 
> +      gfxContext::GraphicsOperator currentOp = mTarget->CurrentOperator();

I don't believe this compiles.

Also, use AutoSetOperator.

::: layout/base/nsCSSRendering.cpp
@@ +1136,2 @@
>        aPresContext->GetTheme()->DrawWidgetBackground(wrapperCtx, aForFrame,
> +          styleDisplay->mAppearance, aFrameArea, nativeRect, clear);

Let the 'clear' parameter be a pointer which can be null to indicate the caller doesn't want the result and can't handle a non-empty region.
Attachment #8381793 - Flags: feedback?(roc) → feedback+
(In reply to Matt Woodrow (:mattwoodrow) from comment #34)
> But I've realized it's not actually useful here. We need OPERATOR_CLEAR, and
> the region isn't a simple rectangle.

Couldn't we use operator "destination out"? nsNativeThemeWin::DrawWidgetBackground could fill the button shape with opaque black and leave the parts around it transparent. Then, during composition, the opaque parts would clear the background, and the transparent parts would leave the background as-is.
(In reply to Markus Stange [:mstange] from comment #42)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #34)
> > But I've realized it's not actually useful here. We need OPERATOR_CLEAR, and
> > the region isn't a simple rectangle.
> 
> Couldn't we use operator "destination out"?
> nsNativeThemeWin::DrawWidgetBackground could fill the button shape with
> opaque black and leave the parts around it transparent. Then, during
> composition, the opaque parts would clear the background, and the
> transparent parts would leave the background as-is.

That's a good idea :)

dest-out isn't part of the mix-blend-mode spec, so I'll just finish up what I've got I think.
Attachment #8381793 - Attachment is obsolete: true
Attachment #8385066 - Flags: review?(roc)
Attachment #8385066 - Flags: review?(bas)
Comment on attachment 8385066 [details] [diff] [review]
Clear window region

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

::: gfx/layers/d3d10/LayerManagerD3D10.fx
@@ +451,5 @@
> +technique10 RenderClearLayer
> +{
> +  pass P0
> +  {
> +	SetRasterizerState( LayerRast );

Fix indent
Attachment #8385066 - Flags: review?(roc) → review+
Component: Theme → Graphics: Layers
Product: Firefox → Core
(In reply to Matt Woodrow (:mattwoodrow) from comment #43)
> (In reply to Markus Stange [:mstange] from comment #42)
> > (In reply to Matt Woodrow (:mattwoodrow) from comment #34)
> > > But I've realized it's not actually useful here. We need OPERATOR_CLEAR, and
> > > the region isn't a simple rectangle.
> > 
> > Couldn't we use operator "destination out"?
> > nsNativeThemeWin::DrawWidgetBackground could fill the button shape with
> > opaque black and leave the parts around it transparent. Then, during
> > composition, the opaque parts would clear the background, and the
> > transparent parts would leave the background as-is.
> 
> That's a good idea :)
> 
> dest-out isn't part of the mix-blend-mode spec, so I'll just finish up what
> I've got I think.

DEST_OUT is very expensive on Windows with D2D.
Comment on attachment 8381793 [details] [diff] [review]
region-to-clear

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

Hideous doesn't begin to describe it... :)
Attachment #8381793 - Flags: review+
Did you mean to review attachment 8385066 [details] [diff] [review]?
Flags: needinfo?(bas)
Comment on attachment 8385066 [details] [diff] [review]
Clear window region

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

I .... did.
Attachment #8385066 - Flags: review?(bas) → review+
Thanks Bas!
Flags: needinfo?(bas)
https://hg.mozilla.org/mozilla-central/rev/36e45a68452b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Depends on: 980452
Depends on: 980808
I hate to speak for Matt Woodrow, but I doubt that the regressions that this is causing are likely to be fixed over the weekend.

I also think I'd rather have our users sometimes have fog over the titlebars when dragging tabs, then a super glitchy titlebar all the time (especially for fullscreen video on Windows 8).

So I'm going to back this one out.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/40b2ac413772
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla30 → ---
Testing the inbound-version with timestamp 1394559727, which has the patch of comment 54, after going in and out of Customization mode, the min/restore/max-buttons are fogged permanently, until the user either drags any tab, maximizes/restores the window or switches to another program.

When the window is maximized during coming out of Customization mode, only the [x] is covered by the fog.
https://hg.mozilla.org/mozilla-central/rev/f4911ad70a26
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
The re-landing of this has returned the problem of foggy windows buttons as reported in 
bug https://bugzilla.mozilla.org/show_bug.cgi?id=980642

I don't even have to enter/exit 'customize' to see the fog over the Windows buttons:
min/max/exit in the upper right corner.

Now what ?  Yet another bug ?  re-open this one ?  re-open 980642
(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #57)
> The re-landing of this has returned the problem of foggy windows buttons as
> reported in 
> bug https://bugzilla.mozilla.org/show_bug.cgi?id=980642
> 
> I don't even have to enter/exit 'customize' to see the fog over the Windows
> buttons:
> min/max/exit in the upper right corner.
> 
> Now what ?  Yet another bug ?  re-open this one ?  re-open 980642

Reopen bug 980642, which I've just done.
Depends on: 982446
I see no maximize, restore and close buttons when I use latest Nightly (changeset:44ae8462d6ab) with Persona theme, so should I repopen bug #966779 ?
Hey Virtual_ManPL - do you have OMTC enabled?
Flags: needinfo?(BernesB)
Depends on: 982700
Yep and this issue will be probably fixed in bug #980642 as I read of MozillaZine forum.
Flags: needinfo?(BernesB)
Matt, do you think this is upliftable for 29 considering the regressions and the scope of the patch? If not, we should probably do a frontend-only workaround at least for 29... which obviously we would like to know about as soon as possible. Thanks!
Flags: needinfo?(matt.woodrow)
I doubt we're going to be able to uplift this since we haven't solved all the regressions yet.
Flags: needinfo?(matt.woodrow)
Depends on: 983638
Depends on: 983046
Whiteboard: [Australis:P2][investigate-fix-on-aurora] → [Australis:P2][see bug 983638 for 29]
I tried to reproduce this issue on a Firefox 29.0a2 build (20140315004007):
It took tens of tries to reproduce once bug 940455 comment 0 and bug 940455 comment 2. I reproduced bug 940455 comment 5 easily though, even with less steps (most times I just entered Customization mode with the browser minimized).

I tested Firefox 30.0 (20140411004002) for verification:
The first two cases didn't reproduce at all. Given how difficult they were reproduced on the buggy build, I can't say this means they were fixed tough.

The third case still reproduces quite easily. Sometimes it's enough to open Customization mode with the browser minimized, other times you have to resize the browser to see the issue.
QA Whiteboard: [good first verify]
Depends on: 1013784
I had this bug on 33.1.1 and found this report... it is now updated to 34.0 and I still have this bug... I can move a tab to the first position and have access to the min restore exit buttons but all the other tabs do not work... also when a dialog comes up I do not have access to any controls or buttons on that either. the controls on the tabs work. but sometimes other controls do not work... what is a good version to work with because this isn't working
(In reply to Strider Collins from comment #66)
> I had this bug on 33.1.1 and found this report... it is now updated to 34.0
> and I still have this bug... I can move a tab to the first position and have
> access to the min restore exit buttons but all the other tabs do not work...
> also when a dialog comes up I do not have access to any controls or buttons
> on that either. the controls on the tabs work. but sometimes other controls
> do not work... what is a good version to work with because this isn't working

You can try turning hardware acceleration off if it isn't already. In any case, you want bug 1059804 which is investigating the current issue, where you may be able to help by using mozregression ( http://mozilla.github.io/mozregression/ ) to figure out when it broke and post the result there.
You need to log in before you can comment on or make changes to this bug.