Cut function in a readonly textarea

RESOLVED FIXED in Firefox 35

Status

()

Firefox for Android
Text Selection
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: Cristian S., Assigned: Manu Jain, Mentored)

Tracking

31 Branch
Firefox 35
All
Android
Points:
---
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox35 fixed, fennec35+)

Details

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

Attachments

(5 attachments)

(Reporter)

Description

4 years ago
Created attachment 8481360 [details]
readonlytextareademo.html

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.
(Reporter)

Updated

4 years ago
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]
(Assignee)

Comment 2

4 years ago
Hi, I would like to patch this bug. Please assist to go forward as this is my first bug.Thanks..

Updated

4 years ago
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).
(Assignee)

Comment 4

4 years ago
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 ;-)
(Assignee)

Comment 6

4 years ago
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.
(Assignee)

Comment 7

4 years ago
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+
(Assignee)

Comment 9

4 years ago
Created attachment 8485294 [details] [diff] [review]
SelectionHandler.js(patch at line 799)

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+
(Assignee)

Updated

4 years ago
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.
(Assignee)

Comment 11

4 years ago
Created attachment 8485383 [details] [diff] [review]
done.patch

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+
(Assignee)

Comment 13

4 years ago
Created attachment 8485406 [details] [diff] [review]
bug1060423.patch

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+
(Assignee)

Comment 15

4 years ago
Created attachment 8485413 [details] [diff] [review]
bug_1060423.patch

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+
(Assignee)

Comment 18

4 years ago
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
Last Resolved: 4 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-
You need to log in before you can comment on or make changes to this bug.