Closed Bug 1674733 Opened 2 years ago Closed 3 months ago

Clean-up root attributes in browser/components/preferences/dialogs

Categories

(Firefox :: Preferences, task, P5)

task

Tracking

()

RESOLVED FIXED

People

(Reporter: ntim, Unassigned)

References

Details

Preferences dialogs are UIs unlikely to be re-designed/re-written, and even if they were, that doesn't necessarily mean that things will be cleaned up. Also, new pref dialog UIs are most of the time copy-pasting old ones (e.g. Add new engine), so setting good practices would be nice.

Regarding attributes used (aside from xmlns, data-l10n-*):

  • screenX/screenY
    In-content dialogs can't be dragged like normal OS windows, so screenX/screenY positions don't really make sense. These are probably leftovers from when prefs weren't in-content.

  • persist (mostly width/height/screenX/screenY)
    persist is only relevant with an ID on the root.
    Some resizable dialogs do persist the width/height, while some don't (Webpage language settings), I think some of these broke with bug 1585482 by moving the ID away from the root.
    screenX, screenY can be removed though.

  • windowtype
    I don't how useful this is tbh.

  • type="child"
    Probably for specifying that these are child documents, but many of the dialogs don't have this and still work fine.

  • onload/onunload/onkeypress
    These aren't too much of an issue I think.

Summary: Clean-up roots in browser/components/preferences/dialogs → Clean-up root attributes in browser/components/preferences/dialogs

Note that not all these dialogs are strictly in-content, some can be triggered e.g. through the MacOS menu bar (like sanitize.xhtml) and opened as new windows.

(In reply to Johann Hofmann [:johannh] from comment #1)

Note that not all these dialogs are strictly in-content, some can be triggered e.g. through the MacOS menu bar (like sanitize.xhtml) and opened as new windows.

That shouldn't be the case for dialogs inside of browser/components/preferences/dialogs, but they can ignored otherwise.

(In reply to Tim Nguyen :ntim from comment #0)

  • type="child"
    Probably for specifying that these are child documents, but many of the dialogs don't have this and still work fine.

https://searchfox.org/mozilla-central/rev/7b07725fe9dc5d59a525990fc1dba78bc8b82af1/toolkit/content/preferencesBindings.js#75-87

We should check that that's unnecessary, and potentially remove the machinery - I don't know that non-instantapply prefs are still supported. May be worth checking with TB but AFAIK they have the same system now, so everything is instantapply.

  • onload/onunload/onkeypress
    These aren't too much of an issue I think.

In fact it would be nice to switch to JS-based handlers instead of inline event handlers, so we can enable CSP, but we might not want to do that here.

Depends on: 1674803
Depends on: 1674806
Depends on: 1325637
Severity: -- → S4
Priority: -- → P5

I filed bug 1776171 for the event handling attributes. I think we can close this out given all the other deps are fixed. I don't think we want to remove the type=child stuff as it will change whether the dialogs are instant apply or not.

Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.