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

Status: RESOLVED FIXED

(Firefox :: Settings UI, task, P5)

Description

[Tim Nguyen :ntim]

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.

Comment 1

[Johann Hofmann [:johannh]]

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.

Comment 2

[Tim Nguyen :ntim]

(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.

Comment 3

[:Gijs (he/him)]

(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.

Comment 4

[:Gijs (he/him)]

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.