1034362 - Test for bug 1034043 fix that when selected some Applications tab actions don't stick in-content

Status: NEW

(Firefox :: Settings UI, defect)

Description

[Bernardo Rittmeyer [:rittme]]

Attachment: applications.gif

When changing actions for content types at the Applications Preference list, some options, when chosen, do not stick. They just change to the next one on the list, or sometimes to the previously selected action if it was an application.

This apparently only happens with the next to last item on the topmost group of actions (before the first horizontal bar).

Comment 1

[Bernardo Rittmeyer [:rittme]]

Attachment: rev 1 - ask action attribute added and test

Comment 2

[Bernardo Rittmeyer [:rittme]]

Attachment: rev 2 - ask action attribute added and test

I forgot to clean some commented lines. Now it should be better.

Comment 3

[Bernardo Rittmeyer [:rittme]]

Attachment: rev 2 - ask action attribute added and test

Sorry, this should be the good one.

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8453478 [details] [diff] [review]
rev 2 - ask action attribute added and test

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

::: browser/components/preferences/in-content/applications.js
@@ +1405,5 @@
>                                                       [this._brandShortName]);
>        else
>          label = this._prefsBundle.getString("alwaysAsk");
>        askMenuItem.setAttribute("label", label);
> +      askMenuItem.setAttribute("action", Ci.nsIHandlerInfo.alwaysAsk);

It seems like this got fixed by bug 1034043 already. Do you agree? It seems like you may need to update your tree. I can help you with that.

::: browser/components/preferences/in-content/tests/browser.ini
@@ +4,5 @@
>    privacypane_tests_perwindow.js
>  
>  [browser_advanced_update.js]
>  [browser_bug410900.js]
> +[browser_bug1034362_select_preferences_actions.js]

Nit: The bug # in the filename makes it more awkward to add more checks to a test file in the future IMO. I can see us wanting to add more tests for this pane so I think browser_applications_actions.js would be better. (no need for "preferences" in the name since this whole directory is about preferences.

::: browser/components/preferences/in-content/tests/browser_bug1034362_select_preferences_actions.js
@@ +28,2 @@
>  
> +  //Gets richlistitem for Podcast

Nit: please put spaces after "//" and use closer to full sentences. e.g. Get the richlistitem for audio podcasts. I think some of the comments like this one aren't necessary since the line below is fairly straightforward.

@@ +32,5 @@
>  
> +  //Wait for the action menu to be rebuilt
> +  executeSoon(function (){
> +    EventUtils.synthesizeMouseAtCenter(podcastItem, {}, win);
> +    //Get the actions menu

I don't think this comment is necessary. Sometimes comments like that in tests are better as info() calls so it's easier to track down where a tests fails.

Comment 5

[Bernardo Rittmeyer [:rittme]]

Attachment: rev 3 - tests the application pane always ask actions

You are right, that other bug already fixed it, so I'll leave only the test.

I fixed what you asked and also changed the way we get the Always Ask action element for a more generic way, using the value provided by Ci.nsIHandlerInfo.alwaysAsk.

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8455617 [details] [diff] [review]
rev 3 - tests the application pane always ask actions

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

Note that you forgot the bug # in the commit message: "Bug 1034362 - "…

r=me with fixes.

::: browser/components/preferences/in-content/tests/browser.ini
@@ +4,5 @@
>    privacypane_tests_perwindow.js
>  
>  [browser_advanced_update.js]
>  [browser_bug410900.js]
> +[browser_applications_actions.js]

Nit: move this up a line to keep it alphabetical

::: browser/components/preferences/in-content/tests/browser_applications_actions.js
@@ +32,3 @@
>  
> +  info("Waiting for the action menu to be rebuilt.");
> +  executeSoon(function (){

Nit: missing space after ")". Please run this test on try to make sure it doesn't become intermittent. If bug 940882 was fixed I would probably get you to use that instead.

@@ +38,5 @@
> +    let actionsMenu =
> +      win.document.getAnonymousElementByAttribute(podcastItem, "class", "actionsMenu").firstChild;
> +    ok(actionsMenu && actionsMenu.childNodes.length > 0, "Actions menu present and populated.");
> +
> +    console.log(actionsMenu);

Leftover debugging code?

Comment 7

[Matthew N. [:MattN]]

Did a test actually get added?

Comment 8

[Jennifer Lynn]

Attachment: same problem

Thanks from trafelpiwin1979.