Closed Bug 456216 Opened 14 years ago Closed 14 years ago

Add attributes "first-visible" and "last-visible" to the first and last visible radio buttons of the Add-ons Manager toolbar

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.1b1

People

(Reporter: mstange, Assigned: mstange)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch patch rev1 (obsolete) — Splinter Review
In bug 456214 I need a CSS selector to match the first / last visible button in the toolbar of the Add-ons manager. This toolbar is implemented as a radiogroup.

:first-child and :last-child can't be used because some of these buttons might be hidden ([hidden=true] or [collapsed=true]). Most of the time the last button is in fact hidden.

The solution I've implemented here just sets the attributes "first-visible" and "last-visible" on the first and last visible radio buttons of a radiogroup and updates them whenever something changes.

I know this is not perfect but I didn't know another way of solving this problem.
Attachment #339598 - Flags: review?(neil)
Attachment #339598 - Flags: review?(neil) → review-
Comment on attachment 339598 [details] [diff] [review]
patch rev1

Personally I don't see what use this has outside of the addon manager which is free to set any useful attributes that you may need.

I happen to know that it's a bad idea to (indirectly) call <radiogroup>'s _getRadioChildren from the <radio>'s constructor.
Component: XUL Widgets → Add-ons Manager
QA Contact: xul.widgets → extension.manager
Summary: Add attributes "first-visible" and "last-visible" to the first and last visible radio buttons of a radiogroup → Add attributes "first-visible" and "last-visible" to the first and last visible radio buttons of the Add-ons Manager toolbar
Attached patch rev2: only Add-ons manager (obsolete) — Splinter Review
Attachment #339598 - Attachment is obsolete: true
Attachment #339632 - Flags: review?(robert.bugzilla)
Attachment #339632 - Flags: review?(robert.bugzilla) → review?(dtownsend)
Comment on attachment 339632 [details] [diff] [review]
rev2: only Add-ons manager

This seems over-complicated. The first pane is always one of two and is decided at startup and never changed. The last pane is always one of 3, and you can see which straight from the booleans in updateOptionalView.

Seems that a couple of if statements would do this more quickly unless I am missing something?
I didn't want it to depend on the actual button order that much, so that it doesn't have to be changed e.g. when new buttons are added.

But if you prefer "hardcoding" it to the known buttons, I'll do that. You're right, it will probably be a lot simpler.
Comment on attachment 339632 [details] [diff] [review]
rev2: only Add-ons manager

Actually I've just come across someone doing crazy things with the UI so we'd better do it this way, however I think you can simplify it.

If in the loop for setting first-visible you also cache the last visible tab you can do away with the second loop I believe.
Attachment #339632 - Flags: review?(dtownsend) → review-
Attachment #339632 - Attachment is obsolete: true
Attachment #340928 - Flags: review?(dtownsend)
Comment on attachment 340928 [details] [diff] [review]
rev3: better loop

Looks good. Can you just rename the method to updateVisibilityFlags though. We aren't changing visibility here so this makes more sense I think.
Attachment #340928 - Flags: review?(dtownsend) → review+
pushed: http://hg.mozilla.org/mozilla-central/rev/c6f2de7c8254
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b1
You need to log in before you can comment on or make changes to this bug.