970321 - UITour: Menu panel broken if tour tab is opened in new window

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Alex Gibson [:agibson]]

STR:

1.) Visit https://www.mozilla.org/en-US/firefox/29.0a2/whatsnew/
2.) Once the page has loaded, drag and drop the browser tab onto the desktop, to open the tab in a new window.
3.) In the door-hanger, click "Let's go"

Expected results:

When the tour starts, the menu panel should open.

Actual results:

The menu panel does not open

Comment 1

[Justin Dolske [:Dolske]]

Attachment: Screen Shot 2014-02-14 at 6.39.35 PM.png

The panel opens for me on OS X, but it's very short.

Comment 2

[Justin Dolske [:Dolske]]

I'd call this P3 or P4 just for the tour part, since this doesn't seem like a super-common action... But... The panel is generally broken in the resulting window. If I open the menu panel manually it's still stubby, and if I go into customization mode it's completely empty! Eep!

Comment 3

[:Gijs (he/him)]

I expect this is because in this case the tour calls into the PanelUI before the browser's delayed startup has happened, and so the lazy getters for the elements aren't setup, and everybody is sad.

Comment 4

[:Gijs (he/him)]

Attachment: UITour: menu panel broken if tour tab is opened in new window

This still needs a test. I'll try to get to writing that later today. Also, with this fix, I'm still seeing 'tab is null' errors in the browser console. I don't understand the relevant code enough to see how best to fix that. If someone else can use this before I get a chance to question you (ie Matt or Blair) on this late tonight/tomorrow, please go ahead. Not taking this bug for now for that reason.

Comment 5

[:Gijs (he/him)]

Attachment: UITour: make menu panel not break if tour tab is opened in new window,

This fixes things more appropriately. Note that my added test is itself setting gTestTab to null because otherwise the test destructor goes haywire as the tab is no longer alive.

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8377505 [details] [diff] [review]
UITour: make menu panel not break if tour tab is opened in new window,

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

Hm, this is really fixing two independent bugs. One is the PanelUI not being ready. One is what happens when a tour tab is detached from it's window.

In the case of the tab being detached, there are two additional concerns I have:
* registerPageID not being processed, and messing up our telemetry data
* Not cleaning up the window the tab was detached from - when the tab is detached we should do a full tour teardown in the old window, so it clears up highlights, pinned tabs, etc

registerPageID thing is tricky. I wonder if a better approach would be to instead of queueing the document, we queue the original event? Then when the tab is fully attached and we know the new window is setup, we can just process the event again. That could save a lot of extra code that needed added (and that type of code generally tends to be error-prone).



