Do not show the invalid form panel when the invalid element is not visible

ASSIGNED
Assigned to
(NeedInfo from)

Status

()

Firefox
General
ASSIGNED
7 years ago
a month ago

People

(Reporter: mounir, Assigned: mounir, NeedInfo)

Tracking

(Depends on: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 .x+, status2.0 wanted)

Details

(Whiteboard: [passed-try][has feedback+])

Attachments

(1 attachment)

(Assignee)

Description

7 years ago
AFAIK, the only think we can do is to check if the element has a frame. If it has no frame we shouldn't try to show the panel because it's position would be completely inappropriate (IIRC, in a window corner).

An alternative would be to show another message explaining that the invalid element is hidden but considering that should not happen (except for buggy websites), it might not worth it.
(Assignee)

Comment 1

7 years ago
Actually, we could also show the panel so it will help to understand issues when a form can't be submitted...
(Assignee)

Comment 2

7 years ago
Created attachment 492300 [details] [diff] [review]
Patch v1

David, is using .getBoundingClientRect() the best way to know if an element is shown?
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
Attachment #492300 - Flags: review?
Attachment #492300 - Flags: feedback?(dbaron)
(Assignee)

Updated

7 years ago
Attachment #492300 - Flags: review? → review?(dao)
(Assignee)

Comment 3

7 years ago
Comment on attachment 492300 [details] [diff] [review]
Patch v1

Jonas, I'm still not convinced that removing the message is really a solution given that the form will not submit but nothing will appear. IIRC, it was your idea so I'm wondering if you still want this.

IMO, the best solution would have been to simply submit the form if an invalid element has no frame (IOW, have .checkValidity() returning true) but that might be more sensitive.
Attachment #492300 - Flags: feedback?(jonas)
Judging visibility is really hard, and in some cases even comes down to a judgement call. It's easy when something doesn't have a frame, but there are lots more situations when something isn't visible.

Because of this I don't want to make visibility have semantic meaning such as affect submission.

I do think it's ok for inexact visibility calculations to affect visibility of the error message panel. If there is no frame there is no logical location to display the panel anyway, right?
Comment on attachment 492300 [details] [diff] [review]
Patch v1

>+    // Check that the element is actually visible.
>+    let rect = element.getBoundingClientRect();
>+    if (rect.width == 0 && rect.height == 0 && rect.top == 0 && rect.left == 0) {
>+      return;
>+    }

Is this the only way to test if an element has a frame?
Comment on attachment 492300 [details] [diff] [review]
Patch v1

feedback+=me assuming there is no better way to check for lack of lack of frame than the quoted code.

Would maybe using getComputedStyle and checking the display property work?
Attachment #492300 - Flags: feedback?(jonas) → feedback+
(Assignee)

Comment 7

