Closed Bug 839516 Opened 11 years ago Closed 11 years ago

Reloading the selected tab doesn't remove transient notification bars

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 22

People

(Reporter: dao, Assigned: dao)

References

Details

Attachments

(1 file, 3 obsolete files)

+++ This bug was initially created as a clone of Bug #825804 +++
Priority: P2 → --
Attached patch patch (obsolete) — Splinter Review
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();
-      }
     }
Blocks: 788584
No longer blocks: 788584
Summary: Notification bars persist across page reloads → Notification bars persist across page reloads and background-tab location changes
Attached patch patch (obsolete) — Splinter Review
updated to tip
Assignee: nobody → dao
Attachment #711863 - Attachment is obsolete: true
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
Attachment #717498 - Flags: review?(gavin.sharp)
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.
Attachment #717498 - Flags: review?(gavin.sharp)
Depends on: 846754
Background-tab location changes fixed in bug 846754.
Summary: Notification bars persist across page reloads and background-tab location changes → Reloading the selected tab doesn't remove transient notification bars
Attached patch patch (obsolete) — Splinter Review
updated to tip
Attachment #717498 - Attachment is obsolete: true
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.
Flags: needinfo?(gavin.sharp)
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.
Flags: needinfo?(gavin.sharp)
Attached patch patch v2Splinter Review
Attachment #721751 - Attachment is obsolete: true
Attachment #727776 - Flags: review?(gavin.sharp)
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.
Attachment #727776 - Flags: review?(gavin.sharp) → review+
Actually I didn't add tests for notification bars, only for popup notifications...
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/256f7ba1ceb8
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Flags: in-testsuite+
Blocks: 915951
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: