Closed Bug 555816 Opened 10 years ago Closed 10 years ago

"Start Page" preferences pane clean up

Categories

(Firefox for Android Graveyard :: General, defect)

Fennec 1.1
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: aakashd, Assigned: mfinkle)

Details

Attachments

(1 file, 2 obsolete files)

Build id:
Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2.3pre) Gecko/20100329 Namoroka/3.6.3pre Fennec/1.1a2pre

Steps to Reproduce:
1. go to the controls panel
2. Go to preferences

Actual Results:
The text, "start page", will be aligned to the top of the row entry rather than centered like all other row entries.

Expected Results:
"Start Page" is vertically aligned to the center of the row entry.
Comment on attachment 435740 [details] [diff] [review]
Patch

I have a different patch for this in the works. It just removes the "desc" attribute.

It also:
* Uses the page title not the URL for the description
* Adds a "Custom Page" entry to the menulist if the user currently has a custom page, so the user can close the menulist without being required to change the setting.
Attachment #435740 - Flags: review?(mark.finkle) → review-
Assignee: nobody → mark.finkle
Attached patch patch 2 (obsolete) — Splinter Review
This patch fixes the "Start Page" centering. It also adds support for the webpage title, but falls back to URL if title is empty.

Lastly, the patch adds a menulist item, "Custom Page", that gives the user something to pick if they don't want to change the current setting.
Attachment #435740 - Attachment is obsolete: true
Attachment #435813 - Flags: review?(21)
Summary: "Start Page" text is off-vertical center in preferences pane → "Start Page" preferences pane clean up
Comment on attachment 435813 [details] [diff] [review]
patch 2


>-      case "custom":
>+      case "currentpage":
>         url = Browser.selectedBrowser.currentURI.spec;
>-        display = url;
>+        display = Browser.selectedBrowser.contentDocument.title;
>+        if (!display.length)
>+          display = url;
>         break;
>     }
display = Browser.selectedBrowser.contentDocument.title || url; should work
> 
>-    // Show or hide the URL of the custom homepage
>-    document.getElementById("prefs-homepage").setAttribute("desc", display);
>+    // Show or hide the title or URL of the custom homepage
>+    if (display)
>+      document.getElementById("prefs-homepage").setAttribute("desc", display);
>+    else
>+      document.getElementById("prefs-homepage").removeAttribute("desc");

We're doing this twice, maybe a function would be nice

>+    // Update the helper "Custom Page" item in the menulist
>+    if (value == "currentpage") {
>+      // If the helper item is not already in the list, we need to put it there
>+      // (this can happen when changing from one custom page to another)
>+      if (options.itemCount < 4) {
>+        let helper = options.appendItem(Elements.browserBundle.getString("homepage.custom2"), "custom");
>+        options.selectedItem = helper;
>+      }
>+    } else {
>+      options.removeItemAt(options.itemCount - 1);
>     }

The removeItemAt is called too many times.
I think you need a to check if the previous value was "custom" then you want to delete the last item.
Attachment #435813 - Flags: review?(21) → review-
(In reply to comment #4)
> (From update of attachment 435813 [details] [diff] [review])
> >+        display = Browser.selectedBrowser.contentDocument.title;
> >+        if (!display.length)
> >+          display = url;

> display = Browser.selectedBrowser.contentDocument.title || url; should work

done

> >+    // Show or hide the title or URL of the custom homepage
> >+    if (display)
> >+      document.getElementById("prefs-homepage").setAttribute("desc", display);
> >+    else
> >+      document.getElementById("prefs-homepage").removeAttribute("desc");
> 
> We're doing this twice, maybe a function would be nice

done

> >+    // Update the helper "Custom Page" item in the menulist
> >+    if (value == "currentpage") {
> >+      // If the helper item is not already in the list, we need to put it there
> >+      // (this can happen when changing from one custom page to another)
> >+      if (options.itemCount < 4) {
> >+        let helper = options.appendItem(Elements.browserBundle.getString("homepage.custom2"), "custom");
> >+        options.selectedItem = helper;
> >+      }
> >+    } else {
> >+      options.removeItemAt(options.itemCount - 1);
> >     }
> 
> The removeItemAt is called too many times.
> I think you need a to check if the previous value was "custom" then you want to
> delete the last item.

I changed the code to look for the value="custom" item itself, instead of looking for itemCount-1

This should only remove the "custom page" item, if it is in the list.
Attached patch patch 3Splinter Review
Addressed review comments
Attachment #435813 - Attachment is obsolete: true
Attachment #435899 - Flags: review?(21)
pushed:
http://hg.mozilla.org/mobile-browser/rev/e90a2c30b3f7
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
verified FIXED On builds:

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2.3pre) Gecko/20100331 Namoroka/3.6.3pre Fennec/1.1a2pre

and

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.3a4pre) Gecko/20100331 Namoroka/3.7a4pre Fennec/1.1a2pre
Status: RESOLVED → VERIFIED
Flags: in-litmus?
litmus testcase updated to regression test this bug:

https://litmus.mozilla.org/show_test.cgi?id=11661
Flags: in-litmus? → in-litmus+
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.