Closed Bug 1347514 Opened 7 years ago Closed 7 years ago

Customize tab doesn't open if it was replaced by a normal web page

Categories

(Firefox :: Toolbars and Customization, defect)

Product:
Firefox
Component:
Toolbars and Customization
Version:
47 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox52 --- wontfix
firefox-esr52 --- wontfix
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: 684sigma, Assigned: mconley)

References

Details

(Keywords: regression)

Attachments

(2 files)

Bug 1347514 - Tear down customize mode in a background tab if the users sends it to a different page.
59 bytes, text/x-review-board-request
dao
: review+
Details
Bug 1347514 - Regression test.
59 bytes, text/x-review-board-request
dao
: review+
Details 
I have a problem with Firefox Beta 52, Firefox Beta 53, Nightly. It doesn't happen in Firefox ESR 45.
Sometimes Customize breaks a bit: browser thinks that Customize is opened in tab with a normal web page.
It happened twice unpredictably, however, I noticed one specific scenario when it happens

1. Open Customize
2. Open new tab, drag any bookmark to the tab with Customize without focusing it (First drag it onto current tab, then onto Customize tab, then drop it. This is default action if you want to open bookmark in existing tab)
3. Click "Open menu" button, click "Customize"

Result: Browser switches to the tab with web page opened from bookmark. Menu stays open.
Expected: Customize should open. Menu should close.

I use Customize mode at least once per week, because sometimes I need to see the title bar (usually I don't).
Has STR: --- → yes
Keywords: regression
Mike or Dão, this looks like a consequence of bug 1327210 to me. Can either of you take a look?
Flags: needinfo?(mconley)
Flags: needinfo?(dao+bmo)
It looks like a regression from bug 1014185, actually.

We need to be able to detect that the tab browsed away, and then unregister it as the "customizing" tab. Patch coming up.
Blocks: 1014185
Flags: needinfo?(mconley)
Flags: needinfo?(dao+bmo)
Assignee: nobody → mconley
Comment on attachment 8849151 [details]
Bug 1347514 - Tear down customize mode in a background tab if the users sends it to a different page.

https://reviewboard.mozilla.org/r/121984/#review124056

::: browser/components/customizableui/CustomizeMode.jsm:71
(Diff revision 1)
>  }
>  
> +var gTabsProgressListener = {
> +  onLocationChange(aBrowser, aWebProgress, aRequest, aLocation, aFlags) {
> +    if (!gTab || gTab.linkedBrowser != aBrowser) {
> +      log.error("gTabsProgressListener is attached to the wrong browser!");

This error doesn't make sense since addTabsProgressListener doesn't attach the listener to a specific browser.
Attachment #8849151 - Flags: review?(dao+bmo) → review-
Comment on attachment 8849151 [details]
Bug 1347514 - Tear down customize mode in a background tab if the users sends it to a different page.

https://reviewboard.mozilla.org/r/121984/#review124332
Attachment #8849151 - Flags: review?(dao+bmo) → review+
Comment on attachment 8849152 [details]
Bug 1347514 - Regression test.

https://reviewboard.mozilla.org/r/121986/#review124334

::: browser/components/customizableui/test/browser_exit_background_customize_mode.js:13
(Diff revision 2)
> +add_task(function* test_exit_background_customize_mode() {
> +  yield startCustomizing();
> +  is(gBrowser.tabs.length, 2, "Should have 2 tabs");
> +
> +  let custTab = gBrowser.selectedTab;
> +  let nonCustomizingTab = gBrowser.tabContainer.querySelector("tab:not([customizemode=true])");

'let nonCustomizingTab = gBrowser.selectedTab' before calling startCustomizing would be simpler. Not sure if you wanted to test the customizemode attribute here.
Attachment #8849152 - Flags: review?(dao+bmo) → review+
Comment on attachment 8849152 [details]
Bug 1347514 - Regression test.

https://reviewboard.mozilla.org/r/121986/#review124334

> 'let nonCustomizingTab = gBrowser.selectedTab' before calling startCustomizing would be simpler. Not sure if you wanted to test the customizemode attribute here.

Yeah, will do both. Thanks!
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b8b0b11b32e0
Tear down customize mode in a background tab if the users sends it to a different page. r=dao
https://hg.mozilla.org/integration/autoland/rev/81cbf82cd3c0
Regression test. r=dao
https://hg.mozilla.org/mozilla-central/rev/b8b0b11b32e0
https://hg.mozilla.org/mozilla-central/rev/81cbf82cd3c0
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Looks like a pretty easy backport? Please request Aurora/Beta/ESR52 approval on this when you get a chance.
status-firefox52: --- → wontfix
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox-esr52: --- → affected
Flags: needinfo?(mconley)
Flags: in-testsuite+
Version: 52 Branch → 47 Branch
(In reply to Ryan VanderMeulen [:RyanVM] from comment #14)
> Looks like a pretty easy backport? Please request Aurora/Beta/ESR52 approval
> on this when you get a chance.

It is, but... despite being an ugly bug, and easy to backport, and with a test... it's kinda hard to trigger it. I don't believe the STR are a common use case (since customize mode itself is not a common use case). Is there a compelling reason to skip the trains here? Or do you think there's a rush to have this in a release?
Flags: needinfo?(mconley) → needinfo?(ryanvm)
Per IRL discussion, let's let this ride the trains then.
status-firefox53: affected → wontfix
status-firefox54: affected → wontfix
status-firefox-esr52: affected → wontfix
Flags: needinfo?(ryanvm)
I have reproduced this issue with Firefox nightly 55.0a1 (2017-03-14) (64-bit) in Manjaro 17.

This bug is now verified as fixed in latest Nightly 55.0a1 

build id    : 20170420100256
user agent  : Mozilla/5.0 (X11; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday-20170419]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: