1035308 - Investigate use of acceptDialog/cancelDialog in in-content prefs tests

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[:Gijs (he/him)]

There are some:

http://mxr.mozilla.org/mozilla-central/search?find=%2Fbrowser%2Fcomponents%2Fpreferences%2Fin-content%2F&string=acceptDialog

http://mxr.mozilla.org/mozilla-central/search?find=%2Fbrowser%2Fcomponents%2Fpreferences%2Fin-content%2F&string=cancelDialog

and because we're getting rid of the dialogs, that seems odd...

Comment 1

[:Gijs (he/him)]

Annnnd another one.

Comment 2

[Marco Mucci [:MarcoM]]

Added to the priority list for Iteration 33.3

Comment 3

[:Gijs (he/him)]

Soooo.

/browser/components/preferences/in-content/tests/browser_connection.js
/browser/components/preferences/in-content/tests/browser_connection_bug388287.js

This dialog is still for reals, and has OK buttons and so on even on OS X, so this should be fine. There's also no dep bug in bug 752197 (needinfo Matt - am I correct in thinking we won't try to move this in-content? Or is the lack of bug me not looking carefully and/or it not yet being filed (but still planned) ?)

/browser/components/preferences/in-content/tests/browser_subdialogs.js

This is fine, as this is using the in-content sub-dialog stuff.

/browser/components/preferences/in-content/tests/browser_proxy_backup.js

This is the same dialog, and passes, /however/: http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/tests/browser_proxy_backup.js#32 scares me a little. Matt, do we have a migration in place to ensure that people don't end up having personally set instantApply to false on mac, for instance, and then end up with a broken network dialog? Perhaps we should just remove the pref and/or make it not apply to in-content stuff... :-\

Comment 4

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #3)
> Soooo.
> 
> /browser/components/preferences/in-content/tests/browser_connection.js
> /browser/components/preferences/in-content/tests/
> browser_connection_bug388287.js
> 
> This dialog is still for reals, and has OK buttons and so on even on OS X,
> so this should be fine. There's also no dep bug in bug 752197 (needinfo Matt
> - am I correct in thinking we won't try to move this in-content? Or is the
> lack of bug me not looking carefully and/or it not yet being filed (but
> still planned) ?)

This got filed as bug 1036815.

> /browser/components/preferences/in-content/tests/browser_proxy_backup.js
> 
> This is the same dialog, and passes, /however/:
> http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/
> in-content/tests/browser_proxy_backup.js#32 scares me a little. Matt, do we
> have a migration in place to ensure that people don't end up having
> personally set instantApply to false on mac, for instance, and then end up
> with a broken network dialog? Perhaps we should just remove the pref and/or
> make it not apply to in-content stuff... :-\

I'd still like to talk more about this before closing this bug.

Comment 5

[Matthew N. [:MattN]]

init_all for in-content prefs does:
  document.documentElement.instantApply = true;

I'm not sure if that somehow gets inherited in subdialogs so perhaps gSubDialog should set that on each sub-dialog too?

I don't think we want to remove the pref as it would affect extension prefWindows too IIUC.

Comment 6

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] (behind on bugmail) from comment #5)
> init_all for in-content prefs does:
>   document.documentElement.instantApply = true;
> 
> I'm not sure if that somehow gets inherited in subdialogs so perhaps
> gSubDialog should set that on each sub-dialog too?
> 
> I don't think we want to remove the pref as it would affect extension
> prefWindows too IIUC.

Makes sense. I'm trying to figure out the subdialog question and getting confused. Here's what I see. The dialogs are all prefwindows, and the preferences elements have this code:

http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/preferences.xml#107

type is taken from the document element's type. That reads an attribute:

http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/preferences.xml#697

which is odd, because for connection.xul, that is set to "child":

http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/connection.xul#12

and so this block:

http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/preferences.xml#619

never reads the preference. Which means it stays at the default (false). Which means this code:

http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/preferences.xml#107

only reads the document element's version, which is never true (assuming my trying to follow the logic here has all been correct).

Which would then lead me to believe the comment in the test is bogus (and in fact, the instantApply pref thing that the test does might also be bogus). Am I missing something?

sets it based on the pref by default. Assuming the subdialogs

Comment 7

[:Gijs (he/him)]

