Closed Bug 1035308 Opened 6 years ago Closed 6 years ago
Investigate use of accept
Dialog/cancel Dialog in in-content prefs tests
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...
Annnnd another one.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 33.3
Points: --- → 1
QA Whiteboard: [qa-]
Added to the priority list for Iteration 33.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) ?) /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... :-\
(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.
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.
(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
(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...)
Maybe we can just nuke all this from orbit: remote: https://tbpl.mozilla.org/?tree=Try&rev=cdcccf3e6eea
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 :) )
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.