arvgta / ajaxify

Ajaxify - The Ajax Plugin
https://4nf.org/
274 stars 124 forks source link

Forms not working anymore in macOS Firefox (latest) #182

Closed tchapi closed 4 years ago

tchapi commented 4 years ago

Hi

I have an ajaxified page with a simple POST form. In macOS Chrome, it's working fine (ajaxify posts the form, gets the response, displays what it should). On macOS Firefox, ajaxify does what it's supposed to do, BUT the page is reloaded anyway (another GET is issued).

How can I mitigate this issue ? Thanks a lot !

arvgta commented 4 years ago

Hi Tchapi,

thanks for pointing that out! I have forwarded it to the guy who debugged the last version. Hope he responds soon...

(it reads as if it were specific to the latest version?)

EDIT: Works on Firefox for Windows 10 against 4nf.org and another test site - however, these employ GETs

Thanks and regards, Arvind

tchapi commented 4 years ago

Thanks. I don't know if it's specific to the latest version, but the exact same site has different behavior on both navigators, and it wasn't this way before, hence my issue...

arvgta commented 4 years ago

Alright - thanks. We'll see what the other guy says... I'm off for the evening - please don't be irritated.

edito commented 4 years ago

Hi @tchapi,

I'll need some additional information about your website preferences for ajaxify and form data that is sent to the page because I couldn't reproduce your issue on Firefox for windows (for both types of request POST/GET).

My current opinion is that it's not related to POST/GET type of request, also have something else on my mind, could you please try the testpatch branch ajaxify file from this link on your development environment and tell us if it behave the same or not?

tchapi commented 4 years ago

Hi and thanks for your answer

I tested with your version but it's the same : Chrome OK, FF NOK.

Here is my config :

$('#content, #my-account, #my-account-mobile, #site-navigation, #csrf-token').ajaxify({
    prefetch: false, // Plugin pre-fetches pages on hover
    canonical: true,
    scrolltop: true, // always scroll to top
    bodyClasses: true, // Copy body classes from target page, set to "true" to enable
    cb: function (error) {
      $('#menu-toggle, #site-header-menu').removeClass('toggled-on')
      $.cache1('f')
    }
  })

The form is extremely simple :

<form method="POST" action="http://localhost:8002" accept-charset="UTF-8" enctype="multipart/form-data">
  <input name="_token" type="hidden" value="ZWJrhCB2OneyGhCpRgFdfrpzhNTGVBHtniAjgMs6">
    <input name="given_name" type="text">
    <div><input type="submit" value="Save"></div>
</form>

When inspecting the two behaviors, the only difference I see is that on Chrome, once the POST comes back with a 302 redirect, Chrome goes fetching (GET) the redirection target without specifying a content-type in the request. On the other hand, FF explicitely specifies "application/x-www-form-urlencoded" as content-type in this GET request, and that may trigger an additional GET afterwards ?

I'm a bit lost ...

arvgta commented 4 years ago

Hi @tchapi,

just my fifty cents:

prefetch: false

has be changed to

prefetchoff: true

in the latest versions

... in order to compare apples to apples. But even if it works with prefetchoff set, it would still be a bug, because your use case should work with prefetch enabled nevertheless.

edito commented 4 years ago

Hi again,

too many questions is passing through my mind. I would like to know if everything works fine on windows firefox browser for your website. To be sure that it's a specific platform&browser issue.

Next thing, I'll recommend you to find this line (264) in latest version if (!pre) location.href = hin; //If not a pre-fetch -> jump to URL as an escape and comment it out. Then check if it still does refresh (please make sure you cleared browser cache after commenting line).

One more thing, I'll recommend you setting memoryoff: true, instead of flushing cache with $.cache1("f") on every request.

tchapi commented 4 years ago

Alright, with the if (!pre) location.href = hin; line commented, it works on Chrome and FF (and Safari, too)

Digging deeper, I have the impression that Firefox gets the response base64-encoded (whereas Chrome / Safari does not). I don't see which header would trigger this, but this is what could trigger a false in _isHtml(xhr) on this specific line ?

edito commented 4 years ago

Hi,

ok, we found the source of the issue. However I would like to be sure that everything works fine on windows platform browsers for your particular case, if it's possible, to know is it a platform related issue without some hidden bugs.

Since I'm not great with writing those header checks, I would like to hear what @arvgta has to say about it.

arvgta commented 4 years ago

Hi @edito,

thanks for all your efforts and finding the line that was causing this issue. I agree that a test on Windows browsers would be useful. Unfortunately, I am no better than you on these header checks.

@tchapi : It would be interesting for the records additionally, to know whether turning off prefetch before commenting out that line with:

prefetchoff: true

...made a difference?

Thank you both!

tchapi commented 4 years ago

I tested a bit more :

arvgta commented 4 years ago

@tchapi

Ok, thanks, very interesting! But when commenting out the line:

if (!pre) location.href = hin;

...it works on all browsers, right?

tchapi commented 4 years ago

That's exact

tchapi commented 4 years ago

A fix would be to allow x-www-form-urlencoded in the isHtml() function, but while this will work in my case, I'm not sure it doesn't have other implications ...

arvgta commented 4 years ago

Is that satisfactory then for all of us as a deliverable of this issue?

(I mean commenting out that line:

if (!pre) location.href = hin;

)

tchapi commented 4 years ago

It's not satisfactory for me — I don't want to modify the ajaxify source code since I want to keep being updated ;)

I'm not an expert enough with MIME to know if allowing application/x-www-form-urlencoded in the isHtml() function is safe for all cases, but I guess it could be. I'm pretty sure any response with this type is still plain, parsable HTML.

What do you think @edito ?

arvgta commented 4 years ago

@tchapi

