Closed Bug 1060423 Opened 11 years ago Closed 11 years ago

Cut function in a readonly textarea

Categories

(Firefox for Android Graveyard :: Text Selection, defect)

Product:
Firefox for Android Graveyard
Component:
Text Selection
Version:
31 Branch
Platform:
All
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox35 fixed, fennec35+)

Status:
RESOLVED FIXED
Milestone:
Firefox 35
Tracking Flags:
Tracking Status
firefox35 --- fixed
fennec 35+ ---

People

(Reporter: cristianstoica85, Assigned: manu.jain13, Mentored)

References

Details

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

Attachments

(5 files)

readonlytextareademo.html
421 bytes, text/html
Details
SelectionHandler.js(patch at line 799)
43.23 KB, patch
manu.jain13
: feedback+
Details | Diff | Splinter Review
done.patch
1.46 KB, patch
capella
: feedback+
Details | Diff | Splinter Review
bug1060423.patch
1.45 KB, patch
capella
: feedback+
Details | Diff | Splinter Review
bug_1060423.patch
1.28 KB, patch
capella
: review+
mfinkle
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release) Build ID: 20140716183446 Steps to reproduce: Create a simple html page with an readonly textarea. <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd"> <html xmlns="http://www.w3.org/1999/xhtml"> <head> <title></title> </head> <body> <h5> Demo bug for Mozilla Firefox 31 for Android. </h5> <textarea readonly="readonly" rows="20" cols="20">The quick brown fox jumps over the lazy dog.</textarea> </body> </html> Actual results: Open the page with Mozilla and select all text/or any text. Press "cut" button. Text is copied to clipboard and then is removed from the textarea. Expected results: "Cut" icon should be disabled/removed from toolbar.
OS: Windows 7 → Android
Hardware: x86_64 → All
Status: UNCONFIRMED → NEW
tracking-fennec: --- → ?
Ever confirmed: true
Component: General → Text Selection
This should be pretty simple ... in the SelectionHandler routine module, we just need to refine the decision we make as to whether or not a field is editable. http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/SelectionHandler.js?rev=fb6d69690255&mark=800-803#799
Mentor: markcapella
Whiteboard: [lang=js][good first bug]
Hi, I would like to patch this bug. Please assist to go forward as this is my first bug.Thanks..
Assignee: nobody → manu.jain13
Status: NEW → ASSIGNED
manu: Have you downloaded Firefox source repo and built the desktop version yet? Also, for a Firefox/Android bug such as this, you should have a *nix OS for building, and optimally an Android device for testing. (And of course this particular patch requires javascript knowledge).
I've built the Firefox desktop version. I have windows and ubuntu OS installed on my PC. I also have a android device. I have some basic javascript knowledge as well. Please guide me forward. Thanks..
Have you built FF and installed / run it on your Android device? Try to reproduce the problem in this bugs description (above) as provided by the reporter ... You can see https://wiki.mozilla.org/Mobile/Fennec/Android for instructions on installing an Android development environment ... and can usually find me (or helpful others) on irc in #introduction and #mobile if you have really basic questions about getting started :-D Comment #1 (also above) basically gives away the answer, but I don't want to make it too easy for you ;-)
I've reproduce the problem on my android device. Yes, it also cut icon in readonly textarea. I've build android development environment on my pc successfully.
I got that function "isElementEditableText"(line 799) of "SelectionHandler routine module" got a bug, that its only checking whether the element is input element or textareaelement. I add up aElement.is("[readonly]")==false in "if" condition but it shows none of the icons(i.e copy, select all, etc.) on selecting textarea element. I didn't understand why all the icon disappears. Please guide. Thanks!!
Without seeing your code, I can only assume that you have a javascript syntax error that's breaking SelectionHandler processing. You can check the android LOGCAT to verify if that's true, you should see error messages there. Additionally, so I or other reviewers you might work with can help better, you can attach the patch that's not working for us to look at. See: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Creating_a_patch
tracking-fennec: ? → 35+
I have made changes in return statement of "isElementEditableText" function. I have add one more condition about attribute("readonly") mapping. But none of the icon appear(i.e copy,select all) on selecting the text in textarea field.
Attachment #8485294 - Flags: feedback+
Attachment #8485294 - Attachment description: SelectionHandler.js → SelectionHandler.js(patch at line 799)
Ok, a couple of things here ... re-read https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Creating_a_patch on how to create and attach a patch/diff. (You've just attached the entire source file). Check on irc in #introduction for me or any other mentors if you need extra help with creating a diff/patch file :-) Don't flag yourself for feedback+, flag me for review? Did you examine your android LOGCAT for the cause of your error? You should see something like JavaScript Error: "aElement.IsAttributeMapped is not a function" The problem here seems to be that you're trying to use what appears to be a C++ method |.IsAttributeMapped| in a Javascript module. The appropriate Javascript method is |.readOnly|. You can see examples in our code base where we use that here: http://mxr.mozilla.org/mozilla-central/search?string=.readOnly&case=on&find=.*.js&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central Finally, your code would prevent us from editing in elements that are .readOnly HTMLTextAreaElement's. (That's good). But we want to block *any* element that is .readOnly. Your code looks like it'll still allow us to edit in .readOnly (text input) HTMLInputElement's.
Attached patch done.patch — — Splinter Review
Bug is removed successfully.
Attachment #8485383 - Flags: review?(markcapella)
Comment on attachment 8485383 [details] [diff] [review] done.patch Review of attachment 8485383 [details] [diff] [review]: ----------------------------------------------------------------- Close! 1) Please remove the comment that says //Bug's patch: Starts from here, and the blank line after it. 2) Please remove the comment that says //Bug's patch: Ends here, and the blank line after it. 3) Please add a valid commit message to the patch. Here's an example from a patch I did last week, (see line #4) : https://bugzilla.mozilla.org/attachment.cgi?id=8477821&action=edit 4) To match mozilla style a little closer, please abbreviate aElement.readOnly==false as simply !aElement.readOnly. After all that >whew!< do a quick test with this quick page: https://www.dropbox.com/s/lice5denjqifkvf/test_bug1060423.xhtml?dl=0 I think we'll be done :-)
Attachment #8485383 - Flags: review?(markcapella) → feedback+
Attached patch bug1060423.patch — — Splinter Review
I have made all changes that you(Mark Capella) told.
Attachment #8485406 - Flags: review?(markcapella)
Comment on attachment 8485406 [details] [diff] [review] bug1060423.patch It looks like this attachment is a diff against your previous diff. Can you just post the basic diff between what's in mozilla-central (the base code) and the final change you're making? This is close enough that I can fix it up and approve it for you if you're not sure. Just let me know what you'd like to do.
Attachment #8485406 - Flags: review?(markcapella) → feedback+
Ok, I've done that also.
Attachment #8485413 - Flags: review?(markcapella)
Comment on attachment 8485413 [details] [diff] [review] bug_1060423.patch Review of attachment 8485413 [details] [diff] [review]: ----------------------------------------------------------------- Awesome ! This works and tests out ok for me locally also. The rest is paperwork :) We need to push your patch to a TRY server to undergo volume / integration testing. Since most new contributors don't have access yet, I've done that for you: https://tbpl.mozilla.org/?tree=Try&rev=c75d1214cf75 Once that passes we can get the change actually moved into mozilla-central. Basically, your work is done as your patch moves forward now (unless we fail in TRY which I don't expect). It's time to find another patch to work on :-D r? to mfinkle ... I've been allowed to r+ things lately, but I'll be cautious for awhile.
Attachment #8485413 - Flags: review?(markcapella)
Attachment #8485413 - Flags: review?(mark.finkle)
Attachment #8485413 - Flags: review+
Comment on attachment 8485413 [details] [diff] [review] bug_1060423.patch LGTM. Once Try run looks good, you can add the checkin-needed keyword
Attachment #8485413 - Flags: review?(mark.finkle) → review+
Finally, I got my first patch approved. Thanks Mark Capella for your help. Your help really guided me as I'm new to this. Now I'm ready to more bugs and solve them :-)
TRY is good ... let's get it checked in
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/603396e6332e
Keywords: checkin-needed
Whiteboard: [lang=js][good first bug] → [lang=js][good first bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/603396e6332e
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [lang=js][good first bug][fixed-in-fx-team] → [lang=js][good first bug]
Target Milestone: --- → Firefox 35
It would be good to add a regression test for this.
Flags: in-testsuite?
A regression test was added in bug 1064657.
Depends on: 1064657
Flags: in-testsuite? → in-testsuite+
status-firefox35: --- → fixed
Flags: qe-verify-
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: