Strangeness under google-chrome on linux (edge/brave on windows)

[rouilj]

I have been testing with firefox. The test suite is working with firefox (except for a title n window issue).

However in google chrome, the popup window is displayed, but it is empty and it has no title. No errors are reported in the console.

This is with Version 124.0.6367.118 (Official Build) (64-bit) on linux, Very weird as I am sure it was working in the past.

Can anybody duplicate this?

[rouilj]

Hmm, it looks like a git pull messed up somehow.

This may be nothing.

[rouilj]

Ok, I have synced to b74ac89b and I still see an issue.

The error is:

TypeError: Cannot read properties of null (reading 'addEventListener') at ClassHelper.openPopUp (classhelper.js:897:23)

and is thrown in this code:

his.openPopUp(initialRequestURL, this.helpurlProps)
                .catch(error => {
                    // Top level error handling for openPopUp method.
                    cleanUpClosure();
                    console.error(error);
                    if (this.popupRef != null) {
                        this.popupRef.close();
                    }
                    window.alert("Error: Failed to open classhelper, check console for more details.");
                    this.helpurl.click();
                });

[rouilj]

Ti looks like

this.popupRef = window.open(CLASSHELPER_POPUP_URL, CLASSHELPER_POPUP_TARGET, popupFeatures);

window.open is returning null. I don't think I have popup blocking turned on.

[rouilj]

Malav can you take a look at this when you get a chance. Even when the fallback occurs, the popup opens in the main window not as a popup.

I have allowed popup windows in the browser.

[patel-malav]

@rouilj I am not able to reproduce this issue

Chrome - Version 124.0.6367.118 (Official Build) (64-bit) at https://github.com/UMB-CS-682-Team-03/tracker/commit/b74ac89b76fb6df99be3682e6b6197146fa4c82c

Is this with front end testing? or just the regular window

[patel-malav]

@rouilj The best we can do is add a null check as window.open(...) can return a null

        if (this.popupRef == null) {
            throw new Error("Browser Failed to open Popup Window");
        }

[rouilj]

Hi Malav:

In message @.***>, Malav Patel writes:

@rouilj The best we can do is add a null check as window.open(...) can return a null

       if (this.popupRef == null) {
           throw new Error("Browser Failed to open Popup Window");
       }

Yes, let's do that and gather as much info as we can as to why it failed.

I can reproduce the failure in my Brave browser on windows, but not in my chrome browser on windows. I have a couple more browsers I can try to narrow this down before I get out the debugger again.

-- -- rouilj John Rouillard

My employers don't acknowledge my existence much less my opinions.

[rouilj]

Also when the fallback triggers, I see the code trying to reset the onclick property. But it's not working right. The click you trigger causes the popup to open in the same window replacing the issue item display. This patch works to open the popup in a popup window:

diff --git a/html/classhelper.js b/html/classhelper.js
index 33ef4384..f9c3cd39 100644
--- a/html/classhelper.js
+++ b/html/classhelper.js
@@ -132,7 +132,7 @@ class ClassHelper extends HTMLElement {
             console.warn("Classhelper not intercepting helpurl.");
             if (this.helpurl != null) {
                 this.helpurl.removeEventListener("click", this.preventDefault);
-                this.helpurl.setAttribute("onclick", this.helpurlScript);
+                this.helpurl.onclick = this.helpurlScript);
             }
             console.error(err);
             return;
