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

RESOLVED FIXED in Firefox 53

Status

()

Core
DOM: Core & HTML
P2
normal
RESOLVED FIXED
9 months ago
5 months ago

People

(Reporter: valentin, Assigned: jdai)

Tracking

({dev-doc-needed, site-compat})

Trunk
mozilla53
dev-doc-needed, site-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

9 months ago
+++ 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

9 months ago
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
Hi John, since you've been doing some validity report bugs, could you please help this out? Thanks!
Flags: needinfo?(jdai)
Priority: -- → P2
(Reporter)

Comment 3

8 months ago
I would really like to reland bug 1275746 this release cycle, and it's blocked on this. Can we get it assigned to someone?
(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.
Assignee: nobody → jdai
Flags: needinfo?(jdai)
(Assignee)

Comment 5

8 months ago
Created attachment 8820528 [details] [diff] [review]
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.
Attachment #8820528 - Flags: feedback?(felipc)
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.
Attachment #8820528 - Flags: feedback?(felipc) → feedback+
(Assignee)

Comment 7

8 months ago
Created attachment 8821416 [details] [diff] [review]
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
Attachment #8820528 - Attachment is obsolete: true
Attachment #8821416 - Flags: review?(felipc)
(Assignee)

Comment 8

8 months ago
Created attachment 8821418 [details] [diff] [review]
Bug 1319078 - Form validation should account for the visibility of the input element. v2

I attached a wrong patch. Here is the correct one.
Attachment #8821416 - Attachment is obsolete: true
Attachment #8821416 - Flags: review?(felipc)
Attachment #8821418 - Flags: review?(felipc)
(Assignee)

Comment 9

8 months ago
Hi Felipe,

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

Thanks,
John
Flags: needinfo?(felipc)

Comment 10

8 months ago
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.
Attachment #8821418 - Flags: review?(felipc) → feedback+
(Assignee)

Updated

7 months ago
Flags: needinfo?(felipc)
(Assignee)

Comment 11

7 months ago
Created attachment 8825618 [details] [diff] [review]
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
Attachment #8821418 - Attachment is obsolete: true
Attachment #8825618 - Flags: review?(gijskruitbosch+bugs)

Comment 12

7 months ago
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.
Attachment #8825618 - Flags: review?(gijskruitbosch+bugs) → feedback+
(Assignee)

Comment 13

7 months ago
> @@ +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
(Assignee)

Comment 14

7 months ago
Created attachment 8826443 [details] [diff] [review]
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
Attachment #8825618 - Attachment is obsolete: true
Attachment #8826443 - Flags: review?(gijskruitbosch+bugs)

Comment 15

7 months ago
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!
Attachment #8826443 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Updated

7 months ago
Attachment #8826443 - Attachment description: Bug 1319078 - Form validation should account for the visibility of the input element. v4 → Bug 1319078 - Form validation should account for the visibility of the input element. r=gijs
(Assignee)

Updated

7 months ago
Keywords: checkin-needed

Comment 16

7 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c36d3ce91e16
Form validation should account for the visibility of the input element. r=gijs
Keywords: checkin-needed

Comment 17

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c36d3ce91e16
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2017/form-validation-no-longer-runs-on-hidden-input-elements/
Keywords: dev-doc-needed
OS: Windows 8.1 → All
Hardware: x86_64 → All
You need to log in before you can comment on or make changes to this bug.