479899 - Remove id selector usage from toolkit themes

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Neil Deakin]

There are a number of places in the toolkit theme where id selectors are used for some reason. These should be investigated and moved/removed.

On all platforms, the #autoscroller should be a class.

On Mac:
 progressmeter.css - #progress, #progress0
 global.css - #ok, #cancel, #launch, #reveal, #pauseResume, #panelFrame, #OCSPDialog,

These should be moved to wherever they belong.

Comment 1

[Jesse Ruderman]

I'm curious -- why are id selectors bad?

Comment 2

[Neil Deakin]

The Mac ones especially are specific to certain dialogs (progress, prefs, etc). It means that, for example, anyone who happens to use an element with an id of 'panelFrame', will wonder why it has a mysterious 10 pixel left padding.

Comment 3

[Dão Gottwald [:dao]]

#autoscroller is bad for having multiple browsers in the same document without cloning our tabbrowser logic.

Comment 4

[Dão Gottwald [:dao]]

Attachment: untested patch

-/* embedding/components/ui/progressDlg/nsProgressDialog.xul */
-#ok, #cancel, #launch, #reveal, #pauseResume {
-	font: menu !important;
-}

I don't really see the point of that.

-/* toolkit/content/widgets/optionsDialog.xml */
-#panelFrame {
-  margin-top: 2px;
-  -moz-padding-start: 10px;
-}

panelFrame exists as an anonid and name, but not as an id.

-/* toolkit/mozapps/preferences/ocsp.xul */
-#OCSPDialog {
-  font: message-box;
-}

But 306166 suggests that the OCSP dialog is already broken, but the dialog's content has changed since then. So it needs to be tested whether the dialog remains ok, remains broken, or gets broken when font:message-box is dropped.

Comment 5

[Dão Gottwald [:dao]]

#progress and #progress0 completely dead, I think.

Comment 6

[Dão Gottwald [:dao]]

And .progressmeter-statusbar[mode="determined"] does nothing that .progressmeter-statusbar wouldn't do already. (Not really related to this bug.)

(In reply to comment #5)
> #progress and #progress0 completely dead, I think.

... are completely dead, I think.

Comment 7

[Neil Deakin]

Dão, are you going to continue working on this?

Comment 8

[Dão Gottwald [:dao]]

I can't answer this, as I don't have a Mac:

> -/* toolkit/mozapps/preferences/ocsp.xul */
> -#OCSPDialog {
> -  font: message-box;
> -}
> 
> Bug 306166 suggests that the OCSP dialog is already broken, but the dialog's
> content has changed since then. So it needs to be tested whether the dialog
> remains ok, remains broken, or gets broken when font:message-box is dropped.

Comment 9

[Neil Deakin]

Removing the OCSPDialog line causes the dialog to use a different font, but it appears fine in either case. I don't see any precedent for using the 'message-box' font specifically for this dialog though. Changing the font isn't the right fix for a dialog sizing issue anyway. So I'd just remove this special case.

Comment 10

[Neil Deakin]

Comment on attachment 364440 [details] [diff] [review]
untested patch

>+          this._autoScrollPopup.id = "autoscroller";

I'm assuming that this is just here for compatibility?

Comment 11

[Dão Gottwald [:dao]]

That, and because tabbrowser itself depends on it (two lines below):

> this.mCurrentBrowser.setAttribute("autoscrollpopup", this._autoScrollPopup.id);

Comment 12

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/39bba05cf179