949663 - UITour: When closing the about:customizing tab, the menu panel does not show again

Status: ASSIGNED

(Firefox :: Tours, defect)

Description

[Alex Gibson [:agibson]]

STR:

- Visit https://www-demo3.allizom.org/b/en-US/firefox/australis/update/

- Start the tour and go to step 1 where the menu panel shows

- Click on 'customize'

- Close the customize window (using the 'x' on the tab)

Expected results:

- When the browser returns to the tour, the menu panel should show again.

Actual results:

- The menu panel does not show.

Note:

- If you return to the tour from a regular tab (i.e. web page), the menu shows correctly.

- If you close about:customizing using using the blue "Exit customize" button, the menu shows correctly.

- I tried using a setTimeout delay of 300-500ms when the tour 'visibilitychange' event fires. This does result in showing the menu when returning to the tour using the 'x' to close about:customizing, but the menu gets drawn incorrectly and fills the height of the browser window.

Comment 1

[Matthew N. [:MattN]]

Thanks Alex, please file related bugs blocking fx-UITour please.

Comment 2

[Matthew N. [:MattN]]

The problem is that PanelUI.show ends up getting called (everything is normal in UITour.jsm) but document.documentElement.hasAttribute("customizing") is true so we hit an early return. I'm not sure why the attribute is still there at the time. I think it's because exiting customization mode is async. We need to ensure that the attribute is removed before the code that causes the visibilitychange event fires.

Comment 3

[Alex Gibson [:agibson]]

Just tested this on the latest Nightly and setting a setTimeout of 800ms seems to fix the issue and the panel redraws to the correct size. Not quite a perfect solution as the delay is more noticeable when returning to a tab, but it works for now :)

Comment 4

[Matthew N. [:MattN]]

Okay, thanks for testing that. I'm glad to know it's an option.

Comment 5

[Drew Willcoxon :adw]

Attachment: make PanelUI.show wait for customization to end

This shows the menu after customization ends like we talked about today.  The patch turned out to be pretty simple.  The CustomizeMode.jsm change is necessary because otherwise visibilitychange is fired before customize-exiting is set.  Setting that attribute synchronously with the exit() call seems like a good idea anyway: exiting starts when exit() is called.

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8372685 [details] [diff] [review]
make PanelUI.show wait for customization to end

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

Mike, is the CustomizeMode change going to be okay with your work on the transition?

Drew, let me know about the listener then flag me for review again (on the new patch or same one).

