Closed Bug 1404088 Opened 3 years ago Closed 3 years ago

Responsive Design Mode tabs stick around forever in Tree Style Tab sidebar

Categories

(DevTools :: Responsive Design Mode, defect, P2)

defect

Tracking

(firefox57 verified, firefox58 verified)

VERIFIED FIXED
Firefox 58
Tracking Status
firefox57 --- verified
firefox58 --- verified

People

(Reporter: dholbert, Assigned: jryans)

References

Details

Attachments

(4 files, 1 obsolete file)

STR:
 
 1. Start Firefox with clean profile.

 2. Install TST: https://addons.mozilla.org/en-US/firefox/addon/tree-style-tab/

 3. Visit http://example.org/

 4. Ctrl+Shift+M to trigger Responsive Design Mode (or Tools|WebDeveloper|ResponsiveDesignMode)

 5. Try to close the tab by using the Tree Style Tab's x-button.
   --> (it doesn't work)

 6. Ctrl+W to close the tab (or click the "x" on the main tabstrip at the top)

 7. Look at the Tree Style Tab sidebar.

ACTUAL RESULTS:
 - As noted in step 5, I cannot close the tab by using using the "x" on its tree-style-tab.
 - At the end of the STR, after I've actually closed the tab, its Tree Style Tab is still around.

EXPECTED RESULTS:
 - I should be able to close the tab in step 5.
 - The Tree Style Tab should disappear when the tab goes away.


Note: when I enter Responsive Design Mode, I see these errors go by in browser console:
======
TypeError: zoom is undefined[Learn More]          viewZoomOverlay.js:47:5
TypeError: win.gBrowser is undefined[Learn More]  ZoomUI.jsm:69:1
win.gBrowser is undefined                         ReaderParent.jsm:70
gBrowser is undefined                             ext-tabs.js:299
======

The first three of those (i.e. all but the ext-tabs.js one) show up regardless of whether the Tree Style Tab extension is installed.
Note: I also filed https://github.com/piroor/treestyletab/issues/1392 on this before realizing that it was probably a bug on Firefox's end.
Thanks for filing this!  I happen to also be a TST user myself. :)

I am _fairly_ sure RDM is to blame, mostly because it does some magic with tabs that is probably confusing WebExtensions code in some way or another.

(For the console errors unrelated to this problem, we have bug 1273721.)
Priority: -- → P2
Summary: Devtools Responsive Design Mode tabs stick around forever in Tree Style Tab sidebar (and also cause a bunch of gBrowser/aBrowser error spam in Browser Console) → Responsive Design Mode tabs stick around forever in Tree Style Tab sidebar
This problem happens on Tab Center Redux also.
This seems related to another bug 1398272.

Steps to reproduce:

 1. Open a new tab. For example it is reported as id=32.
 2. Hit Ctrl-Shift-M to enter responsive design mode.
    1. tabs.onCreated for a new tabs.Tab (for example, id=35) is reported.
    2. tabs.onDetached for a new tabs.Tab (id=35) is reported.
 3. Open Debugger and run following codes: 
    * browser.tabs.get(32).then(t=>console.log(t))
    * browser.tabs.get(35).then(t=>console.log(t))

Actual result:
Both successfully report a same tabs.Tab with id=32.

Expected result:
One of browser.tabs.get(32) or browser.tabs.get(35) fails.


