1319078 - Form validation should account for the visibility/focusability of the input element

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Valentin Gosu [:valentin] (he/him)]

+++ This bug was initially created as a clone of Bug #1305204 +++

Bug 1275746 has been backed out as a workaround, but this should still be addressed.

(In reply to Olli Pettay [:smaug] (vacation Nov 19-26) from bug 1305204 comment #20)
> I think we should fix our behavior.
> The relevant code is around
> http://searchfox.org/mozilla-central/rev/
> d96317a351af8aa78ab9847e7feed964bbaac7d7/browser/modules/FormSubmitObserver.
> jsm#98
> So somewhere there one should check at least if display is "none" or
> visibility "hidden".
> 

(In reply to Olli Pettay [:smaug] (vacation Nov 19-26) from bug 1305204 comment #22)
> Based on blink source code, they seem to use focusability as a check whether
> to show the popup.
> https://chromium.googlesource.com/chromium/src/+blame/master/third_party/
> WebKit/Source/core/html/HTMLFormElement.cpp#240

Comment 1

[Mike]

Again, if the page associated with the bug 1305204 is needed for testing anything, let me know and I can change the password back to something and let you know. I did not delete the account, just changed the password as these pages are public and I indicated that original password on the bug page.- Mike

Comment 2

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi John, since you've been doing some validity report bugs, could you please help this out? Thanks!

Comment 3

[Valentin Gosu [:valentin] (he/him)]

I would really like to reland bug 1275746 this release cycle, and it's blocked on this. Can we get it assigned to someone?

Comment 4

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Valentin Gosu [:valentin] from comment #3)
> I would really like to reland bug 1275746 this release cycle, and it's
> blocked on this. Can we get it assigned to someone?

Sure, talked with John and he will be working on this after the all hands.

Comment 5

[John Dai[:jdai]]

Attachment: Bug 1319078 - Form validation should account for the visibility of the input element.

Hi felipe,
As comment #0 mention, I check if display is "none" or visibility "hidden". I would like to have your feedback to see if I am on the right direction. Thank you.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8820528 [details] [diff] [review]
Bug 1319078 - Form validation should account for the visibility of the input element.

Review of attachment 8820528 [details] [diff] [review]:
-----------------------------------------------------------------

That's the right direction, but you'll want to use getComputedStyle instead because this one only covers styles assigned through the style="" attribute, and not through CSS.

Comment 7

[John Dai[:jdai]]

Attachment: Bug 1319078 - Form validation should account for the visibility of the input element. v2

Hi :felipe,
Thanks for the review. I addressed comment #6 and added an testcase.
Could you help me to review it again? Thank you.

Try result: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=924fa33af1219ed278d8585f4346d8e20f64b953&group_state=expanded&filter-tier=1

Comment 8

[John Dai[:jdai]]

Attachment: Bug 1319078 - Form validation should account for the visibility of the input element. v2

I attached a wrong patch. Here is the correct one.

Comment 9

[John Dai[:jdai]]

Hi Felipe,

It's just a gentle ping, could you help to review my patch?

Thanks,
John

Comment 10

[:Gijs (he/him)]

Comment on attachment 8821418 [details] [diff] [review]
Bug 1319078 - Form validation should account for the visibility of the input element. v2

Review of attachment 8821418 [details] [diff] [review]:
-----------------------------------------------------------------

I'll steal this review from felipe as he hasn't had time for it and I noticed this in my bugmail. :-)

Feel free to get the next review from either of us.

::: browser/base/content/test/general/browser_bug1319078.js
@@ +1,3 @@
> +var gInvalidFormPopup = document.getElementById('invalid-form-popup');
> +ok(gInvalidFormPopup,
> +   "The browser should have a popup to show when a form is invalid");

Nit: all assertions should be in a task (or a test() function if there are no tasks, but here you're using tasks, so just put this ok() in the top of the first task).


Also, can you put this test in browser/modules/test/ instead of browser/base/content/test/general/, please? :-)

@@ +64,5 @@
> +add_task(function* ()
> +{
> +  incrementTest();
> +  let uri = getDocHeader() + "<form target='t'><input type='url'  placeholder='url' value='http://' style='display: none;'><input id='s' type='button' value='check'></form>" + getDocFooter();
> +  let browser = yield openNewTab(uri);

Instead of hand-rolling waiting for the tab, use BrowserTestUtils.openNewForegroundTab, which also returns a promise that returns a tab (you can get the browser off the tab as you do now).

For `clickChildElement` you can use BrowserTestUtils.synthesizeMouse with a selector.

::: browser/modules/FormSubmitObserver.jsm
@@ +118,5 @@
>        return;
>      }
>  
> +    let style = element.ownerGlobal.getComputedStyle(element, null);
> +    if (element instanceof HTMLInputElement &&

This check won't work for <select> elements, which can also have form validation errors. We already return early for non-form elements, so I think you can just omit this part of the check?

@@ +119,5 @@
>      }
>  
> +    let style = element.ownerGlobal.getComputedStyle(element, null);
> +    if (element instanceof HTMLInputElement &&
> +        (style.display == "none" || style.visibility == "hidden")) {

I know Felipe already suggested how to do this, but I wonder if based on the comments on this bug, we should check if the element is focusable, using the elementIsFocusable method on the focus manager ( https://dxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIFocusManager.idl#145 ).

You should be able to use:

Services.focus.elementIsFocusable(element, 0)

to determine if the element is focusable.


The other problem here is what happens if there are multiple invalid elements, and some *are* visible. In that case we should still show the message for those other elements, even if the first element we are told about is invisible / not focusable.

So I think we'll have to loop through the list of elements in aInvalidElements and ignore elements that aren't form elements (which is the check above this) or aren't focusable (the check you're adding). If there are no elements left to warn for, we can return, but otherwise we should still warn for any remaining elements.

You can use a pretty regular for loop, aInvalidElements has a length property you can use for that, and then you'll just have to iterate through from 0 until you find an element that we can use (at which point you can break out of the loop, with `element` set to the element we want to warn for). If we finish the loop without having any element to warn about, we should return without running the rest of the method.

Comment 11

[John Dai[:jdai]]

Attachment: Bug 1319078 - Form validation should account for the visibility of the input element. v3

Hi :Gijs,
Thanks for the review. I addressed comment #10 and revised the testcase.
Could you help me to review it again? Thank you.

Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=77b68800d496536ded57a2fdbdf9fbd5790cfcb7&filter-tier=1&group_state=expanded

Comment 12

[:Gijs (he/him)]

Comment on attachment 8825618 [details] [diff] [review]
Bug 1319078 - Form validation should account for the visibility of the input element. v3

Review of attachment 8825618 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there! :-)

::: browser/modules/FormSubmitObserver.jsm
@@ +106,5 @@
> +      // Insure that this is the FormSubmitObserver associated with the
> +      // element / window this notification is about.
> +      let element = aInvalidElements.queryElementAt(i, Ci.nsISupports);
> +      if (this._content != element.ownerGlobal.top.document.defaultView) {
> +        continue;

I expect this can stay a return; instead of a continue; .

If this isn't the 'right' formsubmitobserver to process this notification, we shouldn't continue within the loop as all the elements will be (should be!) for another observer.

::: browser/modules/test/browser_bug1319078.js
@@ +1,1 @@
> +var gInvalidFormPopup = document.getElementById('invalid-form-popup');

Nit: please add "use strict"; at the top of the file.

@@ +12,5 @@
> +  info("Starting next part of test");
> +}
> +
> +/**
> + * In this test, we check that no popup appears if the element display is none.

Can we add a test prior to this where we do show the popup? You can hide the popup after it's been shown by calling gInvalidFormPopup.hidePopup() after the fact.

Comment 13

[John Dai[:jdai]]

> @@ +12,5 @@
> > +  info("Starting next part of test");
> > +}
> > +
> > +/**
> > + * In this test, we check that no popup appears if the element display is none.
> 
> Can we add a test prior to this where we do show the popup? You can hide the
> popup after it's been shown by calling gInvalidFormPopup.hidePopup() after
> the fact.

Thanks for the feedback. I saw browser_bug561636.js[1] which shows the popup. Should I need to add this testcase?

[1] http://searchfox.org/mozilla-central/source/browser/base/content/test/general/browser_bug561636.js#119-136

Comment 14

[John Dai[:jdai]]

Attachment: Bug 1319078 - Form validation should account for the visibility of the input element. r=gijs

Hi :Gijs,
Thanks for the review. I addressed comment #12 and comment #13. Hence, I didn't add the show popup testcase. Could you help me to review it again? Thank you.

Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=830de4641a8daf8d461b3ccf9f6421694f2165c0&filter-tier=1&selectedJob=68392694&group_state=expanded

Comment 15

[:Gijs (he/him)]

Comment on attachment 8826443 [details] [diff] [review]
Bug 1319078 - Form validation should account for the visibility of the input element. r=gijs

Review of attachment 8826443 [details] [diff] [review]:
-----------------------------------------------------------------

Alright, this looks good then. Thanks!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c36d3ce91e16
Form validation should account for the visibility of the input element. r=gijs

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c36d3ce91e16

Comment 18

[Kohei Yoshino [:kohei]]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2017/form-validation-no-longer-runs-on-hidden-input-elements/