7 years ago
(In reply to comment #5)
> Comment on attachment 492300 [details] [diff] [review]
> Patch v1
> 
> >+    // Check that the element is actually visible.
> >+    let rect = element.getBoundingClientRect();
> >+    if (rect.width == 0 && rect.height == 0 && rect.top == 0 && rect.left == 0) {
> >+      return;
> >+    }
> 
> Is this the only way to test if an element has a frame?

I don't know, that's why I asked a feedback from David. This ways work but I can't tell if it's the best.
I'm seeing quite some inconsistency here. If there's no form submission observer, the form is submitted regardless of its validity. If there's an observer, the form won't be submitted when there's invalid stuff, regardless of whether the observer actually does something. E.g. Firefox doesn't seem to do anything for background tabs.

Are there plans to straighten this up on way or another? I'm asking because if it's not going to be guaranteed that the form won't be submitted with invalid fields, then I don't see a problem with CheckFormValidity() returning true for fields without frames.
(Assignee)

Comment 9

7 years ago
(In reply to comment #8)
> I'm seeing quite some inconsistency here. If there's no form submission
> observer, the form is submitted regardless of its validity. If there's an
> observer, the form won't be submitted when there's invalid stuff, regardless of
> whether the observer actually does something. E.g. Firefox doesn't seem to do
> anything for background tabs.
> 
> Are there plans to straighten this up on way or another? I'm asking because if
> it's not going to be guaranteed that the form won't be submitted with invalid
> fields, then I don't see a problem with CheckFormValidity() returning true for
> fields without frames.

Indeed. We do not check background tabs because it will show the panel in the current tab which is not what we want for the moment. If we can have panels not limited to the tabs (like alerts now), we will be able to change that.
The observer check is only there to not break Gecko consumers like Fennec: if the form submission is blocked with no message, it would be very confusing for the users. This check should be removed later, see bug 587671.
(Assignee)

Updated

7 years ago
Whiteboard: [passed-try][needs-review]

Updated

7 years ago
blocking2.0: --- → .x
status2.0: --- → wanted
(In reply to comment #2)
> Created attachment 492300 [details] [diff] [review]
> Patch v1
> 
> David, is using .getBoundingClientRect() the best way to know if an element is
> shown?

I suppose it depends on what definition of "shown" you're interested in.

The result you're testing for is what you'll get if an element is display:none *or* has a display:none ancestor *or* if an element has zero size and is positioned at the top left corner of the frame that nsLayoutUtils::GetContainingBlockForClientRect returns (generally the root, except in cases where SVG is involved).

Given that you're talking about form controls only, where having zero size would be quite unusual, I think you're reasonably safe here, at least if the resulting definition of "shown" matches "is display:none or inside something display:none".  (In general, though, elements can have zero size even when their descendants are larger.)

As to the question of whether there's a more reliable way to find out if something is display:none or inside display:none... I'm not sure.  Checking element.getClientRects().length == 0 seems slightly more reliable than getBoundingClientRect(), since it avoids that zero-size at top-left problem (though that probably isn't an issue for form controls).

It's possible roc has a better idea.
Attachment #492300 - Flags: feedback?(dbaron) → feedback+
I think it's more important to have a simple definition of "not shown" than a definition that catches more edge cases. Because of this I think "is display:none, or has an ancestor which is display:none" is a good definition.

So sounds like |element.getClientRects().length == 0| is a slight improvement here.
(Assignee)

Comment 12

6 years ago
(In reply to comment #11)
> So sounds like |element.getClientRects().length == 0| is a slight improvement
> here.

I agree. I will update that after Dao's review.
(Assignee)

Updated

6 years ago
Whiteboard: [passed-try][needs-review] → [passed-try][needs-review][has feedback+]
(Assignee)

Comment 13

6 years ago
Comment on attachment 492300 [details] [diff] [review]
Patch v1

Asking a gavin review to gavin in case of he can jump on it before Dao.
Attachment #492300 - Flags: review?(gavin.sharp)
(Assignee)

Comment 14

6 years ago
Actually, I'm wondering if we should not have a message like "This form has an invalid form control, do you want to submit the form? [Yes] [No]". That would be post-4.0 so it doesn't deprecate this bug.

Jonas: what's your opinion?
Feel free to file a new bug on that and we can discuss there.
Comment on attachment 492300 [details] [diff] [review]
Patch v1

r=me, assuming you're going to replace this with:

if (element.getClientRects().length == 0)
  return;
Attachment #492300 - Flags: review?(dao) → review+
(Assignee)

Updated

6 years ago
Duplicate of this bug: 651829
duplicating comment 4 from Bug 651829 :

Why not place the popup where is the sumbmit button ? It will be more easy to
view
(Assignee)

Comment 19

6 years ago
(In reply to comment #18)
> duplicating comment 4 from Bug 651829 :
> 
> Why not place the popup where is the sumbmit button ? It will be more easy to
> view

It's not that easy. That kind of situation can have two meanings: the site is broken and the user will never be able to submit the form or the site does that on purpose. The later sounds unlikely.
If I had to fix that, I would try the solution I described in comment 14.
Hint for web designers / web dev / web integrators :
If you have an invalidable form, and you don't know which input is blocking, use Firebug + jQuery : $(':invalid')
Duplicate of this bug: 630926
Attachment #492300 - Flags: review?(gavin.sharp)
(Assignee)

Updated

6 years ago
Depends on: 613016
(Assignee)

Updated

6 years ago
No longer depends on: 613016

Comment 22

6 years ago
This is actually quite an easy mistake for a web-developer to make, and not catch in testing [I speak from experience!]. We try to improve usability twice, by hiding the advanced clutter, and by using required/placeholder; the result can be completely unusable. [This is actually not so uncommon - consider a long preferences form, to which the user regularly returns; the designer intends to hide the advanced settings until the basic ones are filled in.]

My preference would be:
 1. [Best]: undo the display:none, so that the form control is shown.
 2. Pop up a dialog, saying something like:
"This form cannot be submitted because a required element is not shown. Enter a value for the field called '$name': [.....]"
 3. Submit the form anyway.

BTW, I had just filed a duplicate, bug #693025 , which I'll correct now.

Updated

6 years ago
Duplicate of this bug: 693025

Comment 24

5 years ago
Was this not landed on purpose?

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #14)
> Actually, I'm wondering if we should not have a message like "This form has
> an invalid form control, do you want to submit the form? [Yes] [No]". That
> would be post-4.0 so it doesn't deprecate this bug.
Note: this is one of suggestions in bug 724537.
Blocks: 724537
(Assignee)

Comment 25

5 years ago
Yes, it was on purpose because I thought it might be worse for the user to see nothing and still have the form not submitted. We should show something that allows the user to submit anyway. Unfortunately, we have no one with free cycles to work on that for the moment.
(Assignee)

Updated

5 years ago
No longer blocks: 724537
Depends on: 630495
Duplicate of this bug: 1015305
Dao, as reviewer, perhaps you could follow up on this / shepherd this in sometime soon? I'd argue that we should just take the patch (or an up-to-date version of it).

With or without the patch, forms w/ <input required style="display:none"> will be broken. We have two ways of handling them at our disposal:

 (a) Current behavior -- refuse to submit the form, AND confusingly tell the user that they need to fill in some field, in a popup that may be entirely outside the window, and may be pointing to some piece of browser UI. (not at the page) Super confusing / broken-looking, and probably impossible for the user to react to. Examples: [1] [2]

 (b) Behavior w/ attached patch - just refuse to submit the form.

In both behaviors (a) and (b), the web page is broken and can't be submitted. The only difference is that with our current behavior, we confuse the user more by telling them to do something impossible.  I can imagine this being marginally helpful for web developers, but not for the average user.

So: for the time being, in the absence of a better option, I think (b) is clearly preferable to (a). (and I don't think we should stick with (a) simply because we haven't designed/implemented a better solution than (b) yet)

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8427855
[2] https://bugzilla.mozilla.org/attachment.cgi?id=8427856
Flags: needinfo?(dao)
Whiteboard: [passed-try][needs-review][has feedback+] → [passed-try][has feedback+]

Comment 28

3 years ago
Re #27, I think that (b) is the worst possible case: The user clicks the button and gets absolutely no result, no idea why, and not even a cryptic warning which he could at least google for (or report on the "contact us" page of the site.)  Too much information is *always* better than too little, and silent failures are, I think, the most frustrating.

If we haven't got time to create a proper solution (which would make the offending element visible), I think we should create a popup with the following wording:

"This webpage has an bug: there is a hidden field named "[THE NAME OF THE INPUT ELEMENT]", which you are *required* to fill in, even though it has been hidden from view. Ignore this, and submit form anyway?  [cancel] [ok]"  

Failing which, the simplest, least-worst outcome is to "just succeed in submitting the form" and let the server deal with it (which the server already has to be able to do, for non HTML-5 clients).

Comment 29

a month ago
I agree with #28. I am trying to fix a page with ones of these warnings. It is hard to debug because I cannot easily figure out what element is causing the message.
You need to log in before you can comment on or make changes to this bug.