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

RESOLVED FIXED in mozilla1.9.1b1

Status

()

Toolkit
Add-ons Manager
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: mstange, Assigned: mstange)

Tracking

Trunk
mozilla1.9.1b1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

10 years ago
Created attachment 339598 [details] [diff] [review]
patch rev1

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)

Updated

10 years ago
Attachment #339598 - Flags: review?(neil) → review-

Comment 1

10 years ago
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.
(Assignee)

Updated

10 years ago
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
(Assignee)

Comment 2

10 years ago
Created attachment 339632 [details] [diff] [review]
rev2: only Add-ons manager
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?
(Assignee)

Comment 4

10 years ago
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-
(Assignee)

Comment 6

10 years ago
Created attachment 340928 [details] [diff] [review]
rev3: better loop
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+
(Assignee)

Comment 8

10 years ago
pushed: http://hg.mozilla.org/mozilla-central/rev/c6f2de7c8254
Status: ASSIGNED → RESOLVED
Last Resolved: 10 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.