887489 - selectAllContext seems defective

Status: RESOLVED WORKSFORME

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

Description

[Chris Kitching [:ckitching]]

Currently, the selectAllContext function, apparently used to determine if the select all context menu element is shown, seems to have wonky logic. The body of the function currently looks like this:

      if (SelectionHandler.shouldShowContextMenu(aX, aY))
        return true;

      if (NativeWindow.contextmenus.textContext.matches(aElement))
        return (aElement.selectionStart > 0 || aElement.selectionEnd < aElement.textLength);

      return false;


The return statement in the second if statement appears to be an attempt to prevent the item from showing if you have already selected everything. It seems to be missing a -1 for aElement.textLength.
But this second if statement never gets evaluated, because of the action of SelectioHandler.shouldShowContextMenu - there exists no situation in which this context function is called and shouldShowContextMenu will return true, so the second block of logic never even gets evaluated.
Next, the second if statement seems to use the textContext thing to determine if the selection is taking place inside an input textfield - which seems entirely the wrong thing to be doing, since text other than in such a textfield can readily be selected (And, indeed, always exhibits the select all context menu item anyway because of the strange first if statement)
What seems to be the correct way to do this is something along the lines of:


      if (SelectionHandler.shouldShowContextMenu(aX, aY))
         return (aElement.selectionStart > 0 || aElement.selectionEnd < aElement.textLength);

      return false;


But even that fails because, at the point this method is run, aElement.selectionStart, aElement.selectionEnd, and aElement.textLength are all undefined...

Would appreciate sonme input from someone more familiar with the appropriate code about how to proceed on this one.

Comment 1

[:Margaret Leibovic]

Chris, do you have STR for when we run into an actual problem here? I feel like it's easier to reason about what might be going wrong if we can actually see a problem.

At the very least, this code is difficult to understand, so it can likely be improved. I think what we really want it to be doing is something like this:

If the tap is on an input box, and there's some text in there, return true.
Otherwise, if the tap is on an active selection, return true.
Otherwise, return false.

I don't think we should try to solve the problem of trying to hide the "Select All" menuitem if everything is already selected, because it seems like it would be hard to get that right in the non-input case, and there really isn't that much of a win for doing that.

Comment 2

[Mark Capella [:capella]]

Implementation of Bug 951374 - Lazy load ClipboardHelper removed all this code. Margaret: can we close this?

Comment 3

[:Margaret Leibovic]

(In reply to Mark Capella [:capella] from comment #2)
> Implementation of Bug 951374 - Lazy load ClipboardHelper removed all this
> code. Margaret: can we close this?

Sure, good catch.