Closed
Bug 945191
Opened 9 years ago
Closed 9 years ago
Combined buttons show wrong context menu options when they are in the toolbar
Categories
(Firefox :: Toolbars and Customization, defect)
Firefox
Toolbars and Customization
Tracking
()
VERIFIED
FIXED
Firefox 28
People
(Reporter: pretzer, Assigned: jaws)
References
(Blocks 2 open bugs)
Details
(Whiteboard: [Australis:P2])
Attachments
(1 file, 1 obsolete file)
3.98 KB,
patch
|
Gijs
:
review+
|
Details | Diff | Splinter Review |
STR: 1) Put a combined button (Zoom or Copy/Paste controls) in the toolbar, either via customization mode or via the context menu 2) Leave customization mode, if you were in it 3) Right-click the combined button on the toolbar ER: Context menu shows 'Add to Menu' and 'Remove from Toolbar' AR: Context menu shows 'Add to Toolbar' and 'Remove from Menu' again The options are only wrong when you're not in customization mode. Right clicking while in customization mode shows the expected options.
I would add that the options are greyed out with the bookmark star and the search bar when it shouldn't be.
Updated•9 years ago
|
Blocks: australis-merge
Updated•9 years ago
|
Whiteboard: [Australis:P2]
Assignee | ||
Comment 2•9 years ago
|
||
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attachment #8341510 -
Flags: review?(gijskruitbosch+bugs)
Comment 3•9 years ago
|
||
Comment on attachment 8341510 [details] [diff] [review] Patch Review of attachment 8341510 [details] [diff] [review]: ----------------------------------------------------------------- I don't understand the removal, and I found a bug, so I'm going to clear review on this one. (did you want to address the disabled-ness of the star button's context menu item here or not?) ::: browser/components/customizableui/src/CustomizableUI.jsm @@ -420,5 @@ > continue; > } > } > > - this.ensureButtonContextMenu(node, aAreaNode); This is weird. Why are we removing this? @@ +594,5 @@ > let placements = gPlacements.get(CustomizableUI.AREA_PANEL); > this.buildArea(CustomizableUI.AREA_PANEL, placements, aPanel); > for (let btn of aPanel.querySelectorAll("toolbarbutton")) { > btn.setAttribute("tabindex", "0"); > + if (btn.parentNode.id == CustomizableUI.AREA_PANEL) { AIUI, this is just to ensure we don't add the contextmenu specially on nested buttons, right? However, this means that with the following STR, I don't see the panel context menu any more: 1) new profile 2) Open the panel menu 3) right click the zoom/edit buttons We should probably just make the loop loop through aPanel.children and disregard any non-toolbarbuttons for everything else, and non-toolbarbutton/toolbaritems for the context menu. ::: browser/components/customizableui/test/browser_880164_customization_context_menus.js @@ +243,5 @@ > }, > + { > + desc: "Bug 945191 - Combined buttons show wrong context menu options when they are in the toolbar.", > + run: function () { > + yield startCustomizing(); Nit: please just: setup: startCustomizing,
Attachment #8341510 -
Flags: review?(gijskruitbosch+bugs)
Comment 4•9 years ago
|
||
(In reply to Guillaume C. [:ge3k0s] from comment #1) > I would add that the options are greyed out with the bookmark star and the > search bar when it shouldn't be. Was the search bar in the panel or the toolbar?
Flags: needinfo?(ge3k0s)
(In reply to :Gijs Kruitbosch from comment #4) > (In reply to Guillaume C. [:ge3k0s] from comment #1) > > I would add that the options are greyed out with the bookmark star and the > > search bar when it shouldn't be. > > Was the search bar in the panel or the toolbar? Toolbar
Flags: needinfo?(ge3k0s)
Assignee | ||
Comment 6•9 years ago
|
||
(In reply to Guillaume C. [:ge3k0s] from comment #1) > I would add that the options are greyed out with the bookmark star and the > search bar when it shouldn't be. Please file a separate bug for this.
Assignee | ||
Comment 7•9 years ago
|
||
I can only reproduce this when going through Customization Mode. This is happening because the we place a context attribute on the toolbaritems for the panel, and then when customization mode is entered we stash those away when the items get wrapped. Exiting customization mode restores them. NB: I moved the startCustomizing to the setup function, but I originally didn't have it there since it didn't feel like setup to me, more like just part of the repro-steps. Not a big deal to me, so I moved it, but its previous placement was intentional.
Attachment #8341960 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Updated•9 years ago
|
Attachment #8341510 -
Attachment is obsolete: true
Assignee | ||
Comment 8•9 years ago
|
||
Forgot to qref the moving of startCustomizing, but it's in my local patch.
Comment 9•9 years ago
|
||
Comment on attachment 8341960 [details] [diff] [review] Patch v1.1 Review of attachment 8341960 [details] [diff] [review]: ----------------------------------------------------------------- This makes sense. Hopefully this is the last time we have to adjust this...
Attachment #8341960 -
Flags: review?(gijskruitbosch+bugs) → review+
Assignee | ||
Comment 10•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/d5a62078b4f2
Flags: in-testsuite+
Comment 11•9 years ago
|
||
Hey, sorry for nitpicking but I just saw this has been pushed without "Australis" in the commit message. Thought this would be worth to mention so that you don't run into surprises when merging.
Comment 12•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d5a62078b4f2
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
Comment 13•9 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #11) > Hey, sorry for nitpicking but I just saw this has been pushed without > "Australis" in the commit message. Thought this would be worth to mention so > that you don't run into surprises when merging. Thank you for letting us know. Fixed on m-c directly because that's what we're merging: remote: https://hg.mozilla.org/mozilla-central/rev/e55e2e773ae6 remote: https://hg.mozilla.org/mozilla-central/rev/7476bb2b8e9c
Reporter | ||
Updated•9 years ago
|
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•