734013 - Implement the pane-switching functionality for the in-content preferences

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

The design of the in-content preferences allows for dynamic searches to be run against the preferences.

I have put together a basic example of how this could work (http://people.mozilla.org/~jwein/search.html).

There are some questions about how easy it will be for us to get localizers to fill in the necessary "data-text" attribute which should contain the text of the preferences as well as any keywords that should be added.

We can look at those questions while implementing this though, as there may be other approaches to fix that problem that won't require a complete change of direction.

Comment 1

[MSU Capstone Team]

Attachment: Search implementation for tabs pane

Hey, I'm so sorry I had this patch done before I went on vacation and totally forgot to submit it, I have the search functionality working for the tabs pane as well as the pane switching functionality for the tabs and privacy panes.  I have some questions on how things should be shown but my method works and all I should have to do is apply the search and search-category attributes to the right elements and the search should be working.  I can ask these questions at our meeting tonight.

Comment 2

[MSU Capstone Team]

Attachment: Search patch including search.js

Sorry I forgot to add the search.js file to my original patch, it is included here.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 605035 [details] [diff] [review]
Search patch including search.js

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

The performance of the search algorithm will need to be improved and the patch will need to rebased on top of Jon's in bug 733473.

::: browser/app/profile/firefox.js
@@ +609,2 @@
>  #else
>  pref("browser.preferences.instantApply", true);

The #ifdef and #else case match here. We should just remove the conditional check for Windows.

::: browser/components/incontentPreferences/jar.mn
@@ +33,5 @@
>  *   content/browser/incontentPreferences/preferences.xul
>  *   content/browser/incontentPreferences/privacy.xul
>  *   content/browser/incontentPreferences/privacy.js
>  *   content/browser/incontentPreferences/sanitize.xul
> +*   content/browser/incontentPreferences/search.js

This patch looks like it's based on an earlier obsolete patch of the in-content preferences. Can you rebase this patch on top of Jon's patch that renamed the folder to preferences/in-content/ ?

::: browser/components/incontentPreferences/landing.xul
@@ +42,5 @@
>        <image class="options-icon general-img" id="paneGeneral" type="general"/>
>        <p>&paneGeneral.title;</p>
>      </vbox>
>  
> +    <vbox class="options-box" onclick="searchCategory('tabs')">

Can you test if this is keyboard accessible?

::: browser/components/incontentPreferences/preferences.xul
@@ +112,4 @@
>  </hbox>
>      <hbox class="main-content" id="pref-box">
> +		<xhtml:h1 id="search-results"
> +				  hidden="true">Search Results</xhtml:h1>

Any human-facing text will need to be moved to a DTD or properties file.

::: browser/components/incontentPreferences/privacy.xul
@@ +96,5 @@
>                    type="bool"/>
>  
>      </preferences>
>  
> +<vbox id="privacy-content"

Shouldn't this <vbox> be removed?

@@ +107,5 @@
>      <!-- Tracking -->
>      <groupbox id="trackingGroup">
> +      <caption label="&tracking.label;"
> +				hidden="true"
> +				search-category="privacy"/>

Please prefix the search attributes with "data-". See here for more information: http://www.w3.org/TR/html5/elements.html#embedding-custom-non-visible-data-with-the-data-attributes

::: browser/components/incontentPreferences/search.js
@@ +1,1 @@
> +

There needs to be a license block here. Please use MPL 2.0 license.

@@ +1,4 @@
> +
> +
> +function search() {
> +	var query;

nit: replace tabs with spaces. only use 2 spaces for indents. use let instead of var. See https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide for more information on general style and https://developer.mozilla.org/en/New_in_JavaScript_1.7#let_statement for more information on the |let| statement.

@@ +4,5 @@
> +	var query;
> +	var page = '-content';
> +	query = document.getElementById("header-search").value;
> +
> +	var searchAgainst = new Array();

nit: don't use |new Array()|. use the array literal in its place, such as: |var searchAgainst = [];| Also, please merge this line with the following line (and use |let| too ;) ).

For example: let searchAgainst = document.querySelectorAll("[data-category]");

@@ +5,5 @@
> +	var page = '-content';
> +	query = document.getElementById("header-search").value;
> +
> +	var searchAgainst = new Array();
> +	searchAgainst = document.getElementsByTagName("*");

We need a faster way than getting all of the elements in a page. Can you filter based on the related search attribute? For example: document.querySelectorAll("[data-category]");

See the JS in my seach demo for an example: http://people.mozilla.org/~jwein/search.html

@@ +25,5 @@
> +					searchAgainst[i].hidden = true;
> +				}
> +			}
> +		}
> +		if(results == 0) document.getElementById('no-search-results').hidden = false;

