Cristian Fogel [:cfogel] Reporter
	Description • 6 years ago

[Affected versions]:
- 63.0.1, 64.0b6, 65.0a1 (2018-11-05)

[Affected platforms]:
- Win10, Ubuntu 16.04, macOS 10.13;

[Steps to reproduce]:
1. Launch Firefox;
2. Enable devTools (F12);
3. Click on the RDM toggle button;

[Expected result]:
- RDM is displayed, no extra icons/buttons are displayed;

[Actual result]:
- the caret for the newTabs flashes when RDM is enabled;

[Regression range]:
- durring regression range check I considered bad builds that had the caret/doornhanger button displayed as well(not sure if it was intended at that point);
- bug 1408061 resulted as the issue;
*If it's not the case, I can re-run a check for just the flash;

[Additional notes]:
- attached recording with the issue;

	Cristian Fogel [:cfogel] Reporter
	Comment 1 • 6 years ago

Hey Mark, when you have time can you please take a look at this?
Thank you!

	Mark Striemer [:mstriemer]
	Comment 2 • 6 years ago

swap.js calls gBrowser.hideTab(containerTab) [1] so I think this is (sort of) working as expected. Once the swap is complete the visibility is manually cleared [2].

I don't know all the specifics of how this code works, but manually settings the visibility instead of calling gBrowser.hideTab() appears to fix this.

Dao, would replacing the call from [1] with `containerTab.style.visibility = "collapse";` be a reasonable solution here?

[1] https://searchfox.org/mozilla-central/rev/7c848ac7630df5baf1314b0c03e015683599efb9/devtools/client/responsive.html/browser/swap.js#152
[2] https://searchfox.org/mozilla-central/rev/7c848ac7630df5baf1314b0c03e015683599efb9/devtools/client/responsive.html/browser/swap.js#248

	Dão Gottwald [:dao]
	Comment 3 • 6 years ago