Obviously, I mean actually modifying the release code of Ajaxify centrally. You don't have to change a thing yourself ;)

tchapi commented 4 years ago

Yes but I think you should keep the if (!pre) location.href = hin; anyway, and just adjust the isHtml() function, if safe.

arvgta commented 4 years ago

Ok, just another idea - in that case we could specify all thinkable MIME types that we accept in one go?

Let's see what @edito says...

edito commented 4 years ago

As I understood, this check is here in case that ajax request fails somehow and then it forces standard page navigation as escape metod.

My recommendation would be changing approach to something that is easier and more unifying to check, those headers seems unstable and can vary from browser to browser, even with possibility of changing in future.

Condition can be written to check if xhr returned success (even with some simpler additional checks, for example that data is not empty or that fetched data contains some tags specific to html as head/body, that should do the work with less chance to get false positive).

What you think?

arvgta commented 4 years ago

As I understood, this check is here in case that ajax request fails somehow and then it forces standard page navigation as escape method

Exactly - it enforces, that only HTML-like content can be swapped-in, otherwise, it "jumps" to the link.


Great idea - I think we should choose the most generic check, and something like that seems to be most generic.

I also agree, that we have little influence on the ways how these headers can vary.

I would suspect, that we are not the first people looking for validation like that and we should research the web for a suitable check in JS/jQuery and simply reuse it...

(My mistake originally, for thinking that the MIME type is the only way to go)

arvgta commented 4 years ago

What do you think of this thread at Stackoverflow?

We could alter this existing code:

return (d = x.getResponseHeader("Content-Type")), d && (d.iO("text/html") || d.iO("text/xml"));

to

return (d = x.getResponseHeader("Content-Type")), d && (d.iO("html") || d.iO("form"));

...that is, if checking for the substring "form" generically makes sense? Also, I'm not sure whether to allow XML as a response type nowadays? (I believe, I enabled that content type for XHTML support originally)

edito commented 4 years ago

@tchapi mentioned that he is getting something like base64 encoded headers in macOS firefox. This won't work if text is encoded. But if x.getResponseHeader("Content-Type") becomes x.responseText it will check for both tags inside response data avoiding headers completely.

It will look like this:

var d = x.responseText;
return  d && (d.iO("html") || d.iO("form"));

or to perform more code reduction, it's possible to pass responseText right from call !_isHtml(xhr.responseText), then we will only have

return  x && (x.iO("html") || x.iO("form"));

inside isHtml function, but I don't know how will this last reduction behave in case of error.

What is your opinion?

arvgta commented 4 years ago

Thanks very much Edin. My opinion is, that we should do things as similar as possible to other authors, as this surely ought to be a frequently performed task. That's why I recommended the approach above (Stackoverflow thread).

If we actually search the responseTextfor the two proposed strings, it can easily be criticised, that the search will return "success", even if the hits are not the HTML tags we're after.

Because this issue of @tchapi is the first one of its kind throughout the life of Ajaxify, it might be worth considering, whether he should not simply patch it for his own use case?

I did a second, similar search just now, and this was the top Google result:

(again, they check the MIME type)


I also found this incomplete list of MIME types

Apparently, Ajaxify is not checking for XHTML correctly by doing:

It should rather be something like:

...which specifies it cleary...

tchapi commented 4 years ago

Because this issue of @tchapi is the first one of its kind throughout the life of Ajaxify, it might be worth considering, whether he should not simply patch it for his own use case?

I don't think you should do that. The patch should be compatible and safe for everybody. I think the best option is your proposal :

return (d = x.getResponseHeader("Content-Type")), d && (d.iO("html") || d.iO("form"));
arvgta commented 4 years ago

Thanks! I'll do that exactly after reading this on form MIME types as well, however with tweaking XHTML support, while we're at it:

return (d = x.getResponseHeader("Content-Type")), d && (d.iO("html") || d.iO("xhtml") || d.iO("form"));

(that's if we all agree :) )

Ah ok - "html" is a substring of "xhtml" already -> so this is enough:

return (d = x.getResponseHeader("Content-Type")), d && (d.iO("html") || d.iO("form"));

arvgta commented 4 years ago

Above proposed change committed to:

@tchapi : Feel free to hotlink to that file and test against your use case @edito : If you feel like it, you could even check, whether there is hereby any change in XHTML file handling...

tchapi commented 4 years ago

Superb !

arvgta commented 4 years ago

@tchapi : If you find that the change does the job, I will be happy to create a new version for you...

edito commented 4 years ago

@arvgta you are right, I completely forgot about other occurences of html/form inside response text, even in JSON response content. Insane oversight.

Also I realized that it shouldn't be nice to perform indexOf search on huge string for every request, matter of performance.

Hope @tchapi would return positive feedback and that issue is resolved.

arvgta commented 4 years ago

@edito - No worries! :)

tchapi commented 4 years ago

It does work now indeed ;)

arvgta commented 4 years ago

That's good news - thanks!

I will create a new version later on.

If any of the two of you feel like it, you could check for any more MIME types, we should support... (The list mentioned above is not exhaustive - for example the "form"-like MIME types were not listed there. Maybe you could check against a more complete list)

If we are missing valid MIME types to permit, we are effectively locking-out users by mistake...

arvgta commented 4 years ago

I would like to keep this issue open for a short while, until we all agree, that we have included all salient MIME types...

tchapi commented 4 years ago

👍

arvgta commented 4 years ago

Our substring "html" seems really good - hardly any unintended matches However, "form" might - in theory - produce some unwanted matches

Can you spot any common MIME types, that we are missing?


After reading this article on form MIME types again, I could imagine specifying

instead of

... in order to avoid unwanted matches

What do you think?

arvgta commented 4 years ago

Thanks!

Done :) ("form" replaced by "form-")