Open Bug 1034362 Opened 10 years ago Updated 3 months ago

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

Categories

(Firefox :: Settings UI, defect)

defect

Tracking

()

People

(Reporter: rittme, Unassigned)

References

Details

Attachments

(2 files, 4 obsolete files)

Attached image 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).
Assignee: nobody → bernardo
Keywords: regression
Summary: In Preferences > Applications, when selected some actions doesn't stick → In Preferences > Applications, when selected some actions don't stick in-content
Attachment #8453474 - Flags: review?(MattN+bmo)
I forgot to clean some commented lines. Now it should be better.
Attachment #8453474 - Attachment is obsolete: true
Attachment #8453474 - Flags: review?(MattN+bmo)
Attachment #8453477 - Flags: review?(MattN+bmo)
Sorry, this should be the good one.
Attachment #8453477 - Attachment is obsolete: true
Attachment #8453477 - Flags: review?(MattN+bmo)
Attachment #8453478 - Flags: review?(MattN+bmo)
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.
Attachment #8453478 - Flags: review?(MattN+bmo) → feedback+
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.
Attachment #8453478 - Attachment is obsolete: true
Attachment #8455617 - Flags: review?(MattN+bmo)
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?
Attachment #8455617 - Flags: review?(MattN+bmo) → review+
Blocks: 1034043
No longer blocks: ship-incontent-prefs
Summary: In Preferences > Applications, when selected some actions don't stick in-content → Test for bug 1034043 fix that when selected some Applications tab actions don't stick in-content
Keywords: regression
OS: Mac OS X → All
Hardware: x86 → All
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WORKSFORME

Did a test actually get added?

Assignee: bernardo → nobody
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Status: REOPENED → NEW
Severity: normal → S3
Attachment #9384715 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: