Closed Bug 802456 Opened 12 years ago Closed 12 years ago

ctrl+D bookmark menu folder select stays on screen

Categories

(Core :: Widget: Win32, defect)

18 Branch
x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19
Tracking Status
firefox17 + verified
firefox18 + verified
firefox19 + fixed

People

(Reporter: uranium23567, Assigned: tnikkel)

References

Details

(Keywords: regression)

Attachments

(7 files, 2 obsolete files)

Attached image proof (deleted) —
This is Nightly 19a 2010-10-16 Related to hardware acceleration. reproducible (did on two platforms). One laptop with i965 graphics and desktop with GTX 285. Both win7 64 bit and 64 bit nightly 1: Press Ctrl+D 2: click folder to bring the folder menu out 3: click any place within the "edit bookmark" that should bring the folder menu back up. it stays rendered. Sometimes after the entire edit bookmark is gone from a outside of edit bookmark click with hardware acceleration disabled, it works fine
Attached image also rendered on the page (deleted) —
Regression window(m-c) Good: http://hg.mozilla.org/mozilla-central/rev/f972f1a71e7e Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0 ID:20120830145943 Bad: http://hg.mozilla.org/mozilla-central/rev/6873940886b2 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0 ID:20120830185450 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f972f1a71e7e&tochange=6873940886b2 Regression window(m-i) Good: http://hg.mozilla.org/integration/mozilla-inbound/rev/5034469b1595 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0 ID:20120830091924 Bad: http://hg.mozilla.org/integration/mozilla-inbound/rev/e0ceffe737dd Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0 ID:20120830094924 Pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=5034469b1595&tochange=e0ceffe737dd Regressed by: e0ceffe737dd Timothy Nikkel — Bug 786421. When hiding a popup panel don't bother sizing its view to zero because that is useless and confuses the widget sizing code. r=roc Panels have a min size constraint set on them so when the view code tried to resize the widget to zero it would get bumped up to the min size. The view code would continue trying to resize the widget because of this.
Severity: minor → normal
Status: UNCONFIRMED → NEW
Component: Bookmarks & History → Widget: Win32
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
Version: 19 Branch → 18 Branch
Blocks: 786421
Tim, can you please take a look at this bug as it is a suspected regression from 786421. Thanks !
Assignee: nobody → tnikkel
Bug 786421 caused this and it's on 17, so we might want to track there too.
Tim if you're able to put together a forward fix in the next week or so we could take it on 17, otherwise can you make a call here on whether this regression justifies a backout of bug 786421 or if it's something that shouldn't affect too many users and we can keep tracking for a fix in 18 to give you more time.
I've tried to reproduce on my Windows 7 machine but I haven't been able to. Maybe I'm not doing the steps right? Could someone right out even more detailed steps maybe? Maybe it's because I have d3d9 layers and no d2d. Alice and Paul what kind of acceleration do you have? (about:support has the info).
Steps to Reproduce: 1. Start Firefox with New profile 2. Ctrl+D or Double-click Star UI 3. Click "x" to dismiss "You can access your bookmarks on all your devices with Sync.Lerm more" information pane(if exist), 4. Click "Recent used folder drop down arrow" at Folder row 5. Click page empty area Actual results: You can see garbage of "Recent used folder drop down" Expected results: "Recent used folder drop down" should be hidden. Graphics Adapter Description ATI Radeon HD 4300/4500 Series Adapter Drivers: aticfx64 aticfx64 aticfx32 aticfx32 atiumd64 atidxx64 atiumdag atidxx32 atiumdva atiumd6a atitmm64 Adapter RAM: 512 ClearType Parameters: Gamma: 2200 Pixel Structure: RGB ClearType Level: 50 Enhanced Contrast: 50 Device ID: 0x954f Direct2D Enabled: true DirectWrite Enable: dtrue (6.1.7601.17789) Driver Date: 7-3-2012 Driver Version: 8.970.100.3000 GPU #2 Active: false GPU Accelerated Windows: 1/1 Direct3D 10 Vendor ID: 0x1002 WebGL Renderer: no information AzureCanvasBackend: direct2d AzureContentBackend: direct2d AzureFallbackCanvasBackend: cairo
Thanks. What happens if you disable d2d? Or use d3d9 layers? And the "garbage", what happens if you scroll the page? How can you make it go away?
(In reply to Timothy Nikkel (:tn) from comment #8) > Thanks. > > What happens if you disable d2d? Or use d3d9 layers? > Setting layers.prefer-d3d9 = true and restart: the problem also happens. Disabled HWA from Oprions and restart: the problem is gone. > And the "garbage", what happens if you scroll the page? "garbage"stays on the "Edit Bookmarks Panel". > How can you make it go away? Following step 5 and 6. Click page empty area --- "Edit Bookmarks panel" will be hidden as expected 7. Scroll page --- the "garbage" will be dismissed
You may need to change Window Visual Style to "Aero Basic" or "Classic" to reproduce the problem
(In reply to Alice0775 White from comment #10) > You may need to change Window Visual Style to "Aero Basic" or "Classic" to > reproduce the problem Thank you for this. I can reproduce in Basic and Classic.
tnikkel@gmail.com-ed2919fc72d9 --- Nothing seems to have changed tnikkel@gmail.com-a5b3e1cef487 --- Nothing seems to have changed tnikkel@gmail.com-02562f282827 --- Nothing seems to have changed tnikkel@gmail.com-05f950fc8677 --- Nothing seems to have changed
you are correct, both of the systems are on windows basic. it does not do it with areo enabled. good catch
Does it happen on Windows XP at all?
let me boot to my xp install on my laptop nope. Only 7. If you ask about vista next, I can't answer that, I'm sane enough to keep that away from everything like a plague
The fact that this only occurs with a non-compositing window manager and hardware accelerated layers makes me thing this could be a Windows bug. We have a popup window on top of a popup window. Let's call the top most popup A, the popup below it B, and the main window C. When A is hidden the parts of A that are only over C correctly disappear, but the parts of A over B do not. The weird part is that when B is hidden the left over parts of A still visible remain on top of just C, and they go away if we invalidate the area of C normally.
Hmm, I would have hoped the fix for bug 610713 might have addressed this. One interesting thing I noticed is that if you set focus to the tags edit and try typing, the text edit doesn't update at all. Seems like the top level Bookmarks panel view isn't refreshing after the drop down goes away. I wonder if we are invalidating the window there? If we do and Windows doesn't generate a corresponding paint, then it may be a Windows bug. If we aren't then it's our fault.
Looks like we are invalidating - 63 Invalidate widget=08004440 name=noname id=00B706D0 rect=118,147 1,17 64 Invalidate widget=08004440 name=noname id=00B706D0 rect=118,147 1,17 65 Invalidate widget=08004440 name=noname id=00B706D0 rect=118,147 1,17 From the blinking cursor in the tags edit. One good note - this isn't reproducible on Win8.
One more data point: not all computers show the problem when using a non-compositing window manager and accelerated layers. Dell laptop with ATI graphics has the problem. Old HP netbook with nvidia ion or intel graphics has the problem. Macbook with ATI graphics does not have the problem.
(In reply to Jim Mathies [:jimm] from comment #18) > Hmm, I would have hoped the fix for bug 610713 might have addressed this. I realized that bug 610713 only affects menus. So I made a try build that applies that style to all popup windows. It will be ready in a bit at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tnikkel@gmail.com-9fddc0ff7f1e/
(In reply to Timothy Nikkel (:tn) from comment #22) > (In reply to Jim Mathies [:jimm] from comment #18) > > Hmm, I would have hoped the fix for bug 610713 might have addressed this. > > I realized that bug 610713 only affects menus. So I made a try build that > applies that style to all popup windows. It will be ready in a bit at > > http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tnikkel@gmail.com- > 9fddc0ff7f1e/ This tryserver build fixed this problem!
Perfect! Thanks for all your help Alice! I'll put together a patch that we can land.
And the tryserver build also seems to fix Bug 785708.
yay I helped and now have bragging rights to other techs thanks for the fix, keep up the good work
Attached patch patch (deleted) — Splinter Review
We need to remove the condition on both the popup type and drop shadow being there.
Attachment #673522 - Flags: review?(jmathies)
Comment on attachment 673522 [details] [diff] [review] patch Want to land this ASAP to get as much testing as possible done for a potential uplift to beta.
Attachment #673522 - Flags: review?(roc)
Comment on attachment 673522 [details] [diff] [review] patch I'm wondering if this might cause perf issues, I suppose there's one way to find out - land it on inbound and see what happens in talos tests like tresize.
Attachment #673522 - Flags: review?(jmathies) → review+
(In reply to Jim Mathies [:jimm] from comment #29) > I'm wondering if this might cause perf issues, I suppose there's one way to > find out - land it on inbound and see what happens in talos tests like > tresize. It still only affects popup windows (so menus, tooltips, panels). tresize just resizes the main window right? So I don't think it'll affect that. It could be a perf issue with panels though.
Attachment #673522 - Flags: review?(roc)
We could also make this conditional on a non-compositing window manager being used, or maybe on accelerated layers being used too.
Blocks: 785708
Blocks: 803518
Just noting that the patch in 610713 only applied to menus and not all popup types because I was getting invisible tooltips. Bug 802316 seems to have fixed that though, so keep that in mind if you consider uplifting this to beta.
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
(In reply to James from comment #33) > Just noting that the patch in 610713 only applied to menus and not all popup > types because I was getting invisible tooltips. Bug 802316 seems to have > fixed that though, so keep that in mind if you consider uplifting this to > beta. Yeah, I did see that bug and that was my plan, to uplift it as well. Thanks for the reminder.
(In reply to Jim Mathies [:jimm] from comment #29) > I'm wondering if this might cause perf issues, I suppose there's one way to > find out - land it on inbound and see what happens in talos tests like > tresize. To determine the perf impact we could put an animation in a panel and measure fps or cpu in before after/builds.
Attached patch rollup aurora patch (deleted) — Splinter Review
This is just this bug and bug 802316 combined.
Attached patch beta rollup patch (deleted) — Splinter Review
This is bug 610713, bug 802316, bug 801301, and this bug rolled up into one patch.
Comment on attachment 674398 [details] [diff] [review] rollup aurora patch This is a rollup of this bug and bug 802316. [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 786421 User impact if declined: people running Aero Basic will see buggy rendering in some secondary interactions with tooltips, panels, and menus Testing completed (on m-c, etc.): been on nightly for a few days Risk to taking this patch (and alternatives if risky): low risk String or UUID changes made by this patch: none
Attachment #674398 - Flags: approval-mozilla-aurora?
Comment on attachment 674398 [details] [diff] [review] rollup aurora patch Approving for aurora at this time, please nominate for beta once it has had enough bake time as this is tracking 17
Attachment #674398 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I landed the patch as two separate changesets to preserve author, review, and bug information. https://hg.mozilla.org/releases/mozilla-aurora/rev/4af749bc6e82 https://hg.mozilla.org/releases/mozilla-aurora/rev/b100957a878e
Had to back out from aurora, as it looks like either this or bug 802316 caused an intermittent orange. https://hg.mozilla.org/releases/mozilla-aurora/rev/fd8361463029
(In reply to Timothy Nikkel (:tn) from comment #42) > Had to back out from aurora, as it looks like either this or bug 802316 > caused an intermittent orange. > https://hg.mozilla.org/releases/mozilla-aurora/rev/fd8361463029 Please target getting a fix for this all the way up to beta before Tuesday (b4 build), and email if help is needed investigating the intermittent orange. Thanks!
(In reply to Alex Keybl [:akeybl] from comment #43) > Please target getting a fix for this all the way up to beta before Tuesday > (b4 build), and email if help is needed investigating the intermittent > orange. Thanks! Ok. Given that this only affects Aero Basic we may want to re-evaluate the risk-benefit ratio here.
(In reply to Timothy Nikkel (:tn) from comment #44) > (In reply to Alex Keybl [:akeybl] from comment #43) > > Please target getting a fix for this all the way up to beta before Tuesday > > (b4 build), and email if help is needed investigating the intermittent > > orange. Thanks! > > Ok. > > Given that this only affects Aero Basic we may want to re-evaluate the > risk-benefit ratio here. Is the fix high risk, or the intermittent orange mysterious? Basically, why compromise here?
(In reply to Alex Keybl [:akeybl] from comment #45) > Is the fix high risk, or the intermittent orange mysterious? Basically, why > compromise here? The orange is rather mysterious to me at this point.
http://hg.mozilla.org/mozilla-central/rev/b486c3782846 is what "fixed" the intermittent failure, which is just a large change to the failing test. I'm a little stumped as the change in this bug should not have an effect on this test.
Attached patch alternate aurora/beta patch (obsolete) (deleted) — Splinter Review
This is an alternate patch. It backs out the original regressing patch from bug 786421 and instead adjusts the size constraints of the widget to allow it to be zero sized when we are hiding it. This is an alternative fix for bug 786421, and it should be safer. Although it has had much less testing.
Attachment #676251 - Flags: review?(enndeakin)
Comment on attachment 676251 [details] [diff] [review] alternate aurora/beta patch If the popup is opened again, won't it have the wrong constraints set? The size is only changed when the popup is opened if the layout size has changed at http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsMenuPopupFrame.cpp#452
Attached patch alternate aurora/beta patch (obsolete) (deleted) — Splinter Review
Good point. How about if we apply the size constraints if the size changed or we are going from hidden to shown popup?
Attachment #676251 - Attachment is obsolete: true
Attachment #676251 - Flags: review?(enndeakin)
Attachment #676260 - Flags: review?(enndeakin)
Comment on attachment 676260 [details] [diff] [review] alternate aurora/beta patch Did you mean to use mIsOpenChanged instead of isOpen?
Attached patch alternate aurora/beta patch (deleted) — Splinter Review
Good point. We definitely only want to set the size constraints if isOpen is true. Otherwise we'll end up with a zero sized view whose widget has a non-zero min size constraint. But we also don't need to do it every time LayoutPopup is called while the popup is open. It looks like the meaning of mIsOpenChanged is "mOpen changed, and it changed from false to true", so we could just check mIsOpenChanged. But I think it makes the logic clearer with the "if (isOpen)" just below it to match.
Attachment #676260 - Attachment is obsolete: true
Attachment #676260 - Flags: review?(enndeakin)
Attachment #676269 - Flags: review?(enndeakin)
Comment on attachment 676269 [details] [diff] [review] alternate aurora/beta patch Since the widget's size constraints shouldn't affect the layout, I'm pretty sure that this change is ok. > It looks like the meaning of mIsOpenChanged is "mOpen changed, and it > changed from false to true", so we could just check mIsOpenChanged. But I > think it makes the logic clearer with the "if (isOpen)" just below it to match. That's correct, Checking both is redundant as mIsOpenChanged is only set to true when the popup becomes open, but you can leave it if you think it is clearer to you.
Attachment #676269 - Flags: review?(enndeakin) → review+
Comment on attachment 676269 [details] [diff] [review] alternate aurora/beta patch This backs out the original regressing patch from bug 786421 and instead adjusts the size constraints of the widget to allow it to be zero sized when we are hiding it. This is an alternative fix for bug 786421, and it should be safer. Although it has had much less testing. [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 786421 User impact if declined: rendering glitches in some secondary UI interactions on Aero Basic only Testing completed (on m-c, etc.): verified that is keeps bug 786421 fixed Risk to taking this patch (and alternatives if risky): I think this is the safest option if we want to fix this bug on beta and aurora. String or UUID changes made by this patch: none
Attachment #676269 - Flags: approval-mozilla-aurora?
Comment on attachment 676269 [details] [diff] [review] alternate aurora/beta patch If we want to fix this in beta then I think we should take this patch for the upcoming beta build tomorrow to maximize testing. [Approval Request Comment] Same info as aurora approval request in comment 54.
Attachment #676269 - Flags: approval-mozilla-beta?
To be clear my plan is to land this alternate patch on beta and aurora and not nightly/mozilla-central.
Comment on attachment 676269 [details] [diff] [review] alternate aurora/beta patch This does sound like our safest option for beta/aurora. Please land asap so we can have this in beta 4 (going to build tomorrow) and then if we find regressions in the next week there is time to back out this combo backout/fix before we release 17.
Attachment #676269 - Flags: approval-mozilla-beta?
Attachment #676269 - Flags: approval-mozilla-beta+
Attachment #676269 - Flags: approval-mozilla-aurora?
Attachment #676269 - Flags: approval-mozilla-aurora+
(In reply to Timothy Nikkel (:tn) from comment #58) > https://hg.mozilla.org/releases/mozilla-aurora/rev/c3b3811b06f5 > https://hg.mozilla.org/releases/mozilla-beta/rev/df94e6652a9d This patch does not seem to fix the problem in 17beta and aurora18.0a1 . I checked with http://hg.mozilla.org/releases/mozilla-beta/rev/b90097a37788 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20121029153838 and http://hg.mozilla.org/releases/mozilla-aurora/rev/9712dc86183e Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0 ID:20121029161938
(In reply to Alice0775 White from comment #59) > This patch does not seem to fix the problem in 17beta and aurora18.0a1 . Ok, thanks for testing. I will investigate.
The alternate patch does correctly fix this bug when applied to mozilla-central, I verified using this try build http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tnikkel@gmail.com-f9e3762d0891/try-win32/ Alice if you want to double check that the bug is fixed for you in that build that would be nice. Best guess now is that something that landed since aurora branched is also needed to fix this bug with the alternate patch.
(In reply to Timothy Nikkel (:tn) from comment #61) > The alternate patch does correctly fix this bug when applied to > mozilla-central, I verified using this try build > http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tnikkel@gmail.com- > f9e3762d0891/try-win32/ Alice if you want to double check that the bug is > fixed for you in that build that would be nice. > > Best guess now is that something that landed since aurora branched is also > needed to fix this bug with the alternate patch. The tryserver build works well.
Due to timing, we'll have to target a fix for this regression to Aurora (18). At least on there is a workaround where the user can click on "done" to close the menu folder.
(In reply to Lukas Blakk [:lsblakk] from comment #64) > Due to timing, we'll have to target a fix for this regression to Aurora > (18). At least on there is a workaround where the user can click on "done" > to close the menu folder. See attachment 672123 [details], There is no "Done" button. So, No Workaround
The reason the alternate patch didn't work is that we also need to backout bug 789482, a followup to bug 786421.
Attached patch backout 789482 (deleted) — Splinter Review
[Approval Request Comment] Bug caused by (feature/regressing bug #): 789482 User impact if declined: rendering glitches in some secondary UI interactions on Aero Basic only Testing completed (on m-c, etc.): none Risk to taking this patch (and alternatives if risky): this is backing out a regression fix for 786421, which we are backing out in the alternate patch, so we don't need it String or UUID changes made by this patch: none
Attachment #677846 - Flags: approval-mozilla-aurora?
Attachment #677846 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 676269 [details] [diff] [review] alternate aurora/beta patch Tracking 17 was removed, but I'm requesting approval if the drivers want to consider fixing this bug on beta. [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 786421 User impact if declined: rendering glitches in some secondary UI interactions on Aero Basic only Testing completed (on m-c, etc.): verified that is keeps bug 786421 fixed, been on aurora for a bit now Risk to taking this patch (and alternatives if risky): I think this is the safest option if we want to fix this bug on beta. String or UUID changes made by this patch: none
Attachment #676269 - Flags: approval-mozilla-beta+ → approval-mozilla-beta?
Attachment #677846 - Flags: approval-mozilla-beta?
Comment on attachment 676269 [details] [diff] [review] alternate aurora/beta patch Given comment 57 (which shows the UI doesn't work for windows users as it does for Mac) and the fix being on aurora for a bit without issues, will take this for mozilla-beta. Please land asap to make it into beta 5.
Attachment #676269 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #677846 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Verified on Windows 7 using Firefox 17 RC that the "Recent used folder drop down" is hidden (followed the steps from Comment 7). Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 Build ID: 20121116115405
Verified as fixed on Firefox 18 beta 3 - "Recent used folder drop down" is hidden when clicking on an empty area. Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0 Build ID: 20121205060959
QA Contact: simona.marcu
I still get a problem that sounds like this issue isn't fixed (I came here from Bug 801813). Occasionally if I have the location bar suggestions up for a while before doing something to cause it to close, the black shadow line along the bottom remains on my desktop. It also happens with the search bar if I leave the suggestions up too long, but the shadow s slightly thicker and L-shaped (as if the L were tipped on its side). The former (location bar) artifact tends to stay "behind" windows, but the latter (L-shaped search bar) artifact stays above windows. I can sometimes deliberately cause the artifact by getting one of the menus up and switching to a different application without letting the popup close first. These artifacts persist through browser restarts, or even DWM/Explorer restarts. The only way I can get them to go away is logging out and back in. Problem has remained through two video driver updates and many Firefox updates, and no other applications have ever had this issue. You can see an example of the location bar artifact in this screenshot: https://www.dropbox.com/s/sqthsydgxqy9usm/firefoxartifact.png?dl=0 Graphics Features Compositing Direct3D 11 Asynchronous Pan/Zoom none WebGL Renderer Google Inc. -- ANGLE (Intel(R) HD Graphics 4000 Direct3D11 vs_5_0 ps_5_0) WebGL2 Renderer Google Inc. -- ANGLE (Intel(R) HD Graphics 4000 Direct3D11 vs_5_0 ps_5_0) Audio Backend wasapi Direct2D true DirectWrite true (10.0.14393.1198) GPU #1 Active Yes Description Intel(R) HD Graphics 4000 Vendor ID 0x8086 Device ID 0x0166 Driver Version 10.18.10.4425 Driver Date 4-4-2016 Drivers igdumdim64 igd10iumd64 igd10iumd64 igdumdim32 igd10iumd32 igd10iumd32 Subsys ID 22118086 RAM Unknown Diagnostics ClearType Parameters Gamma: 2.2 Pixel Structure: RGB AzureCanvasAccelerated 0 AzureCanvasBackend direct2d 1.1 AzureContentBackend direct2d 1.1 AzureFallbackCanvasBackend skia ClearType Parameters Gamma: 2.2 Pixel Structure: RGB Decision Log D3D9_COMPOSITING disabled by default: Disabled by default GPU_PROCESS unavailable by runtime: Multi-process mode is not enabled
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: