Matthew N. [:MattN] Assignee
	Description • 10 years ago

Following up on bug 752197 comment 48 - 50 here since that is a meta bug.

I'm going to create the helper functions and event listeners to get get dialogs working in-content with minimal changes to the dialogs themseleves.

This is a very rough proof-of-concept that will I clean up more but it shows the direction that I'm heading. This only covers selectBookmark.xul at the moment but notice how I didn't have to change that dialog at all or any of the code that was checking the return values. If the in-content prefs code was still using document.documentElement.openSubDialog like the windowed version, there would have been less to change. This also avoid @id collisions and variable scoping issues (e.g. const Cc/Ci) if we combine the dialogs in one dialog. There should be a performance benefit to only constructing dialogs on demand as well.

Marco, can you add this to the current iteration?

	Marco Mucci [:MarcoM]
	Comment 1 • 10 years ago

Added to Iteration 32.3

	Richard Marti (:Paenglab)
	Comment 2 • 10 years ago

This is looking very promising.

I hope I don't interrupt you in the progress of this work but I have some questions.

Would it be possible for the prefsDialog to adapt to the size the containing dialog has? Then the size doesn't need to be hardcoded.

Would it be to give the titlebar the correct dialog title?

And for now the last question, would it be possible to inject the in-content preferences.css file to the dialogs? Then the dialogs could use the same styling as the main in-content prefs.

	Matthew N. [:MattN] Assignee
	Comment 3 • 10 years ago

(In reply to Richard Marti (:Paenglab) from comment #2)
> This is looking very promising.
> 
> I hope I don't interrupt you in the progress of this work but I have some
> questions.

Not at all, you know more about the requirements and UI design and it's good to get feedback before I go down the wrong path.

> Would it be possible for the prefsDialog to adapt to the size the containing
> dialog has? Then the size doesn't need to be hardcoded.

Yes, that should be possible. If one was specified we can use that, otherwise either hard-code a default or I can try get sizeToContents to work (which is what I believe it would do in a dialog window).

> Would it be to give the titlebar the correct dialog title?

That should also be easy.

> And for now the last question, would it be possible to inject the in-content
> preferences.css file to the dialogs? Then the dialogs could use the same
> styling as the main in-content prefs.

Yes, that's the plan.

Could you point me to any spec on how this should work/look to make sure this approach will work with all requirements?

Thanks for the feedback and ideas.

	Richard Marti (:Paenglab)
	Comment 4 • 10 years ago

(In reply to Matthew N. [:MattN] from comment #3)
> 
> Could you point me to any spec on how this should work/look to make sure
> this approach will work with all requirements?

The spec should be from Chameleon, <http://people.mozilla.org/~jgruen/chameleon/>. The buttons etc. should look like on the normal in-content prefs.

With adding the in-content css link as last in the dialog's css link chain it should automatically work like the normal in-content prefs. Then it can be easily adapted to the UI needs.

	Matthew N. [:MattN] Assignee
	Comment 5 • 10 years ago

I didn't see anything specific to dialogs in the chameleon spec though. e.g. the background color, titlebar, etc.

	Richard Marti (:Paenglab)
	Comment 6 • 10 years ago

mmaslaney wrote in bug 752197 comment 23 in reply to my screenshot, it's looking okay like it is but the specs aren't finished. But as I wrote before with the in-content css this can be easily adapted after the specs are finished.

	Matthew N. [:MattN] Assignee
	Comment 7 • 10 years ago

One limitation of this approach over including all dialogs in the one document is that the strings from the dialogs are not easily searchable for the upcoming search feature. Looking through all of the dialogs, I don't think that's a big deal since we can reference any relevant dialog strings from the regular panes to help index them in the few cases where it will be useful.

	Matthew N. [:MattN] Assignee
	Comment 8 • 10 years ago

Hey Jared,

This is a working patch that needs some polish but I think there's enough that's likely not going to change that I'd like to start the review in case there are problems with the approach.

This still needs automated tests which I will work on in parallel to review.

Richard, would you mind testing this patch? I addressed your previous comments.

Thanks

Notes to myself:
TODO: add automated test for key handlers from https://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/dialog.xml#410
TODO: check temporary colors with mmaslaney
TODO: check a11y
TODO: JSDoc comments

	Richard Marti (:Paenglab)
	Comment 9 • 10 years ago

Comment on attachment 8437516 [details] [diff] [review]
v.0.9 Add gSubDialog helper

This is looking great.

Only two things:
- The Fonts dialog (Content/Advanced) is not tall enough and the Colors dialog is to tall with the 400px. Is it possible to give a custom height to the calls?
- Both dialogs behind Content/Exceptions are using a Close button using oncommand="close();" instead of the Cancel and can't be closed with this button.

	Dão Gottwald [:dao]
	Comment 10 • 10 years ago

(In reply to Richard Marti (:Paenglab) from comment #9)
> - The Fonts dialog (Content/Advanced) is not tall enough and the Colors
> dialog is to tall with the 400px. Is it possible to give a custom height to
> the calls?

If custom heights are needed, they'll need to be locale-specific and em based (not px).

	Richard Marti (:Paenglab)
	Comment 11 • 10 years ago

This are some CSS tweaks on top of your patch. It makes the dialogClose button having the correct width/height. Adds the colorpicker to the CSS and changes the dialog background color to white like the main preferences to better accent the buttons.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 12 • 10 years ago

Comment on attachment 8437516 [details] [diff] [review]
v.0.9 Add gSubDialog helper

Review of attachment 8437516 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this is better than moving all of the dialogs to a separate file.

::: browser/components/preferences/in-content/content.js
@@ +61,5 @@
>      params.windowTitle = bundlePreferences.getString("popuppermissionstitle");
>      params.introText = bundlePreferences.getString("popuppermissionstext");
>  
> +    gSubDialog.open("chrome://browser/content/preferences/permissions.xul",
> +                    "resizable=yes", params);

The "Close" button doesn't work in this dialog.

@@ +152,5 @@
>     * configured.
>     */  
>    configureFonts: function ()
>    {
> +    gSubDialog.open("chrome://browser/content/preferences/fonts.xul");

The Fonts dialog is not as large as the pop-out version, http://screencast.com/t/QZmfHWcVP7 which causes the bottom of the dialog to be clipped.

::: browser/components/preferences/in-content/preferences.js
@@ +107,5 @@
> +
> +  _onLoad: function(aEvent) {
> +    if (aEvent.target.contentWindow.location == "about:blank")
> +      return;
> +    this._overlay.style.visibility = "visible";

At this point, isn't the visibility already "visible"? So this is a no-op.

::: browser/components/preferences/in-content/preferences.xul
@@ +176,5 @@
>  
>    </hbox>
> +
> +  <!-- TODO: aria -->
> +  <!-- TODO: fix indentation? -->

the indentation was set up this way originally so as not to break the hg-blame of the hg-copy'd file. i think we can go ahead and make the change now though, but it will mean that hg-blame users will have to look at this parent cset to see the history of this file. if you do make this change, please land it as a separate commit.

	Matthew N. [:MattN] Assignee
	Comment 13 • 10 years ago

Now with tests!

Flagging for a11y-review since I'm making "fake" in-content dialogs containing a frame with the existing dialog window contents and I want to make sure that it's accessible. You can test four sub-dialogs from the first two pages of in-content preferences: "Use Bookmark…", "Exceptions…", "Colors…", & "Choose…". Others will be converted in follow-up bugs.

Richard, can you test for any blocking bugs that should be fixed before landing and comment here. Other issues can be filed as a follow-up (dependency).

(In reply to Richard Marti (:Paenglab) from comment #9)
> - The Fonts dialog (Content/Advanced) is not tall enough and the Colors
> dialog is to tall with the 400px. Is it possible to give a custom height to
> the calls?

The Font dialog issue is because of the larger element sizes I think (from the injected CSS). I've left that dialog call alone for now so it can be handled in a follow-up since there is enough going on in this patch.

> - Both dialogs behind Content/Exceptions are using a Close button using
> oncommand="close();" instead of the Cancel and can't be closed with this
> button.

I've fixes this now by injecting a custom close function since the default one doesn't work when it's not in a top-level window.

(In reply to Richard Marti (:Paenglab) from comment #11)
> Created attachment 8437855 [details] [diff] [review]
> incontent_dialog_csstweaks.patch
> 
> This are some CSS tweaks on top of your patch. It makes the dialogClose
> button having the correct width/height. Adds the colorpicker to the CSS and
> changes the dialog background color to white like the main preferences to
> better accent the buttons.

I've included some of these into the patch where it would cause a big regression without the change. You can submit the rest of the changes to a new bug once this lands.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #12)
> 
> @@ +152,5 @@
> >     * configured.
> >     */  
> >    configureFonts: function ()
> >    {
> > +    gSubDialog.open("chrome://browser/content/preferences/fonts.xul");
> 
> The Fonts dialog is not as large as the pop-out version,
> http://screencast.com/t/QZmfHWcVP7 which causes the bottom of the dialog to
> be clipped.

I've postponed the Fonts dialog to a follow-up for now as I think part of the problem there is the larger UI widgets/fonts. I think this should be better now anyways. You can test by running the quoted addition in the web console while on the prefs page.

> ::: browser/components/preferences/in-content/preferences.js
> @@ +107,5 @@
> > +
> > +  _onLoad: function(aEvent) {
> > +    if (aEvent.target.contentWindow.location == "about:blank")
> > +      return;
> > +    this._overlay.style.visibility = "visible";
> 
> At this point, isn't the visibility already "visible"? So this is a no-op.

Yes, it is but that's only because of the hack I have now so the focus works. See the TODO comment in the DOMContentLoaded function above. Once the temporary hack lines are removed, this is the line that should be setting the visibility.

> ::: browser/components/preferences/in-content/preferences.xul
> @@ +176,5 @@
> >  
> >    </hbox>
> > +
> > +  <!-- TODO: aria -->
> > +  <!-- TODO: fix indentation? -->
> 
> the indentation was set up this way originally so as not to break the
> hg-blame of the hg-copy'd file.

I was referring to changing the indentation because of the <stack> I added, not existing indentation problems. I was also trying to avoid changing the blame from adding the <stack> which is why I had a question mark to think about whether I should change it.

	Richard Marti (:Paenglab)
	Comment 14 • 10 years ago

Comment on attachment 8441918 [details] [diff] [review]
v.1 Add gSubDialog helper with tests

This is looking good.

I have already moved all dialogs to in-content for bug 752197. Everything is working except for the master password dialogs in security.js. I haven't checked deeply but it looks like the gSubDialog.open() returns immediately and not like openDialog() returns after closing (this is only a guess but it looks like this because the this._initMasterPasswordUI() isn't updating the new state). See: http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/security.js#205

If this can be fixed here, then okay for me ;). If not then I can try to fix this in bug 752197. I'll upload my WIP patch this evening, so you can test it with it.

	Matthew N. [:MattN] Assignee
	Comment 15 • 10 years ago

(In reply to Richard Marti (:Paenglab) from comment #14)
> I have already moved all dialogs to in-content for bug 752197. Everything is
> working except for the master password dialogs in security.js. I haven't
> checked deeply but it looks like the gSubDialog.open() returns immediately
> and not like openDialog() returns after closing (this is only a guess but it
> looks like this because the this._initMasterPasswordUI() isn't updating the
> new state). See:
> http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/
> in-content/security.js#205

Yes, gSubDialog.open doesn't wait for the dialog to close so you need to do like I did for the homepage bookmark picker and use the new 4th argument to open which is a closing callback where you can run this._initMasterPasswordUI();

Thanks for testing!

	Richard Marti (:Paenglab)
	Comment 16 • 10 years ago

Comment on attachment 8441918 [details] [diff] [review]
v.1 Add gSubDialog helper with tests

Review of attachment 8441918 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/shared/incontentprefs/preferences.css
@@ +832,5 @@
> +  display: -moz-box;
> +}
> +
> +.close-icon {
> +  background: none !important;

This should be background-color: transparent
Linux uses background-image to show the glyph and this rule makes it not showing.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 17 • 10 years ago

Comment on attachment 8441918 [details] [diff] [review]
v.1 Add gSubDialog helper with tests

Review of attachment 8441918 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/preferences/in-content/tests/browser_subdialogs.js
@@ +87,5 @@
> +  run: function* () {
> +    let rv = { acceptCount: 0 };
> +    let deferredClose = Promise.defer();
> +    let dialogPromise = openAndLoadSubDialog(gDialogURL, null, rv,
> +                                             (aEvent) => dialogClosingCallback(deferredClose, aEvent));   let dialog = yield dialogPromise;

`let dialog = yield dialogPromise;` should be on its own line.

::: browser/components/preferences/in-content/tests/head.js
@@ +90,5 @@
> + * @param aEventName the event to wait for
> + * @param aTimeoutMs the number of miliseconds to wait before giving up
> + * @returns a Promise that resolves to the received event, or to an Error
> + */
> +function waitForEvent(aSubject, aEventName, aTimeoutMs, aTarget) {

Any way we can look in to consolidating this waitForEvent code? Perhaps putting this in SimpleTest.js and then filing a bug for others to migrate to the shared implementation?

::: browser/themes/shared/incontentprefs/preferences.css
@@ +827,5 @@
> +}
> +
> +#dialogBox {
> +  border: 1px solid #666;
> +  box-shadow: 6px 6px 5px rgba(0,0,0,0.3);

Let's drop the box-shadow from here since we don't have a box shadow on tab-modal dialogs like alert(). Please also file a follow-up and assign it to mmaslaney to get some styling defined for these dialogs.

	Marco Zehe (:MarcoZ)
	Comment 18 • 10 years ago

Comment on attachment 8441918 [details] [diff] [review]
v.1 Add gSubDialog helper with tests

Review of attachment 8441918 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/preferences/in-content/preferences.xul
@@ +177,5 @@
>  
>    </hbox>
> +
> +    <vbox id="dialogOverlay" align="center" pack="center">
> +      <groupbox id="dialogBox" role="dialog" orient="vertical" pack="end">

Add an aria-labelledby="dialogTitle" here, otherwise the association will not work correctly and the dialog will be titleless for screen readers.

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 19 • 10 years ago

Comment on attachment 8441918 [details] [diff] [review]
v.1 Add gSubDialog helper with tests

Removing feedback. I'll be doing a more thorough review of sighted keyboard use for in content prefs later anyway.

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 20 • 10 years ago

I managed to test the in-content "Use Bookmark" dialog but was unable to use the keyboard to interact with the dialog :(

(Note I've filed bug 1029586 as a catch all for keyboard access)

	Matthew N. [:MattN] Assignee
	Comment 21 • 10 years ago

(In reply to David Bolter [:davidb] from comment #20)
> I managed to test the in-content "Use Bookmark" dialog but was unable to use
> the keyboard to interact with the dialog :(

Weird, it works for me on OS X. What OS were you on? 

> (Note I've filed bug 1029586 as a catch all for keyboard access)

Does that mean that this issue doesn't need to block landing?

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 22 • 10 years ago

(In reply to Matthew N. [:MattN] from comment #21)
> (In reply to David Bolter [:davidb] from comment #20)
> > I managed to test the in-content "Use Bookmark" dialog but was unable to use
> > the keyboard to interact with the dialog :(
> 
> Weird, it works for me on OS X. What OS were you on? 

10.9.3

> 
> > (Note I've filed bug 1029586 as a catch all for keyboard access)
> 
> Does that mean that this issue doesn't need to block landing?

It would regress a11y so we should block. But let me test again...

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 23 • 10 years ago

(In reply to David Bolter [:davidb] from comment #22)
> (In reply to Matthew N. [:MattN] from comment #21)
> > (In reply to David Bolter [:davidb] from comment #20)
> > > I managed to test the in-content "Use Bookmark" dialog but was unable to use
> > > the keyboard to interact with the dialog :(
> > 
> > Weird, it works for me on OS X. What OS were you on? 
> 
> 10.9.3

As per IRC (thanks!), this does work but there was no initial indication of focus. You mentioned that was a known issue?

	Matthew N. [:MattN] Assignee
	Comment 24 • 10 years ago

(In reply to Jared Wein [:jaws] from comment #17)
> ::: browser/components/preferences/in-content/tests/head.js
> > …
> > +function waitForEvent(aSubject, aEventName, aTimeoutMs, aTarget) {
> 
> Any way we can look in to consolidating this waitForEvent code? Perhaps
> putting this in SimpleTest.js and then filing a bug for others to migrate to
> the shared implementation?

I'll file a follow-up since that would require a testing peer review and then there will be bikeshedding over the API.

> Please also file a follow-up and assign it
> to mmaslaney to get some styling defined for these dialogs.

Will do tomorrow.

(In reply to David Bolter [:davidb] from comment #23)
> (In reply to David Bolter [:davidb] from comment #22)
> > (In reply to Matthew N. [:MattN] from comment #21)
> > > (In reply to David Bolter [:davidb] from comment #20)
> > > > I managed to test the in-content "Use Bookmark" dialog but was unable to use
> > > > the keyboard to interact with the dialog :(
> > > 
> > > Weird, it works for me on OS X. What OS were you on? 
> > 
> > 10.9.3
> 
> As per IRC (thanks!), this does work but there was no initial indication of
> focus. You mentioned that was a known issue?

I was thinking of bug 1010115 but the bad state their was a bit different. Either way, this bug didn't regress it for in-content prefs.

Pushed accidentally to inbound instead of fx-team: https://hg.mozilla.org/integration/mozilla-inbound/rev/24fb014f1ae3

	Carsten Book [:Tomcat]
	Comment 25 • 10 years ago

sorry had to back this out for memory leaks like https://tbpl.mozilla.org/php/getParsedLog.php?id=42422182&tree=Mozilla-Inbound

	Matthew N. [:MattN] Assignee
	Comment 26 • 10 years ago

Possible leak/intermittent fix: https://tbpl.mozilla.org/?tree=Try&rev=a9f4c289820c

	Matthew N. [:MattN] Assignee
	Comment 27 • 10 years ago

https://hg.mozilla.org/integration/fx-team/rev/0c9ed31fcb96

QA can test four sub-dialogs from the first two pages of in-content preferences: "Use Bookmark…", "Exceptions…", "Colors…", & "Choose…". Others will be converted in follow-up bugs.

	Ryan VanderMeulen [:RyanVM]
	Comment 28 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/0c9ed31fcb96

	Sören Hentzschel
	Comment 29 • 10 years ago

Will the alignment issues and the squashed close icon (at least on OS X with Retina display) be fixed by bug 752197 and/or other follow-up existing bugs or should I file new bugs?

	Richard Marti (:Paenglab)
	Comment 30 • 10 years ago

The alignment issues should be fixed in bug 752197. The squashed close button needs a change in global.css.

http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/themes/osx/global/global.css#355 needs a new selector: .close-icon > .button-box > .button-icon.

Please file for this a new bug and cc me to it. I'll add then a patch.

	Camelia Badau [:cbadau], Desktop QA
	Comment 31 • 10 years ago

Verified on Windows 7 64bit, Ubuntu 13.10 32bit and Mac 0SX 10.9.2 using latest Nightly 33.0a1 (buildID: 20140629030206) and I have the following comments: 
- the Close button is missing from Exceptions (Content tab) on Mac OSX (on Windows and Ubuntu the Close button is correctly displayed) - is this ok? is this intended behaviour?  
- on Mac OSX: some button labels of the "Choose" sub-dialog (Content tab) aren't entirely visible. Please see screenshot "Choose.PNG"

	Richard Marti (:Paenglab)
	Comment 32 • 10 years ago

In bug 752197 I'm widening the dialogs from 30em to 35em. Maybe this solves the problem of the cut of labels.

	Dão Gottwald [:dao]
	Comment 33 • 10 years ago

(In reply to Richard Marti (:Paenglab) from comment #32)
> In bug 752197 I'm widening the dialogs from 30em to 35em. Maybe this solves
> the problem of the cut of labels.

The label size varies across locales.

	Richard Marti (:Paenglab)
	Comment 34 • 10 years ago

(In reply to Camelia Badau, QA [:cbadau] from comment #31)
> Created attachment 8447953 [details]
> Choose.png
> 
> Verified on Windows 7 64bit, Ubuntu 13.10 32bit and Mac 0SX 10.9.2 using
> latest Nightly 33.0a1 (buildID: 20140629030206) and I have the following
> comments: 
> - the Close button is missing from Exceptions (Content tab) on Mac OSX (on
> Windows and Ubuntu the Close button is correctly displayed) - is this ok? is
> this intended behaviour?

The Close button is on OS X on compile removed. Maybe file a bug to ask UX if this button should added to OS X

> - on Mac OSX: some button labels of the "Choose" sub-dialog (Content tab)
> aren't entirely visible. Please see screenshot "Choose.PNG"

I fixed this in bug 752197.

PS: The menulist (Select a Language to...) has always a ellipsis also when the dialog is wide enough.

	Camelia Badau [:cbadau], Desktop QA
	Comment 35 • 10 years ago

I filled bug 1032790 for the missing Close button on Mac OSX.

	Matthew N. [:MattN] Assignee
	Comment 36 • 10 years ago

(In reply to Matthew N. [:MattN] (behind on bugmail) from comment #24)
> (In reply to Jared Wein [:jaws] from comment #17)
> > ::: browser/components/preferences/in-content/tests/head.js
> > > …
> > > +function waitForEvent(aSubject, aEventName, aTimeoutMs, aTarget) {
> > 
> > Any way we can look in to consolidating this waitForEvent code? Perhaps
> > putting this in SimpleTest.js and then filing a bug for others to migrate to
> > the shared implementation?
> 
> I'll file a follow-up since that would require a testing peer review and
> then there will be bikeshedding over the API.

Filed bug 1036089.

> > Please also file a follow-up and assign it
> > to mmaslaney to get some styling defined for these dialogs.

Filed bug 1036090.