Various issues with disabled form controls

RESOLVED FIXED in Firefox 42

Status

()

Firefox for Android
General
P3
normal
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: Martijn Wargers (zombie), Assigned: Thang, Mentored)

Tracking

({testcase})

Trunk
Firefox 42
ARM
Android
testcase
Points:
---

Firefox Tracking Flags

(firefox42 fixed, fennec+)

Details

(Whiteboard: [good next bug][lang=css])

Attachments

(4 attachments)

(Reporter)

Description

6 years ago
Created attachment 590393 [details]
testcase

See the rather extensive testcase.
I seem various problems regarding form controls with the testcase:
- The comboboxes inside the disabled fieldset are clickable/changable, also the styling is enabled, it should be disabled
- The buttons inside the disabled fieldset seem to have the wrong border, like for enabled buttons
- The text inputs don't seem to have the disabled look (although the difference between disabled inputs and enbabled inputs is rather small visually)
- The textareas and multi-select combobobox in the disabled fieldset look more brown than the disabled textarea and disabled multi-select combobox further down the page
- Tapping on disabled form controls still gives you the blue background feedback
Assignee: nobody → mbrubeck
tracking-fennec: --- → +
Priority: -- → P3
Assignee: mbrubeck → nobody

Comment 1

5 years ago
Wes, maybe combined with bug 717821, this would be a good project for a contributor.
Whiteboard: [mentor=margaret]

Updated

4 years ago
Whiteboard: [mentor=margaret] → [mentor=margaret][good next bug]
Mentor: margaret.leibovic
Whiteboard: [mentor=margaret][good next bug] → [good next bug]

Comment 2

3 years ago
wesj, how many of these issues are still relevant? Where would someone start with this if they wanted to work on it?
Mentor: wjohnston
Flags: needinfo?(wjohnston)
Whiteboard: [good next bug] → [good next bug][lang=css]
Yeah, these still need love. Note, normal disabled inputs work here. In this case the fieldset containing the form elements is disabled (which should disable everything inside).

If someone wanted to work on it, the form css we use is in http://mxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/content.css. My first attempt would be to remove all the places where we use [disabled] selectors and replace them with :disabled

I also see some things like selects allow opening even if they're disabled. I'll open a separate bug for that.
Flags: needinfo?(wjohnston)
Blocks: 1130614
Blocks: 1130617
No longer blocks: 1130614
(Assignee)

Comment 4

3 years ago
Hey there,

I would like to work on this, though I am a newbie in this whole Open Source world. So I would really appreciate any help or guidance which will lead to successfully fixing it. Thank you very much.

Comment 5

3 years ago
Hi, Thang!

It would be great if you took a run at this - you can get set up by starting with the documentation here:

https://developer.mozilla.org/en-US/docs/Introduction

... and once you're set up to build Firefox for Android, you can take a look at what needs fixing here. If you've got any questions, you can ask them here or in the #introduction channel on IRC. 

Thanks, and good luck!
(Assignee)

Comment 6

3 years ago
Created attachment 8604687 [details]
Preview before/after changes
(Assignee)

Comment 7

3 years ago
Hello again,

I finally get the Fennec builded. I located the file :wesj mentioned and made changes he suggested. The result can be seen in attachments(Preview before/after changes). Its a printscreen from testcase before and after changes.

What is the next step now?

Thank you and sorry it took such long time.
Flags: needinfo?(mhoye)

Comment 8

3 years ago
Thanks, Thang. Wesj, can you take a look at those changes and provide some guidance?
Flags: needinfo?(mhoye) → needinfo?(wjohnston)
(Assignee)

Comment 9

3 years ago
Created attachment 8608584 [details] [diff] [review]
Replaced [disabled] by :disabled pseudo-class
WesJ is rather consumed by the iOS project. Lets try some of the Android team.
Flags: needinfo?(michael.l.comella)
Flags: needinfo?(markcapella)
Hey Thang! This looks fine to me with your changes based on wesj's suggestion ... summarizing the original bug report:

) The comboboxes inside the disabled fieldset are clickable/changeable

  This was *mostly* fixed in bug #717821 - we still seem to have an issue with disabled <input> tags with an associated <datalist>. I think this should be broken into a separate bug, and I'll file that.

) also the styling is enabled (it should be disabled).

   Yes, needed to be fixed, and now seems to be.

) The buttons inside the disabled fieldset seem to have the wrong border (like for enabled buttons).

   Yes, needed to be fixed, and now seems to be.

) The text inputs don't seem to have the disabled look.

   Yes, needed to be fixed, and now seems to be.

) The textareas and multi-select combobobox in the disabled fieldset look more brown than the disabled textarea and disabled multi-select combobox further down the page.

   Yes, needed to be fixed, and now seems to be.

) Tapping on disabled form controls still gives you the blue background feedback.

   Yes, needed to be fixed, and now seems to be.


Interestingly, for the fieldset labelled "fieldset" with the <input> text "input disabled", we match our FF desktop correctly (the text isn't changeable), but both Chrome desktop and Chrome Mobile allow that field to be edited. Not sure if this is an evangelism issue, though it doesn't seem relevant to this bug.

After you get all your feedback, you'll most likely need to ping margaret for the final ?review request.
Flags: needinfo?(markcapella)
Flags: needinfo?(michael.l.comella)
fyi followup Bug 1172086 - Empty, but disabled <input> with associated <datalist> incorrectly allows input
Thang, will you be completing this? You just need to ask for review at this point, to git 'er done :)
Flags: needinfo?(pandothang)
Flags: needinfo?(wjohnston)

Comment 14

3 years ago
Comment on attachment 8608584 [details] [diff] [review]
Replaced [disabled] by :disabled pseudo-class

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

Talked to capella about this IRL, we should land this.
Attachment #8608584 - Flags: review+
push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ccd4d4c2e60
Assignee: nobody → pandothang
Status: NEW → ASSIGNED
reftest 5/6 are failing, but it's due to "UNEXPECTED-PASS", so this patch fixed a good thing... we'll have to tweak that before going further.
(Assignee)

Comment 17

3 years ago
Hello Mark,

Im sorry, Ive been busy a bit so forgot to answer. Is there anything I can do now to get this done?
Flags: needinfo?(pandothang)
You finished the basic patch, and margaret reviewed and approved it for push to m-c. As part of preparation for that, I had pushed the patch to our integration testing environment ("TRY server") as noted in comment #15 - 16 above.

It looks like there's tests in the reftest 5/6 suite that are known to fail on Android without this patch, and were previously configured to consider that normal.

Now that we've fixed the code, the tests "fail to fail" so to speak, and present an "UNEXPECTED-PASS" message. We need to identify and reconfigure those tests to expect success before we can complete this.

I was planning on looking into this next week. Are you comfortable with trying to correct this last bit? If not, you can consider your work on this patch done, and I'll mop up.

Let me know?
(Assignee)

Comment 19

3 years ago
Mark, honestly I rather let you finish it, if you dont mind that.

Thank you so much for helping me guys.
See you at next bugs :)
Created attachment 8628650 [details] [diff] [review]
bug720050-fixTests.diff

Ok, repairs R5/R6 and R3 failures due to corrected "disabled" element visuals.
Attachment #8628650 - Flags: review?(margaret.leibovic)

Comment 22

3 years ago
Comment on attachment 8628650 [details] [diff] [review]
bug720050-fixTests.diff

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

I'm not very familiar with our refests infrastructure, but if it's green, it works for me :)
Attachment #8628650 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/1d33b75f1038
https://hg.mozilla.org/mozilla-central/rev/2dc24ba0d75f
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
You need to log in before you can comment on or make changes to this bug.