Closed
Bug 658179
Opened 13 years ago
Closed 9 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)
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)
1.93 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
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.
Reporter | ||
Comment 2•13 years ago
|
||
OK minor
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → WONTFIX
Reporter | ||
Updated•13 years ago
|
Status: RESOLVED → VERIFIED
Comment 3•13 years ago
|
||
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 → ---
Updated•13 years ago
|
Severity: normal → minor
tracking-firefox6:
? → ---
Updated•13 years ago
|
Whiteboard: [good first bug]
Comment 4•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
Marco, thank you, now it's clear for me. I'm going to work on a fix.
Comment 7•13 years ago
|
||
Middle click is also paid attention to. Makes it tough to use a wheel mouse a lot.
Comment 8•11 years ago
|
||
Rodrigo, are you still interested in working on this? Do you need any help? Thanks!
Flags: needinfo?(rmagalhaes85)
Updated•11 years ago
|
Flags: needinfo?(rmagalhaes85)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → l0p3s222
Assignee | ||
Comment 9•10 years ago
|
||
Attached attempted patch
Attachment #8538962 -
Flags: review?(mak77)
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8538962 -
Attachment is obsolete: true
Attachment #8538962 -
Flags: review?(mak77)
Attachment #8538964 -
Flags: review?(mak77)
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8538964 -
Attachment is obsolete: true
Attachment #8538964 -
Flags: review?(mak77)
Attachment #8538969 -
Flags: review?(mak77)
Comment 12•9 years ago
|
||
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)
Assignee | ||
Comment 13•9 years ago
|
||
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 14•9 years ago
|
||
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+
Assignee | ||
Comment 15•9 years ago
|
||
Made requested changes.
Attachment #8540462 -
Attachment is obsolete: true
Attachment #8543375 -
Flags: review?(mak77)
Comment 16•9 years ago
|
||
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+
Assignee | ||
Comment 17•9 years ago
|
||
I do not have commit access. I would appreciate if you could push this. Thanks
Updated•9 years ago
|
Mentor: mak77
Comment 18•9 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=08acd0b8ddce
Flags: needinfo?(mak77)
Updated•9 years ago
|
Flags: needinfo?(mak77)
Keywords: checkin-needed
Comment 19•9 years ago
|
||
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: 13 years ago → 9 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.
Description
•