Closed Bug 733469 Opened 12 years ago Closed 12 years ago

Move the applications preferences to in-content UI

Categories

(Firefox :: Settings UI, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 15

People

(Reporter: jon.rietveld, Assigned: jon.rietveld)

References

Details

(Whiteboard: [testday-20120622])

Attachments

(1 file, 10 obsolete files)

Move the current Preferences Applications pane to an in-content applications UI
Blocks: 718011
Attached patch in-content applications pane (obsolete) — Splinter Review
Moved the applications pane to in-content UI. Only changes I made were in the applications.xul file changing the wrapping box.

*Note: unsure about the correctness of the way I styled the applications pane for getting overflow to work.
Attachment #603378 - Flags: feedback?(jwein)
Attachment #603378 - Flags: feedback?(bmcbride)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment on attachment 603378 [details] [diff] [review]
in-content applications pane

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

::: browser/components/preferences/in-content/applications.xul
@@ +39,5 @@
> +# the terms of any one of the MPL, the GPL or the LGPL.
> +#
> +# ***** END LICENSE BLOCK *****
> +-->
> +<vbox id="applications-content" hidden="true">

We should remove this <vbox> and place a <vbox> around each preference as part of the search implementation.

::: browser/components/preferences/in-content/in-content.css
@@ +187,4 @@
>    color: #444;
>    text-shadow: 0 0 3px white;
>    background: -moz-linear-gradient(rgba(251,252,253,0.95), rgba(246,247,248,0) 49%,
> +              rgba(211,212,213,0.45) 51%, rgba(225,226,229, 0.3));

nit: Can you fix the indentation here to match the opening parenthesis of the -moz-linear-gradient( ?

@@ +236,4 @@
>  
>  #encrytionPanel {
>  
> +}

Can you remove the empty #encryptionPanel rule?
Attachment #603378 - Flags: feedback?(jwein) → feedback+
Attached patch applications in-content pane (obsolete) — Splinter Review
Fixed all issues addressed in Jared's feedback.
Attachment #605536 - Flags: feedback?(jwein)
Attachment #605536 - Flags: feedback?(bmcbride)
Attachment #603378 - Attachment is obsolete: true
Attachment #603378 - Flags: feedback?(bmcbride)
Assignee: nobody → jon.rietveld
Status: NEW → ASSIGNED
Comment on attachment 605536 [details] [diff] [review]
applications in-content pane

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

::: browser/components/preferences/in-content/applications.js
@@ +39,5 @@
> +# and other provisions required by the GPL or the LGPL. If you do not delete
> +# the provisions above, a recipient may use your version of this file under
> +# the terms of any one of the MPL, the GPL or the LGPL.
> +#
> +# ***** END LICENSE BLOCK *****

This should be MPL2.

@@ +53,5 @@
> +var Ci = Components.interfaces;
> +var Cr = Components.results;
> +/*
> +#endif
> +*/

These will need to be removed after the feedback is addressed for bug 733473.

::: browser/components/preferences/in-content/applications.xul
@@ +38,5 @@
> +# the provisions above, a recipient may use your version of this file under
> +# the terms of any one of the MPL, the GPL or the LGPL.
> +#
> +# ***** END LICENSE BLOCK *****
> +-->

MPL2 here too.

::: browser/components/preferences/in-content/in-content.css
@@ +182,4 @@
>    color: #444;
>    text-shadow: 0 0 3px white;
>    background: -moz-linear-gradient(rgba(251,252,253,0.95), rgba(246,247,248,0) 49%,
> +                              rgba(211,212,213,0.45) 51%, rgba(225,226,229, 0.3));

Please fix the indentation here.
Attachment #605536 - Flags: feedback?(jwein) → feedback+
Attached patch applications in-content pane (obsolete) — Splinter Review
fixed all feedback from Jared, and fixed indentation
Attachment #605536 - Attachment is obsolete: true
Attachment #608497 - Flags: feedback?(jwein)
Attachment #608497 - Flags: feedback?(bmcbride)
Attachment #605536 - Flags: feedback?(bmcbride)
Comment on attachment 608497 [details] [diff] [review]
applications in-content pane

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

::: browser/themes/winstripe/preferences/in-content/preferences.css
@@ +107,5 @@
>  
> +/* Applications Pane Styles */
> +
> +#applications-content {
> +  padding: 15px;

Is this padding duplicated for other categories too?
Attachment #608497 - Flags: feedback?(jwein) → feedback+
no, it is specifically used for the box in applications
Attached patch applications in-content pane (obsolete) — Splinter Review
fixed jar file spacing
Attachment #608497 - Attachment is obsolete: true
Attachment #608510 - Flags: feedback?(jwein)
Attachment #608510 - Flags: feedback?(bmcbride)
Attachment #608497 - Flags: feedback?(bmcbride)
Attachment #608510 - Flags: feedback?(jwein) → feedback+
Attached patch applications in-content pane (obsolete) — Splinter Review
added consistent title
Attachment #608510 - Attachment is obsolete: true
Attachment #609063 - Flags: feedback?(jwein)
Attachment #609063 - Flags: feedback?(bmcbride)
Attachment #608510 - Flags: feedback?(bmcbride)
Attached patch applications in-content pane (obsolete) — Splinter Review
included search vbox's, and fixed title
Attachment #609063 - Attachment is obsolete: true
Attachment #609224 - Flags: feedback?(jwein)
Attachment #609224 - Flags: feedback?(bmcbride)
Attachment #609063 - Flags: feedback?(jwein)
Attachment #609063 - Flags: feedback?(bmcbride)
Attachment #609224 - Flags: feedback?(jwein) → feedback+
Comment on attachment 609224 [details] [diff] [review]
applications in-content pane

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

::: browser/components/preferences/in-content/applications.js
@@ +7,5 @@
> +
> +/*
> +#ifndef XP_MACOSX
> +*/
> +var Cr = Components.results;

IIRC, this is an old hack that's no longer needed. Remove it all (comment markers, #ifdef, Components.results). And add Components.results to preferences.js, same as Components.classes et al.

::: browser/components/preferences/in-content/applications.xul
@@ +59,5 @@
> +
> +<keyset>
> +  <key key="&focusSearch1.key;" modifiers="accel" oncommand="gApplicationsPane.focusFilterBox();"/>
> +  <key key="&focusSearch2.key;" modifiers="accel" oncommand="gApplicationsPane.focusFilterBox();"/>
> +</keyset>

Nit: Move this <keyset> to above the <vbox> header (we generally try to group non-visible elements together at the top).

@@ +72,5 @@
> +</vbox>
> +
> +<separator class="thin"/>
> +
> +<vbox data-category="applications" hidden="true">

This and the <seperator> and the textbox above need to all be inside the same <vbox> (you can't use the filter without the list of things that it filters).

::: browser/themes/gnomestripe/preferences/in-content/preferences.css
@@ +98,5 @@
> +  padding: 15px;
> +}
> +
> +#handlersView {
> +  border: 1px solid rgba(31,64,100,0.4);

Use ThreeDLightShadow as the color here (on gnomestripe, we try to blend in with the system theme, so we usually can't use custom colours).

@@ +99,5 @@
> +}
> +
> +#handlersView {
> +  border: 1px solid rgba(31,64,100,0.4);
> +  height: 500px;

We generally don't use 'px' units when dealing with sizes that aren't for cosmetic purposes - 'em' is generally more appropriate. About 30em seems right here (applies to all themes).

::: browser/themes/winstripe/preferences/in-content/preferences.css
@@ +94,5 @@
> +  padding: 15px;
> +}
> +
> +#handlersView {
> +  border: 1px solid rgba(31,64,100,0.4);

I assume you got this colour from the winstripe theme - you'll need to find the relevant colour from the pinstripe theme (it's different).
Attachment #609224 - Flags: feedback?(bmcbride) → feedback+
Attached patch applications in-content patch (obsolete) — Splinter Review
removed vbox from heading, fixed blairs feedback, removed vbox around 1st child elements and added data-category and hidden to 1st child element instead
Attachment #609224 - Attachment is obsolete: true
Attachment #613231 - Flags: feedback?(jwein)
Attachment #613231 - Flags: feedback?(bmcbride)
Attachment #613231 - Flags: feedback?(jwein) → feedback+
Attachment #613231 - Attachment is obsolete: true
Attachment #614502 - Flags: feedback?(jwein)
Attachment #614502 - Flags: feedback?(bmcbride)
Attachment #613231 - Flags: feedback?(bmcbride)
Attachment #614502 - Attachment is obsolete: true
Attachment #614534 - Flags: feedback?(jwein)
Attachment #614534 - Flags: feedback?(bmcbride)
Attachment #614502 - Flags: feedback?(jwein)
Attachment #614502 - Flags: feedback?(bmcbride)
Attachment #614534 - Flags: feedback?(jwein) → feedback+
Attachment #613679 - Attachment is obsolete: true
Comment on attachment 614534 [details] [diff] [review]
applications in-content patch for new landing patch

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

::: browser/themes/gnomestripe/preferences/in-content/preferences.css
@@ +111,5 @@
> +}
> +
> +#handlersView {
> +  border: 1px solid ThreeDLightShadow;
> +  height: 30em;

Looking at this, it doesn't take up much space in a big window. It should really scale in such cases.

So change this to be max-height (same with the other themes), and add flex="1" to the <richlistbox> and it's parent <vbox>. That should make it adjust better to both large and small windows, and for when there are multiple things showing (when we get proper text search working).

::: browser/themes/pinstripe/preferences/in-content/preferences.css
@@ +113,5 @@
> +  padding: 15px;
> +}
> +
> +#handlersView {
> +  border: 1px solid ThreeDShadow;

The colour here should match what's used in the pinstripe theme - rgba(60,73,97,0.5)

Also, for the border colour to have any affect, you need to add the following (applies to gnomestripe and winstripe as well):

  -moz-appearance: none;
Attachment #614534 - Flags: feedback?(bmcbride) → feedback+
removed the height style completely, why waste available space?
Attachment #614534 - Attachment is obsolete: true
Attachment #616998 - Flags: feedback?(bmcbride)
Attachment #616998 - Flags: feedback?(bmcbride) → review+
https://hg.mozilla.org/mozilla-central/rev/bb179b7cf722
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
I verify fixed this bug, tested at Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120621 Firefox/15.0a2.
Status: RESOLVED → VERIFIED
Whiteboard: [testday-20120622]
You need to log in before you can comment on or make changes to this bug.