Closed Bug 1058039 Opened 10 years ago Closed 10 years ago

Locked plugin state is broken in Add-on manager view

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
normal
Points:
3

Tracking

()

VERIFIED FIXED
mozilla34
Iteration:
34.3
Tracking Status
firefox31 --- unaffected
firefox32 --- unaffected
firefox33 --- unaffected
firefox34 + verified

People

(Reporter: gfritzsche, Assigned: alexbardas)

Details

(Keywords: regression)

Attachments

(3 files, 4 obsolete files)

Attached image Capture.PNG
(Jason from bug 1034679, comment #47)
> Created attachment 8477880 [details]
> Image of correct and broken plug-in states
> 
> I just tested this in Nightly 2014-08-23.
> 
> It seems to work by graying out the control for the plug-in, but it is not
> showing the actual state, anymore.  The drop-down list is completely empty,
> and the drop-down arrow itself seems to be going off to the side of the
> control, just kind of floating.  Honestly, it looks broken.  
> 
> I have attached an image of it.  The top plug-in, "Shockwave Flash," is the
> broken one; it is locked to "Always Activate."  The one below it, "Microsoft
> Office 2013," is not locked and is, therefore, not broken.
> 
> I would imagine this isn't intentional, is it?
Flags: firefox-backlog+
Alex, can you look into this?
Flags: needinfo?(abardas)
[Tracking Requested - why for this release]:
With bug 1034679 landed, the activation state for locked prefs looks broken now.
Flags: qe-verify+
Assignee: nobody → abardas
Status: NEW → ASSIGNED
Flags: needinfo?(abardas)
Iteration: --- → 34.3
It seems that the problem is with the disabled state which hides all menuitems from that selectbox (although on an OS X there was a visible label there).

This fix shows the selected element every time (it will be disabled, but visible). I also had to add a fix to reset a default padding of 7px.
Attachment #8478415 - Flags: review?(georg.fritzsche)
QA Contact: andrei.vaida
Comment on attachment 8478415 [details] [diff] [review]
Fixed locked plugin state in Add-on Manager

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

Can we add test coverage for this issue please?
The test would be that there is a selected .addon-control element which is not disabled. (in this way, the selectbox will inherit its width).
Attachment #8478415 - Attachment is obsolete: true
Attachment #8478415 - Flags: review?(georg.fritzsche)
Attachment #8478702 - Flags: review?(georg.fritzsche)
Added reviewer name in commit name.
Attachment #8478702 - Attachment is obsolete: true
Attachment #8478702 - Flags: review?(georg.fritzsche)
Attachment #8478707 - Flags: review?(georg.fritzsche)
Comment on attachment 8478707 [details] [diff] [review]
Fixed locked plugin state in Add-on Manager

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

Ah, thanks for test-coverage.

My open questions are below, but i'm not too sure about the extensions.xml/.css parts.
I think Unfocused is the right reviewer here and should know more about the CSS and XUL properties involved.

::: toolkit/mozapps/extensions/content/extensions.xml
@@ +1323,5 @@
>                ["ask_to_activate", "enable", "disable"].some(perm => this.hasPermission(perm));
>              this._stateMenulist.disabled = !hasActivatePermission;
>              this._stateMenulist.hidden = false;
>              this._stateMenulist.classList.add('no-auto-hide');
> +            this._stateMenulist.selectedItem.disabled = false;

Do we need to set .disabled=false here?
Isn't there some CSS we can change so it is still visible when we disable it?

::: toolkit/mozapps/extensions/test/browser/browser_plugin_enabled_state_locked.js
@@ +64,5 @@
>      "State menu should" + (locked === true ? "" : " not") + " be disabled.");
> +
> +  Assert.notEqual(selectedMenuItem, undefined);
> +  Assert.equal(selectedMenuItem.disabled, false,
> +    "State menu's selected item should not be disabled.");

We should just be able to use is_element_visible() instead of .disabled() here, right?
Attachment #8478707 - Flags: review?(georg.fritzsche)
(In reply to Georg Fritzsche [:gfritzsche] from comment #9)
> Comment on attachment 8478707 [details] [diff] [review]
> Fixed locked plugin state in Add-on Manager
> 
> Review of attachment 8478707 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ah, thanks for test-coverage.
> 
> My open questions are below, but i'm not too sure about the
> extensions.xml/.css parts.
> I think Unfocused is the right reviewer here and should know more about the
> CSS and XUL properties involved.
> 
> ::: toolkit/mozapps/extensions/content/extensions.xml
> @@ +1323,5 @@
> >                ["ask_to_activate", "enable", "disable"].some(perm => this.hasPermission(perm));
> >              this._stateMenulist.disabled = !hasActivatePermission;
> >              this._stateMenulist.hidden = false;
> >              this._stateMenulist.classList.add('no-auto-hide');
> > +            this._stateMenulist.selectedItem.disabled = false;
> 
> Do we need to set .disabled=false here?
> Isn't there some CSS we can change so it is still visible when we disable it?
> 
> :::
> toolkit/mozapps/extensions/test/browser/browser_plugin_enabled_state_locked.
> js
> @@ +64,5 @@
> >      "State menu should" + (locked === true ? "" : " not") + " be disabled.");
> > +
> > +  Assert.notEqual(selectedMenuItem, undefined);
> > +  Assert.equal(selectedMenuItem.disabled, false,
> > +    "State menu's selected item should not be disabled.");
> 
> We should just be able to use is_element_visible() instead of .disabled()
> here, right?

Thank you for the review Georg. Indeed, it can be fixed only from css. I guess this should be the preferred way. The test suggestion is also very good, I read the implementation of is_element_visible and is what we need here.
Attachment #8478707 - Attachment is obsolete: true
Attachment #8479122 - Flags: review?(bmcbride)
Comment on attachment 8479122 [details] [diff] [review]
Make sure that subitems from the State Menu list in Plugin Add-on Manager are visible

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

::: toolkit/themes/osx/mozapps/extensions/extensions.css
@@ +1107,5 @@
>  .addon-control[disabled="true"] {
>    display: none;
>  }
>  
> +.addon-control.no-auto-hide, .no-auto-hide .addon-control {

Merge this with the above rule, using :not() ? And I think we only need .no-auto-hide to be a direct parent.

@@ +1112,5 @@
>    display: block;
>  }
>  
> +.no-auto-hide > .menulist-dropmarker {
> +  -moz-padding-start: 0px!important;

Seems this is only needed on OSX.

Also: Space before the "!".
Attachment #8479122 - Flags: review?(bmcbride) → review-
Removed the css rules which are not needed for linux & windows and merged other rules.
Attachment #8479122 - Attachment is obsolete: true
Attachment #8480298 - Flags: review?(bmcbride)
Attachment #8480298 - Flags: review?(bmcbride) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/141dc6c20917
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla34
Verified fixed on Nightly 34.0a1 (2014-08-31) using Windows 7 64-bit, Ubuntu 14.04 LTS 32-bit and Mac OS X 10.9.4. Testing was based on Comment 0 of Bug 1034769, following the available guidelines [1] for the "plugin.state.java" pref.

Acceptance criteria for this patch:
- The disabled plug-in state is displayed properly in Add-ons Manager > Plugins, with the options dropdown button grayed-out.
- The grayed-out options dropdown button is aligned / positioned properly across platforms.


[1] http://kb.mozillazine.org/Locking_preferences
Status: RESOLVED → VERIFIED
Works for me, as well.
You need to log in before you can comment on or make changes to this bug.