Closed Bug 658179 Opened 9 years ago Closed 5 years ago

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

Categories

(Firefox :: Bookmarks & History, defect, minor)

x86
Windows 7
defect
Not set
minor

Tracking

()

RESOLVED FIXED
Firefox 38

People

(Reporter: alice0775, Assigned: l0p3s222, Mentored)

References

Details

(Keywords: regression, Whiteboard: [good first bug])

Attachments

(1 file, 4 obsolete files)

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.
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.
OK minor
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
Status: RESOLVED → VERIFIED
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.
Status: VERIFIED → REOPENED
Resolution: WONTFIX → ---
Severity: normal → minor
Whiteboard: [good first bug]
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?
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.
Marco, thank you, now it's clear for me. I'm going to work on a fix.
Middle click is also paid attention to.  Makes it tough to use a wheel mouse a lot.
Rodrigo, are you still interested in working on this? Do you need any help? Thanks!
Flags: needinfo?(rmagalhaes85)
Flags: needinfo?(rmagalhaes85)
Assignee: nobody → l0p3s222
Attached patch bug-658179.patch (obsolete) — Splinter Review
Attached attempted patch
Attachment #8538962 - Flags: review?(mak77)
Attached patch bug-658179.patch (obsolete) — Splinter Review
Attachment #8538962 - Attachment is obsolete: true
Attachment #8538962 - Flags: review?(mak77)
Attachment #8538964 - Flags: review?(mak77)
Attached patch bug-658179.patch (obsolete) — Splinter Review
Attachment #8538964 - Attachment is obsolete: true
Attachment #8538964 - Flags: review?(mak77)
Attachment #8538969 - Flags: review?(mak77)
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/
Attachment #8538969 - Flags: review?(mak77)
Attached patch bug-658179.patch (obsolete) — Splinter Review
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.
Attachment #8538969 - Attachment is obsolete: true
Attachment #8540462 - Flags: review?(mak77)
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
Attachment #8540462 - Flags: review?(mak77) → review+
Attached patch bug-658179.patchSplinter Review
Made requested changes.
Attachment #8540462 - Attachment is obsolete: true
Attachment #8543375 - Flags: review?(mak77)
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.
Attachment #8543375 - Flags: review?(mak77) → review+
I do not have commit access. I would appreciate if you could push this. Thanks
Mentor: mak77
Flags: needinfo?(mak77)
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/02afde269c5b
Keywords: checkin-needed
Whiteboard: [good first bug] → [good first bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/02afde269c5b
Status: REOPENED → RESOLVED
Closed: 9 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][fixed-in-fx-team] → [good first bug]
Target Milestone: --- → Firefox 38
You need to log in before you can comment on or make changes to this bug.