Closed Bug 926477 Opened 8 years ago Closed 8 years ago

Replace className comparisons with classList.contains in browser-places.js

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: jaws, Assigned: waldion)

Details

(Whiteboard: [good first bug][mentor=jaws][lang=js])

Attachments

(1 file, 1 obsolete file)

+++ This bug was initially created as a clone of Bug #926471 +++

See the following search for each instance of this: http://mxr.mozilla.org/mozilla-central/search?string=.className+%3D%3D&find=browser-places.js&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

If someone adds a new className to the element, these checks will fail. They should instead be using .classList.contains().
Can this bug be assigned to me please?
Assignee: nobody → waldion
Status: NEW → ASSIGNED
Any update on this?
Flags: needinfo?(waldion)
Fixed the bug, running the tests now.
Flags: needinfo?(waldion)
The fix is included in the patch uploaded by betschart.ma@gmail.com (for bug 926487)
Attached patch Replace className comparisons (obsolete) — Splinter Review
Attachment #8344187 - Flags: review?(jaws)
Comment on attachment 8344187 [details] [diff] [review]
Replace className comparisons

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

Can you please add a patch summary of: "Bug 926477: Replace className comparisons with classList.contains in browser-places.js. r=jaws" and reupload?

::: browser/base/content/browser-places.js
@@ +102,5 @@
>                this.cancelButtonOnCommand();
>              break;
>            case KeyEvent.DOM_VK_RETURN:
> +            if (aEvent.target.classList.contains("expander-up") ||
> +                 aEvent.target.classList.contains("expander-down") ||

This looks good, but an extra space got added in the beginning of this line.
Attachment #8344187 - Flags: review?(jaws) → review+
Attached patch 926477.patchSplinter Review
Attachment #8345311 - Flags: review?(jaws)
Comment on attachment 8345311 [details] [diff] [review]
926477.patch

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

This looks good. I had to fix the commit message as all of the patch details got pulled in to the summary somehow.

I've pushed this to the fx-team repository. If all goes as planned, it should be merged to mozilla-central in a day or two, and then the following day the changes will appear in the Nightly build of Firefox. Congratulations on your first patch! :)

https://hg.mozilla.org/integration/fx-team/rev/764e877917a4
Attachment #8345311 - Flags: review?(jaws) → review+
Attachment #8344187 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/764e877917a4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
You need to log in before you can comment on or make changes to this bug.