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)
Toolkit
Add-ons Manager
Tracking
()
Tracking | Status | |
---|---|---|
firefox31 | --- | unaffected |
firefox32 | --- | unaffected |
firefox33 | --- | unaffected |
firefox34 | + | verified |
People
(Reporter: gfritzsche, Assigned: alexbardas)
Details
(Keywords: regression)
Attachments
(3 files, 4 obsolete files)
(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+
Reporter | ||
Comment 2•10 years ago
|
||
[Tracking Requested - why for this release]: With bug 1034679 landed, the activation state for locked prefs looks broken now.
tracking-firefox34:
--- → ?
Reporter | ||
Updated•10 years ago
|
Flags: qe-verify+
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → abardas
Status: NEW → ASSIGNED
Flags: needinfo?(abardas)
Updated•10 years ago
|
Iteration: --- → 34.3
Assignee | ||
Comment 3•10 years ago
|
||
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)
Assignee | ||
Comment 4•10 years ago
|
||
Updated•10 years ago
|
QA Contact: andrei.vaida
Reporter | ||
Comment 5•10 years ago
|
||
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?
Assignee | ||
Comment 6•10 years ago
|
||
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)
Assignee | ||
Comment 7•10 years ago
|
||
Added reviewer name in commit name.
Attachment #8478702 -
Attachment is obsolete: true
Attachment #8478702 -
Flags: review?(georg.fritzsche)
Attachment #8478707 -
Flags: review?(georg.fritzsche)
Assignee | ||
Comment 8•10 years ago
|
||
Try results: https://tbpl.mozilla.org/?tree=Try&rev=e4de220ea2eb
Reporter | ||
Comment 9•10 years ago
|
||
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)
Assignee | ||
Comment 10•10 years ago
|
||
(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.
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8478707 -
Attachment is obsolete: true
Attachment #8479122 -
Flags: review?(bmcbride)
Updated•10 years ago
|
Assignee | ||
Comment 12•10 years ago
|
||
I've repushed to try here: https://tbpl.mozilla.org/?tree=Try&rev=c487aa7c5a06
Comment 13•10 years ago
|
||
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-
Assignee | ||
Comment 14•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8480298 -
Flags: review?(bmcbride) → review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 15•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/141dc6c20917
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment 16•10 years ago
|
||
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
Comment 17•10 years ago
|
||
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
Comment 18•10 years ago
|
||
Works for me, as well.
You need to log in
before you can comment on or make changes to this bug.
Description
•