Created attachment 545683 [details] Pointer to pull request #211
Attachment #545683 - Flags: review?(myk)
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-
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 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+
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.
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?
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
Matteo's commits and merges: https://github.com/mozilla/addon-sdk/commit/36b7c24f89b7d2b162c606dc37321ed9cbe657ef https://github.com/mozilla/addon-sdk/commit/32f002f3077bd88f50ee8c78cbf4518d099e0c62 https://github.com/mozilla/addon-sdk/commit/447a77fc62a7d517d4a5d34ff16d4574c5cf1c37 https://github.com/mozilla/addon-sdk/commit/665a531395dcd30079cc5b4e7388d1d298794165 https://github.com/mozilla/addon-sdk/commit/00f5ea0307bc7f1c9985ec2f091c291876513a82 https://github.com/mozilla/addon-sdk/commit/b492f9457cd8fa26772a3be63975b7654ed33249 some follow-up nit fixes I made: https://github.com/mozilla/addon-sdk/commit/2e72476105a771607267490ea70f50f45afd9d40 master merge I made: https://github.com/mozilla/addon-sdk/commit/e85ac7daf9546d4754821b47f51b7ac29964c792
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.1
You need to log in before you can comment on or make changes to this bug.