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

FEATURE: Support HTML5 dialog method #1396

Merged
merged 5 commits into from Oct 24, 2018

Conversation

Projects
None yet
4 participants
@niko5
Copy link
Contributor

niko5 commented Sep 23, 2018

Update to support 'dialog' method used in HTML5 element
eg. <form method="dialog" ...>

See form submission point 20.

niko5 and others added some commits Sep 23, 2018

Include HTML5 dialog method
Update to support 'dialog' method used in HTML5 <dialog> element
eg. `<form method="dialog" ...>`
See form submission point 20.
https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#form-submission-2 

(note I am not sure if any change required  for` protected function renderCsrfTokenField()`
Update FormViewHelper.php
added space
Update FormViewHelper.php
Update to support 'dialog' method used in HTML5 <dialog> element
eg. `<form method="dialog" ...>`
See form submission point 20.
https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#form-submission-2 

(note I am not sure if any change required  for` protected function renderCsrfTokenField()`
@albe

This comment has been minimized.

Copy link
Member

albe commented Oct 4, 2018

Hey and welcome! Thanks a lot for this! I wasn't even aware there was a "dialog" method for forms.

This could probably target 4.3 instead of just master. Also a small test that checks that the "dialog" method is accepted and rendered correctly would be cool. I don't think this needs some change for the CSRF Token rendering, though.

@albe albe changed the base branch from master to 4.3 Oct 4, 2018

@albe albe changed the base branch from 4.3 to master Oct 4, 2018

@albe

This comment has been minimized.

Copy link
Member

albe commented Oct 4, 2018

This needs to be rebased on 4.3 for retargeting. You can create a new PR or just force-push to this, then change the target branch.

@niko5

This comment has been minimized.

Copy link
Contributor

niko5 commented Oct 4, 2018

@albe any examples of similar tests that I could use as a template? I did of course test it by using in my project but don't think that would be appropriate.

@albe

This comment has been minimized.

Copy link
Member

albe commented Oct 6, 2018

You can take a look at the tests in https://github.com/neos/flow-development-collection/blob/master/Neos.FluidAdaptor/Classes/ViewHelpers/FormViewHelper.php for example.

However, after reading a bit more about method "dialog", this seems to be a purely client-side tech. So the question is, if this is really needed in the Fluid Form ViewHelper, since that is supposed to help with client->server side communication. For all other cases, one can simply just add a normal <form method="dialog"...> to the Fluid Template. Or what exactly would the use-case be? Maybe we can solve this with just documenting this instead of adding code?

@niko5

This comment has been minimized.

Copy link
Contributor

niko5 commented Oct 6, 2018

I did just use a normal form element when I discovered that the Form viewhelper doesn’t support it.

However I don’t quite follow your argument as the Fluid elements do have support for JavaScript ie client side technology already. I would think adding support for this keeps Fluid form viewhelper in line with standards?

Thanks also for link to tests - I should have looked there already - I will take a look and see if I can see how I would add a test for this.

@albe

This comment has been minimized.

Copy link
Member

albe commented Oct 10, 2018

However I don’t quite follow your argument as the Fluid elements do have support for JavaScript ie client side technology already. I would think adding support for this keeps Fluid form viewhelper in line with standards?

Well, yes, but the Fluid ViewHelpers are not meant to just duplicate the available standards in a slightly different way. With that aproach, we would just end up chasing behind the living standard that HTML has become and that is an uphill battle we can only lose. So maybe it is time to take a clearer stance on that. Anyway, not trying to devalue your contribution at all, just having some thoughts about the backgrounds and how to maybe approach the issue at hand from a different perspective.

Would love to hear the thoughts of @kitsunet and maybe @bwaidelich on this

@bwaidelich

This comment has been minimized.

Copy link
Member

bwaidelich commented Oct 10, 2018

At some point it was possible to do

<f:form method="DELETE">

and it would render a hidden field with name="__method" value="delete" but apparently that was "lost in translation" with the move to TYPO3Fluid.

Anyways, for these cases we introduced the additionalAttributes argument back then.
So that should work:

<f:form additionalAttributes="{method: 'dialog'}">

Unfortunately it does not and that's the bug IMO.

So instead of adding more specific rules we could just change

if (strtolower($this->arguments['method']) === 'get') {
    $this->tag->addAttribute('method', 'get');
} else {
    $this->tag->addAttribute('method', 'post');
}

to s.th. like

if (strtolower($this->arguments['method']) === 'get') {
    $this->tag->addAttribute('method', 'get');
} elseif (!isset($this->arguments['additionalAttributes']['method'])) {
    $this->tag->addAttribute('method', 'post');
}
@albe

This comment has been minimized.

Copy link
Member

albe commented Oct 15, 2018

What if we just pass the attribute as is, unless it is empty? i.e.

if ((string)$this->arguments['method'] === '') {
    $this->tag->addAttribute('method', 'post');
} else {
    $this->tag->addAttribute('method', strtolower($this->arguments['method']));
}

I mean, currently, as of the spec (https://www.w3.org/TR/html5/sec-forms.html#element-attrdef-form-method) only the three values post, get and dialog are allowed. But: Should we enforce this? Or say "we don't care", or "we only support post, get, for anything other, don't use form VH"?

@bwaidelich

This comment has been minimized.

Copy link
Member

bwaidelich commented Oct 15, 2018

What if we just pass the attribute as is, unless it is empty?

Yes, that would be an option. The reason why this check was added is:

  • Unify the casing (no method="POST") to reduce errors when consuming the value
  • Render the hidden __method field if the method is not supported by Flow

Both not really important (especially because the 2nd was lost anyways apparently)

But I wonder.. Why use a VH at all if you just want to close a dialog?
<form method="dialog"... should just work fine!?

@niko5

This comment has been minimized.

Copy link
Contributor

niko5 commented Oct 15, 2018

using the VH with method="dialog" is still useful as you get all the background binding say to the eg. when using objectName="theFormCommand" .

@bwaidelich

This comment has been minimized.

Copy link
Member

bwaidelich commented Oct 15, 2018

using the VH with method="dialog" is still
useful as you get all the background binding

Not to argue against you (I think we should fix the bug) but FYI: You could also use the formmethod attribute on the button input

@bwaidelich
Copy link
Member

bwaidelich left a comment

(approved by reading because the change is an improvement without technical depth IMO)

@niko5

This comment has been minimized.

Copy link
Contributor

niko5 commented Oct 16, 2018

You could also use the formmethod attribute on the button input

thanks I wil take a look at that too and see how that works.

in terms of the change will this be merged? do I need to do anything else?

@albe

albe approved these changes Oct 16, 2018

Copy link
Member

albe left a comment

Fine for me. A small test would still be cool though.

@daniellienert daniellienert changed the title Support HTML5 dialog method FEATURE: Support HTML5 dialog method Oct 24, 2018

@daniellienert daniellienert merged commit f900652 into neos:master Oct 24, 2018

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment