Closed Bug 664897 Opened 8 years ago Closed 8 years ago

Details view should scroll down when it has inline settings and options button in list view is clicked

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: darktrojan, Assigned: darktrojan)

References

Details

Attachments

(3 files, 1 obsolete file)

We should scroll down to show as much of the settings grid as possible, since that's what the user asked for.
Assignee: nobody → geoff
Status: NEW → ASSIGNED
Attached patch patch (obsolete) — Splinter Review
There's a few things you're going to hate me for here, but I'm used to it. I've deliberately kept the extra indenting to keep the patch from being three times longer.
Attachment #556520 - Flags: review?(bmcbride)
Comment on attachment 556520 [details] [diff] [review]
patch

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

That works. Was thinking of making passing arguments more generically via gViewController.parseViewId(), but that gets messy with all the arguments .show() already has :\

r=me on the code. But I'd like to get ui-review on this, since scrolling the prefs into view can mean there's no identifying element to show what the prefs are related to (without scrolling up, which seems counter-intuative).

(Note: Boriss is away until the end of the week)

::: toolkit/mozapps/extensions/content/extensions.js
@@ +2837,5 @@
>      while (lastRow.nextSibling)
>        rows.removeChild(rows.lastChild);
>    },
>  
> +  fillSettingsRows: function (aScrollToPreferences) {

Nit: Since you're changing this line already... remove the extra space after "function".

::: toolkit/mozapps/extensions/test/browser/browser_inlinesettings.js
@@ +439,4 @@
>    });
>  });
>  
>  // Addon with options.xul, disabling and enabling should hide and show settings UI

This comment no longer seems accurate.
Attachment #556520 - Flags: ui-review?(jboriss)
Attachment #556520 - Flags: review?(bmcbride)
Attachment #556520 - Flags: review+
Attached image Steps
I think the breadcrumb trail (bug 660726) would help here, as the addon name would be visible then.
Attachment #556756 - Flags: ui-review?(jboriss)
(In reply to Blair McBride (:Unfocused) from comment #2)
> Comment on attachment 556520 [details] [diff] [review]
> patch
> 
> Review of attachment 556520 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> That works. Was thinking of making passing arguments more generically via
> gViewController.parseViewId(), but that gets messy with all the arguments
> .show() already has :\
> 
> r=me on the code. But I'd like to get ui-review on this, since scrolling the
> prefs into view can mean there's no identifying element to show what the
> prefs are related to (without scrolling up, which seems counter-intuative).
> 
> (Note: Boriss is away until the end of the week)
> 
> ::: toolkit/mozapps/extensions/content/extensions.js
> @@ +2837,5 @@
> >      while (lastRow.nextSibling)
> >        rows.removeChild(rows.lastChild);
> >    },
> >  
> > +  fillSettingsRows: function (aScrollToPreferences) {
> 
> Nit: Since you're changing this line already... remove the extra space after
> "function".

FWIW, I go by and ask people to go by: "If a function literal is anonymous, there should be one space between the word function and the ( (left parenthesis). If the space is omited, then it can appear that the function's name is function, which is an incorrect reading." -- http://javascript.crockford.com/code.html (section Function Declarations)
By the power of Greyskull, Try builds with this patch will magically appear here: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bmcbride@mozilla.com-68a45d49a56d/
Boriss: Here's a sample extension to test with, in case you don't already have one that uses inline options.
Duplicate of this bug: 712563
Blocks: abp
On a related note, some add-ons like to provide their own widgets to open their settings. Is there a standard way to open the add-ons manager with the Details page+options exposed?
Hey, could you tap Greyskull for another try build?  My power sword's gone flaccid
Try run for e81a7d18efc7 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e81a7d18efc7
Results (out of 4 total builds):
    success: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bmcbride@mozilla.com-e81a7d18efc7
Try run for e81a7d18efc7 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e81a7d18efc7
Results (out of 4 total builds):
    success: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bmcbride@mozilla.com-e81a7d18efc7
Try run for e81a7d18efc7 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e81a7d18efc7
Results (out of 4 total builds):
    success: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bmcbride@mozilla.com-e81a7d18efc7
Try run for e81a7d18efc7 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e81a7d18efc7
Results (out of 4 total builds):
    success: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bmcbride@mozilla.com-e81a7d18efc7
Try run for e81a7d18efc7 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e81a7d18efc7
Results (out of 4 total builds):
    success: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bmcbride@mozilla.com-e81a7d18efc7
Comment on attachment 556520 [details] [diff] [review]
patch

Looks good, and thanks for the builds!
Attachment #556520 - Flags: ui-review?(jboriss) → ui-review+
Attached patch updated patchSplinter Review
I've also removed the binding hacks in the test which are no longer required due to bug 708397.
Attachment #556520 - Attachment is obsolete: true
Attachment #586258 - Flags: review?(bmcbride)
Attachment #556756 - Flags: ui-review?(jboriss)
(In reply to Brian King (Briks) [:kinger] from comment #8)
> On a related note, some add-ons like to provide their own widgets to open
> their settings. Is there a standard way to open the add-ons manager with the
> Details page+options exposed?

Not really - just filed bug 715716 (which describes some alternatives until it's fixed).
Comment on attachment 586258 [details] [diff] [review]
updated patch

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

::: toolkit/mozapps/extensions/content/extensions.js
@@ +2972,5 @@
> +    }
> +  },
> +
> +  scrollToPreferencesRows: function() {
> +    document.removeEventListener("ViewChanged", gDetailView.scrollToPreferencesRows, false);

Not needed any more.
Attachment #586258 - Flags: review?(bmcbride) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/b91361c61e7d
Flags: in-testsuite+
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/b91361c61e7d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.