Empty, but disabled <input> with associated <datalist> incorrectly allows input

RESOLVED FIXED in Firefox 41

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: capella, Assigned: p.calligaris.code, Mentored)

Tracking

unspecified
Firefox 41
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

(Whiteboard: [lang=js][good first bug])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
See the example testcase [0]

Initially tapping the disabled <input> provides the dropdown datalist, and allows item selection via tap.

Interestingly, once the field is populated, subsequent tapping behaviour is as expected (not allowed).

[0] https://www.dropbox.com/s/5xl2sm7ehcpdu95/bug_Disabled_Input_wDatalist.html?dl=0
(Reporter)

Comment 1

4 years ago
This is a fairly simple bug for a new contributor with basic Javascript skills, to learn the Mozilla development process.

You should have an android device for testing your solution locally, and should be able to build Firefox Mobile on your desktop. See [1] for instructions if this is new to you, and ask questions on irc channels #introduction or #mobile.

After building and testing to confirm that you can reproduce the issue, you can review the method / code that is at play here [2]. You should be able to enhance the first method it calls |_isAutoComplete()| [3] to fail in this additional situation, to solve the problem.

[1] https://wiki.mozilla.org/Mobile/Fennec/Android
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=8c37c4b99d99&mark=5929-5929#5924
[3] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=8c37c4b99d99#5851
Mentor: markcapella
(Reporter)

Updated

4 years ago
Whiteboard: [lang=js][good first bug]
(Assignee)

Comment 2

4 years ago
Can I take it?
(Reporter)

Comment 3

4 years ago
The bug will be assigned to whoever can attach a viable patch for review. If you've completed the requirements as in comment #1, please post your work.
(Assignee)

Comment 4

4 years ago
Created attachment 8616384 [details] [diff] [review]
Proposed Patch

Here there is a proposed patch. 

The attribute disabled (http://www.w3.org/TR/html5/forms.html#attr-fe-disabled) is a boolean attribute (http://www.w3.org/TR/html5/infrastructure.html#boolean-attribute). I quote 

"The presence of a boolean attribute on an element represents the true value, and the absence of the attribute represents the false value."   

Then it is sufficient to check if there is or not the attribute using the method .hasAttribute .

Is it right?

I built and check the patch on my android device and seems to work.
(Reporter)

Comment 5

4 years ago
Close! We normally prefer the use of the element property vs. it's attribute ... can you try
|aElement.disabled| ?

This is normally preferred as simpler and easier to quickly read, and also sometimes for visible controls actually more correct. See additional reading [0], and [1].

[0] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/disabled
[1] https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes#Content_versus_IDL_attributes

Also, a slight style "nit" for your patch, I don't think it'll be necessary to wrap the new condition in parens
|(aElement.hasAttribute("disabled"))| vs. |aElement.hasAttribute("disabled")|

(See the earlier free-standing condition for |aElement.readOnly|)
Assignee: nobody → p.calligaris.code
Status: NEW → ASSIGNED
(Assignee)

Comment 6

4 years ago
Created attachment 8616451 [details] [diff] [review]
Updated Proposed Patch

I updated the patch as you suggested. Thank you for the useful links!
Attachment #8616384 - Attachment is obsolete: true
(Reporter)

Comment 7

4 years ago
This looks fine, and wfm locally ... let's push to TRY / automated test-farm.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6a286029b55

If this works ok, we'll have you change |r=reviewers| to |r=margaret| in your patch commit message, and re-post flagging her for review=?
(Reporter)

Comment 8

4 years ago
Ok, (as expected), this looks fine. As mentioned in the previous comment, go ahead and:

1) change |r=reviewers| to |r=margaret| in your patch commit message
2) repost the new patch, and flag it for her review
(Assignee)

Comment 9

4 years ago
Created attachment 8616875 [details] [diff] [review]
bug1172086_input_element_disabled_property_issue.patch

Last proposed patch. Thanks for the help Mark!
Attachment #8616451 - Attachment is obsolete: true
Attachment #8616875 - Flags: review?(margaret.leibovic)

Comment 10

4 years ago
Comment on attachment 8616875 [details] [diff] [review]
bug1172086_input_element_disabled_property_issue.patch

Review of attachment 8616875 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, thanks! And thanks to capella for mentoring!
Attachment #8616875 - Flags: review?(margaret.leibovic) → review+

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/99660e46937c
Whiteboard: [lang=js][good first bug] → [lang=js][good first bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/99660e46937c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Whiteboard: [lang=js][good first bug][fixed-in-fx-team] → [lang=js][good first bug]
Target Milestone: --- → Firefox 41
You need to log in before you can comment on or make changes to this bug.