vaadin / framework

Vaadin 6, 7, 8 is a Java framework for modern Java web applications.
http://vaadin.com/
Other
1.78k stars 730 forks source link

Window.executeJavaScript() #877

Closed vaadin-bot closed 14 years ago

vaadin-bot commented 15 years ago

Originally by @jojule


Add possibility of executing javascript snippets from server. This should be added as an API in the Window -class.

Something like:

myWindow.executeJavaScript("alert('foo');"); 

We should also provide references to DOM elements like this:

myWin.executeJavaScript("$1.style.backgroundColor='red';$2.style.backgroundColor='blue'", 
aTextFieldinMyWin, someOtherComponentInMyWin);

(In the above example the references would be to the outmost DOM element in the widget that implements the client counterpart for the given server-side UI component)

This probably should have some relation to #3067


Imported from https://dev.vaadin.com/ issue #3589

vaadin-bot commented 15 years ago

Originally by dll


Voting +1 for this feature. I think, it would be very useful, because to my experience, in almost every huge project such requirement (to do something non-standrad with JS) raises up. Joonas's proposal looks exactly what is needed.

vaadin-bot commented 15 years ago

Originally by @Peppe


Even if I was at first against this I would say this would be a nice idea. It should be a possiblity for the most special cases where you need Javascript, but not promoted way of doing things.

vaadin-bot commented 14 years ago

Originally by Jani Laakso


Marko, Please document this method to Book of Vaadin. Marko, see also #3601

vaadin-bot commented 14 years ago

Originally by syam


My vote too.

vaadin-bot commented 14 years ago

Originally by @jojule


Jani, why should this be documented before it is implemented?

vaadin-bot commented 14 years ago

Originally by Jani Laakso


Of course documentation comes after implementation is finished. This is just an idea for Marko, perhaps Marko would like to check "Affects Documentation" or add himself as cc to this ticket?

vaadin-bot commented 14 years ago

Originally by @magi42


Please create a separate documentation ticket after fixed.

vaadin-bot commented 14 years ago

Originally by @jojule


Implemented in [9658] and merged back to 6.2 in [9659]

vaadin-bot commented 14 years ago

Originally by @samie


The second example is something that does actually break the way Vaadin is designed: Server-side Java, client-side Java.

If not too late, I'd suggest that the component referencing feature is left out from the official release at this point.

I voting for that in cases where you need to call external JavaScript it should not require any hacks using Label or CustomLayout (the case in example 1).

In case you wish to modify Vaadin widgets somehow, that should be made in Java+GWT and CSS at logical level. Based on this example 2 this feature would encourage to break that abstraction altogether.

vaadin-bot commented 14 years ago

Originally by @jojule


What is so bad in letting developers to get their hands dirty with javascript when they really really want to?

Overusing this API is by no means the correct way of developing things, but prohibiting the developers from writing a bit of javascript (that can modify the DOM tree) when there really is no better way of achieving what they want to do is just evil. And as appearance of extensions like http://dev.vaadin.com/svn/incubator/NativeJS/ show that developers will be doing this whether core framework supports it or not.

To summarize - IMO we should NOT purposely make developers life harder by forcing (a good) abstraction layer and server driven architecture. Making bypassing abstraction easier is not same as breaking architecture - we just give developers tools and freedom to break the abstraction when they know what they are doing.

I did not understand the comment "cases where you need to call external JavaScript it should not require any hacks using Label or CustomLayout? (the case in example 1)". The sole purpose of this new API is to remove the need for hacks.

vaadin-bot commented 14 years ago

Originally by @samie


Don't intend to reopen this anymore (let you to do it). My point was missed, so to clarify my statements above:

There is a big difference just calling JS or allowing Vaadin component DOM referencing the way this API also allows.

First, there is nothing (or minor) bad letting users to call JS like in example 1. This removes the need for hacks as you stated. Vote for simply this:

public void executeJavaScript(String script)

However, it is a bad practice to modify Vaadin widgets DOM like in example 2. This will require one to know about DOM structure of widgets and makes server-side implementations dependent on widget client-side implementation - thus breaking the abstraction. Furthermore, I've not seen anyone asking for this feature.

IMO, better practice would be that if something needs to access a widget DOM, that is made with Java at client-side/GWT, not at the server-side with JS.

I suggest to rethink the need for paintable DOM referencing:

public void executeJavaScript(String script, Paintable... paintables)

before making this API official. It is always possible to add it later on, iff it is really needed.

vaadin-bot commented 14 years ago

Originally by @jojule


Added a warning to javadocs discouraging the use of the method. Also cleaned up one javascript variable from window. [9674].