(In reply to Mark Striemer [:mstriemer] from comment #2)
> Once the swap is complete the visibility is
> manually cleared [2].

Is it? Note that this sets visibility on the browser rather than tab.

> I don't know all the specifics of how this code works, but manually settings
> the visibility instead of calling gBrowser.hideTab() appears to fix this.
> 
> Dao, would replacing the call from [1] with `containerTab.style.visibility =
> "collapse";` be a reasonable solution here?

It would work but I don't think we expect external code to mess with tabbrowser internals like this.

It sounds like swap.js is trying to do something that isn't really supported by the tabbrowser API. Why does it need "a temporary, hidden tab to load the tool UI"? Can it load the UI outside of tabbrowser?

	Mark Striemer [:mstriemer]
	Comment 4 • 6 years ago

(In reply to Dão Gottwald [::dao] from comment #3)
> (In reply to Mark Striemer [:mstriemer] from comment #2)
> > Once the swap is complete the visibility is
> > manually cleared [2].
> 
> Is it? Note that this sets visibility on the browser rather than tab.

Ah, you're right.

> > I don't know all the specifics of how this code works, but manually settings
> > the visibility instead of calling gBrowser.hideTab() appears to fix this.
> > 
> > Dao, would replacing the call from [1] with `containerTab.style.visibility =
> > "collapse";` be a reasonable solution here?
> 
> It would work but I don't think we expect external code to mess with
> tabbrowser internals like this.

I also noticed that if you disable popup auto-hide the temporary tab appears in the hidden tabs list, and is never removed. We're listening for TabAttrModified events, which we get [1] but the TabOpen and TabClosed events are suppressed by swap.js [2].

> It sounds like swap.js is trying to do something that isn't really supported
> by the tabbrowser API. Why does it need "a temporary, hidden tab to load the
> tool UI"? Can it load the UI outside of tabbrowser?

That seems like it would be a better choice. Moving this off of tabbrowser could be a better choice for the future. Gabriel, does this seem possible?

[1] https://searchfox.org/mozilla-central/rev/b096dcf0ea226af628fe03f7e7acb56a25853533/browser/modules/TabsList.jsm#41-42
[2] https://searchfox.org/mozilla-central/rev/b096dcf0ea226af628fe03f7e7acb56a25853533/devtools/client/responsive.html/browser/swap.js#58-76

	Gabriel Luong [:gl] (ΦωΦ)
	Comment 5 • 6 years ago

Gonna forward this one to jryans.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 6 • 6 years ago

Before going into the details of this bug, I agree that RDM makes use of some APIs (including tabbrowser) in ways they were not originally intended.  RDM is effectively constructing a bridge from a browser tab and associated APIs to the viewport content (skipping over the RDM UI in the middle), which is unusual and perhaps the only case of something like this.

At the moment, RDM uses existing tabbrowser and other browser APIs, sometimes in well supported ways and other times less so.  The current approach was chosen as an attempt to reduce code duplication into RDM as the browser evolves by relying on code paths used for other browser features.

Since RDM is doing some unexpected things, DevTools has historically taken on the maintenance tasks to tweak RDM as needed when complications arise (such as this bug).  For the long term, it would likely be best to learn from RDM's needs and construct better supported APIs that are understood and supported by both the Browser Front-end and DevTools teams.  However, such steps could be complex and require a good deal of coordination.  I would also guess that both teams may not have time to take on such a project.  (I am no longer a MoCo employee, so I am just guessing about team availability here.)

(In reply to Dão Gottwald [::dao] from comment #3)
> It sounds like swap.js is trying to do something that isn't really supported
> by the tabbrowser API. Why does it need "a temporary, hidden tab to load the
> tool UI"? Can it load the UI outside of tabbrowser?

One reason that a hidden tab is used is that we want the RDM UI with content viewport to be part of a browser tab.  The prior existence of `gBrowser.swapBrowsersAndCloseOther` (which accepts two tabs despite its name) suggested that creating a hidden tab and swapping from it back into the visible tab would achieve this with less code duplication than other approaches.

Aside from code duplication, I believe this path handled the tab listeners and filters in a way that was helpful, but I can't be sure of the details now.  Also, more swapping APIs have been added to tabbrowser since RDM was built, so one of them might be a better path than making a hidden tab.

It's quite possible there is a better approach to this and perhaps other things RDM does.  If you have suggestions in mind, discussing it over IRC, Slack, or a new bug would be great.

(In reply to Mark Striemer [:mstriemer] from comment #4)
> I also noticed that if you disable popup auto-hide the temporary tab appears
> in the hidden tabs list, and is never removed. We're listening for
> TabAttrModified events, which we get [1] but the TabOpen and TabClosed
> events are suppressed by swap.js [2].

If I understand correctly, this sounds like a separate bug that only occurs when using the "disable popup auto-hide" debugging feature is used, so it might be good to file this as a new bug.  For background, swap.js currently suppresses these events because they confused WebExtension APIs, but if the suppression is causing side effects, maybe we should tweak it.

(In reply to Mark Striemer [:mstriemer] from comment #2)
> I don't know all the specifics of how this code works, but manually settings
> the visibility instead of calling gBrowser.hideTab() appears to fix this.
> 
> Dao, would replacing the call from [1] with `containerTab.style.visibility =
> "collapse";` be a reasonable solution here?

It seems like RDM's usage of hidden tabs is causing the problem here, since the browser UI wants to present the all tabs list (and the caret in this bug) when any hidden tabs exist.  Replacing `hideTab` with the tab visibility style you mentioned may be a good way to fix this bug for the moment, if a quick fix is desired here.  While it may indeed be better to do away with the temporary tab entirely, it seems like it could be a complex change to make.

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 7 • 5 years ago

Marking fix-optional for 65 and 66 so that these already triaged issues don't show up repeatedly in weekly regression triage. Happy to take a patch in nightly.