Closed Bug 479899 Opened 11 years ago Closed 11 years ago

Remove id selector usage from toolkit themes

Categories

(Toolkit :: XUL Widgets, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: enndeakin, Assigned: dao)

References

Details

Attachments

(1 file)

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.
I'm curious -- why are id selectors bad?
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.
#autoscroller is bad for having multiple browsers in the same document without cloning our tabbrowser logic.
Attached patch untested patchSplinter Review
-/* 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.
#progress and #progress0 completely dead, I think.
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.
Dão, are you going to continue working on this?
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.
Attachment #364440 - Flags: review?(enndeakin)
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 on attachment 364440 [details] [diff] [review]
untested patch

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

I'm assuming that this is just here for compatibility?
Attachment #364440 - Flags: review?(enndeakin) → review+
That, and because tabbrowser itself depends on it (two lines below):

> this.mCurrentBrowser.setAttribute("autoscrollpopup", this._autoScrollPopup.id);
http://hg.mozilla.org/mozilla-central/rev/39bba05cf179
Assignee: nobody → dao
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Blocks: 482218
You need to log in before you can comment on or make changes to this bug.