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

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Markus Stange [:mstange]]

Attachment: 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.

Comment 1

[[e-mail]]

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.

Comment 2

[Markus Stange [:mstange]]

Attachment: rev2: only Add-ons manager

Comment 3

[Dave Townsend [:mossop]]

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?

Comment 4

[Markus Stange [:mstange]]

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 5

[Dave Townsend [:mossop]]

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.

Comment 6

[Markus Stange [:mstange]]

Attachment: rev3: better loop

Comment 7

[Dave Townsend [:mossop]]

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.

Comment 8

[Markus Stange [:mstange]]

pushed: http://hg.mozilla.org/mozilla-central/rev/c6f2de7c8254