Sami, I continue to disagree - it is frustrates the developers if we force them to abuse debug ids to get hold of DOM elements. In most cases it is much better to write a GWT-based widget, but when a developer just needs to do a really little thing in DOM, knows what he is doing and accepts the responsibility that things might blow up - we should let him do it. (He will do it in any case, but if we make it harder than necessary he won't be too pleased).

vaadin-bot commented 14 years ago

Originally by @jojule


Some added examples, tests and correct javascript execution context added in [9678] and [9679].

vaadin-bot commented 14 years ago

Originally by @jojule


Included some use-cases in http://dev.vaadin.com/browser/versions/6.2/tests/src/com/vaadin/tests/tickets/Ticket3589.java for the ones wondering why is this API even needed, Admittedly, all of the listed use-cases and "hacks" and should implemented properly with client-side widgets or custom themes, but the "correct" implementations would be much more laborious to implement (especially for the ones who do not know how to implement them).

vaadin-bot commented 14 years ago

Originally by @samie


In general, there should not be any need to "get hold of DOM elements" at the server-side as it does not know about the rendering (consider button or select with different styles). Using this method to manipulate Vaadin widgets should really be avoided.

Make sure this feature does not get used in wrong way and document the valid uses cases in #3601

vaadin-bot commented 14 years ago

Originally by @jojule


I agree that we should explicitly discourage the use of this API in documentation, explain why breaking through abstraction layers is bad and point to better solutions.

vaadin-bot commented 14 years ago

Originally by @mstahv


I have already noted my concerns about this ticket directly to Joonas, but I'll type them here as a memo too.

Why I would drop the second method:

About the fixes I suggested on Friday (and now in trunk partially).

debugger; var str # document.body.textContent; strstr.replace(/(java)script/gi, 'pure $1 with Vaadin'); $1.innerHTML = str;


 - subwindows case should be handled somehow
vaadin-bot commented 14 years ago

Originally by dll


I'd like to put my vote to the option #1 - e.g. to be able to execute arbitrary JS code. And this is actually not to hack Vaadin client side but really often needed for non-standard integrations. Also, in some cases execute JS code is really much simplier to write a new widget for this.

As for the option #2 for accessing DOM tree of Vaadin widgets - I do agree that this is less needed and make a new client widget for such usecases is more preferable.

So, please, please, do not kill the executeJS method

vaadin-bot commented 14 years ago

Originally by @jojule


I think that pretty much everyone agrees that the basic Window.executeJavaScript(String) without DOM references is needed. Also there has not been any discussions/critique for the method name or placing.

There is still discussion going on two design aspects: 1) Should we make it easy to refer to the DOM elements that implement UI components 2) If so, should we use $1, $2, ... in the script to mark those references

IMO the second aspect is not too controversial. If there is a better proposal than $1, $2, this is easy to change and arguments in favor of changing this are valid. On the other hand - the problem might not be too bad as the developer can freely use $1 when there are no arguments and can call executeJavaScript(..) multiple times in sequence. Any proposals?

The real question is DOM references.

Arguments in favor of removing DOM references from API:

Arguments in favor of keeping DOM references in API:

I have no new arguments to this debate. My proposal and opinion is already committed to SVN. If someone still have new arguments to this discussions, add them here. In the end Artur should make the decision on what to do with these two questions.

vaadin-bot commented 14 years ago

Originally by @jojule


Reposting to correct formatting...

I think that pretty much everyone agrees that the basic Window.executeJavaScript(String) without DOM references is needed. Also there has not been any discussions/critique for the method name or placing.

There is still discussion going on two design aspects: 1) Should we make it easy to refer to the DOM elements that implement UI components 2) If so, should we use $1, $2, ... in the script to mark those references

IMO the second aspect is not too controversial. If there is a better proposal than $1, $2, this is easy to change and arguments in favor of changing this are valid. On the other hand - the problem might not be too bad as the developer can freely use $1 when there are no arguments and can call executeJavaScript(..) multiple times in sequence. Any proposals?

The real question is DOM references.

Arguments in favor of removing DOM references from API:

Arguments in favor of keeping DOM references in API:

I have no new arguments to this debate. My proposal and opinion is already committed to SVN. If someone still have new arguments to this discussions, add them here. In the end Artur should make the decision on what to do with these two questions.

vaadin-bot commented 14 years ago

Originally by @Artur-


We should provide the possibility to execute javascript from server side.

I think the print example provided (javadoc example #4) is invalid as there is a way of opening new windows in Vaadin without custom javascript. Printing is even documented in the Book of Vaadin http://vaadin.com/book/-/page/advanced.printing.html but it is really ugly. The problem is not missing DOM references but the absence of executeJS() which adds the need for Label/CustomLayout hacks to execute "window.print()". If you really want to print raw HTML from a Label you can pass the HTML in the javascript string (probably you don't as you need a doctype, body element, css etc to make the print version look nice).

If you want to make visual effects I think you really want to use a js library like JQuery. All JQuery examples I have seen so far (admittedly not very many) use id references so for that the DOM references are probably not needed. Doing visual effects without a library the code is often more complex than a few lines of javascript code, see e.g. http://dev.vaadin.com/browser/incubator/Blink/src/com/vaadin/incubator/blink/client/ui/VBlink.java which basically "blinks the widget in red for 500ms".

Changing appearances permanently for a widget is not possible using executeJS() as changes are lost on a refresh or widget update so the "use example 3" in the javadoc is confusing.

I see here that a majority thinks the DOM references are unnecessary and in

3601 everybody agrees that the DOM references should not be documented

in the Book of Vaadin. The debugId can be used for getting the hands on an element if a quick hack needs that. The debugId should regardless of this be documented.

Documenting in javadoc with examples and leaving out the documentation from the Book seems like a really bad idea. Also adding a new feature and documenting that it should not be used makes little sense.

So, let's remove the custom DOM references and let users use debugId for making standard DOM references if they really want to. If there is a huge demand for this feature we can implement it at a later stage.

vaadin-bot commented 14 years ago

Originally by @jojule


Agreed. I'll do the changes.

vaadin-bot commented 14 years ago

Originally by @Artur-


Removed DOM references and renamed test case to com.vaadin.tests.components.window.ExecuteJavaScript in [9746].