Last Comment Bug 733469 - Move the applications preferences to in-content UI
: Move the applications preferences to in-content UI
Status: VERIFIED FIXED
[testday-20120622]
:
Product: Firefox
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 15
Assigned To: Jon Rietveld
:
: Jared Wein [:jaws] (please needinfo? me)
Mentors:
Depends on:
Blocks: 718011
  Show dependency treegraph
 
Reported: 2012-03-06 11:06 PST by Jon Rietveld
Modified: 2012-06-22 04:43 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
in-content applications pane (78.69 KB, patch)
2012-03-06 11:25 PST, Jon Rietveld
jaws: feedback+
Details | Diff | Splinter Review
applications in-content pane (78.38 KB, patch)
2012-03-13 14:14 PDT, Jon Rietveld
jaws: feedback+
Details | Diff | Splinter Review
applications in-content pane (75.58 KB, patch)
2012-03-22 15:06 PDT, Jon Rietveld
jaws: feedback+
Details | Diff | Splinter Review
applications in-content pane (75.57 KB, patch)
2012-03-22 15:43 PDT, Jon Rietveld
jaws: feedback+
Details | Diff | Splinter Review
applications in-content pane (75.78 KB, patch)
2012-03-24 20:08 PDT, Jon Rietveld
no flags Details | Diff | Splinter Review
applications in-content pane (76.08 KB, patch)
2012-03-25 23:06 PDT, Jon Rietveld
jaws: feedback+
blair: feedback+
Details | Diff | Splinter Review
applications in-content patch (75.97 KB, patch)
2012-04-08 22:51 PDT, Jon Rietveld
jaws: feedback+
Details | Diff | Splinter Review
Applications with updated data-category (75.98 KB, patch)
2012-04-10 11:07 PDT, MSU Capstone Team
no flags Details | Diff | Splinter Review
applications in-content patch for new landing patch (75.99 KB, patch)
2012-04-12 12:39 PDT, Jon Rietveld
no flags Details | Diff | Splinter Review
applications in-content patch for new landing patch (76.09 KB, patch)
2012-04-12 13:19 PDT, Jon Rietveld
jaws: feedback+
blair: feedback+
Details | Diff | Splinter Review
applications in-content patch (76.02 KB, patch)
2012-04-20 09:26 PDT, Jon Rietveld
jaws: review+
blair: review+
Details | Diff | Splinter Review

Description Jon Rietveld 2012-03-06 11:06:56 PST
Move the current Preferences Applications pane to an in-content applications UI
Comment 1 Jon Rietveld 2012-03-06 11:25:49 PST
Created attachment 603378 [details] [diff] [review]
in-content applications pane

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.
Comment 2 Jared Wein [:jaws] (please needinfo? me) 2012-03-09 14:18:26 PST
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?
Comment 3 Jon Rietveld 2012-03-13 14:14:07 PDT
Created attachment 605536 [details] [diff] [review]
applications in-content pane

Fixed all issues addressed in Jared's feedback.
Comment 4 Jared Wein [:jaws] (please needinfo? me) 2012-03-22 11:15:47 PDT
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.
Comment 5 Jon Rietveld 2012-03-22 15:06:27 PDT
Created attachment 608497 [details] [diff] [review]
applications in-content pane

fixed all feedback from Jared, and fixed indentation
Comment 6 Jared Wein [:jaws] (please needinfo? me) 2012-03-22 15:12:31 PDT
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?
Comment 7 Jon Rietveld 2012-03-22 15:30:29 PDT
no, it is specifically used for the box in applications
Comment 8 Jon Rietveld 2012-03-22 15:43:17 PDT
Created attachment 608510 [details] [diff] [review]
applications in-content pane

fixed jar file spacing
Comment 9 Jon Rietveld 2012-03-24 20:08:50 PDT
Created attachment 609063 [details] [diff] [review]
applications in-content pane

added consistent title
Comment 10 Jon Rietveld 2012-03-25 23:06:12 PDT
Created attachment 609224 [details] [diff] [review]
applications in-content pane

included search vbox's, and fixed title
Comment 11 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-04-03 05:25:36 PDT
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).
Comment 12 Jon Rietveld 2012-04-08 22:51:21 PDT
Created attachment 613231 [details] [diff] [review]
applications in-content patch

removed vbox from heading, fixed blairs feedback, removed vbox around 1st child elements and added data-category and hidden to 1st child element instead
Comment 13 MSU Capstone Team 2012-04-10 11:07:51 PDT
Created attachment 613679 [details] [diff] [review]
Applications  with updated data-category
Comment 14 Jon Rietveld 2012-04-12 12:39:02 PDT
Created attachment 614502 [details] [diff] [review]
applications in-content patch for new landing patch
Comment 15 Jon Rietveld 2012-04-12 13:19:23 PDT
Created attachment 614534 [details] [diff] [review]
applications in-content patch for new landing patch
Comment 16 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-04-16 02:48:45 PDT
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;
Comment 17 Jon Rietveld 2012-04-20 09:26:28 PDT
Created attachment 616998 [details] [diff] [review]
applications in-content patch

removed the height style completely, why waste available space?
Comment 18 Jared Wein [:jaws] (please needinfo? me) 2012-05-08 22:40:05 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb179b7cf722
Comment 19 Ed Morley [:emorley] 2012-05-09 07:43:02 PDT
https://hg.mozilla.org/mozilla-central/rev/bb179b7cf722
Comment 20 Alfredos-Panagiotis Damkalis [:fredy] 2012-06-22 04:43:10 PDT
I verify fixed this bug, tested at Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120621 Firefox/15.0a2.

Note You need to log in before you can comment on or make changes to this bug.