Open Bug 309953 Opened 20 years ago Updated 3 years ago

toolbarbuttons aren't in sync with their commands after customizing

Categories

(Core :: XUL, defect)

1.8 Branch
defect

Tracking

()

People

(Reporter: asaf, Unassigned, NeedInfo)

References

Details

Attachments

(3 files)

When a toolbarbutton is wrapped for toolbars customizing, we save its command attribute as an "itemcommand" attribute, on the wraper; we set it back (as "command") when the toolbarbutton is unwraped. However, at this point, the command attributes (e.g. "disabled") aren't in sync with the toolbarbutton. See also bug 287105.
Assignee: nobody → general
Component: Toolbars and Toolbar Customization → XBL
Flags: blocking1.9a1?
Product: Toolkit → Core
QA Contact: nobody → ian
Version: unspecified → 1.8 Branch
So what does this have to do with XBL, exactly? That is, what is the bug in content/xbl/src that needs fixing? I see neither bug description nor steps to reproduce.
I might be wrong in the component, although i recall we are inheriting the attributes from the command element (based on the command attribute) in contnet/xbl. If firefox can be considered a test case, the steps described in bug 287105 comment 0 raise this bug.
> although i recall we are inheriting the attributes from the command element What command element? And no, Firefox is not a testcase. The toolbar customization dialog is a huge hack that does all sorts of weird (and some unsupported) stuff; just because it has an issue doesn't mean it's not cauing the issue itself. If someone ever provides something resembling a real bug description here, I'll look at this at that point.
Keywords: qawanted
Assignee: general → nobody
Component: XBL → Toolbars and Toolbar Customization
Product: Core → Toolkit
QA Contact: ian → nobody
Flags: blocking1.9a1?
Testcase: One thing is certain: when the initial state of a toolbar button is disabled, and it is not one of the default buttons, the button is saved in the toolbox's palette with a disabled state. However with the reload button: its state is most of the time not disabled. But when you remove it from the toolbar its state is remembered as disabled. But that is not the real issue here. The issue is that when you drag a disabled toolbar button to the toolbar its state does not reflect its commands state. This is what the testcase shows. But after this whenever the commands state is updated, the state of the toolbar button will automatically be update to, see bug 287105 comment 0 (add new tab: button is enabled)
Attached patch simple patchSplinter Review
This will fix the state of newly added toolbar buttons reflecting the state of their commands.
Assignee: nobody → twanno
Status: NEW → ASSIGNED
Attachment #287618 - Flags: review?(mano)
Asking for blocking 1.9, because (most) extensions don't have their buttons on a toolbar by default. And when for instance the state of a command is enabled once and never disabled again in a single browser session, a newly added button can't be enabled until Firefox is restarted (bad first impression for an extension).
Flags: blocking1.9?
not a blocker, would take a reviewed patch before b2
Flags: wanted1.9+
Flags: blocking1.9?
Flags: blocking1.9-
QA Contact: nobody → toolbars
Comment on attachment 287618 [details] [diff] [review] simple patch This ought to be fixed in core imo, or we should fix our way of setting the command on the toolbar-element, I didn't read this code for a long while.
Attachment #287618 - Flags: review?(mano)
Blocks: 419161
Blocks: 287105
Keywords: qawanted
No longer blocks: 419161
Blocks: 440702
Bug 440702 is adding a few more instances of the hack from bug 287105 - seems like it would be better to take this "simple patch" until the core bug is fixed. Mano, what do you think?
This applies on top of bug 440702, so maybe this bug should depend on that bug instead of blocking it. This is just a hack until the core issue is fixed (but at least it's better then adding new hacks everytime this problem occurs).
Attachment #334809 - Flags: review?(mano)
Attachment #334809 - Flags: review?(mano) → review+
Comment on attachment 334809 [details] [diff] [review] patch - same as "simple patch" but this also removes the hack added in 440702 (checked in) I'm fine with taking this as a better wallpaper, r=mano. Please don't close the bug though.
Keywords: checkin-needed
Comment on attachment 334809 [details] [diff] [review] patch - same as "simple patch" but this also removes the hack added in 440702 (checked in) http://hg.mozilla.org/mozilla-central/rev/fea1965d16bc
Attachment #334809 - Attachment description: patch - same as "simple patch" but this also removes the hack added in 440702 → patch - same as "simple patch" but this also removes the hack added in 440702 (checked in)
Assignee: twanno → nobody
Component: Toolbars and Toolbar Customization → XUL
Keywords: checkin-needed
Product: Toolkit → Core
QA Contact: toolbars → xptoolkit.widgets
Status: ASSIGNED → NEW
Seems like we should add a comment referencing this bug to the code added in customizeToolbar.js, no?
(In reply to comment #13) > Seems like we should add a comment referencing this bug to the code added in > customizeToolbar.js, no? done: http://hg.mozilla.org/mozilla-central/rev/e28f00d256d1
Depends on: 540215
Blocks: 1388990

Hey Mano,
Can you still reproduce this or should we close it? The test case isn't working for me.

Flags: needinfo?(asaf)

Redirect a needinfo that is pending on an inactive user to the triage owner.
:enndeakin, since the bug has high severity, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(asaf) → needinfo?(enndeakin)

Going to redirect to dao on the off-chance he remembers about this.

Flags: needinfo?(enndeakin) → needinfo?(dao+bmo)

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.

Severity: major → --
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: