+++ This bug was initially created as a clone of Bug #825804 +++

causes test_keycodes.xul failures

https://hg.mozilla.org/try/rev/2ad32930942f alone causes the failures, so seemingly something somewhere depends on notification bars being unaffected by reloads or history.pushState/popState. That something is probably outside of test_keycodes.xul. I'm not sure how to track it down.

(In reply to Dão Gottwald [:dao] from comment #2) > https://hg.mozilla.org/try/rev/2ad32930942f alone causes the failures For future reference: var selectedBrowser = gBrowser.selectedBrowser; - if (selectedBrowser.lastURI) { - let oldSpec = selectedBrowser.lastURI.spec; - let oldIndexOfHash = oldSpec.indexOf("#"); - if (oldIndexOfHash != -1) - oldSpec = oldSpec.substr(0, oldIndexOfHash); - let newSpec = location; - let newIndexOfHash = newSpec.indexOf("#"); - if (newIndexOfHash != -1) - newSpec = newSpec.substr(0, newIndexOfHash); - if (newSpec != oldSpec) { - // Remove all the notifications, except for those which want to - // persist across the first location change. + if (aRequest && aWebProgress.DOMWindow == content) { let nBox = gBrowser.getNotificationBox(selectedBrowser); nBox.removeTransientNotifications(); - } }

updated to tip

Comment on attachment 717498 [details] [diff] [review] patch For unknown reasons this is now green on try... https://tbpl.mozilla.org/?tree=Try&rev=1228884188ad

Comment on attachment 717498 [details] [diff] [review] patch (In reply to Dão Gottwald [:dao] from comment #5) > Comment on attachment 717498 [details] [diff] [review] > patch > > For unknown reasons this is now green on try... > https://tbpl.mozilla.org/?tree=Try&rev=1228884188ad It looks like this only ran mochitest-browser-chrome, but the test in question is part of mochitest-other.

Background-tab location changes fixed in bug 846754.

updated to tip

masayuki, any idea why this patch would interfere with test_keycodes.xul? https://tbpl.mozilla.org/?tree=Try&rev=6e025f007868

Although, I'm not sure what's expected by the patch. But perhaps, it changes focus to unexpected element?

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #10) > Although, I'm not sure what's expected by the patch. But perhaps, it changes > focus to unexpected element? But does test_keycodes.xul show any notification bars? Maybe there are notification bars leaking from other tests?

(In reply to Dão Gottwald [:dao] from comment #11) > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #10) > > Although, I'm not sure what's expected by the patch. But perhaps, it changes > > focus to unexpected element? > > But does test_keycodes.xul show any notification bars? Maybe there are > notification bars leaking from other tests? Hmm, but if the focus is confused all over the test, all tests should be failed. The test adds key event listeners at bubbling phase to the document and synthesizes key events from widget level. And then, checks the coming key events into the listeners. If something in the content calls stopPropagation(), then, it could cause the failures. However, the contents are made by the test. So, this possibility is low. If something in chrome eats the key events, it also could cause. If so, the key combination launches something of chrome and it consumes the events. If something in chrome steals focus from the content document temporary, it also could cause. Can you reproduce the failure when you run only the test_keycodes.xul on your Mac? If so, you might be able to see the cause visually.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #12) > Can you reproduce the failure when you run only the test_keycodes.xul on > your Mac? If so, you might be able to see the cause visually. I don't have a Mac :/

Hmm, okay. If I could, I'll check it. However, I'm too busy this week. If you can find another person who can do it, please ask him/her.

Sorry for the delay. Perhaps, I could check it next week. (I hope!)

I can NOT reproduce the failure when I runs only the test. And I can NOT reproduce the failure when I runs all tests under widget/tests. However, I CAN reproduce the failure when I runs all chrome tests. Then, I see "Additional plugins are required to display all media on this page." notification bar. That's the difference between the cases. Does this help you?

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #16) > Then, I see "Additional plugins are required to display all media on this > page." notification bar. That's the difference between the cases. > > Does this help you? Thanks, it's a start. Now I need to figure out where that notification is coming from.

Apparently chrome://mochitests/content/chrome/content/base/test/chrome/file_bug391728_2.html triggers that notification bar, and then I assume it stays because we never navigate away from chrome://mochikit/content/harness.xul. This doesn't explain why the notification bar wasn't there without my patch, though.

Should I check whether the notification bar stays there without your patch?

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #19) > Should I check whether the notification bar stays there without your patch? I pushed a diagnosis patch to the try server and according to that, the notification bar isn't there without my patch.

Okay, so, the notification bar is there on Windows, but it becomes orange only on Mac?? If so, the key events are consumed by the notification only on Mac. That sounds odd... And looks like the failure occurs randomly. https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=4352f27d8f5e The failure logs aren't same... Hmm...

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #21) > Okay, so, the notification bar is there on Windows, but it becomes orange > only on Mac?? If so, the key events are consumed by the notification only on > Mac. That sounds odd... Yeah, I don't know what's going on there. But I think I see why the notification bar is there now. The current code in XULBrowserWindow.onLocationChange doesn't filter out sub-document location changes. It just nonsensically compares the sub document's location with selectedBrowser.lastURI and if they don't match, it calls removeTransientNotifications. Note that the same was true for popup notifications prior to bug 825804. Not calling removeTransientNotifications for sub-document location changes means that notifications could become stale if they refer to sub documents that go away. On the other hand, notifications can be entirely unrelated to sub documents that go away. So I'm not sure what to do here.

Looks like Control + 0x22 ('I' key) is the cause of other random failures. If so, I think that it isn't matter to comment out the test because the key combination isn't so important on Mac. If you won't understand the reason clearly, it's the last resort...

Short-term, it seems like maintaining the status quo and having sub-document loads clear the notification is acceptable, right? Assuming the most common use of subframes is for visible frames whose new loads are perceived by the user as being a "new page", that behavior is actually reasonable. No idea whether that assumption holds! Of course we should also be consistent between notification bars and popup notifications, so I guess we should switch those back too.

Comment on attachment 727776 [details] [diff] [review] patch v2 I guess it would be good to get some test coverage in browser_popupNotifications for this case (subframe load should clear notification). A more explicit test for notification bars would be good too. Rather than an early return, can you put the code for the DOMWindow==contentWindow case in an if() block? Makes it clearer that it happens only in some cases, and a bit harder for people to mess up where they add things to this function.

Actually I didn't add tests for notification bars, only for popup notifications...