It's a better style to make these conditionals handle the truthy case first. For example: if (results) { ... } else { ... }.

@@ +41,5 @@
> +	document.getElementById('landing-content').hidden = true;
> +	for(var i=0; i<max; i++){
> +		var search = searchAgainst[i].getAttribute('search-category');
> +		if(search){
> +			if(search == category){

How would this handle mixed case queries, such as PriVaCy or General?

::: browser/components/incontentPreferences/tabs.xul
@@ +26,5 @@
>  				preference="browser.link.open_newwindow"
>  				onsyncfrompreference="return gTabsPane.readLinkTarget();"
>  				onsynctopreference="return gTabsPane.writeLinkTarget();"
> +				class="indent"
> +				search="open new windows in a new tab instead"

For this first pass, lets just get categories working and wait on searching the plain-text. For now, just remove the |search| attribute from this patch.

Comment 4

[MSU Capstone Team]

Attachment: Pane switching using search methods

Ok, the pane switching works for all the panes I have (Tabs, Privacy, General, Advanced) and the back and forward buttons work as well.  For hiding I hid entire groupboxes together because I don't know how were going to want to do it for the individual search (if we show the whole groupbox or just the preference(s) that matches the search).  So on the next pass I may have to make more vboxes for hiding that are more refined but these work fine for now.  Jon mentioned that Blair had some suggestions on how to make ht push/pop state better, but I am still waiting on him for that information, so there may be some changes to that in the future.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 605467 [details] [diff] [review]
Pane switching using search methods

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

Overall nit: Please update your editor to only use spaces and remove any tab characters. It sounds very nitpicky, but it makes it hard to review the code because the indentation is all screwed up.

::: browser/components/preferences/in-content/advanced.xul
@@ +92,5 @@
>  #endif
>  
>  <!-- id=advancedPrefs-->
> +<vbox data-category='advanced' hidden='true'>
> +<tabbox id="advanced-content" flex="1"

How does the tabbox work with the child <vbox>s for search? I still don't know if we can go down this path. Can you investigate this?

::: browser/components/preferences/in-content/landing.xul
@@ +12,5 @@
>    <html:h1>&brandShortName;</html:h1>
>    <hbox id="preferences-home">
>  
>      <button label="&paneGeneral.title;" orient="vertical" class="options-box"
> +        oncommand="search('general', 'data-category'); push_history('general');">

There's a lot of duplication here. Can you make another function that does both of these and just takes the category as a single parameter?

::: browser/components/preferences/in-content/search.js
@@ +12,5 @@
> +     let categories = document.querySelectorAll("[data-category]");
> +	 for(var i=0; i<categories.length; i++){
> +	   let element = categories.item(i);
> +	   let attributeValue = element.getAttribute(attribute);
> +	   if(attributeValue.match(query)){

String.match uses a regular expression, which is kinda overkill for doing the string comparison that we need here (https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/String/match). Can you just use == or indexOf?

Comment 6

[MSU Capstone Team]

Attachment: Updated Pane Switching

Here is the updated pane switching using the search, it still only have 4 panes (general, tabs, advanced, and privacy), I will be adding others this afternoon.

Comment 7

[MSU Capstone Team]

Attachment: Updated Pane Switching

I'm sorry I uploaded the wrong patch the first time, this one is correct.

Comment 8

[MSU Capstone Team]

Attachment: Updated Pane Switching + Sync, Content, and Applications

This patch includes the sync, content, and applications panes.  There are some problems with the applications pane I need to talk to Jon about but the pane switching functionality is there and working (still no forward button though, I am kind of confused about that).

Comment 9

[MSU Capstone Team]

Attachment: Pane switching for all panes using search

So all panes now switch using the search, back and forward buttons work but there is still a problem with the applications pane I need to straight out, but the panw switching functionality is all there.

Comment 10

[MSU Capstone Team]

Attachment: Pane switching for all panes using search (forgot search.js on last)

Forgot to add search.js before submitting the last patch, sorry.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 608169 [details] [diff] [review]
Pane switching for all panes using search (forgot search.js on last)

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

::: browser/components/preferences/in-content/advanced.xul
@@ +4,5 @@
>     - You can obtain one at http://mozilla.org/MPL/2.0/.
>     -
> +   - Contributor(s):
> +   -   Joe Chen <[e-mail]>
> +   -

There's no contributor list for MPL2. hg log contains all the relevant history we need :)

@@ +92,4 @@
>  #endif
>  
>  <!-- id=advancedPrefs-->
> +<vbox data-category='advanced' hidden='true'>

Please use double-quotes for attribute values to be consistent with the rest of the file.

@@ +104,4 @@
>      <tab id="encryptionTab" label="&encryptionTab.label;" helpTopic="prefs-advanced-encryption"/>
>    </tabs>
>  
> +

Please remove this blank line.

@@ +108,4 @@
>    <tabpanels flex="1">
>  
>      <!-- General -->
> +	<vbox data-category='advanced' hidden='true'>

Indent with spaces and no tab characters please. It makes it hard to review since the indentation looks awkward.

::: browser/components/preferences/in-content/applications.xul
@@ +90,4 @@
>  <key key="&focusSearch1.key;" modifiers="accel" oncommand="gApplicationsPane.focusFilterBox();"/>
>  <key key="&focusSearch2.key;" modifiers="accel" oncommand="gApplicationsPane.focusFilterBox();"/>
>  </keyset>
> +<vbox data-category="applications" hidden="true">

Does changing the id="applications-content" to data-category="applications" break any of the CSS?

::: browser/components/preferences/in-content/home.js
@@ -1,3 @@
> -/* - This Source Code Form is subject to the terms of the Mozilla Public
> -   - License, v. 2.0. If a copy of the MPL was not distributed with this file,
> -   - You can obtain one at http://mozilla.org/MPL/2.0/. */

We'll still need a license header for this file.

@@ +1,1 @@
>  const Ci = Components.interfaces;

See https://bugzilla.mozilla.org/show_bug.cgi?id=733473#c17. We can remove the declaration of Ci here.

@@ +2,5 @@
>  var currentPageState;
>  
>  function gotoPref(page) {
> +  search(page, 'data-category');
> +  window.history.pushState(page,document.title);

nit: space after the comma here.

@@ +31,5 @@
> +}
> +
> +function onStatePopped(aEvent) {
> +  updateCommands();
> +  // TODO To ensure we can't go forward again we put an additional entry

Does this comment relate to any code here? I don't see an additional entry being added here and I don't think we need to worry about that bug here.

@@ +39,5 @@
> +}
> +
> +function updateCommands() {
> +    if(canGoBack()) {
> +        document.getElementById("back-btn").disabled = false;

Please review the feedback that was given in https://bugzilla.mozilla.org/show_bug.cgi?id=733473#c17 related to this file and coordinate with Jon over who will make those changes.

::: browser/components/preferences/in-content/landing.xul
@@ +1,4 @@
>  <!-- This Source Code Form is subject to the terms of the Mozilla Public
>     - License, v. 2.0. If a copy of the MPL was not distributed with this file,
>     - You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<vbox data-category="landing" hidden="false">

Please add a blank line between the license header and the start of the code.

@@ +6,5 @@
>  
>  <hbox id="preferences-home" flex="1">
>  
>      <button label="&paneGeneral.title;" orient="vertical" class="options-box"
> +        oncommand="gotoPref('general');">

nit: please line up the oncommand attribute with the first attribute of the <button>

::: browser/components/preferences/in-content/search.js
@@ +4,5 @@
> +   - You can obtain one at http://mozilla.org/MPL/2.0/.
> +   -
> +   - ***** END LICENSE BLOCK ***** */
> +   
> +  function search(query, attribute){

Can this function be moved to home.js (which should be renamed to preferences.js)?

@@ +6,5 @@
> +   - ***** END LICENSE BLOCK ***** */
> +   
> +  function search(query, attribute){
> +    let categories = document.querySelectorAll("[data-category]");
> +  for(var i=0; i<categories.length; i++){

nit: spaces around = and < operators and a space after the |for| keyword.

@@ +9,5 @@
> +    let categories = document.querySelectorAll("[data-category]");
> +  for(var i=0; i<categories.length; i++){
> +    let element = categories.item(i);
> +    let attributeValue = element.getAttribute(attribute);
> +    if(attributeValue==query.toLowerCase()){

nit: space after the |if| keyword and spaces around the equality operator.

@@ +11,5 @@
> +    let element = categories.item(i);
> +    let attributeValue = element.getAttribute(attribute);
> +    if(attributeValue==query.toLowerCase()){
> +      element.hidden=false;
> +    } else element.hidden=true;

nit: no curly brackets are necessary here since only one line after the if and only one line after the else. Further, the code for the else-block should be on its own line.

::: browser/components/preferences/in-content/sync.js
@@ +65,4 @@
>      if (Weave.Status.service == Weave.CLIENT_NOT_CONFIGURED ||
>          Weave.Svc.Prefs.get("firstSync", "") == "notReady") {
>        this.page = PAGE_NO_ACCOUNT;
> +

Can this newline be removed?

Comment 12

[Jon Rietveld]

Attachment: search in-content pane

Fixed indentations and everything so that it works ontop of the new landing patch, etc. I think I fixed all feedback from Jared. Gonna try to push to try server after the bball game. GO STATE!

Comment 13

[Jon Rietveld]

Attachment: search in-content pane

added consistent title

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 609064 [details] [diff] [review]
search in-content pane

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

Most of the changes in this patch (adding <vbox>s, and the associated indentation changes) are changes to patches that are still being worked on. Whenever there's a change to one of those patches, it will horribly break this patch, and you'll have to manually fix all the conflicts. I think you'd be *much* better off making changes to advanced.xul, applications.xul, etc in their relevant bugs.

::: browser/components/preferences/in-content/jar.mn
@@ +18,4 @@
>  *  content/browser/preferences/in-content/sync.js
>  *  content/browser/preferences/in-content/security.xul
>  *  content/browser/preferences/in-content/security.js
> +*  content/browser/preferences/in-content/search.js

This file doesn't need to be run through the preprocessor, remove the "*".

::: browser/components/preferences/in-content/preferences.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */

This change (and AFAICT every other change in this file) should go in the patch in bug 733473, not here.

::: browser/components/preferences/in-content/search.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +	   
> +function search(query, attribute){

Names of arguments should be in the form "aName". See https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#Prefixes

@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +	   
> +function search(query, attribute){
> +  let categories = document.querySelectorAll("[data-category]");

I know Jared suggested using querySelectorAll like this, but it's still looking for that attribute on an order of magnitude more elements than it needs to. ie, it's looking at all elements in the document for that attribute - but we only expect to see matching elements as direct children of the <prefpane>. The .children property will give you a list of direct children. ie:
  let elements = document.getElementById("id-of-prefpane").children;

(As an aside, "categories" isn't a great name for that variable - they're not categories, they're members of a category.)

@@ +4,5 @@
> +	   
> +function search(query, attribute){
> +  let categories = document.querySelectorAll("[data-category]");
> +  for(var i=0; i<categories.length; i++){
> +    let element = categories.item(i);

Bonus points if you switch to using the new for..of loop style - https://developer.mozilla.org/en/JavaScript/Reference/Statements/for...of

ie:
  for (let element of elements) {

@@ +8,5 @@
> +    let element = categories.item(i);
> +    let attributeValue = element.getAttribute(attribute);
> +    if(attributeValue==query.toLowerCase()){
> +      element.hidden=false;
> +    } else element.hidden=true;

This function needs a style exorcism. Space after "for" and "if". Space before "{". Space on either side of comparison and assignment operators (=, ==, <, etc). If the if-statement uses curly braces, then the associated else-statement should too.

Comment 15

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 608169 [details] [diff] [review]
Pane switching for all panes using search (forgot search.js on last)

(Remember to mark old patches as obsolete.)

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Blair McBride (:Unfocused) from comment #14)
> Most of the changes in this patch (adding <vbox>s, and the associated
> indentation changes) are changes to patches that are still being worked on.
> Whenever there's a change to one of those patches, it will horribly break
> this patch, and you'll have to manually fix all the conflicts. I think you'd
> be *much* better off making changes to advanced.xul, applications.xul, etc
> in their relevant bugs.

Forgot to mention: If all those patches are ready to land, then keep this as-in. But I'm pretty sure most of them will have additional changes before landing - at the very least, the <header> needs fixed up in each of them.

Comment 17

[Jon Rietveld]

Attachment: search functionality

fixed some of the feedback, mainly just moving changes into different patches.

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 609225 [details] [diff] [review]
search functionality

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

Can you please make the changes requested in https://bugzilla.mozilla.org/show_bug.cgi?id=734013#c14 ?

::: browser/components/preferences/in-content/preferences.xul
@@ +74,5 @@
>            src="chrome://browser/content/utilityOverlay.js"/>
>    <script type="application/javascript"
>            src="chrome://browser/content/preferences/in-content/preferences.js"/>
> +  <script type="application/javascript" 
> +          src="chrome://browser/content/preferences/in-content/search.js"/>

The code within search.js should be moved inside of preferences.js.

::: browser/components/preferences/in-content/sync.js
@@ +6,5 @@
>  Components.utils.import("resource://gre/modules/Services.jsm");
>  
> +PAGE_NO_ACCOUNT = 0;
> +PAGE_HAS_ACCOUNT = 1;
> +PAGE_NEEDS_UPDATE = 2;

I don't think we'll want to remove the const from these variables.

Comment 19

[MSU Capstone Team]

Attachment: search.js removed, search function in preferences.js

feedback fixed, search function moved to preferences.js and constants out back in.

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 609930 [details] [diff] [review]
search.js removed, search function in preferences.js

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

::: browser/components/preferences/in-content/preferences.js
@@ +74,5 @@
>                 .canGoForward;
>  }
> +
> +function search(query, attribute){
> +  let categories = document.querySelectorAll("[data-category]");

This is still missing the changes that were requested in https://bugzilla.mozilla.org/show_bug.cgi?id=734013#c14

::: browser/components/preferences/in-content/preferences.xul
@@ +74,5 @@
>            src="chrome://browser/content/utilityOverlay.js"/>
>    <script type="application/javascript"
>            src="chrome://browser/content/preferences/in-content/preferences.js"/>
> +  <script type="application/javascript" 
> +          src="chrome://browser/content/preferences/in-content/search.js"/>

This file doesn't exist anymore. These two lines shouldn't be added anymore.

Comment 21

[MSU Capstone Team]

Attachment: Pane swtiching with changes requested in comment 14

All requests in Comment 14 have been fixed

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 609963 [details] [diff] [review]
Pane swtiching with changes requested in comment 14

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

::: browser/components/preferences/in-content/preferences.js
@@ +77,5 @@
> +function search(aQuery, aAttribute) {
> +  let elements = document.getElementById("mainPrefPane").children;
> +  for (let element of elements) {
> +    let attributeValue = element.getAttribute(aAttribute);
> +    if (attributeValue == aQuery.toLowerCase()) {

element.hidden = (attributeValue != aQuery.toLowerCase());

Comment 23

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 609963 [details] [diff] [review]
Pane swtiching with changes requested in comment 14

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

What Jared said.

::: browser/components/preferences/in-content/preferences.js
@@ +27,4 @@
>  }
>  
>  function gotoPref(page) {
> +  search(page, 'data-category');

Use double quotes (").

Comment 24

[Jon Rietveld]

Attachment: search patch for new data-category attributes

Modified to work with the new data-catergories paneSomething

Comment 25

[Jon Rietveld]

Attachment: search patch for new data-category attributes

added feedback from jared & blair & fixed for new data categories

Comment 26

[Jared Wein [:jaws] (please needinfo? me)]

\o/ Pushed to mozilla-inbound. These patches should be merged to mozilla-central within the next day or two. Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d02fc5fc5c3

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4d02fc5fc5c3