658179 - "Edit This Bookmark" dialog: checking an existing tag in the tags list scrolls the tag out of view if another tag was selected before.

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Alice0775 White]

Build Identifier: 
http://hg.mozilla.org/mozilla-central/rev/dec16a247230
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110518 Firefox/6.0a1 ID:20110518030622

This is not a duplication of Bug 631374.

When assigning an existing tag to a bookmark or deleting a tag from a bookmark in "Edit This Bookmark" dialog by checking or unchecking the checkbox for this tag in the expanded tags list, 

The tags list scrolls to previously selected tag in to view.so moving the just (un)checked tag out of view.

Reproducible: Always

Steps to Reproduce:
1. Ensure that you have a lot of tags (>16 should be OK)
2. Load a bookmark, click on the star to edit it, expand the tags list
3. Scroll the tags list one or more pages down
4. Right Click tag label for a tag
5. Scroll to the list to top
6. Click (or uncheck if already checked) for a tag.

Actual Results:  
The tags list scrolls ("jumps") one page up, the just (un)checked tag is moved out of view.
I.e. the tags list scrolls to previously selected tag in to view.



Expected Results:  
The scroll position of the tags list doesn't change.

Comment 1

[Marco Bonardo [:mak]]

Why should anyone right click on a tag? most likely we should just either disallow right clicking or, even better, on command just set the selection on the changed tag (since I could also select a tag with the keyboard but remove another one with the mouse).
If anyone wants to make a patch doing the latter, I could review it.

Btw, minor issue since it's caused by non common operations.

Comment 2

[Alice0775 White]

OK minor

Comment 3

[Marco Bonardo [:mak]]

ehr, I didn't say it's a wontfix, it's just a minor bug, it can be fixed easily as I said in comment 1.
Sorry if I was unclear.

Comment 4

[Rodrigo Magalhaes]

Marco, what do you mean with "on command just set the selection on the changed tag"?

At a first glance, I understood that a right click must select an item close to the deleted tag, then the page wouldn't jump. But you say "set the selection on the changed tag": based on my initial thoughts, if my last change were a remotion of a tag, then it would be impossible to select "the changed tag". Thus, that's why I think my initial thoughts are incorrect.

Could you explain it to us?

Comment 5

[Marco Bonardo [:mak]]

the listbox can be scrolled to a certain index, on command that is changing a tag we should scroll that tag, if it has been removed we should scroll to the position it was.  Does this work for you? I'm open to suggestions if you wish to fix this bug.

Comment 6

[Rodrigo Magalhaes]

Marco, thank you, now it's clear for me. I'm going to work on a fix.

Comment 7

[Perry Wagle]

Middle click is also paid attention to.  Makes it tough to use a wheel mouse a lot.

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

Rodrigo, are you still interested in working on this? Do you need any help? Thanks!

Comment 9

[Mike Lopez]

Attachment: bug-658179.patch

Attached attempted patch

Comment 10

[Mike Lopez]

Attachment: bug-658179.patch

Comment 11

[Mike Lopez]

Attachment: bug-658179.patch

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 8538969 [details] [diff] [review]
bug-658179.patch

Review of attachment 8538969 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/places/content/editBookmarkOverlay.js
@@ +886,5 @@
>        else {
>          var indexOfItem = tags.indexOf(aEvent.target.label);
> +        if (indexOfItem != -1) {
> +          var tagsSelector = this._element("tagsSelector");
> +          tagsSelector.selectedIndex = indexOfItem;

This works when unchecking a tag, more specifically:

1. right click on a tag (to select but not change it)
2. scroll it out of view
3. uncheck a visible tag

but it doesn't cover checking a tag:

3. check a visible tag

I think we should do this for both cases with some small refactoring, for example in both cases we do tags.indexOf(aEvent.target.label), so we could do that out if the if/else and also assign aEvent.target.label to a temp var. And clearly also set the selectedIndex out of the if/else.

also, please use "let" instead of "var" in new code.

you should also check these changes don't break current tags selector scroll tests, by running ./mach test browser/components/places/tests/chrome/

Comment 13

[Mike Lopez]

Attachment: bug-658179.patch

I couldn't use the index when it is a checked event since the index doesn't exist yet. Instead I just used the listitem that was the target of the event.

Ran the test suite and all tests passed.

Comment 14

[Marco Bonardo [:mak]]

Comment on attachment 8540462 [details] [diff] [review]
bug-658179.patch

Review of attachment 8540462 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the following changes

::: browser/components/places/content/editBookmarkOverlay.js
@@ +876,5 @@
>    handleEvent: function EIO_nsIDOMEventListener(aEvent) {
>      switch (aEvent.type) {
>      case "CheckboxStateChange":
>        // Update the tags field when items are checked/unchecked in the listbox
>        var tags = this._getTagsArrayFromTagField();

while here, please change this "var" to "let"

@@ +877,5 @@
>      switch (aEvent.type) {
>      case "CheckboxStateChange":
>        // Update the tags field when items are checked/unchecked in the listbox
>        var tags = this._getTagsArrayFromTagField();
> +      let curTagIndex = tags.indexOf(aEvent.target.label);

to improve readability, please assign "let tagCheckbox = aEvent.target;" before this, and use that in the code...

@@ +880,5 @@
>        var tags = this._getTagsArrayFromTagField();
> +      let curTagIndex = tags.indexOf(aEvent.target.label);
> +
> +      let tagsSelector = this._element("tagsSelector");
> +      tagsSelector.selectedItem = aEvent.target;

...also here...

@@ +885,2 @@
>  
>        if (aEvent.target.checked) {

...here...

@@ +888,1 @@
>            tags.push(aEvent.target.label);

...and here

Comment 15

[Mike Lopez]

Attachment: bug-658179.patch

Made requested changes.

Comment 16

[Marco Bonardo [:mak]]

Comment on attachment 8543375 [details] [diff] [review]
bug-658179.patch

Review of attachment 8543375 [details] [diff] [review]:
-----------------------------------------------------------------

looks good, thank you.

Do you have commit access to the Try server? Otherwise I can push this for you.

Comment 17

[Mike Lopez]

I do not have commit access. I would appreciate if you could push this. Thanks

Comment 18

[Marco Bonardo [:mak]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=08acd0b8ddce

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/02afde269c5b

Comment 20

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

https://hg.mozilla.org/mozilla-central/rev/02afde269c5b