Closed Bug 1258344 Opened 4 years ago Closed 4 years ago

"Exit Customize" stays visible in hamburger menu after closing Customize mode from a 2nd instance

Categories

(Firefox :: Toolbars and Customization, defect)

47 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 48
Tracking Status
firefox46 --- unaffected
firefox47 + fixed
firefox48 + verified

People

(Reporter: epinal99-bugzilla2, Assigned: Gijs)

References

(Depends on 1 open bug)

Details

(Keywords: regression, Whiteboard: [mozfr-community])

Attachments

(1 file)

STR:

1) Open tab A
2) Open tab B and enter in Customize mode
3) Switch to tab A and enter in Customize mode
4) Exit Customize mode
NB: you are focused on tab B
5) Click on hamburger menu

Result: Menu displays "Exit Customize" in gray instead of "Customize".

Regression range:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=f9e4a56281d86c553b8cedd9cb86025813946ec0&tochange=c34fe673bb97d511920d2986cb84057f62e0c4a0

Dão Gottwald — Bug 1014185 - Remove about:customizing and use about:blank for customize mode instead. r=jaws
Blocks: 1014185
Flags: needinfo?(dao)
Keywords: regression
Whiteboard: [mozfr-community]
The URL bar is also greyed out in this case, and the browser generally looks unusable. I think Dão is on PTO, so I'll see if I can take a look.
Flags: needinfo?(gijskruitbosch+bugs)
Duplicate of this bug: 1258675
I have a patch, just need to finish writing the automated test.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(dao)
https://reviewboard.mozilla.org/r/41743/#review38135

::: browser/components/customizableui/test/head.js
(Diff revision 1)
> -
> -    //XXXgijs so some tests depend on this tab being about:blank. Make it so.
> -    let newTabBrowser = aWindow.gBrowser.selectedBrowser;
> -    newTabBrowser.stop();

Removed this and the rest of the extra promise block because AIUI we don't need to do this anymore - startCustomizing always opens a new tab, and endCustomizing will then close the existing tab. Tests pass on my local machine with this change, but let's find out if try agrees: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e3d52695bf2
Comment on attachment 8733376 [details]
MozReview Request: Bug 1258344 - fix customizemode re-entering issues when switching tabs, r?jaws

https://reviewboard.mozilla.org/r/41743/#review38929

::: browser/components/customizableui/test/head.js
(Diff revision 1)
>    aWindow.gCustomizeMode.exit();
>  
> -  return deferredEndCustomizing.promise.then(function() {
> +  return deferredEndCustomizing.promise;
> -    let deferredLoadNewTab = Promise.defer();
> -
> -    //XXXgijs so some tests depend on this tab being about:blank. Make it so.

I guess this is no longer true?
Attachment #8733376 - Flags: review?(jaws) → review+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6)
> Comment on attachment 8733376 [details]
> MozReview Request: Bug 1258344 - fix customizemode re-entering issues when
> switching tabs, r?jaws
> 
> https://reviewboard.mozilla.org/r/41743/#review38929
> 
> ::: browser/components/customizableui/test/head.js
> (Diff revision 1)
> >    aWindow.gCustomizeMode.exit();
> >  
> > -  return deferredEndCustomizing.promise.then(function() {
> > +  return deferredEndCustomizing.promise;
> > -    let deferredLoadNewTab = Promise.defer();
> > -
> > -    //XXXgijs so some tests depend on this tab being about:blank. Make it so.
> 
> I guess this is no longer true?

I think this stopped being true after bug 1014185 and the test that was removed there, yes.
(In reply to Pulsebot from comment #8)
> https://hg.mozilla.org/integration/fx-team/rev/12ff599ce0e2

And this is getting backed out because OSX debug only leaks that I don't understand. :-(
Flags: needinfo?(gijskruitbosch+bugs)
The test doesn't remove the handler it's attaching, which apparently causes a leak. I also noticed that closeGlobalTab in CustomizeMode.jsm doesn't null out gTab, which seemed wrong - unregisterGlobalTab does, and the tab is no longer in the browser, so the reference from CustomizeMode.jsm could keep a window alive until we GC CustomizeMode. So I killed that, too.
Flags: needinfo?(gijskruitbosch+bugs)
https://hg.mozilla.org/mozilla-central/rev/7d3eb3e1e135
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Comment on attachment 8733376 [details]
MozReview Request: Bug 1258344 - fix customizemode re-entering issues when switching tabs, r?jaws

Approval Request Comment
[Feature/regressing bug #]: bug 1014185
[User impact if declined]: bad bugs when switching between tabs to re-enter customize mode
[Describe test coverage new/current, TreeHerder]: added test coverage for this
[Risks and why]: low risk, has tests, minor changes, recent regression, aurora isn't merging for a few weeks still
[String/UUID change made/needed]: no
Attachment #8733376 - Flags: approval-mozilla-aurora?
Affected:   Win7_64, Nightly 48, 32bit, ID 20160402030226
Fixed on:   Win7_64, Nightly 48, 32bit, ID 20160403030243

What was fixed:
(1) Comment 0, (2) bug 1258675, (3) all items being partially draggable after exiting Customize mode
(after performing STR similar to comment 0 and bug 1258675)
Status: RESOLVED → VERIFIED
Depends on: 1262013
Comment on attachment 8733376 [details]
MozReview Request: Bug 1258344 - fix customizemode re-entering issues when switching tabs, r?jaws

e10s related regression that was verified on Nightly, Aurora47+
Attachment #8733376 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.