::: browser/components/customizableui/content/panelUI.js
@@ +127,5 @@
>      }
>  
> +    // If customize mode is exiting, show the panel once it exits.
> +    if (exiting) {
> +      CustomizableUI.addListener({

Does the listener get removed?

Comment 7

[Drew Willcoxon :adw]

Attachment: make PanelUI.show wait for customization to end v2

Whoops, it should.

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 8374457 [details] [diff] [review]
make PanelUI.show wait for customization to end v2

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

::: browser/components/customizableui/content/panelUI.js
@@ +127,5 @@
>      }
>  
> +    // If customize mode is exiting, show the panel once it exits.
> +    if (exiting) {
> +      let onCustomizeEnd = {

Perhaps rename this to cuiListener or onCustomizeEndListener since it looks weird with two lines defining onCustomizeEnd.

Comment 9

[Drew Willcoxon :adw]

Comment on attachment 8374457 [details] [diff] [review]
make PanelUI.show wait for customization to end v2

Will do.

Mike:

(In reply to Matthew N. [:MattN] from comment #6)
> Mike, is the CustomizeMode change going to be okay with your work on the
> transition?

Comment 10

[Drew Willcoxon :adw]

I refreshed my tree just now and now my patch is causing something weird.  Sometimes the menu panel gets stuck in a rut where whenever you open it, it's very long and skinny and empty.  It seems that if the menu is first opened manually after starting the app, then it's fine from then on.  But if UITour opens it first (as a result of a page load), then it's messed up from then on, even when you later open it manually.

The tip of my tree before I refreshed it was:

> changeset:   167087:c06525b87f03
> tag:         qparent
> user:        Gijs Kruitbosch <[e-mail]>
> date:        Wed Feb 05 22:30:46 2014 +0000
> summary:     Bug 968251 - fix gradient background on Australis' panel anchor's arrow background, r=jaws

I'll try to find out what's going on.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8374457 [details] [diff] [review]
make PanelUI.show wait for customization to end v2

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

Tentative r- because of the likelihood of this introducing jank during customize mode exit.

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +315,5 @@
>      this._transitioning = true;
>  
> +    // Call _doTransition now so that customize-exiting is set synchronously
> +    // with this exit call.
> +    let transitionPromise = this._doTransition(false);

I'm concerned about this, because this allows us to transition while depopulating the palette at the same time, which will likely introduce jank...

Comment 12

[Drew Willcoxon :adw]

Aha, the problem is the recreateMenu call introduced in bug 966072.  When I comment out that call in UITour.showMenu, the menu appears as expected.  Matt, do you know why that might cause the effect I mentioned in comment 10?  Did you test bug 966072 on a platform other than Linux?  (I'm on OS X.)

Comment 13

[Matthew N. [:MattN]]

(In reply to Drew Willcoxon :adw from comment #12)
> Aha, the problem is the recreateMenu call introduced in bug 966072.  When I
> comment out that call in UITour.showMenu, the menu appears as expected. 
> Matt, do you know why that might cause the effect I mentioned in comment 10?
> Did you test bug 966072 on a platform other than Linux?  (I'm on OS X.)

I can't reproduce the problem on 10.9.1 on either my HiDPI or regular DPI screens on m-c (18e7634d4094) with your patch applied. I tried using the close tab X and the exit customize button and my panel always looks normal. Any more tips?

Comment 14

[Drew Willcoxon :adw]

Clearing the needinfo since we talked on IRC.

Comment 15

[Drew Willcoxon :adw]

Er.  (Also, for reference here's a link to the test page: http://jsbin.com/ciqeq/1)

Comment 16

[Matthew N. [:MattN]]

The problem isn't reproducible with the current tour on mozilla.org but I'd like to understand the problem better.

Comment 17

[Drew Willcoxon :adw]

Something else we could try is changing tabbrowser to call CustomizeMode.exit directly and then wait for its promise to be resolved before actually closing the page when you click the tab's X.  I don't know how kosher it would be to make tabbrowser sniff for certain pages like that, but it already does in the case of about:newtab.  Seems like that would address Gijs's points but not Matt's concern that this bug might happen in other situations.

Comment 18

[Matthew N. [:MattN]]

(In reply to Matthew N. [:MattN] from comment #16)
> The problem isn't reproducible with the current tour on mozilla.org but I'd
> like to understand the problem better.

I filed bug 974804 with a fix.

(In reply to Drew Willcoxon :adw from comment #17)
> Seems like that would address Gijs's points but not Matt's concern that this bug might happen in
> other situations.

I was mostly worried about other situations where the tab got closed but I suppose there could be other ways to hit this too. I don't really like the sounds putting that logic in tabbrowser but perhaps you could achieve a similar result by stopping the initial tab close with an event listener and then doing the actual close after the exit promise resolves.

Comment 19

[Drew Willcoxon :adw]

Attachment: on TabClose, call exit() and prevent the tab from closing

I still like the idea that no matter how the customization page is closed, it ought to call exit() to properly uninitialize itself.  I like that idea much better than making PanelUI.show() show the menu after an ongoing customization has ended.

So I like your idea of having an event listener that stops the page from closing.  Problem is, when you click a tab's X, tabbrowser.removeTab() is called on it immediately, and there's no way I can see of stopping it.  So this patch makes the TabClose event cancelable... D:  With that, the CustomizeMode part of the patch is nice and simple.  I made the TabClose listener's lifetime the same as the keypress's, although keypress is added inside enter()'s task when it seems like it should be added synchronously with the enter() call.

Comment 20

[Matthew N. [:MattN]]

Comment on attachment 8383455 [details] [diff] [review]
on TabClose, call exit() and prevent the tab from closing

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

I'm fine with the approach but have compatibility concerns. Dão, are you fine with what I propose or do you think Drew's change to tabbrowser.xml is fine.

::: browser/base/content/tabbrowser.xml
@@ +1922,5 @@
>  
> +            // Fire TabClose.  Fire it before any teardown so that event
> +            // listeners can inspect the tab that's about to close.
> +            var evt = document.createEvent("UIEvent");
> +            evt.initUIEvent("TabClose", true, true, window, aTabWillBeMoved ? 1 : 0);

I think this is a breaking compatibility change we may not want. Consumers are likely to be relying on this event to do cleanup operations but with this change it's possible the tab may not actually close after. I think we'd need to use a new event before the close similar to how quit-application-requested and quit-application-granted work e.g. TabCloseRequested

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 8383455 [details] [diff] [review]
on TabClose, call exit() and prevent the tab from closing

Yes, this does break assumptions about the TabClose event.

Also the whole "prevent the tab from closing and then close it" approach feels weird and like a hack. Ideally, CustomizeMode.jsm would call removeTab where it currently calls exit, and the rest of what's currently exit should be a TabClose handler. I'm sure this is easier said than done and some code would need to be refactored.

Comment 22

[Drew Willcoxon :adw]

It can't call removeTab because it's not the one removing the tab.  This is the case where the tab's X is clicked.  That bypasses CustomizeMode's exit handling altogether.

Comment 23

[Dão Gottwald [:dao]]

You misunderstood what I said. The current setup of exit calling removeTab and doing the teardown at the same time is broken. It should instead be:

1. removeTab is called
2. CustomizeMode.jsm sees the TabClose event and tears down customization mode

Whether 1. happens inside CustomizeMode.jsm or comes from the tab close button doesn't matter at that point. All methods of exiting customization mode would take the same code path.

Comment 24

[Drew Willcoxon :adw]

I see.  That is better.

Comment 25

[Matthew N. [:MattN]]

Lower priority since we have a workaround in place and it's only a problem if the tab close button is used, not the Exit Customize button. It is a bit scary that the workaround is a setTimeout so it could break for some people if that delay isn't big enough. I haven't seen any other reports yet though.

Comment 26

[Matthew N. [:MattN]]

Moving open UITour bugs to Firefox::Tours. Filter on firefox-tours-20150121.