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

Status: NEW

(Firefox :: General, defect)

Description

[Mounir Lamouri (:mounir)]

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.

Comment 1

[Mounir Lamouri (:mounir)]

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

Comment 2

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

David, is using .getBoundingClientRect() the best way to know if an element is shown?

Comment 3

[Mounir Lamouri (:mounir)]

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.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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?

Comment 7

[Mounir Lamouri (:mounir)]

(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.

Comment 8

[Dão Gottwald [:dao]]

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.

Comment 9

[Mounir Lamouri (:mounir)]

(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.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 12

[Mounir Lamouri (:mounir)]

(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.

Comment 13

[Mounir Lamouri (:mounir)]

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.

Comment 14

[Mounir Lamouri (:mounir)]

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?

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Feel free to file a new bug on that and we can discuss there.

Comment 16

[Dão Gottwald [:dao]]

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;

Comment 18

[Xavier Mouton-Dubosc]

duplicating comment 4 from Bug 651829 :

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

Comment 19

[Mounir Lamouri (:mounir)]

(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.

Comment 20

[Xavier Mouton-Dubosc]

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')

Comment 22

[Richard Neill]

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.

Comment 24

[Nickolay_Ponomarev]

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.

Comment 25

[Mounir Lamouri (:mounir)]

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.

Comment 27

[Daniel Holbert [:dholbert]]

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

Comment 28

[Richard Neill]

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

[Liam Morland]

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.

Comment 30

[Dão Gottwald [:dao]]

(clearing stale needinfo request; I don't have anything to add to what I wrote in comment 8.)

Comment 31

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.

Comment 32

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S3. However, the bug has 4 duplicates.
:mossop, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 33

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

The last needinfo from me was triggered in error by recent activity on the bug. I'm clearing the needinfo since this is a very old bug and I don't know if it's still relevant.