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

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Geoff Lankow (:darktrojan)]

We should scroll down to show as much of the settings grid as possible, since that's what the user asked for.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: patch

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.

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Steps

I think the breadcrumb trail (bug 660726) would help here, as the addon name would be visible then.

Comment 4

[Dão Gottwald [:dao]]

(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)

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

By the power of Greyskull, Try builds with this patch will magically appear here: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-68a45d49a56d/

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Sample extension with inline options

Boriss: Here's a sample extension to test with, in case you don't already have one that uses inline options.

Comment 8

[Brian King [:kinger]]

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?

Comment 9

[Jennifer Morrow [:Boriss] (UX)]

Hey, could you tap Greyskull for another try build?  My power sword's gone flaccid

Comment 10

[Mozilla RelEng Bot]

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/[e-mail]-e81a7d18efc7

Comment 11

[Mozilla RelEng Bot]

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/[e-mail]-e81a7d18efc7

Comment 12

[Mozilla RelEng Bot]

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/[e-mail]-e81a7d18efc7

Comment 13

[Mozilla RelEng Bot]

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/[e-mail]-e81a7d18efc7

Comment 14

[Mozilla RelEng Bot]

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/[e-mail]-e81a7d18efc7

Comment 15

[Jennifer Morrow [:Boriss] (UX)]

Comment on attachment 556520 [details] [diff] [review]
patch

Looks good, and thanks for the builds!

Comment 16

[Geoff Lankow (:darktrojan)]

Attachment: updated patch

I've also removed the binding hacks in the test which are no longer required due to bug 708397.

Comment 17

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(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 18

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 19

[Geoff Lankow (:darktrojan)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b91361c61e7d

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b91361c61e7d