(AFAICT, on current mac nightly, the connections dialog has instantApply set to false, despite the relevant pref being set to 'true', as is the default on mac, so comment #6 seems correct...)

Comment 8

[:Gijs (he/him)]

Maybe we can just nuke all this from orbit:

remote:   https://tbpl.mozilla.org/?tree=Try&rev=cdcccf3e6eea

Comment 9

[:Gijs (he/him)]

22:40	MattN	Gijs: did you see https://bugzilla.mozilla.org/show_bug.cgi?id=566444#c7 ?
22:40	firebot	Bug 566444 — FIXED, fracture91 — Make Proxy Exceptions Multi Line
22:40	Gijs	MattN: no.
22:40	* Gijs goes to read
22:40	MattN	ahurle says that the dialog didn't work on Windows without instantApply set
22:41	MattN	I guess we'll see if the same happens on try
22:41	Gijs	MattN: yeah. I wonder if something changed since then :\
22:42	Gijs	MattN: I mean, if the dialog doesn't work on Windows without instantApply set, that's an issue outside of the test, too :P
22:43	MattN	right, which is what jaws said there IIRC
22:44	MattN	Gijs: I know what changed. See https://hg.mozilla.org/try/annotat...wser/app/profile/firefox.js#l677
22:44	MattN	instantApply is always true now
22:44	MattN	that seems wrong probably
22:45	Gijs	MattN: huh?
22:45	Gijs	MattN: why?
22:45	MattN	that link is to when the patch landed
22:45	Gijs	oh!
22:45	Gijs	it's true by default now?
22:45	MattN	now we don't have the ifdef
22:45	MattN	yes
22:45	MattN	on all platforms
22:45	MattN	which means cancelling a dialog on Windows won't revert the change
22:46	Gijs	MattN: that seems very wrong
22:46	MattN	I agree
22:46	Gijs	MattN: are... are we sure about this?
22:46	MattN	I'm not sure that it won't revert but it's what it should do
22:46	* Gijs points back to him saying the pref doesn't make a difference and instantApply is always false on the document node
22:46	Gijs	when did this get changed?
22:46	MattN	not sure, I was going to look
22:46	Gijs	I find it hard to believe this would survive a nightly cycle before being noticed
22:47	MattN	Gijs: but you're talking about the connection dialog specifically. I'm talking about other dialogs too
22:47	MattN	some extension dialogs might not be "child"ren
22:47	Gijs	MattN: yes, though I'm technically talking about child dialogs generally
22:47	MattN	right
22:47	Gijs	also, I thought the display of buttons depended on instantApply anyway
22:47	* MattN too
22:47	Gijs	ie if instantApply is true you wouldn't have buttons on Windows
22:48	MattN	right
22:48	Gijs	so they were manually disabled by jaws for 32, readding the ifdef
22:48	Gijs	oh
22:48	Gijs	except then that broke tests and got backed out
22:48	Gijs	oh, because it should have been landed on aurora
22:48	Gijs	hmmm
22:51	MattN	Gijs: bug #?
22:53	Gijs	MattN: bug 1023957
22:53	firebot	https://bugzil.la/1023957 — FIXED, jaws — Disable the in-content preferences for Firefox 32
22:54	Gijs	argh, just missed mconley :(
22:56	MattN	Gijs: bug 738797 changed it
22:56	firebot	https://bugzil.la/738797 — FIXED, jaws — Enable the in-content preferences by default
22:58	Gijs	MattN: so, this is probably fine for in-content prefs, but not so much for regular dialogs :\
22:58	MattN	right
22:58	* Gijs pokes at windows nightly
22:58	MattN	I'll file a follow-up to fix this a different way
22:59	Gijs	MattN: oh, actually... it seems the buttons *do* change
22:59	Gijs	at least on Windows I'm seeing a "Close" button and a "Help" button, but no more OK/cancel
22:59	MattN	so how are people on Windows surviving? Clicking the X after changing something?
22:59	MattN	doesn't that feel weird
22:59	MattN	?
23:00	Gijs	MattN: they get in-content prefs by default
23:00	Gijs	you'd have to go to about:config to change it
23:00	Gijs	(and not change the instantApply pref, too)
23:00	Gijs	and 'cancel' still shows and works in the child dialogs
23:01	Gijs	MattN: so it's not altogether in a bad state
23:01	MattN	ok, so extensions that create non-child dialogs are kinda broken (non-native feel)
23:01	Gijs	although I'd argue the current state of a mismash of prefs and document element XBL fields isn't super-clear and could potentially be improved
23:01	Gijs	yes
23:01	Gijs	but not super-broken
23:01	Gijs	and tbh
23:02	Gijs	I see more people writing add-ons that assume pref windows work the way they do on their platform and making mistakes because of that
23:02	Gijs	than I'd see people that would make assumptions about this and get it right in the previous system but wrong when using dialogs with the pref twiddled.
23:04	MattN	I get the feeling we can make the in-content preference dialogs save properly (windowed or as an in-content subdialog) without changing the pref and risking compat issues
23:05	MattN	Thoughts?
23:06	MattN	(We can still make about:preferences ignore the pref if we want without affecting other pref windows)
23:10	Gijs	that sounds plausible, yes
23:10	Gijs	MattN: want to file that?
23:11	Gijs	Maybe we can do it next iteration
23:11	MattN	sure
23:13	Gijs	MattN: in any case, that sounds like we should leave the prefs in those tests until we fix the bug you're filing.
23:14	MattN	yeah, it's probably necessary
23:14	Gijs	MattN: and I can close this investigation bug, which got a little sidetracked, perhaps (but hopefully for a good cause :) )