Closed Bug 1603370 Opened 4 years ago Closed 4 years ago

Make RDM support target switching

Categories

(DevTools :: Responsive Design Mode, enhancement, P1)

enhancement

Tracking

(Fission Milestone:M6, firefox74 fixed)

RESOLVED FIXED
Firefox 74
Fission Milestone M6
Tracking Status
firefox74 --- fixed

People

(Reporter: ochameau, Assigned: daisuke)

References

Details

(Whiteboard: dt-fission-m2-mvp)

Attachments

(3 files, 1 obsolete file)

RDM would require some dedicated work, different from all panels listed in bug 1592575.
Mostly because it isn't hosted into a Toolbox.

The issue here is that when you navigate between two URLs which are loaded in distinct process, the TargetFront will change in favor of a new one.
As of today, it only happens when you navigate from special pages loaded in the parent process to websites (ex: about:robots to mozilla.org).
But if you enable Fission, it will happen if you switch between wikipedia.org and mozilla.org.

We should review all the call involving a target front and ensure that it supports this:
https://searchfox.org/mozilla-central/search?q=target&case=false&regexp=false&path=responsive

I think that the only code to tweak would be this one:
https://searchfox.org/mozilla-central/rev/62a130ba0ac80f75175e4b65536290b52391f116/devtools/client/responsive/ui.js#392-393
It memoizes the emulationFront and this will break if a target is being switched.
We could:

Whiteboard: dt-fission-m2-mvp

Tracking dt-fission-m2 bugs for Fission Nightly (M6)

Fission Milestone: --- → M6
Priority: -- → P2
Assignee: nobody → daisuke
Status: NEW → ASSIGNED
Priority: P2 → P1

Depends on D58904

Attachment #9119315 - Attachment is obsolete: true
Attachment #9119027 - Attachment description: Bug 1603370: Reflect target switching. r?jdescottes!,ochameau! → Bug 1603370: Reflect target switching. r?mtigley!,ochameau!
Attachment #9119028 - Attachment description: Bug 1603370: Add a test for target switching on RDM. r?jdescottes!,ochameau! → Bug 1603370: Add a test for target switching on RDM. r?mtigley!,ochameau!
Pushed by dakatsuka.birchill@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b567b33ed95a
Define currentTarget getter. r=jdescottes,ochameau
https://hg.mozilla.org/integration/autoland/rev/1d819921c8a3
Reflect target switching. r=ochameau,mtigley
https://hg.mozilla.org/integration/autoland/rev/0124af9f8ded
Add a test for target switching on RDM. r=ochameau,mtigley

Thanks Andreea!

Hi Micah!
I investigated this issue, it seems that the mix test of new UI and the old one caused the assertion.
And also I found browser_typeahead_find.js test which was failed also is passed if we remove the below line.
https://searchfox.org/mozilla-central/source/devtools/client/responsive/ui.js#274
We actually remove the class in destroy() (and I confirmed the class has been removing after closing tab.), but it seems that the style of the element will not be reverted perfectly? And this might be the cause.
https://searchfox.org/mozilla-central/source/devtools/client/responsive/ui.js#365
Do you have any ideas?

Or, I talked with Julian a bit ago, can we separate the browser.ini into two? One is for new UI, the other is not.
This also intended to avoid conflicting new UI and old. What do you think?

Flags: needinfo?(daisuke) → needinfo?(mtigley)

