1276120 - Autocomplete suggestions just don't go away after Win+Down

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P2)

Description

[arni2033]

Attachment: screenshot 1 - Autocomplete suggestions just don't go away.png

>>>   My Info:   Win7_64, Nightly 49, 32bit, ID 20160426044609
STR:
1. Open new window, switch it to normal (not maximized) mode
2. Focus urlbar, type "a"
3. Press Win+Down
4. Click button on Windows Taskbar to open the window again
5.(bonus) Focus searchbar, type "b". Move the window a bit (drag-n-drop)

AR:  After step 4 autocomplete suggestions are open. They don't hide/move in Step 5.
ER:  Either no autocomplete suggestions after step 4, or something more smart.

This is regression from bug 1264983. Regression range:
> https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=9774863180f92d75e7851f138bed71fe9828122a&tochange=45f98cbddc015c5659abe141de0b8fa6bd534051

Comment 1

[Marco Bonardo [:mak]]

annoyance, but not trivial to hit.
If it's related to a platform bug with toplevel popups it could also be expensive to fix.

Comment 2

[arni2033]

(In reply to Marco Bonardo [::mak] from comment #1)
> annoyance, but not trivial to hit.
Simply press Win+Down. How is that not trivial?

> If it's related to a platform bug with toplevel popups it could also be expensive to fix.
Back out bug 1264983 then?
An "improvement" isn't a good excuse to have several way worse bugs, unless that's your goal.

Comment 3

[Marco Bonardo [:mak]]

(In reply to arni2033 from comment #2)
> (In reply to Marco Bonardo [::mak] from comment #1)
> > annoyance, but not trivial to hit.
> Simply press Win+Down. How is that not trivial?

Ah wait, step 5 confused my tries to reproduce the bug. sorry for the confusion, yes, this is worth fixing. I don't think we should backout bug 1264983, since that would introduce a bug in OS X to workaround one on Windows. We should rather try to fix this if possible and keep the backout as a last opportunity.

The first 4 steps make so that the popup is "stuck" open forever, until you type again in the location bar (and then we reposition it).

Maybe Neil knows if this is a known issue with level="parent" or it's something we are not handling

Comment 4

[arni2033]

(In reply to Marco Bonardo [::mak] from comment #3)
Luckily you read my replies...

> I don't think we should backout bug 1264983, since that would introduce a bug in OS X
> to workaround one on Windows.
> If it's related to a platform bug with toplevel popups
If this is Windows-specific (as said in bug 1268735 comment 1), you could (probably) just back out it 
from Windows and have enough time to get rid of _this_ bug. That's all my suggestions as of now.

Comment 5

[Marco Bonardo [:mak]]

(In reply to arni2033 from comment #4)
> (In reply to Marco Bonardo [::mak] from comment #3)
> Luckily you read my replies...

It's not luck, I read hundreds bugzilla comments every day!

> If this is Windows-specific (as said in bug 1268735 comment 1), you could
> (probably) just back out it 
> from Windows and have enough time to get rid of _this_ bug. That's all my
> suggestions as of now.

that's a possibility we can consider, a windows only ifndef. But first let's see what Neil thinks of the issue.

Comment 6

[Neil Deakin]

The popup is being closed during the minimize at step 4 but it looks like Windows for some reason decides to show the popup again itself when the main window is unminimized. However, we don't think the popup is open so no events get routed to it.

I'll investigate why this might be.

Comment 7

[Neil Deakin]

Attachment: Hide popups on minimize

This patch seems to work, but is a bit aggressive in that it closes all popups whenever any window is minimized.

Comment 8

[Neil Deakin]

Comment on attachment 8757391 [details] [diff] [review]
Hide popups on minimize

OK, so background windows don't seem to get WM_SHOWWINDOW so this is probably ok, and WM_WINDOWPOSCHANGED doesn't give enough information in this case.

Comment 9

[Jim Mathies [:jimm]]

The win+down arrow trick doesn't impact select dropdown windows. I wonder what the difference is here?

Comment 10

[Neil Deakin]

They are toplevel popups whereas the autocomplete popup was changed in bug 1264983 to not be toplevel. This means that it does not have WS_EX_TOPMOST set.

Comment 11

[Neil Deakin]

Attachment: Hide popups on minimize, v2

Forgot the return at the end.

Comment 12

[Neil Deakin]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fefb7adc384abcc33f4da47496787bfdac1c9f85
Bug 1276120, hide popups when a parent window is being minimized, handles the case where the window is minimized via a keyboard shortcut, r=jmathies

Comment 13

[Marco Bonardo [:mak]]

[Tracking Requested - why for this release]: It's a new regression for the awesomebar redesign

Comment 14

[Carsten Book [:Tomcat]]

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

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking since this is a recent regression. Marco, is this something you intend to uplift to beta?

Comment 16

[Marco Bonardo [:mak]]

ehr sorry, yes it would be great to have this in beta.
But I can't comment regarding the risk, I prefer Neil or Jim to evaluate that.

Comment 17

[Neil Deakin]

I guess it has less risk than 1264983.

Comment 18

[Sylvestre Ledru [:Sylvestre]]

OK, could you fill the uplift request then?
Thanks

Comment 19

[Marco Bonardo [:mak]]

Comment on attachment 8759749 [details] [diff] [review]
Hide popups on minimize, v2

Approval Request Comment
[Feature/regressing bug #]: new awesomebar design
[User impact if declined]: The awesomebar popup detaches and covers the browser window if it was minimized using keyboard shortcuts.
[Describe test coverage new/current, TreeHerder]: nightly/aurora
[Risks and why]: less risky than the fix that caused this regression (bug 1264983), should be an easy backout in case issues are found (just a few lines of code)
[String/UUID change made/needed]: none

Comment 20

[Gerry Chang [:gchang]]

Comment on attachment 8759749 [details] [diff] [review]
Hide popups on minimize, v2

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

The patch fixes a regression. Take it in 48 beta 5.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/432e034161c2

Comment 22

[Rezaul Huque Nayeem]

Reproduced the bug in firefox nightly 49.0a1 (2016-05-26) with windows 10 (64 bit)

Verified as fixed with latest firefox beta 48.0b7 (Build ID: 20160711002726)
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:48.0) Gecko/20100101 Firefox/48.0

and latest firefox aurora 49.0a2 (Build ID: 20160711004013)
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:49.0) Gecko/20100101 Firefox/49.0

Comment 23

[Moin Shaikh]

Verified the bug as fixed on Firefox Aurora 49.0a2.

[testday-20160722]

Comment 24

[Carmen]

I reproduced the issue using Firefox nightly 49.0a1 (2016-05-26) on Windows 10 Pro x86_64.
I have verified the fix on the latest Beta 49.0b7.

Build ID: 20160825132718
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0

[testday-20160826]