`SelectionContext` in context-menu module returns `false` instead of `true` when the selection is in a text box or text area

RESOLVED FIXED in 1.1

Status

Add-on SDK
General
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: zer0, Assigned: zer0)

Tracking

(Depends on: 1 bug)

unspecified

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Updated

7 years ago
Depends on: 85686
(Assignee)

Updated

7 years ago
Assignee: nobody → zer0
(Assignee)

Comment 1

7 years ago
Created attachment 545683 [details]
Pointer to pull request #211
Attachment #545683 - Flags: review?(myk)

Comment 2

7 years ago
Comment on attachment 545683 [details]
Pointer to pull request #211

Nice, thanks for adding this.  BTW, you can ask me for context-menu reviews.  I'm stealing Myk's review to r- this.  The reason is that this patch forces the user to right-click directly on the form field, since it relies on popupNode, but when there's a normal, non-form-field selection, the user doesn't need to click directly on the selected text to activate the SelectionContext.  One way to fix that would be to use querySelectorAll('textfield, input[type="text"]') and check each form field individually.

I also made a bunch of style nits inline on the GutHub page.  (I'm 0c0w3.)
Attachment #545683 - Flags: review?(myk) → review-

Updated

7 years ago
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 3

7 years ago
Thanks! If the user have a selection in a text field, and right-click on a different element than the text field itself, the selection is gone. That's because I relied on popupNode. Is there some OS where Firefox have a different behavior?

Comment 4

7 years ago
Comment on attachment 545683 [details]
Pointer to pull request #211

Oh, you're right.  OK, r+, let's go with this patch then.  It works well and is much better than nothing.

Do you have commit access?  If so, before landing, please add a brief comment that explains why we rely on popupNode rather than searching the page for any form field with a selection, and please fix the style nits.  If not, let me know and I can do all of that and land it for you.
Attachment #545683 - Flags: review- → review+
(Assignee)

Comment 5

7 years ago
I don't have commit access yet, you actually reviewed my first patch ever in the Addon-SDK (and in Github as well). :)

I fixed the style nits, but there is an open question yet – I ask to you in github. As soon as you can give me a reply, I will pull the new code.

Comment 6

7 years ago
I think I answered all your questions over on GitHub, but I didn't see any new commits.  If you'd still like me to land your changes, let me know here in the bug when you're finished.

Also, I was thinking, this change means that sometimes window.getSelection() will return an empty selection, even though a SelectionContext exists.  So developers will have to check if the selection is empty, and if so, then check if the node is a form field.  That's a hassle, and it can potentially lead to problems if the developer is not aware of the distinction between selected text in the page and selected text in a form field.  But at the same time, for developers who are aware, this change is useful.  Do you have any thoughts about that?
(Assignee)

Comment 7

7 years ago
Yes, you answered all my questions, I was at the jetpack work week in Paris, that's because I didn't have a chance to commit before today, sorry about that, I will do it right now. :)

I'm not sure if I followed you in your thoughts, but if I did, the addon developer will be no bothered about this issue because they should use our selection module, that will take care of that (this bug was created because I was working on the bug on selection module, actually). However, we can't deal in the same way with not contiguous selection in text fields because the text field APIs selections simply doesn't support them.
I already discussed with Myk and he's fine with that limitation. Of course these fixes are here just because the bug 85686. Hope that it will be fixed soon.

Here the bug related to selection module: https://bugzilla.mozilla.org/show_bug.cgi?id=581982

Comment 8

7 years ago
Comment on attachment 545683 [details]
Pointer to pull request #211

Thanks, looks good, r+.

Running test-context-menu.js now produces a bunch of identical warnings in the error console on the Firefox Nightly:

console: [JavaScript Warning: "Use of getAttributeNode() is deprecated. Use getAttribute() instead." {file: "resource://addon-kit-addon-kit-tests/test-context-menu.html" line: 0}]

Simply removing the textarea from test-context-menu.html makes the warning go away, so as far as I can tell it's not the test's fault.  It looks like the warning was added by bug 661327.  I assume something in Gecko is causing it.
You need to log in before you can comment on or make changes to this bug.