720050 - Various issues with disabled form controls

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P3)

Description

[Martijn Wargers (dead)]

Attachment: 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

Comment 1

[:Margaret Leibovic]

Wes, maybe combined with bug 717821, this would be a good project for a contributor.

Comment 2

[:Margaret Leibovic]

wesj, how many of these issues are still relevant? Where would someone start with this if they wanted to work on it?

Comment 3

[Wesley Johnston (:wesj)]

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.

Comment 4

[Thang]

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

[Mike Hoye [:mhoye]]

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!

Comment 6

[Thang]

Attachment: Preview before/after changes

Comment 7

[Thang]

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.

Comment 8

[Mike Hoye [:mhoye]]

Thanks, Thang. Wesj, can you take a look at those changes and provide some guidance?

Comment 9

[Thang]

Attachment: Replaced [disabled] by :disabled pseudo-class

Comment 10

[Kevin Brosnan [Ex-Mozilla]]

WesJ is rather consumed by the iOS project. Lets try some of the Android team.

Comment 11

[Mark Capella [:capella]]

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.

Comment 12

[Mark Capella [:capella]]

fyi followup Bug 1172086 - Empty, but disabled <input> with associated <datalist> incorrectly allows input

Comment 13

[Mark Capella [:capella]]

Thang, will you be completing this? You just need to ask for review at this point, to git 'er done :)

Comment 14

[:Margaret Leibovic]

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.

Comment 15

[Mark Capella [:capella]]

push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ccd4d4c2e60

Comment 16

[Mark Capella [:capella]]

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.

Comment 17

[Thang]

Hello Mark,

Im sorry, Ive been busy a bit so forgot to answer. Is there anything I can do now to get this done?

Comment 18

[Mark Capella [:capella]]

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?

Comment 19

[Thang]

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 :)

Comment 20

[Mark Capella [:capella]]

Attachment: bug720050-fixTests.diff

Ok, repairs R5/R6 and R3 failures due to corrected "disabled" element visuals.

Comment 21

[Mark Capella [:capella]]

forgot the proof :-)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=067bf4da59ad

Comment 22

[:Margaret Leibovic]

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 :)

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/1d33b75f1038
https://hg.mozilla.org/integration/fx-team/rev/2dc24ba0d75f

Comment 24

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1d33b75f1038
https://hg.mozilla.org/mozilla-central/rev/2dc24ba0d75f