(In reply to Daisuke Akatsuka (:daisuke) from comment #8)

We actually remove the class in destroy() (and I confirmed the class has been removing after closing tab.), but it seems that the style of the element will not be reverted perfectly? And this might be the cause.
https://searchfox.org/mozilla-central/source/devtools/client/responsive/ui.js#365
Do you have any ideas?

Thank you for looking into this Daisuke! In the test we added, we set the pref for devtools.responsive.browserUI.enabled and devtools.target-switching.enabled to true. So for devtools.responsive.browserUI.enabled = true this means the code will run the lines you mentioned above. But at the end of test, I don't think the test will clear the prefs? I initially thought this might have been the case but after some investigating I still get the same failure even if I set those prefs back to false.

Or, I talked with Julian a bit ago, can we separate the browser.ini into two? One is for new UI, the other is not.
This also intended to avoid conflicting new UI and old. What do you think?

We recently extended addRDMTask to flip prefs between the old and new UIs. The idea was to ensure that existing code for the old RDM would still be functioning correctly while also running the same tests for the new RDM. But as you mentioned, there seems to still be a problem where the styling is not reverted completely, even though this function is setting the prefs as expected before this particular test runs.

If it's alright, I'd like to continue investigating why this particular test is failing before trying to split the browser.ini into two. Brad, did you have any thoughts on this?

Flags: needinfo?(mtigley) → needinfo?(bwerth)

I'm sorry, I don't have any additional insight into this. I don't know what it would mean to split browser.ini. If that solves the problem, it sounds reasonable.

Since the browser_typeahead_find.js test fails anyway, we can consider landing this patch by also disabling the test as long as the behavior the test is supposed to exercise works interactively with this patch applied. In other words, does typeahead find work in RDM with this patch applied? If so, just disable the test entirely, because we don't have confidence in it.

Flags: needinfo?(bwerth)

Thank you for your comments, Micah, Brad!

(In reply to Brad Werth [:bradwerth] from comment #10)

I'm sorry, I don't have any additional insight into this. I don't know what it would mean to split browser.ini. If that solves the problem, it sounds reasonable.

If this failure may be occurred by mixing both tests with/without fission, we thought separating the test is one of the ideas. So, I intended that we define something like fission-browser.ini to test only fission related tests.

(In reply to Micah Tigley [:mtigley] from comment #9)

If it's alright, I'd like to continue investigating why this particular test is failing before trying to split the browser.ini into two.

Yeah, if it's possible, I'd like you to investigate this one :)
Can you have time to take a look, Micah?

Flags: needinfo?(mtigley)

(In reply to Daisuke Akatsuka (:daisuke) from comment #11)

Yeah, if it's possible, I'd like you to investigate this one :)
Can you have time to take a look, Micah?

For sure! I will follow-up with you on this.

Flags: needinfo?(mtigley)

(In reply to Daisuke Akatsuka (:daisuke) from comment #11)

If this failure may be occurred by mixing both tests with/without fission, we thought separating the test is one of the ideas. So, I intended that we define something like fission-browser.ini to test only fission related tests.

I think this sounds fine. I'm still unsure of what's causing the test failure for browser_typeahead_find.js, but I don't think we should keep this from blocking the patch from moving forward any longer than necessary. Daisuke, would you be able to update the revision that splits the fission-only tests as you proposed?

Thanks, Micah!

I investigated a bit more, I thought this is related to fission but it seems that this was not. (I'm sorry!)
browser_typeahead_find.js test will be failed when doing the test after doing the following test.

add_task(async function() {
  await pushPref("devtools.responsive.browserUI.enabled", true);
  const tab = await addTab("http://example.com/");
  await openRDM(tab);
  await wait(3000);
  await closeRDM(tab);
  await removeTab(tab);
});

So I changed the order so as to test browser_typeahead_find.js after browser_viewport_resolution_restore.js, then tried it, because it seemed that browser_viewport_resolution_restore.js run on new browserUI as well.
However, there were no failures, I didn't understand why the test was passed.
I investigated this. The reason is that the test on new UI never run even if we pass true for includeBrowserEmbeddedUI in addRDMTaskWithPreAndPost.
https://searchfox.org/mozilla-central/source/devtools/client/responsive/test/browser/browser_viewport_resolution_restore.js#62
We can confirm that the value of devtools.responsive.browserUI.enabled is always false at the below point.
https://searchfox.org/mozilla-central/source/devtools/client/responsive/test/browser/head.js#165-167
Because the async test will be done after setting the original value here as we didn't wait for the async test.
https://searchfox.org/mozilla-central/source/devtools/client/responsive/test/browser/head.js#209-212

So, now, we should do followings:

  1. Find the reason why the browser_typeahead_find.js is failed after testing the new UI and fix that.
  2. Fix the addRDMTaskWithPreAndPost behavior to run on the new UI.

So, for this bug, can we file a follow-up bug that fixes the failure and skips this test?
And about addRDMTaskWithPreAndPost, let's file a bug if you also could confirm.
What do you think, Micah?

Flags: needinfo?(mtigley)

(In reply to Daisuke Akatsuka (:daisuke) from comment #14)

So, for this bug, can we file a follow-up bug that fixes the failure and skips this test?
And about addRDMTaskWithPreAndPost, let's file a bug if you also could confirm.
What do you think, Micah?

Thanks Daisuke for doing more investigation into this. I agree that we can skip the test for now and fix the issue in a follow-up bug. I'll create it.

Flags: needinfo?(mtigley)
Depends on: 1610588

Daisuke, thank you for working through all of this. It looks like I built the addRDMTask function incorrectly and I need to make it properly await the added tests before changing prefs and triggering another test. I've opened Bug 1610588 to address this.

See Also: → 1610717

(In reply to Micah Tigley [:mtigley] from comment #15)

I agree that we can skip the test for now and fix the issue in a follow-up bug. I'll create it.

Thanks! I will make the test skip. And I filed bug 1610717.

(In reply to Brad Werth [:bradwerth] from comment #16)

Daisuke, thank you for working through all of this. It looks like I built the addRDMTask function incorrectly and I need to make it properly await the added tests before changing prefs and triggering another test. I've opened Bug 1610588 to address this.

Thank you for filing!

Pushed by dakatsuka.birchill@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ac263c823285
Define currentTarget getter. r=jdescottes,ochameau
https://hg.mozilla.org/integration/autoland/rev/7e29e22ea816
Reflect target switching. r=ochameau,mtigley
https://hg.mozilla.org/integration/autoland/rev/f2c8e5f773ae
Add a test for target switching on RDM. r=ochameau,mtigley
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 74
Regressions: 1621316
Regressions: 1625501
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: