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

Do not submit disabled <input> elements #248

Closed
5j9 opened this issue Nov 11, 2018 · 14 comments
Closed

Do not submit disabled <input> elements #248

5j9 opened this issue Nov 11, 2018 · 14 comments

Comments

@5j9
Copy link

5j9 commented Nov 11, 2018

https://www.w3.org/TR/html52/sec-forms.html#element-attrdef-disabledformelements-disabled

The disabled attribute is used to make the control non-interactive and to prevent its value from being submitted.

MechanicalSoup ignores disabled attributes which should be fixed.

Some additional notes: (from https://www.wufoo.com/html5/disabled-attribute/)

  • If the disabled attribute is set on a <fieldset>, the descendent form controls are disabled.
  • A disabled field can’t be modified, tabbed to, highlighted, or have its contents copied. Its value is also ignored when the form goes thru constraint validation.
  • The disabled value is Boolean, and therefore doesn’t need a value. But, if you must, you can include disabled="disabled".
  • Setting the value of the disabled attribute to null does not remove the effects of the attribute. Instead use removeAttribute('disabled').
  • You can target elements that are disabled with the :disabled pseudo-class. Or, if you want to specifically target the presence of the attribute, you can use input[disabled]. Similarly, you can use :enabled and input:not([disabled]) to target elements that are not disabled.
  • You do not need to include aria-disabled="true" when including the disabled attribute because disabled is already well supported. However, if you are programmatically disabling an element that is not a form control and therefore the disabled attribute does not apply, include aria-disabled="true".
  • The disabled attribute is valid for all form controls including all <input> types, <textarea>, <button>, <select>, <fieldset>, and <keygen>.
@hemberger
Copy link
Contributor

Indeed, it looks like we're not dealing with this appropriately in MechanicalSoup. It will take some work to make us compliant with all the details of disabled attributes, but I think as a first pass we can easily add it into the Browser._request method when we process the form data.

If you'd like to submit a PR, we would happily welcome it and can provide any guidance that you need. Otherwise, we'll get to this as soon as we can.

Thanks for submitting this issue!

@rational-kunal
Copy link

I would like to work on this issue !

@moy
Copy link
Collaborator

moy commented Dec 2, 2018

@rationa-kunal: thanks! Let us know if you need help, and don't hesitate to post drafts if you want feedback.

@rational-kunal
Copy link

I was thinking of adding an extra boolean variable in Form class as dev_option. If that is set to True then we can edit disabled values and if it is False we cant edit it.
for example .

if i.has_attr("disabled") and not self.dev_op:
    raise LinkNotFoundError("This is disabled field you have to set dev_op to true to edit this field " + `name)```

@moy
Copy link
Collaborator

moy commented Dec 3, 2018

The most important point IMHO is that disabled fields should be ignored at submission time, unconditionally. Then, why not a feature to protect against edition of disabled fields. I don't think it'd be sane to allow editing disabled fields anyway (since they'll be ignored at submission time, there's no point editing them), but a feature to easily turn a disabled field to an enabled one would be nice.

@rational-kunal
Copy link

@moy i didnt understand your previous reply, can you please simplify

this is my pr with my original idea
#254

@moy
Copy link
Collaborator

moy commented Dec 3, 2018

The problem with disabled fields is not the ability to edit them. It is that these items are currently considered when submitting the form. For example, if you have

<input type="text" name="someName" value="someValue" disabled="disabled">

Then someName=someValue will be added to the form's data (i.e. in the URL in the case of GET forms). It should not.

MechanicalSoup already allows the user to edit things that are not meant to be editable in the page: the user of MechanicalSoup gets a completely editable BeautifulSoup object representing the HTML, and can do anything with it. It is not like an interactive browser where disabled elements must be explicitly disabled in the graphical user interface, to prevent users from doing mistakes. Here, the user is a programmer who can be expected to know what he or she is doing.

@rational-kunal
Copy link

So the idea is to just not edit the disabled inputs .... And drop the dev_op option !
That would be fairly simple !

@Quetzalcohuatl
Copy link

There are some (disabled) fields that I want to edit, so leaving the option to edit them would be appreciated. This is desired by more than just me (https://stackoverflow.com/questions/9249996/mechanize-cannot-read-form-with-submitcontrol-that-is-disabled-and-has-no-value)

@moy
Copy link
Collaborator

moy commented Jan 16, 2019

Currently disabled fields are editable in MechanicalSoup, so @Quetzalcohuatl the behavior you expect is the current one (the question you link to is about mechanize, a different library).

@Quetzalcohuatl
Copy link

You're right. I post because I want to emphasize that I moved from mechanize to this library specifically for that feature. So if an edit comes, it should definitely be optional to ignore disabled elements.

@hemberger
Copy link
Contributor

The change that would make the most sense to me is to simply not submit inputs that have the disabled attribute (just like the browser does), but to continue to allow that attribute to be removed by manipulating the Form object (as @moy suggests).

It would not be backwards compatible (e.g. if you were relying on submitting disabled inputs by default, then you would need to change your code to remove the disabled attribute), but the break seems justified since we are quite incorrectly deviating from the standard here.

For example, to enable all disabled inputs, you might write something like:

form = br.get_current_form()
for item in form.form.find_all("input"):
    if "disabled" in item.attrs:
        del item.attrs["disabled"]

Would it be useful to have a function that could add/remove the disabled attribute from an input, or is this simple enough?

@Quetzalcohuatl
Copy link

Your code @hemberger is a good solution to me. Didn't realize it was so simple.

@moy
Copy link
Collaborator

moy commented Jan 16, 2019

@hemberger : fully agree with you here.

@Quetzalcohuatl : Indeed: keep in mind that MechanicalSoup is a very thin layer on top of requests and BeautifulSoup (both very solid libraries). Everything is possible by looking under the hood ;-).

hemberger added a commit to hemberger/MechanicalSoup that referenced this issue Jan 28, 2019
Closes MechanicalSoup#248.

MechanicalSoup was incorrectly ignoring `disabled` attributes
in form elements.
@moy moy closed this as completed in #267 Jan 28, 2019
moy pushed a commit that referenced this issue Jan 28, 2019
Closes #248.

MechanicalSoup was incorrectly ignoring `disabled` attributes
in form elements.
tiboroche pushed a commit to tiboroche/MechanicalSoup that referenced this issue Mar 1, 2019
Closes MechanicalSoup#248.

MechanicalSoup was incorrectly ignoring `disabled` attributes
in form elements.
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

5 participants