Closed Bug 622405 Opened 11 years ago Closed 11 years ago

Container of browser.xul identity-popup-more-info-button (an hbox) is missing ID

Categories

(Firefox :: General, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
Firefox 4.0b9

People

(Reporter: Debloper, Assigned: Debloper)

Details

(Whiteboard: [good first bug])

Attachments

(3 files, 3 obsolete files)

User-Agent:       Mozilla/5.0 (Windows NT 6.1; rv:2.0b8) Gecko/20100101 Firefox/4.0b8
Build Identifier: Mozilla/5.0 (Windows NT 6.1; rv:2.0b8) Gecko/20100101 Firefox/4.0b8

In the site identity-popup, the last element ("More Information..." button, id="identity-popup-more-info-button") is contained within an hbox without any id.

If for any reason, a button is required to be added in that hbox set, it takes relatively complex approach to get it done. Adding an id to the hbox can fix it.

Even at the time of this bug-reporting, the latest revision of browser.xul has this problem - http://hg.mozilla.org/mozilla-central/file/a05e91710adb/browser/base/content/browser.xul

Reproducible: Always
Version: unspecified → Trunk
Confirmed with Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b9pre) Gecko/20110101 Firefox/4.0b9pre ID:20110101030401
Status: UNCONFIRMED → NEW
Component: General → Theme
Ever confirmed: true
QA Contact: general → theme
Component: Theme → General
QA Contact: theme → general
Whiteboard: [good first bug]
Not sure if I've added the reviewer string right...
Attachment #500696 - Flags: review?
Attachment #500696 - Flags: review? → review?(johnath)
I guess, dao is available for this, & hope I've done the right thing!
Attachment #500696 - Attachment is obsolete: true
Attachment #500752 - Flags: review?
Attachment #500752 - Flags: review? → review?(dao)
Comment on attachment 500752 [details] [diff] [review]
Diff created after adding id="identity-popup-more-info" to the hbox to fix

Your editor replaced all \n line endings with \r\n ones, resulting in an enormous patch. Please fix ;)
Attachment #500752 - Flags: review?(dao)
Thanks to irc:Kwierso for helping me out.
I'm pretty sure, I got it right this time.

@Dao: thanks for having patients with me; really appreciate it.
Attachment #500752 - Attachment is obsolete: true
Attachment #500759 - Flags: review?(dao)
I'd suggest "identity-popup-button-container" as the id, given this rationale:

> If for any reason, a button is required to be added in that hbox set, it takes
> relatively complex approach to get it done. Adding an id to the hbox can fix
> it.
(In reply to comment #9)
> I'd suggest "identity-popup-button-container" as the id, given this rationale:
> 
> > If for any reason, a button is required to be added in that hbox set, it takes
> > relatively complex approach to get it done. Adding an id to the hbox can fix
> > it.

Nice suggestion & I've also given similar thoughts before.
One thing kept me from doing it is, all the child nodes descendant from "identity-popup" are inheriting the 1st part of their parent's id, then appending their own. But giving that ID will break it for the contained button's id. (I'd request taking a look at lines 342-384)

Although I know having "identity-popup-button-container" sounds much better, but this previous convention (albeit not mine) is not very bad anyway. I have no bias, for any id (all I want is a ID, which should be added there).

Let me know; I'll change & resubmit.
(In reply to comment #10)
> Although I know having "identity-popup-button-container" sounds much better,
> but this previous convention (albeit not mine) is not very bad anyway.

It's not an intended convention and doesn't need to be followed through.
Attachment #500759 - Attachment is obsolete: true
Attachment #500878 - Flags: review?(dao)
Attachment #500759 - Flags: review?(dao)
Attachment #500878 - Flags: review?(dao) → review+
Attachment #500878 - Flags: approval2.0?
Attachment #500878 - Flags: approval2.0? → approval2.0+
Keywords: checkin-needed
Assignee: nobody → debloper
http://hg.mozilla.org/mozilla-central/rev/37e106e25d9c
Status: NEW → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b9
You need to log in before you can comment on or make changes to this bug.