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

Status: RESOLVED FIXED

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

Description

[Mark Capella [:capella]]

Followup from bug 1060423 comment #22 ... where I let a new contributor slide thinking no-one was paying attention ;-)

Comment 1

[Mark Capella [:capella]]

Attachment: bug1064657.diff

Comment 2

[Wesley Johnston (:wesj)]

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

Comment 3

[Mark Capella [:capella]]

Attachment: bug1064657.diff

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

Comment 4

[Mark Capella [:capella]]

TRY push
https://tbpl.mozilla.org/?tree=Try&rev=9c38d0a7396a

Comment 5

[Mark Capella [:capella]]

On to fx-team
https://hg.mozilla.org/integration/fx-team/rev/5ea18dc3e655

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5ea18dc3e655