572492 - accel-click generates null selection when there are existing selections

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P2)

Description

[Myk Melez [:myk] [@mykmelez]]

If there are one or more existing discontiguous selections, and you click on the page while holding down the accelerator key (control on Windows/Linux, command on Mac), the selection API's onSelect handler is called, and it presents a discontiguous selection that includes both the existing selections and a new one whose text value is the null value.

Steps to Reproduce:

1. save attachment 451563 [details] to jetpack-sdk/packages/jetpack-core/lib/;
2. change to jetpack-sdk/packages/jetpack-core/lib/;
3. cfx run -a firefox;
4. wait five seconds after page loads;
5. select text on the page;
6. hold down the control key and click outside the selection.

Expected Results: after step 6, nothing new is displayed on the console.

Actual Results: after step 6, both the selection from step 5 and a null selection are displayed on the console.

I don't think this blocks the 0.5 release, but it should definitely be relnoted <https://wiki.mozilla.org/Labs/Jetpack/Release_Notes/0.5>, and I'd be happy to take a low-risk fix during the freeze.

Comment 1

[Dietrich Ayala (:dietrich)]

Eric, do you have time to fix this before we release?

Comment 2

[Eric Jung [:ericjung]]

I looked into it once already; a simple null-check before calling the onSelect listeners fixed it but caused another problem. So I'm not sure it's a simple fix. I'll look into it again tonight.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Matteo, can you fix this?

Comment 5

[Myk Melez [:myk] [@mykmelez]]

More particularly: does the work you've done elsewhere to fix selection bugs also address this one?

Comment 6

[Matteo Ferretti [:zer0] [:matteo]]

No Myk, because this one is related to the same issue we discussed the last time about isContiguous, iterator and empty range.

If you remember, I said that internally `safeGetRange` method returns null for empty range; and we consider empty range as no selection. But both `isContiguous` and iterator method takes in account just the count of the ranges. And when we click on a page holding the accelerator key, we create a range, on platform level. It doesn't matter if we select a text or not. Of course if we don't select a text, the range created it's collapsed. Ideally, on JS, you can still modify that range if you want. But because we threat empty ranges as "no selection", if we try to change the content we get an exception. So it's useless.

Also, if we decided to go for the approach we thought the last time (be sure that `require('selection').text` returns the first non-null selection, but keep them in the iterator to having the same index) this issue won't be solved.

If the correct behavior should be that holding accelerator key and click should be behave like the click without accelerator key, than should be fixed at platform level.

We can of course add a workaround, like if the selection just added is empty string, we can remove all current selections.
Notice that this approach can't work in text fields, that still will displays the selections, because text field selection API doesn't support multiple selection, so we can't check "the last selection added".

I would say the best approach is change that on platform level.
If we want to do it on Add-on SDK side, then to minimize the inconsistency, it's better keep the current behavior (so, keep the selections) but don't display the `null` selection during the iteration: that's bring to us the problem about the indexes, if an addon developer store a selection that is not the main one in the a variable, but we can said in the docs that store multiple selections in variables is not reliable because selection can change.

Comment 7

[Myk Melez [:myk] [@mykmelez]]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #6)

> I would say the best approach is change that on platform level.
> If we want to do it on Add-on SDK side, then to minimize the inconsistency,
> it's better keep the current behavior (so, keep the selections) but don't
> display the `null` selection during the iteration

I think the platform fix is going to be hard in this case, so we should fix it as you suggest on the SDK.


> that's bring to us the
> problem about the indexes, if an addon developer store a selection that is
> not the main one in the a variable, but we can said in the docs that store
> multiple selections in variables is not reliable because selection can
> change.

Yup, let's make sure to document this.

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(Pushing all open bugs to the --- milestone for the new triage system)