7.54 KB, text/html
106.15 KB, image/jpeg
3.28 KB, patch
|Details | Diff | Splinter Review|
6.25 KB, patch
|Details | Diff | Splinter Review|
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
tracking-fennec: --- → +
Priority: -- → P3
Wes, maybe combined with bug 717821, this would be a good project for a contributor.
Whiteboard: [mentor=margaret] → [mentor=margaret][good next bug]
Whiteboard: [mentor=margaret][good next bug] → [good next bug]
wesj, how many of these issues are still relevant? Where would someone start with this if they wanted to work on it?
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.
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.
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!
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.
Thanks, Thang. Wesj, can you take a look at those changes and provide some guidance?
Flags: needinfo?(mhoye) → needinfo?(wjohnston)
WesJ is rather consumed by the iOS project. Lets try some of the Android team.
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.
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 :)
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+
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.
Hello Mark, Im sorry, Ive been busy a bit so forgot to answer. Is there anything I can do now to get this done?
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?
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)
forgot the proof :-) https://treeherder.mozilla.org/#/jobs?repo=try&revision=067bf4da59ad
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+
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.