Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
tchapi opened this issue Nov 28, 2019 · 37 comments
Closed

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

tchapi opened this issue Nov 28, 2019 · 37 comments
Labels

Comments

@tchapi
Copy link

@tchapi tchapi commented Nov 28, 2019

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

This comment has been minimized.

Copy link
Owner

@arvgta arvgta commented Nov 28, 2019

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

This comment has been minimized.

Copy link
Author

@tchapi tchapi commented Nov 28, 2019

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

This comment has been minimized.

Copy link
Owner

@arvgta arvgta commented Nov 28, 2019

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

@edito

This comment has been minimized.

Copy link
Contributor

@edito edito commented Nov 29, 2019

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

This comment has been minimized.

Copy link
Author

@tchapi tchapi commented Nov 29, 2019

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

This comment has been minimized.

Copy link
Owner

@arvgta arvgta commented Nov 29, 2019

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

This comment has been minimized.

Copy link
Contributor

@edito edito commented Nov 29, 2019

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

This comment has been minimized.

Copy link
Author

@tchapi tchapi commented Nov 29, 2019

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 ?

@tchapi tchapi changed the title Forms not working anymore in Firefox (latest) Forms not working anymore in macOS Firefox (latest) Nov 29, 2019
@edito

This comment has been minimized.

Copy link
Contributor

@edito edito commented Nov 29, 2019

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

This comment has been minimized.

Copy link
Owner

@arvgta arvgta commented Nov 29, 2019

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

This comment has been minimized.

Copy link
Author

@tchapi tchapi commented Nov 29, 2019

I tested a bit more :

  • prefetchoff: true makes no difference
  • I tested on a virtual Win 7 machine : exactly the same behavior than on the macOS host : Chrome OK (all cases), FF OK only if line commented, else NOK
@arvgta

This comment has been minimized.

Copy link
Owner

@arvgta arvgta commented Nov 29, 2019

@tchapi

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

if (!pre) location.href = hin;

...it works on all browsers, right?

@tchapi

This comment has been minimized.

Copy link
Author

@tchapi tchapi commented Nov 29, 2019

That's exact

@tchapi

This comment has been minimized.

Copy link
Author

@tchapi tchapi commented Nov 29, 2019

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

This comment has been minimized.

Copy link
Owner

@arvgta arvgta commented Nov 29, 2019

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

This comment has been minimized.

Copy link
Author

@tchapi tchapi commented Nov 29, 2019

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

This comment has been minimized.

Copy link
Owner

@arvgta arvgta commented Nov 29, 2019

@tchapi

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

@tchapi

This comment has been minimized.

Copy link
Author

@tchapi tchapi commented Nov 29, 2019

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

@arvgta

This comment has been minimized.

Copy link
Owner

@arvgta arvgta commented Nov 29, 2019

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

This comment has been minimized.

Copy link
Contributor

@edito edito commented Nov 29, 2019

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

This comment has been minimized.

Copy link
Owner

@arvgta arvgta commented Nov 29, 2019

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

This comment has been minimized.

Copy link
Owner

@arvgta arvgta commented Dec 3, 2019

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

This comment has been minimized.

Copy link
Contributor

@edito edito commented Dec 3, 2019

@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

This comment has been minimized.

Copy link
Owner

@arvgta arvgta commented Dec 3, 2019

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 responseText for 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:

  • d.iO("text/xml")

It should rather be something like:

  • d.iO("xhtml")

...which specifies it cleary...

@tchapi

This comment has been minimized.

Copy link
Author

@tchapi tchapi commented Dec 3, 2019

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

This comment has been minimized.

Copy link
Owner

@arvgta arvgta commented Dec 3, 2019

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

This comment has been minimized.

Copy link
Owner

@arvgta arvgta commented Dec 3, 2019

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

This comment has been minimized.

Copy link
Author

@tchapi tchapi commented Dec 3, 2019

Superb !

@arvgta

This comment has been minimized.

Copy link
Owner

@arvgta arvgta commented Dec 3, 2019

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

@edito

This comment has been minimized.

Copy link
Contributor

@edito edito commented Dec 3, 2019

@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

This comment has been minimized.

Copy link
Owner

@arvgta arvgta commented Dec 4, 2019

@edito - No worries! :)

@tchapi

This comment has been minimized.

Copy link
Author

@tchapi tchapi commented Dec 4, 2019

It does work now indeed ;)

@arvgta

This comment has been minimized.

Copy link
Owner

@arvgta arvgta commented Dec 4, 2019

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

This comment has been minimized.

Copy link
Owner

@arvgta arvgta commented Dec 4, 2019

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

This comment has been minimized.

Copy link
Author

@tchapi tchapi commented Dec 4, 2019

👍

@arvgta

This comment has been minimized.

Copy link
Owner

@arvgta arvgta commented Dec 5, 2019

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

  • "form-"

instead of

  • "form"

... in order to avoid unwanted matches

What do you think?

@arvgta

This comment has been minimized.

Copy link
Owner

@arvgta arvgta commented Dec 5, 2019

Thanks!

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

@arvgta arvgta closed this Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.