Hiperlinks are broken

[asizon]

Href attribute is broken and unable to load the requested links. This occurs throughout all of the Configurator Hiperlinks:

I can see that removing ref: noopener nooreferrer it works as expected, but idont know if remove all refs is the right solution:

[McGiverGim]

Maybe I'm wrong, but I think this is only a secondary effect of the new-win-policy not executing.

betaflight-configurator/src/js/main.js

Lines 87 to 103 in f9235b5

	if (GUI.isNWJS()) {
	GUI.nwGui.Window.getAll(function (windows) {
	windows.forEach(function (win) {
	win.on('close', function () {
	sendCloseEvent();
	this.close(true);
	});
	win.on('new-win-policy', function(frame, url, policy) {
	// do not open the window
	policy.ignore();
	// and open it in external browser
	GUI.nwGui.Shell.openExternal(url);
	});
	});
	});
	} else if (!GUI.isOther()) {

Doing a debug, the windows variable seems empty, so it is not executing the close or the new-window-policy listeners.

[McGiverGim]

It seems the open in external browser policy is broken since some versions ago... I have tested version 10.3 and it works perfectly, with or without the noopener noreferrer opening the link in a new browser.
With actual master or 10.5 it is not being opened in a external browser, it opens in a new window but inside de application, and this seems to not like the new links.

[Docteh]

I thought it was working. I know its been wrestled with a few times.

[Docteh]

Links open properly in my install of 10.6.0,

I looked into this a bit and I'm pretty sure this is related to NW.js changing how they interact with windows https://nwjs.io/blog/nw2-mode/

nwjs/nw.js#7377 mentions that we can temporarily disable NW2 for now, and I've filed nwjs/nw.js#7451

@mikeller can you confirm either failure or success on opening links on your end?

[McGiverGim]

Yes, I have been testing things thinking the same. 10.6 works. Executing 10.7 with --disable-features=nw2 does not work neither.

Maybe has changed when the window is available. I will look back to see what is the latest version working of nw.js.

We can fix it too with jquery intercepting the external link calls, but I don't know if it is the best solution...

[Docteh]

Try reverting the code, the last change there to iterate through getAll is where it stops working with nw1

…

On Fri, Apr 24, 2020, 2:39 AM Míguel Ángel Mulero Martínez < ***@***.***> wrote: Yes, I have been testing things thinking the same. 10.6 works. Executing 10.7 with --disable-features=nw2 does not work neither. Maybe has changed when the window is available. I will look back to see what is the latest version working of nw.js. We can fix it too with jquery intercepting the external link calls, but I don't know if it is the best solution... — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1980 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABOQ2ZRPW2RH5XHUNRCF7LROFM4LANCNFSM4MPGKHGQ> .

[McGiverGim]

I have used the simple get() that we have before.

Tested:

• 0.42.4 NOT WORKING
• 0.42.3 WORKING

I will look at the changelog, but I doubt I can see something.

[McGiverGim]

Is the first version with NW2 mode... so you are right @Docteh

[McGiverGim]

I think we maybe use the windows in a different manner of the rest of people. Here is the issue that made the getAll() method appear, and it seems to work for the other user: nwjs/nw.js#7227

So maybe our use of nw.js is a little strange...

[McGiverGim]

I think maybe I know what happens...
Our app is divided into two:

• A background page (js/eventPage.js)
• Our "real" app (that starts in main.html)

According to node documentation, the background page does not have a corresponding window, because is a "batch" execution.

In our code we CLOSE the NW window (file main_nwjs.html) and we open the main.html from the background page. Maybe since nw2 the context of this open window remains without associated window?

I suppose this was done in this way to maintain compatibility with Chrome OS.

I'm not too sure, but this maybe explains why this bug seems only affect to us.

[McGiverGim]

I've been playing with this with different results.

After some "patches" to ignore the Chrome part and open directly the nw window, I can attach the new function for the new window policy, but I get this error after the first click on a link:

(BLESSED_EXTENSION context for ajdoecjhplhoahggfmgflfnlechcljoc) extensions::nw.Window:718: Uncaught TypeError: Cannot read property 'onNewWinPolicy' of null{TypeError: Cannot read property 'onNewWinPolicy' of null
    at dispatchEventIfExists (extensions::nw.Window:718:21)
    at onNewWinPolicy (extensions::nw.Window:727:3)}

Someone knows how to fix that? Maybe some permissions missing in the manifest?

The most curious thing is that if after that I execute this in the console: nw.Window.get() the opening in external browser start to work...

[mikeller]

Interesting - I have been testing with hyperlinks both on 10.6.0 and master recently when working on the landing page, and they seem to be working fine on both on linux. Can I let you guys do the fixing / testing here, as I am not super well set up to test in Windows? Happy to test / verify particular things if you let me know.

[Docteh]

We can probably handle it. I'm lost in the weeds trying to replicate success :) just tried master via X11 forwarding and in a virtualbox

For some reason GUI.nwGui.Window.getAll (or nw.Window.getAll comes back with an empty array.

@McGiverGim are those changes available in a public/private branch? I've forgotten some of the git terminology, I want to pull your change down from your fork if its pushed there.

[McGiverGim]

No, I will try to push it in some hours, but is only a patch to verify my supposition, is not a solution to the problem.

[mikeller]

@Docteh: Sorry, that was a red herring - when testing again I found that it is broken in linux as well - I suspect the successful testing was in a pull request that came before the one where I added rel="noopener noreferrer" to the links on the landing page.
A couple of remarks:

• I think @McGiverGim is right, and the problem stems from our main app not having a window associated - when checking the event handler that we use to open links in an external app I found that this is not actually set for any Window:
  

  betaflight-configurator/src/js/main.js

  Lines 88 to 102 in aa98588

  	GUI.nwGui.Window.getAll(function (windows) {
  	windows.forEach(function (win) {
  	win.on('close', function () {
  	sendCloseEvent();
  	this.close(true);
  	});
  	win.on('new-win-policy', function(frame, url, policy) {
  	// do not open the window
  	policy.ignore();
  	// and open it in external browser
  	GUI.nwGui.Shell.openExternal(url);
  	});
  	});
  	});

• if we manage to fix this (might need some jiggery pokery to pass in the window of the main thread), and the new-win-policy handler gets used again, then I am not sure if we need the rel="noopener noreferrer" since we are already opening the links in an external app, thusly preventing forcing the new page to be loaded in a new process;
• now checking if upgrading NW.js to 0.45 will fix this, otherwise I recommend downgrading to 0.43 until we have found a fix for this.

[Docteh]

https://github.com/Docteh/betaflight-configurator/tree/nw2 This is what I came up with, but ports do not appear to be scanned initially, and the onClose stuff in eventPage.js is not dealt with.

src/js/port_handler.js has some 10 second timeouts that might be relevant to the serial ports not showing immediately.

rel="noopener noreferrer" is something sonar cloud complained about. Gotta have secure links, yo.

Since we're intending to assign the new-win-policy to the current window, I think we should revert back to nw.Window.get instead of nw.Window.getAll. Well, back to GUI.nwGui.Window.get()

With NWjs we can disable the nw2 "feature". https://nwjs.io/blog/nw2-mode/ Says that 0.42.4 is first version with NW2, so 0.43 may have this feature? If you pick a version I can check it out. Looks like 0.43.0 to 0.43.6 are your options.

I'll check back in a few more hours after @McGiverGim looks at things. 🛏️

[McGiverGim]

@Docteh my branch is here: https://github.com/McGiverGim/betaflight-configurator/tree/test_fix_external_url

I simply changed the event page where we open a Chrome window:

chrome.app.window.create('main.html', {

By a node_main.js page where we open a nw window:

nw.Window.open('main.html',

With this the links work because the getAll gets all the windows.
This is not a fix, because:

• We lost compatibility with Chrome.
• We need to add the close listener to the nw windows to close the connections, like it is done in the Chrome window. Can be added easy, but it has not been done.

So it seems clear that maybe the nw.js people have forgotten to attach the nw window at the chrome window in nw2.

@mikeller I think is a good idea to maintain the noopener noreferrer in the links. Maybe in a future we can publish this as a web app and any change makes the translators change it again. And the problem is not in the clausule, simply it makes the exisiting bug appear bigger.

[McGiverGim]

I forgot to say: 0.45 does not fix this. I tested yesterday. The latest version that works out of the box is 0.42.3 (the latest without nw2).

[Docteh]

Oh, NWjs is intentionally migrating away from the chrome.app interface with nw2, so we'll lose it eventually anyways.

[McGiverGim]

Then I think we must release with version 0.42.3 and remove the Chrome support for next version, as we have discussed several times :(

[mikeller]

#1982 should fix this. Turns out that #782 was always a hack, it would have sufficed back then to add an id for the application window to enable size / position restore.

Re noopener noreferrer, a couple of points:

• setting this does not do anything when opening links in an external app;
• the functionality is is already incomplete as it is, as this is not set in any dynamically loaded links (like the release notes).

To me, both of these together strongly suggest that we should not go on and add more complexity to set this on dynamically loaded links for something that does not do anything.

Re. future plans for a webapp, let's try and not build things for future eventuality - experience tells me that this is almost always a waste of time, as by the time something is used the conditions will have changed.

Also re translators, I think this is a good argument for not setting noopener noreferrer in the way we are doing it - it just adds to the effort required for translations. When we get to a point of needing it, we should rather look into adding this dynamically.

But pulling these out can wait until 10.7 has gone out.

[asizon]

Thankyou guys for getting involved in solving this, seems that finnaly we have a temporal solution for NW2(remove it temporaly).