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

onclick should not always have event.preventDefault() #355

Closed
pec0ra opened this issue Jan 16, 2020 · 25 comments
Closed

onclick should not always have event.preventDefault() #355

pec0ra opened this issue Jan 16, 2020 · 25 comments

Comments

@pec0ra
Copy link
Contributor

pec0ra commented Jan 16, 2020

When adding an onclick listener to a widget, it adds the following onclick attributes to the html object:

sendCallback('XXXXXXXX','onclick');event.stopPropagation();event.preventDefault();

The event.preventDefault() part causes issues if you put any widget with a "default" action like a checkbox or a link inside a container which has an onclick listener. The default action will be stopped at the parent and the checkbox, link, etc won't work.

I can understand why the event.preventDefault() makes sense. If you add an onclick listener directly on a link, checkbox, etc, you probably want it to replace its default action. It would however be nice to have the option to skip it when adding onclick listeners to containers.

My example use case

I have created a Modal widget which consists of a backdrop, a modal and a content (code can be found here).

The backdrop has an onclick listener to close the modal on click.

The modal has a dummy onclick listener which does nothing but stopping the event propagation (otherwise clicking on the modal would trigger the backdrop listener).

The content has a form with inputs and links. The checkboxes and links don't work since the parents (both modal and backdrop) have event.preventDefault() in their onclick attribute.

A temporary hack that I found to overcome this issue is to re-implement checkbox functionality in an onclick listener:

checkbox.onclick.do(lambda w: checkbox.set_value(not checkbox.get_value()))

but this is not very practical.

@dddomodossola
Copy link
Collaborator

dddomodossola commented Jan 17, 2020

Hello @pec0ra ,

event.preventDefault() prevents an event to be dispatched to parents and not children, as far as I know. And so it should not cause the problem you are getting.
Here is an example where both the container and the contained button have an onclick listener:

    # -*- coding: utf-8 -*-

    from editor import *
    from remi.gui import *
    from remi import start, App

    """
        Created with the Online Remi Editor at https://remiguieditor.daviderosa.repl.co/
    """

    class untitled(App):
        def __init__(self, *args, **kwargs):
            #DON'T MAKE CHANGES HERE, THIS METHOD GETS OVERWRITTEN WHEN SAVING IN THE EDITOR
            if not 'editing_mode' in kwargs.keys():
                super(untitled, self).__init__(*args, static_file_path={'my_res':'./res/'})

        def idle(self):
            #idle function called every update cycle
            pass
        
        def main(self):
            return untitled.construct_ui(self)
            
        @staticmethod
        def construct_ui(self):
            #DON'T MAKE CHANGES HERE, THIS METHOD GETS OVERWRITTEN WHEN SAVING IN THE EDITOR
            vbox0 = VBox()
            vbox0.attr_editor = False
            vbox0.attr_editor_newclass = False
            vbox0.css_align_items = "center"
            vbox0.css_display = "flex"
            vbox0.css_flex_direction = "column"
            vbox0.css_height = "250px"
            vbox0.css_justify_content = "space-around"
            vbox0.css_left = "20px"
            vbox0.css_margin = "0px"
            vbox0.css_position = "absolute"
            vbox0.css_top = "20px"
            vbox0.css_width = "250px"
            vbox0.identifier = "140356290501456"
            button0 = Button()
            button0.attr_editor = False
            button0.attr_editor_newclass = False
            button0.css_height = "30px"
            button0.css_left = "None"
            button0.css_margin = "0px"
            button0.css_order = "140356281306000"
            button0.css_position = "static"
            button0.css_right = "None"
            button0.css_top = "20px"
            button0.css_width = "100px"
            button0.identifier = "140356281306000"
            button0.text = "button"
            vbox0.append(button0,'button0')
            vbox0.onclick.do(self.onclick_vbox0)
            vbox0.children['button0'].onclick.do(self.onclick_button0)
            

            self.vbox0 = vbox0
            return self.vbox0
        
        def onclick_vbox0(self, emitter):
            self.vbox0.css_background_color = "red"

        def onclick_button0(self, emitter):
            self.vbox0.css_background_color = "green"



    #Configuration
    configuration = {'config_project_name': 'untitled', 'config_address': '0.0.0.0', 'config_port': 8081, 'config_multiple_instance': True, 'config_enable_file_cache': True, 'config_start_browser': True, 'config_resourcepath': './res/'}

    if __name__ == "__main__":
        # start(MyApp,address='127.0.0.1', port=8081, multiple_instance=False,enable_file_cache=True, update_interval=0.1, start_browser=True)
        start(untitled, address=configuration['config_address'], port=configuration['config_port'], 
                            multiple_instance=configuration['config_multiple_instance'], 
                            enable_file_cache=configuration['config_enable_file_cache'],
                            start_browser=configuration['config_start_browser'])

and it works correctly. There should be an error in your code. I will try it today.

Regards

@pec0ra
Copy link
Contributor Author

pec0ra commented Jan 17, 2020

Thank you for your answer.

What I meant by "default action" is a html action like for example opening a link (not a javascript onclick action).

Your code does work but if you replace your button by a link:

        button0 = Link("https://google.com", "Link")

(and optionally remove its onclick listener), the link will not work. (Same for a checkbox and probably other html elements)

Using my remi fork (pip install git+https://github.com/pec0ra/remi) which removes the preventDefault() does however work.

BTW: Here is some documentation about event.preventDefault(): https://www.w3schools.com/jsref/event_preventdefault.asp

@dddomodossola
Copy link
Collaborator

Thank you a lot for reporting, I was not aware about this. Let me think about a solution, I'm not sure about making preventDefault optional (maybe it is the best solution, but I have to think about it).

@dddomodossola
Copy link
Collaborator

What do you think about replacing the default onclick listener for Link and CheckBox ? This way the user have to take no actions, and all should work, independently if the parent widget has preventDefault or not.

@pec0ra
Copy link
Contributor Author

pec0ra commented Jan 19, 2020

Do you mean implementing these "default actions" ourselves, like opening the link with javascript?

If you mean this, then I wouldn't recommend it.
The list of elements with default actions is not limited to links and checkboxes (e.g. submitting forms with buttons). Finding an exhaustive list of these element might be difficult and we might find elements that are both containers and have default actions.
Elements might also have some extra functionality like target="_blank" which wouldn't work out of the box.
In the end, the code might behave unexpectedly if people use custom elements or attributes for their widgets.

Here are a few possibilities that I see to solve the issue:

Keep it as it is:
It works for most usages so keeping the event.preventDefault() is of course not so dramatic.
I would in this case document this, explain to the developers how it behaves and potential hacks to solve it.
If someone has this particular issue, he then could create custom widgets like this to overcome the problem:

class InnerCheckbox(gui.Checkbox):
    """
    A Checkbox which works inside a container with onclick listener
    """
    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self.onclick.do(lambda w: self.set_value(not self.get_value()))

I prefer this option over replacing the default action as a default because it keeps the html working as normal html unless the user is in this special case.

Remove event.preventDefault() everywhere`
It would fix this issue but then people would not be able to "replace" the default action by an onclick listener.
I don't think this is a good solution

Make it optional
If think this is the cleanest way.

It could for example be a parameter in the events' do(callback) methods. We could set the default to have the event.preventDefault() (for retro-compatibility) and add the option to remove it by a call like this:

widget.onclick.do(callback, keep_default_action=True)

Note that I would find it even better to have the opposite: not having event.preventDefault() and adding it by a parameter prevent_default=True but some code using the library might already rely on the event.preventDefault() being there and such code wouldn't work any more.

I haven't looked into the technical details so I might be wrong but I believe this could be easily implemented by simply adding a third placeholder inside decorate_event_js' value.

Let me know what you think.

@mcondarelli
Copy link
Contributor

I don't know how common this scenario is, but I hit the wall just by putting a FileDownloader in a TableWidget cell.

A (rather short) analysis shows onclick event is activated on all table cells in order to support "click-on-row" even if nobody uses it, "just in case...".

This seems a quite special case completely outside user control... and makes a perfectly legal link almost completely unusable (you can activate it by right-click -> Save link with name...).

As this is a rather special-case (the under-the-hood onclick listener, I mean) I think it should be treated as such and remove event.preventDefault() just in this case (and, possibly, in similar occurrences).
I.e.: if the user sets the hook then it's right to stop processing, if the library sets a hook processing shouldn't be stopped.

Did I miss some relevant point?

@pec0ra
Copy link
Contributor Author

pec0ra commented Jan 20, 2020

What do you think about replacing the default onclick listener for Link and CheckBox ?

Or maybe you meant just adding event.stopPropagation() to all links, checkboxes, buttons,etc... This would also work since the event is not propagated up to the parent which has the event.preventDefault().
It would fix my issue and @mcondarelli 's while keeping the old behaviour. If the user adds a new onclick listener to one of these element, it would overwrite the old one (event.stopPropagation()) with a new one which also has event.preventDefault() and therefore "replace" the default action.

We would still need to find all elements that have a default action for any event though.

By the way, I have just written a little proof of concept for optional event.preventDefault() and it is indeed as easy to implement as I thought.

@dddomodossola
Copy link
Collaborator

@pec0ra

Replacing the default action with event.stopPropagation is exactly what I mean.
There are some points that makes me unsure about the optional preventDefault:

  1. In case of multiple nested containers, the keep_default_action=True must be set for all parents, and this could be not practical (suppose a container with a table and rows and cells, with inside a link, you should set that parameters for all the parents of the link);
  2. The developer must be aware of that option, and this requires knowledge. First approching developers can experience headaches before making it working;
  3. A most complex implementation in respect to the event.stopPropagation.

What do you think about these points?
Actually I would prefer replacing the default action.

@pec0ra
Copy link
Contributor Author

pec0ra commented Jan 20, 2020

Here is a quick comment about your points:

  1. I agree with you. I guess the solution to this would be to deactivate per default event.preventDefault() and have it optional. The downside is that this would be a breaking change (I don't know how many people rely on this event.preventDefault() though).
  2. Having event.preventDefault() on all elements with onclick might also create headaches. Whatever the decision you take, we should try to document it so that the devs quickly find a solution. Hopefully this github issue will also be well referenced on google so that the devs might find it.
  3. You probably have a typo here but, as I said, the optional preventDefault shouldn't be too complicated to implement.

Actually I would prefer replacing the default action.

Whatever your decision is, let me know if you need help implementing it.

@dddomodossola
Copy link
Collaborator

Today I'm really busy at work. However I would like to replace the default action. I suppose to do in widgets constructor (Link, CheckBox):

self.attributes[Widget.EVENT_ONCLICK]` = "event.stopPropagation()"

If you have the time to make and test the fix, I will be happy to merge a pull request. Otherwise no problem, I will make it soon.

@pec0ra
Copy link
Contributor Author

pec0ra commented Jan 20, 2020

Ok, I will have a look at it.

I could already successfully try your fix for onclick events on links and checkboxes but I will try to see if I can find a list of all possible actions for all possible event. There seem to be a lot of them. For example keydown would block writing in an input, mousedown might block text selection or focus on inputs or touchmove might prevent scrolling on mobile devices.

@mcondarelli
Copy link
Contributor

Is there anything I can do on my code to work around the specific (link in TableItem) problem I have?
I would like to be able to use "stock RemI".

@dddomodossola
Copy link
Collaborator

@mcondarelli you will get the fix released in remi at least Tomorrow ;-)