(In reply to :Gijs Kruitbosch from comment #5)
> Note that my added test is itself
> setting gTestTab to null because otherwise the test destructor goes haywire
> as the tab is no longer alive.

Yep, that's by design - some of the other tests do that too.

::: browser/modules/UITour.jsm
@@ +365,5 @@
> +        let pendingDoc;
> +        if (this.pendingDoc && (pendingDoc = this.pendingDoc.get())) {
> +          let browser = window.gBrowser.getBrowserForDocument(pendingDoc);
> +          if (selectedTab.linkedBrowser == browser) {
> +            let isPinned = selectedTab.pinned;

You don't actually use isPinned

Comment 7

[:Gijs (he/him)]

Attachment: UITour: make menu panel not break if tour tab is opened in new window,

How about this, then? I think I need the pendingDoc ref in order to figure out whether the tab selected even has a uitour thing in it... The only thing I've noticed wrong with this is that the highlight on the appmenu in the detached window isn't correctly centered on the button. Don't know why that is. Doesn't seem as bad a deal to me as the current state. I'd even probably suggest we could land the PanelUI fix so that the panel doesn't go completely bonkers if you accidentally detach a tab, and polish the rest of the fix separately.

Comment 8

[Matthew N. [:MattN]]

(In reply to :Gijs Kruitbosch from comment #7)
> The only thing I've noticed wrong with this is that the highlight on the appmenu in the
> detached window isn't correctly centered on the button. Don't know why that
> is. Doesn't seem as bad a deal to me as the current state.

Perhaps this is bug 968039.

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8378574 [details] [diff] [review]
UITour: make menu panel not break if tour tab is opened in new window,

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

::: browser/modules/UITour.jsm
@@ +377,5 @@
> +        let pendingDoc;
> +        if (this._detachingTab && this._pendingDoc && (pendingDoc = this._pendingDoc.get())) {
> +          let browser = window.gBrowser.getBrowserForDocument(pendingDoc);
> +          if (selectedTab.linkedBrowser == browser) {
> +            let tabType = selectedTab.pinned ? "pinnedTabs" : "originTabs";

This is confusing me...

pendingDoc is a document from which a mozUITour event was sent. In which case it won't go in the pinnedTabs Set, which is used for tracking pinned tabs that a tour page has requested be opened.

Am I misunderstanding something, or are you?

Comment 10

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] from comment #8)
> (In reply to :Gijs Kruitbosch from comment #7)
> > The only thing I've noticed wrong with this is that the highlight on the appmenu in the
> > detached window isn't correctly centered on the button. Don't know why that
> > is. Doesn't seem as bad a deal to me as the current state.
> 
> Perhaps this is bug 968039.

Yes, that sounds like it.

(In reply to Blair McBride [:Unfocused] from comment #9)
> Comment on attachment 8378574 [details] [diff] [review]
> UITour: make menu panel not break if tour tab is opened in new window,
> 
> Review of attachment 8378574 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/modules/UITour.jsm
> @@ +377,5 @@
> > +        let pendingDoc;
> > +        if (this._detachingTab && this._pendingDoc && (pendingDoc = this._pendingDoc.get())) {
> > +          let browser = window.gBrowser.getBrowserForDocument(pendingDoc);
> > +          if (selectedTab.linkedBrowser == browser) {
> > +            let tabType = selectedTab.pinned ? "pinnedTabs" : "originTabs";
> 
> This is confusing me...
> 
> pendingDoc is a document from which a mozUITour event was sent. In which
> case it won't go in the pinnedTabs Set, which is used for tracking pinned
> tabs that a tour page has requested be opened.
> 
> Am I misunderstanding something, or are you?

I'm misunderstanding something. I'll put up a new patch which just always adds stuff to originTabs, and adds comments to the member declarations as to what they're used for. Because the handling of both types of tabs is intertwined in the tab event handlers, I didn't understand that they were separate types of tabs (other than one being pinned and one not being pinned).

Comment 11

[:Gijs (he/him)]

Attachment: UITour: make menu panel not break if tour tab is opened in new window,

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8378964 [details] [diff] [review]
UITour: make menu panel not break if tour tab is opened in new window,

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

Excellent :)

Good to go, assuming just a couple of small fixes...

::: browser/modules/UITour.jsm
@@ +378,5 @@
>  
> +        let pendingDoc;
> +        if (this._detachingTab && this._pendingDoc && (pendingDoc = this._pendingDoc.get())) {
> +          let browser = window.gBrowser.getBrowserForDocument(pendingDoc);
> +          if (selectedTab.linkedBrowser == browser) {

getBrowserForDocument() iterates over all tabs until it finds the right one. Its far more efficient to just do:
  selectedTab.linkedBrowser.contentDocument == pendingDoc

(Which should also make it easier to fix bug 941428, when we get better APIs for dealing with document IDs.)

@@ +386,5 @@
> +            this.originTabs.get(window).add(selectedTab);
> +            this.pendingDoc = null;
> +            this._detachingTab = false;
> +            while (this._queuedEvents.length) {
> +              this.onPageEvent(this._queuedEvents.shift());

Hm, put the onPageEvent() call in a try/catch block? Normally any exception thrown in there wouldn't affect any other event. With this, it would currently kill all other pending events. Shouldn't happen, but I'd rather guard against it anyway - and not have it be catastrophic.

Comment 13

[:Gijs (he/him)]

Fair points all around. Landed with nits fixed,

remote:   https://hg.mozilla.org/integration/fx-team/rev/74f8a4d343a0

Comment 14

[Matthew N. [:MattN]]

dev-doc-needed for the TabBecomingWindow event

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/74f8a4d343a0

Comment 16

[:Gijs (he/him)]

Comment on attachment 8378964 [details] [diff] [review]
UITour: make menu panel not break if tour tab is opened in new window,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis' UITour
User impact if declined: broken primary UI when (accidentally or otherwise) detaching the tour tab)
Testing completed (on m-c, etc.): on m-c, has automated test
Risk to taking this patch (and alternatives if risky): relatively low. Lower impact would be disabling tour, which has too many other negatives associated with it.
String or IDL/UUID changes made by this patch: none

Comment 17

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/3c1dc7a61073

Comment 18

[Catalin Varga [QA][:VarCat]]

Verified as fixed using the following environment:
FF 29
Build id:20140305004002

FF 30
Buid id: 20140304030204

OS: Win 8.1 x32, Mac Os X 10.8.5, Ubuntu 12.04 x84