Closed Bug 1064657 Opened 5 years ago Closed 5 years ago

Add testSelectionHandler test to ensure readOnly input elements can't be cut

Categories

(Firefox for Android :: Text Selection, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 35

People

(Reporter: capella, Assigned: capella)

References

Details

Attachments

(1 file, 1 obsolete file)

Followup from bug 1060423 comment #22 ... where I let a new contributor slide thinking no-one was paying attention ;-)
Attached patch bug1064657.diff (obsolete) — Splinter Review
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attachment #8486160 - Flags: review?(wjohnston)
Comment on attachment 8486160 [details] [diff] [review]
bug1064657.diff

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

I don't really want to add this check before doing an action. It covers up a bug (that the action was available to you).

::: mobile/android/chrome/content/SelectionHandler.js
@@ +122,4 @@
>        case "TextSelection:Action":
>          for (let type in this.actions) {
>            if (this.actions[type].id == aData) {
> +            if (this.actions[type].selector.matches(this._targetElement)) {

I don't love doing this :( i.e. we shouldn't (and won't) be firing this action if matches was false when we build the list of available actions. We really need some tests that do a bit more on the Java side when the action mode is shown. If you wanted to be tricky here though, you could try stealing Messaging now and then intercepting the call to _updateMenu. i.e.

return new Promise((resolve, reject) => {
  let oldMessaging = Messaging;
  Messaging = {
    sendRequest: function(request) {
      // check type and actions
      resolve();
    }
  }
  sh.startSelection(readOnlyNode); // This will trigger sendRequest above.. maybe more than once. ugh :(
  Messaging = oldMessaging;
});

Not sure if that's better or not :)
Attachment #8486160 - Flags: review?(wjohnston) → review-
Attached patch bug1064657.diffSplinter Review
Yes, I expanded the scope a bit, as in reality the UI shouldn't allow the cut_action to be generated in the first place.

Would a more-targeted solution/test of the new code such as the attached be sufficient for now?

I can follow up with a bug to address the more-unlikely case where a cut_action is generated by something like an add-on, if you feel there's value in protecting there.

I'm still scoping how much additional attention we'll be paying into the SelectionHandler area with the new non-native selection logic sneaking up on us :-)
Attachment #8486160 - Attachment is obsolete: true
Attachment #8488839 - Flags: review?(wjohnston)
Attachment #8488839 - Flags: review?(wjohnston) → review+
https://hg.mozilla.org/mozilla-central/rev/5ea18dc3e655
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Blocks: 1060423
You need to log in before you can comment on or make changes to this bug.