A quick fix would be to do on all your link objects mylink[Widget.EVENT_ONCLICK] = "event.stopPropagation()"

@mcondarelli
Copy link
Contributor

@dddomodossola: I'm not sure I follow you:

                    d = datetime.date.fromisoformat(m.group(1))
                    b, _ = path.splitext(f)
                    g = path.join(self.base, b + '.rsp_')
                    if path.isfile(g):
                        rsp = FileDownloader('Risposta A. E.', g)
                        rsp[Widget.EVENT_ONCLICK] = "event.stopPropagation()"
                    elif path.isfile(path.join(self.base, b + '.rko_')):
                        rsp = 'ERRORE da A.E.'
                    else:
                        rsp = 'NON ANCORA INVIATO'
                    dl = FileDownloader(b, path.join(self.base, f))
                    dl[Widget.EVENT_ONCLICK] = "event.stopPropagation()"
                    lst.append({'fil': dl, 'dat': d.strftime('%d %B %Y'), 'nch': m.group(2), 'rsp': rsp})

results in:

remi.server.ws   ERROR    error parsing websocket
Traceback (most recent call last):
  File "/home/mcon/.virtualenvs/Client/lib/python3.7/site-packages/remi/server.py", line 261, in on_message
    callback(**param_dict)
  File "/home/mcon/.virtualenvs/Client/lib/python3.7/site-packages/remi/gui.py", line 163, in __call__
    return self.callback(self.event_source_instance, *callback_params, **self.kwuserdata)
  File "/home/mcon/vocore/Applicazione/GUI2/RGUIxgestore.py", line 93, in _corrispettivi
    rsp[Widget.EVENT_ONCLICK] = "event.stopPropagation()"
TypeError: 'FileDownloader' object does not support item assignment

Should I useLink() instead of FileDownloader()?

Note: reason why I ask is because I can update my code, but I would like to avoid to have users to update their systems (and RemI with it).

@pec0ra
Copy link
Contributor Author

pec0ra commented Jan 20, 2020

The .attributes is missing, this was probably a typo. You should use:

dl.attributes[Widget.EVENT_ONCLICK] = "event.stopPropagation()"

@dddomodossola
Copy link
Collaborator

dddomodossola commented Jan 20, 2020

@pec0ra

I found these interesting information https://javascript.info/default-browser-action

However I tried to put a TextInput inside a container, registered an onclick listener for the container (so having preventDefault), and the text is selectable (it works). I don't understand why. It should be prevented.. I did some further tests, a simple example about TextInput, CheckBox, Link, Slider, Date, List:

# -*- coding: utf-8 -*-

from remi.gui import *
from remi import start, App


class untitled(App):
    def __init__(self, *args, **kwargs):
        #DON'T MAKE CHANGES HERE, THIS METHOD GETS OVERWRITTEN WHEN SAVING IN THE EDITOR
        if not 'editing_mode' in kwargs.keys():
            super(untitled, self).__init__(*args, static_file_path={'my_res':'./res/'})

    def idle(self):
        #idle function called every update cycle
        pass
    
    def main(self):
        return untitled.construct_ui(self)
        
    @staticmethod
    def construct_ui(self):
        #DON'T MAKE CHANGES HERE, THIS METHOD GETS OVERWRITTEN WHEN SAVING IN THE EDITOR
        vbox0 = VBox()
        vbox0.attr_editor_newclass = False
        vbox0.css_align_items = "flex-start"
        vbox0.css_display = "flex"
        vbox0.css_flex_direction = "column"
        vbox0.css_height = "350px"
        vbox0.css_justify_content = "space-between"
        vbox0.css_position = "absolute"
        vbox0.variable_name = "vbox0"
        hbox0 = HBox()
        hbox0.attr_editor_newclass = False
        hbox0.css_align_items = "center"
        hbox0.css_display = "flex"
        hbox0.css_flex_direction = "row"
        hbox0.css_justify_content = "space-around"
        hbox0.css_margin = "0px"
        hbox0.css_order = "2185000297080"
        hbox0.css_position = "static"
        hbox0.variable_name = "hbox0"
        label0 = Label()
        label0.attr_editor_newclass = False
        label0.css_height = "30px"
        label0.css_margin = "0px"
        label0.css_order = "2185025997008"
        label0.css_position = "static"
        label0.css_top = "20px"
        label0.css_width = "100px"
        label0.text = "TextInput"
        label0.variable_name = "label0"
        hbox0.append(label0,'label0')
        textinput0 = TextInput()
        textinput0.attr_editor_newclass = False
        textinput0.css_height = "30px"
        textinput0.css_margin = "0px"
        textinput0.css_order = "2185025949312"
        textinput0.css_position = "static"
        textinput0.css_top = "20px"
        textinput0.css_width = "100px"
        textinput0.text = ""
        textinput0.variable_name = "textinput0"
        hbox0.append(textinput0,'textinput0')
        vbox0.append(hbox0,'hbox0')
        hbox1 = HBox()
        hbox1.attr_editor_newclass = False
        hbox1.css_align_items = "center"
        hbox1.css_display = "flex"
        hbox1.css_flex_direction = "row"
        hbox1.css_justify_content = "space-around"
        hbox1.css_margin = "0px"
        hbox1.css_order = "2185043903320"
        hbox1.css_position = "static"
        hbox1.variable_name = "hbox1"
        label1 = Label()
        label1.attr_editor_newclass = False
        label1.css_height = "30px"
        label1.css_margin = "0px"
        label1.css_order = "2185080177328"
        label1.css_position = "static"
        label1.css_top = "20px"
        label1.css_width = "100px"
        label1.text = "CheckBox"
        label1.variable_name = "label1"
        hbox1.append(label1,'label1')
        checkbox0 = CheckBox()
        checkbox0.attr_editor_newclass = False
        checkbox0.css_height = "30px"
        checkbox0.css_margin = "0px"
        checkbox0.css_order = "2185056679528"
        checkbox0.css_position = "static"
        checkbox0.css_top = "20px"
        checkbox0.css_width = "30px"
        checkbox0.variable_name = "checkbox0"
        hbox1.append(checkbox0,'checkbox0')
        vbox0.append(hbox1,'hbox1')
        hbox2 = HBox()
        hbox2.attr_editor_newclass = False
        hbox2.css_align_items = "center"
        hbox2.css_display = "flex"
        hbox2.css_flex_direction = "row"
        hbox2.css_justify_content = "space-around"
        hbox2.css_order = "2185101175272"
        hbox2.css_position = "static"
        hbox2.variable_name = "hbox2"
        label2 = Label()
        label2.attr_editor_newclass = False
        label2.css_height = "30px"
        label2.css_margin = "0px"
        label2.css_order = "2185128892232"
        label2.css_position = "static"
        label2.css_top = "20px"
        label2.css_width = "100px"
        label2.text = "Link"
        label2.variable_name = "label2"
        hbox2.append(label2,'label2')
        link0 = Link()
        link0.attr_editor_newclass = False
        link0.attr_href = "http://www.google.it"
        link0.css_height = "30px"
        link0.css_margin = "0px"
        link0.css_order = "2185129134848"
        link0.css_position = "static"
        link0.css_top = "20px"
        link0.css_width = "100px"
        link0.text = "google"
        link0.variable_name = "link0"
        hbox2.append(link0,'link0')
        vbox0.append(hbox2,'hbox2')
        hbox3 = HBox()
        hbox3.attr_editor_newclass = False
        hbox3.css_align_items = "center"
        hbox3.css_display = "flex"
        hbox3.css_flex_direction = "row"
        hbox3.css_justify_content = "space-around"
        hbox3.css_order = "2185151999112"
        hbox3.css_position = "static"
        hbox3.variable_name = "hbox3"
        label3 = Label()
        label3.attr_editor_newclass = False
        label3.css_height = "30px"
        label3.css_margin = "0px"
        label3.css_order = "2185175812360"
        label3.css_position = "static"
        label3.css_top = "20px"
        label3.css_width = "100px"
        label3.text = "Slider"
        label3.variable_name = "label3"
        hbox3.append(label3,'label3')
        slider0 = Slider()
        slider0.attr_editor_newclass = False
        slider0.css_height = "30px"
        slider0.css_margin = "0px"
        slider0.css_order = "2185201046584"
        slider0.css_position = "static"
        slider0.css_top = "20px"
        slider0.css_width = "100px"
        slider0.variable_name = "slider0"
        hbox3.append(slider0,'slider0')
        vbox0.append(hbox3,'hbox3')
        hbox4 = HBox()
        hbox4.attr_editor_newclass = False
        hbox4.css_align_items = "center"
        hbox4.css_display = "flex"
        hbox4.css_flex_direction = "row"
        hbox4.css_justify_content = "space-around"
        hbox4.css_order = "2185206672520"
        hbox4.css_position = "static"
        hbox4.variable_name = "hbox4"
        label4 = Label()
        label4.attr_editor_newclass = False
        label4.css_height = "30px"
        label4.css_margin = "0px"
        label4.css_order = "2185165391632"
        label4.css_position = "static"
        label4.css_top = "20px"
        label4.css_width = "100px"
        label4.text = "Date"
        label4.variable_name = "label4"
        hbox4.append(label4,'label4')
        date0 = Date()
        date0.attr_editor_newclass = False
        date0.css_height = "30px"
        date0.css_margin = "0px"
        date0.css_order = "2185243332000"
        date0.css_position = "static"
        date0.css_top = "20px"
        date0.css_width = "100px"
        date0.variable_name = "date0"
        hbox4.append(date0,'date0')
        vbox0.append(hbox4,'hbox4')
        hbox5 = HBox()
        hbox5.attr_editor_newclass = False
        hbox5.css_align_items = "center"
        hbox5.css_display = "flex"
        hbox5.css_flex_direction = "row"
        hbox5.css_justify_content = "space-around"
        hbox5.css_order = "2185276576432"
        hbox5.css_position = "static"
        hbox5.variable_name = "hbox5"
        label5 = Label()
        label5.attr_editor_newclass = False
        label5.css_height = "30px"
        label5.css_margin = "0px"
        label5.css_order = "2185243737504"
        label5.css_position = "static"
        label5.css_top = "20px"
        label5.css_width = "100px"
        label5.text = "List"
        label5.variable_name = "label5"
        hbox5.append(label5,'label5')
        listview0 = ListView()
        listview0.attr_editor_newclass = False
        listview0.css_height = "100px"
        listview0.css_margin = "0px"
        listview0.css_order = "2185254699248"
        listview0.css_position = "static"
        listview0.css_top = "20px"
        listview0.css_width = "100px"
        listview0.variable_name = "listview0"
        listitem0 = ListItem()
        listitem0.attr_editor_newclass = False
        listitem0.css_margin = "0px"
        listitem0.text = "list item1"
        listitem0.variable_name = "listitem0"
        listview0.append(listitem0,'listitem0')
        listitem1 = ListItem()
        listitem1.attr_editor_newclass = False
        listitem1.css_margin = "0px"
        listitem1.text = "list item2"
        listitem1.variable_name = "listitem1"
        listview0.append(listitem1,'listitem1')
        listitem2 = ListItem()
        listitem2.attr_editor_newclass = False
        listitem2.css_margin = "0px"
        listitem2.text = "list item3"
        listitem2.variable_name = "listitem2"
        listview0.append(listitem2,'listitem2')
        listitem3 = ListItem()
        listitem3.attr_editor_newclass = False
        listitem3.css_margin = "0px"
        listitem3.text = "list item4"
        listitem3.variable_name = "listitem3"
        listview0.append(listitem3,'listitem3')
        hbox5.append(listview0,'listview0')
        vbox0.append(hbox5,'hbox5')
        vbox0.onclick.do(self.onclick_vbox0)
        

        self.vbox0 = vbox0
        return self.vbox0
    
    def onclick_vbox0(self, emitter):
        pass