Then I have a question: what is the "expected" result for the case?
Should the responsive tab completely same to the original tab?
Or, should the responsive tab behave a new tab and the original tab is treated as closed?
Assignee: nobody → jryans
Status: NEW → ASSIGNED
(In reply to YUKI "Piro" Hiroshi from comment #4)
> This seems related to another bug 1398272.
> 
> Steps to reproduce:
> 
>  1. Open a new tab. For example it is reported as id=32.
>  2. Hit Ctrl-Shift-M to enter responsive design mode.
>     1. tabs.onCreated for a new tabs.Tab (for example, id=35) is reported.
>     2. tabs.onDetached for a new tabs.Tab (id=35) is reported.
>  3. Open Debugger and run following codes: 
>     * browser.tabs.get(32).then(t=>console.log(t))
>     * browser.tabs.get(35).then(t=>console.log(t))
> 
> Actual result:
> Both successfully report a same tabs.Tab with id=32.
> 
> Expected result:
> One of browser.tabs.get(32) or browser.tabs.get(35) fails.

Thanks for bringing up this issue as well!  The RDM variant here may or may not have the same fix as the general problem in bug 1398272.  I'll watch that bug to see where it leads.

> Then I have a question: what is the "expected" result for the case?
> Should the responsive tab completely same to the original tab?
> Or, should the responsive tab behave a new tab and the original tab is
> treated as closed?

A tab with RDM enabled is meant to be the same as the original tab.  It has the same session history, etc.

At the moment, it seems like WebExtension APIs make it appear as a different tab, so we should strive to change that.

For now, I would suggest TST not do anything special for RDM tabs.  We should find a fix for this in Firefox, so that tab management add-ons don't worry about it.
Things to verify:

* RDM tab should have same tab ID as original tab
* getTabValue etc. should have the same data for a tab independent for RDM state
* Avoid spurious events like tab created, etc. when toggling RDM
Comment on attachment 8913883 [details]
Bug 1404088 - Standardize RDM's exposed browser properties.

https://reviewboard.mozilla.org/r/185286/#review190614
Attachment #8913883 - Flags: review?(poirot.alex) → review+
Comment on attachment 8913884 [details]
Bug 1404088 - Add gBrowser to the browser property list.

https://reviewboard.mozilla.org/r/185288/#review190616
Attachment #8913884 - Flags: review?(poirot.alex) → review+
Comment on attachment 8913885 [details]
Bug 1404088 - Waive Xrays when exposing properties for RDM.

https://reviewboard.mozilla.org/r/185290/#review190600

::: devtools/client/responsive.html/browser/tunnel.js:227
(Diff revision 1)
>        // Expose various properties from the browser window on the RDM tool's global.  This
>        // aids various bits of code that expect to find a browser window, such as event
>        // handlers that reach for the window via the event's target.
> +      let innerGlobal = Cu.waiveXrays(inner.ownerGlobal);
>        for (let property of PROPERTIES_FROM_BROWSER_WINDOW) {
> -        Object.defineProperty(inner.ownerGlobal, property, {
> +        Object.defineProperty(innerGlobal, property, {

This is surprising it doesn't break the previous usages of this code.
Are you sure you don't break previous usages of PROPERTIES_FROM_BROWSER_WINDOW in order to fix WebExt one?

Before, you were adding these properties onto the xray object, but now you add it on the unpriviledged object.
Comment on attachment 8913886 [details]
Bug 1404088 - Hide RDM temporary tabs from WebExtensions.

https://reviewboard.mozilla.org/r/185292/#review190618

Now that the new RDM is the default and the old is gone, we should start thinking about integrating into /browser/ and prevent such long list of hacks.
Attachment #8913886 - Flags: review?(poirot.alex) → review+
Comment on attachment 8913886 [details]
Bug 1404088 - Hide RDM temporary tabs from WebExtensions.

https://reviewboard.mozilla.org/r/185292/#review190618

Yes, I agree in principle that we should merge more directly with browser code where we can.  I think that requires a bit more investment in RDM to make it worth the time spent.  So, if we work heavily on RDM again, let's try to push forward with that.
Attachment #8913885 - Attachment is obsolete: true
Attachment #8913885 - Flags: review?(poirot.alex)
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7e5773d59626
Standardize RDM's exposed browser properties. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/c78ebf798ca6
Add gBrowser to the browser property list. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/25cdc47d74bf
Hide RDM temporary tabs from WebExtensions. r=ochameau
:dholbert, could you verify that the issue is fixed in the next Nightly for you?
Flags: needinfo?(dholbert)
Yup, verified fixed in 58.0a1 (2017-10-05) (64-bit)

The tabs' "x" in the TST sidebar now works correctly -- and when I close the tab [that way or via Ctrl+W or via the normal tabstrip], it now correctly disappears from the TST sidebar.

Thanks!
Status: RESOLVED → VERIFIED
Flags: needinfo?(dholbert)
Comment on attachment 8913886 [details]
Bug 1404088 - Hide RDM temporary tabs from WebExtensions.

Approval Request Comment
[Feature/Bug causing the regression]: RDM sends noisy tab events that can confused tab management WebExtensions such as Tree Style Tabs
[User impact if declined]: If declined, WebExtensions like Tree Style Tabs can break in various ways, such as the tabs won't close or won't disappear when they have been closed.  It would be nice to take this for 57 to offer correct tab functionality for such add-ons to avoid user confusion.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes, manually verified by developer and  bug reporter.
[Needs manual test from QE? If yes, steps to reproduce]: No 
[List of other uplifts needed for the feature/fix]: None, just this patch.
[Is the change risky?]: No
[Why is the change risky/not risky?]: Only affects RDM tabs
[String changes made/needed]:
Attachment #8913886 - Flags: approval-mozilla-beta?
Comment on attachment 8913886 [details]
Bug 1404088 - Hide RDM temporary tabs from WebExtensions.

Fix was verified in Nightly, this seems like a good fix to uplift, Beta57+
Attachment #8913886 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attached image closing tab works.gif
This bug is verified on Firefox 57.0b7 (20171009192146) and Firefox 58.0a1 (20171010100200) under Windows 10 64bit/Mac OS X 10.12.3.

Please see the attached video.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.