Closed mbaechtold closed 6 years ago
/cc @deiferni
The idea of this change is that we are actually looking for z3cform-field which have errors, not HTML input nodes. So iterating over the inputs makes it much more complicated.
I did a testrun of opengever.core
with these changes. Unfortunately that will break our tests as not all labels are extracted correctly any more 😢 . See https://ci.4teamwork.ch/builds/128922/tasks/200976 for the test results (Careful the tests have swapped expected/actual so the error messages it gives you are misleading). I'll investigate a bit.
That's what I feared. I had such a case in https://github.com/4teamwork/ftw.shop/pull/69/files, but it seems more correct now.
In our case the label returned by erroneous_fields
seems empty now, which should not be correct 🤔 . I'll dig in.
What i've found so far:
z3c.form.browser.checkbox.SingleCheckBoxFieldWidget
the markup for that field looks like:
<div class="field z3cformInlineValidation kssattr-fieldname-[fieldname] error" data-fieldname="[fieldname]" id="formfield-form-widgets-[fieldname]">
<label for="form-widgets-[fieldname]" class="horizontal">
<span class="formHelp"></span>
</label>
<div class="fieldErrorBox">
<div class="error">[field error message]</div>
</div>
<span class="option" id="form-widgets-[fieldname]">
<input id="form-widgets-[fieldname]-0" name="[fieldname]:list" class="single-checkbox-widget bool-field" value="selected" type="checkbox">
<label for="form-widgets-[fieldname]-0">
<span class="label">[field title]</span>
</label>
</span>
<input name="[fieldname]-empty-marker" type="hidden" value="1">
- the new css selector now picks up the field's empty help text, instead of the field's label
i think the new behaviour as introduced in this PR does not behave as it should in all cases. it works in the tests, but the tests are, unfortunately, a bit basic and seem to not test with many different widgets. how would you proceed? revert? or amend and improve tests?
Since it is a checkbox, there is probably an empty <label />
or <div class="label" />
before the input. But there is probably also a label after the input, which would actually contain the text.
I'd probably search for the first label, div.label
which actually has text and use that. This approach should be quite robust for the common z3cform widgets.
The HTML-snippet above could be used in a test: it can be loaded statically with .open_html
.
@jone your suggestion will only work when the description of the field is empty. otherwise the description will be found and returned as field label, which is also not desired. i don't think we should use that approach.
I fear the label extraction should be widget-specific. we already have some widgets in ftw.testbrowser.widgets
, should we use them here, too?
We could probably use form.find_field(labe_or_name)
=> https://github.com/4teamwork/ftw.testbrowser/blob/edba405255c0514bdade8c3417b86984a34a1193/ftw/testbrowser/form.py#L43 🤔 this could work
Update: the solution proposed here works, after all. I had a close look at our errors and discovered that they should all be fixed in opengever.core
, so now the tests there pass with the changes introduced in this version.
This fixes a side-effect introduced in https://github.com/4teamwork/ftw.testbrowser/pull/80 where empty marker fields would also be added to the list of erroneous form fields.