#Configuration
configuration = {'config_project_name': 'untitled', 'config_address': '0.0.0.0', 'config_port': 8081, 'config_multiple_instance': True, 'config_enable_file_cache': True, 'config_start_browser': True, 'config_resourcepath': './res/'}

if __name__ == "__main__":
    # start(MyApp,address='127.0.0.1', port=8081, multiple_instance=False,enable_file_cache=True, update_interval=0.1, start_browser=True)
    start(untitled, address=configuration['config_address'], port=configuration['config_port'], 
                        multiple_instance=configuration['config_multiple_instance'], 
                        enable_file_cache=configuration['config_enable_file_cache'],
                        start_browser=configuration['config_start_browser'])

Only the Link has the problem. The other known problematic default event is for FileDownloader as mentioned by @mcondarelli .
Tried also the FileUploader, it has the problem too.
So the problematic widgets are: Link, FileDownload, FileUploader.. (others to be tested)

@pec0ra
Copy link
Contributor Author

pec0ra commented Jan 20, 2020

@dddomodossola

It works because other elements have default actions for other events, not onclick.
Try adding an onmousedown listener and nothing works anymore. onkeydown will also block some functionalities.

vbox0.onmousedown.do(self.onclick_vbox0)
vbox0.onkeydown.do(self.onclick_vbox0)

