Closed Bug 1021618 Opened 6 years ago Closed 6 years ago

Create the foundation for converting preference dialogs to be in-content

Categories

(Firefox :: Preferences, defect)

defect
Not set
Points:
8

Tracking

()

VERIFIED FIXED
Firefox 33
Iteration:
33.2

People

(Reporter: MattN, Assigned: MattN)

References

Details

Attachments

(2 files, 3 obsolete files)

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?
Flags: needinfo?(mmucci)
Flags: firefox-backlog+
Added to Iteration 32.3
Flags: needinfo?(mmucci)
Whiteboard: p=5 → p=5 s=it-32c-31a-30b.3 [qa?]
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.
(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.
Flags: needinfo?(richard.marti)
(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.
Flags: needinfo?(richard.marti)
I didn't see anything specific to dialogs in the chameleon spec though. e.g. the background color, titlebar, etc.
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.
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.
Whiteboard: p=5 s=it-32c-31a-30b.3 [qa?] → p=5 s=33.1 [qa?]
Attached patch v.0.9 Add gSubDialog helper (obsolete) — Splinter Review
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
Attachment #8435672 - Attachment is obsolete: true
Attachment #8437516 - Flags: review?(jaws)
Attachment #8437516 - Flags: feedback?(richard.marti)
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.
Attachment #8437516 - Flags: feedback?(richard.marti) → feedback+
(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).
Attached patch incontent_dialog_csstweaks.patch (obsolete) — Splinter Review
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.
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.
Attachment #8437516 - Flags: review?(jaws)
Attachment #8437516 - Flags: feedback?
Attachment #8437516 - Flags: feedback+
Whiteboard: p=5 s=33.1 [qa?] → p=5 s=33.1 [qa+]
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.
Attachment #8437516 - Attachment is obsolete: true
Attachment #8437855 - Attachment is obsolete: true
Attachment #8437516 - Flags: feedback?
Attachment #8441918 - Flags: review?(jaws)
Attachment #8441918 - Flags: feedback?(richard.marti)
Attachment #8441918 - Flags: a11y-review?(dbolter)
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.
Attachment #8441918 - Flags: feedback?(richard.marti) → feedback+
(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!
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.
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.
Attachment #8441918 - Flags: review?(jaws) → review+
Attachment #8441918 - Flags: feedback?(dbolter)
Attachment #8441918 - Flags: a11y-review?(marco.zehe)
Attachment #8441918 - Flags: a11y-review?(dbolter)
Iteration: --- → 33.2
Points: --- → 5
QA Whiteboard: [qa+]
Whiteboard: p=5 s=33.1 [qa+]
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.
Attachment #8441918 - Flags: a11y-review?(marco.zehe) → a11y-review+
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.
Attachment #8441918 - Flags: feedback?(dbolter)
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)
(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?
(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...
(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?
(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
Points: 5 → 8
Flags: needinfo?(MattN+bmo)
Flags: in-testsuite+
QA Contact: camelia.badau
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.
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/0c9ed31fcb96
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
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?
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.
Attached image 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?  
- on Mac OSX: some button labels of the "Choose" sub-dialog (Content tab) aren't entirely visible. Please see screenshot "Choose.PNG"
In bug 752197 I'm widening the dialogs from 30em to 35em. Maybe this solves the problem of the cut of labels.
(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.
(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.
I filled bug 1032790 for the missing Close button on Mac OSX.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa+] → [qa!]
Depends on: 1032790
Depends on: 1033570
(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.
Flags: needinfo?(MattN+bmo)
You need to log in before you can comment on or make changes to this bug.