@@ -156,7 +156,7 @@ class ClassHelper extends HTMLElement {
             console.warn("Classhelper not intercepting helpurl.");
             this.removeEventListener("click", handleClickEvent);
             this.helpurl.removeEventListener("click", this.preventDefault);
-            this.helpurl.setAttribute("onclick", this.helpurlScript);
+            this.helpurl.onclick = this.helpurlScript;
         }

         const handleClickEvent = (event) => {

I think the difference is I am setting the property onclick and not the attribute onclick. The attribute IIUC is the serialized HTML version of the thing. Some attributes are synced to properties and some are not.

https://jakearchibald.com/2024/attributes-vs-properties/ makes it clearer but....

Still trying to track down where the failure is that's causing the web component to trigger the fallback.

[rouilj]

Hmm, it looks like the load event for the window is never firing (or is sent before the eventhandler for load is established). So it's left empty.

I tested this by adding a breakpoint inside the load handler and it is never reached.

Maybe before you establish the eventhander for load, check to see if https://developer.mozilla.org/en-US/docs/Web/API/Document/readyState readyState is already complete and fire the code to populate the popup?

Thoughts?

[rouilj]

I changed the code in openPopUp() to read:

        const popupFeatures = CLASSHELPER_POPUP_FEATURES(props.width, props.height);
        this.popupRef = window.open(CLASSHELPER_POPUP_URL, CLASSHELPER_POPUP_TARGET, popupFeatures);

        console.log(this.popupRef.document.readyState)

        this.popupRef.addEventListener("load", (event) => {
            const doc = event.target;
            const body = doc.body;

and console.log is showing complete every time and the window is empty in brave.

In windows chrome I see: interactive and in windows FF I see: uninitialized. In edge I also see complete and the popup doesn't initialize.

So it looks like a lifecycle timing difference between browsers. Fun.

[patel-malav]

Umm setTimeout() 3 secs ?

Or I open popup before making the first api request

Sent from Proton Mail Android

-------- Original Message -------- On 15/05/24 15:28, John P. Rouillard wrote:

I changed the code in openPopUp() to read:

const popupFeatures = CLASSHELPER_POPUP_FEATURES(props.width, props.height); this.popupRef = window.open(CLASSHELPER_POPUP_URL, CLASSHELPER_POPUP_TARGET, popupFeatures);

    console.log(this.popupRef.document.readyState)

    this.popupRef.addEventListener("load", (event) => {
        const doc = event.target;
        const body = doc.body;

and console.log is showing complete every time and the window is empty in brave.

In windows chrome I see: interactive and in windows FF I see: uninitialized. In edge I also see complete and the popup doesn't initialize.

So it looks like a lifecycle timing difference between browsers. Fun.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were assigned.Message ID: @.***>

[rouilj]

I'm not sure what you want to trigger with the setTimeout(...,3000). I dislike (but have used) magic timing numbers in general. Also waiting three seconds to populate the displayed popup window isn't great.

Would this work:

1 check the status of readyState 2 set up the load handler as you currently have it 3 set up the keyboard handler as you currently have it 4 if readyState was 'complete' do a setTimeout to send a 'load' event to the popup window.

for 4 something like:

const event = new Event("load");

// Dispatch the event.
setTimeout(this.popUpRef.dispatchEvent, 0, event)

This should trigger the load event handler on the window in the next interval (as soon as openPopUp returns most likely).

It is possible you may get two load events. The load event generated by the browser could arrive after the load event handler is established even if readyState == complete before the load handler is set up. If that happens, the second load event sent by setTimeout arrives after the classhelper popup has been populated. I think this second event can be ignored if the event handler check to see if doc.body is empty. If doc.body is empty just return from the load event handler assuming the load event handler has already run.

I am assuming that the readyState won't change to "complete" while javascript to setup the event handlers is executing since JS is single threaded. This might be incorrect as the popup window may have it's own interpreter but... (can't really find anything that discusses how the lifecycle works between js in one window an a window.open'ed window).

Thoughts?

Also since I was able to reproduce this with Edge, can @Nikunj-1308 or @BharathKanama se the issue on their windows boxes?

[rouilj]

Sigh. Replacing:

                this.helpurl.setAttribute("onclick", this.helpurlScript);

with

                this.helpurl.onclick = this.helpurlScript;

breaks the fallback popup under firefox (the popup replaces the original issue item). So we need to set both the property and the attribute to get chromium and gecko engines to work:

                this.helpurl.onclick = this.helpurlScript;
                this.helpurl.setAttribute("onclick", this.helpurlScript);

With luck this will also work on safari.... 8-).

[rouilj]

Ok, I think this works. At least it works for linux chrome, firefox and windows brave, ff and chrome. I'm not happy with it because of the closure that gets created and my inability to make a real 'load' event.

diff --git a/html/classhelper.js b/html/classhelper.js
index 0526a8f2..6d399f4d 100644
--- a/html/classhelper.js
+++ b/html/classhelper.js
@@ -132,6 +132,7 @@ class ClassHelper extends HTMLElement {
             console.warn("Classhelper not intercepting helpurl.");
             if (this.helpurl != null) {
                 this.helpurl.removeEventListener("click", this.preventDefault);
+                this.helpurl.onclick = this.helpurlScript;
                 this.helpurl.setAttribute("onclick", this.helpurlScript);
             }
             console.error(err);
@@ -156,6 +157,7 @@ class ClassHelper extends HTMLElement {
             console.warn("Classhelper not intercepting helpurl.");
             this.removeEventListener("click", handleClickEvent);
             this.helpurl.removeEventListener("click", this.preventDefault);
+            this.helpurl.onclick = this.helpurlScript;
             this.helpurl.setAttribute("onclick", this.helpurlScript);
         }

@@ -904,10 +906,17 @@ class ClassHelper extends HTMLElement {
             throw new Error("Browser Failed to open Popup Window");
         }

+        let docState = this.popupRef.document.readyState
         this.popupRef.addEventListener("load", (event) => {
-            const doc = event.target;
+            const doc = event.target == this.popupRef ?
+       event.target.document: event.target;
             const body = doc.body;

+       if (!!body.children.length) {
+         /* window already populated w/children */
+         return;
+       }
+     
             const itemDesignator = window.location.pathname.split("/").at(-1);
             let title = `${itemDesignator} - Classhelper`;

@@ -1011,6 +1020,11 @@ class ClassHelper extends HTMLElement {
                 }
             }
         });
+      if (docState == 'complete') {
+   const loadEvent = new Event("load");
+   const genLoadEvent = () => {this.popupRef.dispatchEvent(loadEvent, {bubbles: true});}
+   setTimeout(genLoadEvent, 0);
+      }
     }

     /** method when next or previous button is clicked

Note the weird way I assign doc. Here's the joke. The event.target sent to the window is the document when the browser generates the load event (second note). I can't find a way I can generate a load event with a mutated event.target. So I access this.popupRef inside the load handler. If the popupRef window is event.target, I know I am dealing with the synthetic load event and need to extract the document 8-(.

Also, it looks like the second note on the link above about 'load' events not bubbling is correct. When I dispatched the event on popupRef.document it never triggered the 'load' event on the window even when I added {bubble: true}.

Please feel free to pull this apart and show me where there is a simpler way to handle it.

Since you can't replicate the problem, can you confirm that this doesn't break things in your environment.

Thanks.

[patel-malav]

So, me and @Nikunj-1308 were able to reproduce the issue and came up to the following solution taking your patch into account.

We can say that we did end up finding a simpler yet complex solution.

Instead of us relying upon generating a synthetic load event, we create a new custom event on the classhelper,

which is registered with other event handlers.

Now inside the openPopup method, we create a DocumentFragment that is a root (html tag) populate it with the all other fragments (search, pagination, table, accumulator) and inside the head tag we create the title tag and the css link to stylesheet, etc.

Instead of previously doing that inside the "load" event handler.

Now we still listen for the load event but in the Event handler we dispatch the new CustomEvent("popupReady") on the classhelper, we pass the DocumentFragment as detail for this new event.

then we check if popupRef.document.readyState === "complete" we dispatch the new customevent again.

Thus, this is relatively same solution but in the event handler for this custom event we replace the whole popup root html with the page passed in via the detail

If this event handler is triggered multiple times, I am assuming that browser implementers are smart enough that they will at check for the reference for both being same and not do any hard work.

[patel-malav]

@rouilj We have tested it in windows - chrome, ff, brave, linux - chrome, ff, brave on (2 laptops)

https://github.com/UMB-CS-682-Team-03/tracker/commit/200df2e23322f3ef216d02770f5a7c85d8da60cb

[rouilj]

Hi Malav and Nikunj:

I'll throw comments in the commit as well as there seems to be a lot more moving parts to your solution. I also noticed that it does not look like you are handling the case where popupRef == null anymore.

Instead of us relying upon generating a synthetic load event, we create a new custom event on the classhelper, which is registered with other event handlers.

Ok. So the handler for the custom event (popupReadyEvent) populates the popup window using a generated document fragment passed in the detail part of the event.

The openPopup generates a document fragment that is passed as the detail to the custom event.

The load event on the new window now generates a popupReadyEvent using the (closure) of the document fragment created by openPopup. Question, where is the document fragment destroyed once a copy of it is placed in the popup?

Instead of previously doing that inside the "load" event handler.

So all of the work done inside the load handler is now done at the openPopup level and stored for later use by the handler for the popupReady event.

then we check if popupRef.document.readyState === "complete" we dispatch the new customevent again.

Got it.

Thus, this is relatively same solution but in the event handler for this custom event we replace the whole popup root html with the page passed in via the detail

Understood.

If this event handler is triggered multiple times, I am assuming that browser implementers are smart enough that they will at check for the reference for both being same and not do any hard work.

I'm not sure that's true. You may need to debounce it by verifying that the popup window is unpopulated. But it's probably an edge case that we can ignore for now as we are getting short on time.

[patel-malav]

@rouilj Pull Request https://github.com/UMB-CS-682-Team-03/tracker/pull/59

I would have merged into main but I wanted you to looking over my comments

Please merge this if its okay

PS: finally done with this issue (phew)

[patel-malav]

From john - https://github.com/UMB-CS-682-Team-03/tracker/pull/59#pullrequestreview-2062053217

Closing the Issue