I would suggest that you reconsider having the event.stopPropagation() and event.preventDefault() everywhere. Did you have a good reason to put them everywhere? They change how html and javascript normally work and this could make the code behave unexpectedly for someone new to remi.

@dddomodossola
Copy link
Collaborator

dddomodossola commented Jan 20, 2020

I can of course reconsider it.
I don't exactly remember the motivation behind that decision.

I tried to remove preventDefault at all, and I can't see problems. Tested with some remi examples and with the Graphical Editor.

@pec0ra
Copy link
Contributor Author

pec0ra commented Jan 20, 2020

I tried to remove preventDefault at all, and I can't see problems. Tested with some remi examples and with the Graphical Editor.

Sounds great! What about event.stopPropagation()?

@dddomodossola
Copy link
Collaborator

If I click on an element, I think it should not trigger the event of a parent.
Why you suggest to remove stopPropagation?

@pec0ra
Copy link
Contributor Author

pec0ra commented Jan 20, 2020

If I click on an element, I think it should not trigger the event of a parent.
Why you suggest to remove stopPropagation?

I am not sure... but it's the default behavior in the browsers so it is probably what web developers will expect.

@dddomodossola
Copy link
Collaborator

I understand this point of view, however remi should respect the laws of a standard gui library (not web development). Do you see possible problems on using stopPropagation considering this point?

@mcondarelli
Copy link
Contributor

My understanding is javascript specification itself requires double traversal of hierarchy as the "standard event path" and should be messed with only for specific needs, not as a general rule.
In the specific case why do you want to prevent parent from knowing if some of its children received an event? Even "Standard GUI libraries" (e.g.: Qt) leave to the individual handler to decide if event processing is "complete" or if it should be bubbled up to siblings/parent.
IMHO any event handler inserted "under the hood" cannot be considered "final" as other (user-defined) handlers might need it.

@pec0ra
Copy link
Contributor Author

pec0ra commented Jan 20, 2020

@dddomodossola
This makes sense.

I don't really see an issue with keeping stopPropagation, no.

@dddomodossola
Copy link
Collaborator

@mcondarelli that's right! Today I thought about this and what @pec0ra says is correct, the preventDefault should be a parameter, and maybe the same is valid for stopPropagation.

dddomodossola added a commit that referenced this issue Jan 21, 2020
… js_prevent_default and js_stop_propagation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants