Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can't open control settings after editing control prefix #4288

Closed
ebruchez opened this issue Dec 3, 2019 · 8 comments
Closed

Can't open control settings after editing control prefix #4288

ebruchez opened this issue Dec 3, 2019 · 8 comments

Comments

@ebruchez
Copy link
Collaborator

ebruchez commented Dec 3, 2019

  • insert Number
  • Control Settings
  • update prefix
  • apply settings
  • try Control Settings again
  • dialog doesn't open

What happens after adding the prefix is that the control changes its id. For example in my case it coes from control-3-control to xf-311. When you try to open the the Control Settings again, Form Builder passes xf-311 and the server can't find that control in the form definition.

@ebruchez
Copy link
Collaborator Author

ebruchez commented Dec 3, 2019

This is because we remove the id attribute. Regressed with #1433 in a0176d4.

@ebruchez
Copy link
Collaborator Author

ebruchez commented Dec 3, 2019

  • don't remove the id attribute
  • more to fix as Control Settings doesn't read things like prefix anymore either

@ebruchez
Copy link
Collaborator Author

ebruchez commented Dec 3, 2019

The way things work since that commit is that we delete attributes which don't exist on the view template in the XBL component. fr:number doesn't have for example prefix in that template.

So the question is whether we are happy with that and need to add all the attribute on the template. The drawback is that we get extra attributes in the form. Some might be not desirable, like grouping-separator, which I think have a different meaning when empty.

I am just wondering why we are deleting these attributes. To discuss with @avernet.

@ebruchez
Copy link
Collaborator Author

Following discussion with @avernet:

This was done so that when you change a control's appearance, you don't keep extra attributes which might be relevant to the older appearance. So a list of relevant attributes for an appearance is required.

Now, specifically, fr:number only has a single appearance. Could we make it so that in such cases we don't prune attributes? Or do this only when we prune appearances?

@ebruchez
Copy link
Collaborator Author

What #1433 did when inserting the control to the dynamic editor is twofold:

  • add attributes that are in the view template but not in the control (with the same value as on the template)
  • remove attributes that are in the control but not in the template

It does this in fb-process-xbl-fb-control-details:

  • when the dialog loads
  • when the dialog moves from control to control (prev/next)
  • when the value of the appearance changes in the dialog

@ebruchez
Copy link
Collaborator Author

Implicit notions:

  • supported attributes
  • required attributes

Right now, after #1433, the code assumes that all supported and required attributes are on the view template. fr:number assumes it does all the work on its own.

@ebruchez
Copy link
Collaborator Author

So what should happen if you have fr:my-foo and fr:my-bar which represent two appearances foo and bar of fr:my. But for some reason, fr:my-foo and fr:my-bar support different attribute sets.

I understand that we should probably prune unsupported attributes.

However, I am not sure we should add all missing attributes. It would be better for the editor to figure that out. Unless we have another notion of the attributes being required.

In short this is a bit of a headache.

@ebruchez
Copy link
Collaborator Author

As an immediate fix, what if we only add/remove the attributes when there is an appearance change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant