Closed Bug 591026 Opened 14 years ago Closed 14 years ago

popupnotification element IDs are too generic

Categories

(Toolkit :: General, defect)

Product:
Toolkit
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla2.0b5
Tracking Flags:
Tracking Status
blocking2.0 --- beta5+

People

(Reporter: Gavin, Assigned: Gavin)

Details

(Whiteboard: [has reviewed patch][doorhanger])

Attachments

(1 file, 5 obsolete files)

with tested test fixes
(deleted), patch
Dolske
: review+
Details | Diff | Splinter Review
People use generic words like "geolocation" for notification IDs, without really considering that this gets used as-is for the popupnotification element's ID. I think we should append "-notification" to the ID we use for that to avoid potential ID-conflict issues. (This won't affect the notification ID itself, just what we use for the ID attribute. So people overriding the popupnotification binding by ID will need to adjust their CSS accordingly.)
Attached patch patch (obsolete) (deleted) — — Splinter Review
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #469584 - Flags: review?(dtownsend)
This will impact the patches for bug 573536, bug 571409, and bug 588266.
blocking2.0: --- → beta5+
Attached patch correct patch (obsolete) (deleted) — — Splinter Review
Attachment #469584 - Attachment is obsolete: true
Attachment #469587 - Flags: review?(dolske)
Attachment #469584 - Flags: review?(dtownsend)
Comment on attachment 469587 [details] [diff] [review] correct patch r=sdwilsh
Attachment #469587 - Flags: review?(dolske) → review+
Attached patch for checkin (obsolete) (deleted) — — Splinter Review
Attachment #469587 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/c584dd85b461
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
I forgot that the id is inherited by the icon (as "popupid")... That makes changing this much more troublesome, since it affects all the styling as well. I think WONTFIX :( https://hg.mozilla.org/mozilla-central/rev/988b28c714ca
Resolution: FIXED → WONTFIX
(In reply to comment #7) > I forgot that the id is inherited by the icon (as "popupid") Adjusting this is trivial, no?
Attached patch like this (obsolete) (deleted) — — Splinter Review
Attachment #469590 - Attachment is obsolete: true
Attachment #469839 - Flags: review?(gavin.sharp)
Comment on attachment 469839 [details] [diff] [review] like this Hmm, yeah. There are also test changes to make (checkPopup in browser_popupNotification.js needs to use notificationObj.id+"-notification"). With that the tests should pass
Attachment #469839 - Flags: review?(gavin.sharp) → review+
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Attached patch with test fixes (obsolete) (deleted) — — Splinter Review
Attachment #469839 - Attachment is obsolete: true
Attached patch with tested test fixes (deleted) — — Splinter Review
Attachment #469904 - Attachment is obsolete: true
Attachment #469924 - Flags: review?(dolske)
Comment on attachment 469924 [details] [diff] [review] with tested test fixes rs=me. (what, me worry?)
Attachment #469924 - Flags: review?(dolske) → review+
Whiteboard: [has reviewed patch]
https://hg.mozilla.org/mozilla-central/rev/f0184863691e
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b5
Whiteboard: [has reviewed patch] → [has reviewed patch][doorhanger]
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: