Closed Bug 1341190 Opened 7 years ago Closed 7 years ago

Date picker panel and email input validation popup close immediately after opening

Categories

(Toolkit :: UI Widgets, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla54
Tracking Status
firefox53 blocking verified
firefox54 --- verified
firefox55 --- verified

People

(Reporter: scottwu, Assigned: scottwu)

References

(Blocks 2 open bugs)

Details

(Keywords: regression)

Attachments

(1 file)

Date picker panel and email input validation popup close immediately after opening. This bug appears after bug 1109868 landed.

The date picker uses the same implementation as email input validation, so it's not surprising they are both affected.

Email validation bug STR:

1. Visit the following URL
data:text/html,<form><input type="email" value="invalid_email"/><button type="submit">Send</button></form>
2. Click "Send" button
Expected: 
An invalid email warning popup should show under the input box.
Actual:
A popup opens and closes immediately.

Date picker bug STR:
0. Set dom.forms.datetime to true in about:config
1. Visit the following URL: 
data:text/html,<input type="date"/>
2. Click on the date input box
Expected:
A date picker popup should show under the input box.
Actual:
A popup opens and closes immediately.
The problem seem to be caused by the `visibility: hidden` rule:
http://searchfox.org/mozilla-central/rev/12cf11303392edac9f1da0c02e3d9ad2ecc8f4d3/browser/base/content/browser.css#826

If I understand correctly, one of the changes implemented in bug 1109868 is that when an anchor's visibility is set to hidden, its popup closes. Removing the visibility rule solves the problem, but looking at the original commit in bug 691601, that line seemed to serve a purpose: https://bugzilla.mozilla.org/show_bug.cgi?id=691601#c53

:jimm, I know it's been a while, but do you remember why that rule is necessary? and if it's still the case? Popup still works for me when I remove the rule.
Flags: needinfo?(jmathies)
(In reply to Scott Wu [:scottwu] from comment #3)
> The problem seem to be caused by the `visibility: hidden` rule:
> http://searchfox.org/mozilla-central/rev/
> 12cf11303392edac9f1da0c02e3d9ad2ecc8f4d3/browser/base/content/browser.css#826
> 
> If I understand correctly, one of the changes implemented in bug 1109868 is
> that when an anchor's visibility is set to hidden, its popup closes.
> Removing the visibility rule solves the problem, but looking at the original
> commit in bug 691601, that line seemed to serve a purpose:
> https://bugzilla.mozilla.org/show_bug.cgi?id=691601#c53
> 
> :jimm, I know it's been a while, but do you remember why that rule is
> necessary? and if it's still the case? Popup still works for me when I
> remove the rule.

I don't really remember but if the popup shows and tests pass should be fine.
Flags: needinfo?(jmathies)
Thanks :jimm, the popup looks fine and I don't see any big issue with tests. Though I don't think this was covered at first place otherwise we would've seen test failures on bug 1109868.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9295248c422dcafd6355eb6bb42ae3383a0e290d
Attachment #8839330 - Flags: review?(mconley)
Comment on attachment 8839330 [details]
Bug 1341190 - Remove .popup-anchor visibility rule

https://reviewboard.mozilla.org/r/114004/#review116484

::: browser/base/content/browser.css
(Diff revision 2)
>  }
>  
>  .popup-anchor {
>    /* should occupy space but not be visible */
>    opacity: 0;
> -  visibility: hidden;

The popupAnchor is set with hidden = true... which makes it display: none. Why is that not affected by bug 1109868?

Also.. I've just realized that we never .remove() the popupAnchor when a tab has been removed. We should probably file a follow-up to do that, as we're potentially leaking a node per tab.
Comment on attachment 8839330 [details]
Bug 1341190 - Remove .popup-anchor visibility rule

https://reviewboard.mozilla.org/r/114004/#review116484

> The popupAnchor is set with hidden = true... which makes it display: none. Why is that not affected by bug 1109868?
> 
> Also.. I've just realized that we never .remove() the popupAnchor when a tab has been removed. We should probably file a follow-up to do that, as we're potentially leaking a node per tab.

Before calling openPopup, the anchor.hidden is set to false:
datetime: http://searchfox.org/mozilla-central/rev/60ae6514e4c559c0c234f0e7aefccb101b8beb2e/toolkit/modules/DateTimePickerHelper.jsm#118,150
validation: http://searchfox.org/mozilla-central/rev/60ae6514e4c559c0c234f0e7aefccb101b8beb2e/browser/modules/FormValidationHandler.jsm#101,130

Could I prevent the leak if I set the reference to anchor to null after using it? The FormValidationHandler does it, but I didn't do that in DateTimePickerHelper.
Comment on attachment 8839330 [details]
Bug 1341190 - Remove .popup-anchor visibility rule

https://reviewboard.mozilla.org/r/114004/#review116484

> Before calling openPopup, the anchor.hidden is set to false:
> datetime: http://searchfox.org/mozilla-central/rev/60ae6514e4c559c0c234f0e7aefccb101b8beb2e/toolkit/modules/DateTimePickerHelper.jsm#118,150
> validation: http://searchfox.org/mozilla-central/rev/60ae6514e4c559c0c234f0e7aefccb101b8beb2e/browser/modules/FormValidationHandler.jsm#101,130
> 
> Could I prevent the leak if I set the reference to anchor to null after using it? The FormValidationHandler does it, but I didn't do that in DateTimePickerHelper.

Ah, okay, I had forgotten that users of the anchor were required to unhide it.

I guess this is okay. I'm also wrong about the leak - the anchor is applied to the browser's <xul:stack>, and that stack goes away when the tab does, so we're alright. I was mistaken. :)
Comment on attachment 8839330 [details]
Bug 1341190 - Remove .popup-anchor visibility rule

https://reviewboard.mozilla.org/r/114004/#review116748

Thanks for the patch! It's kinda too bad that we have to hack around arrow panels like this. It'd be nice if we could just pass it some coordinates to point to instead.
Attachment #8839330 - Flags: review?(mconley) → review+
Thanks Mike! You mentioned in the other bug that I could use the land commits feature in mozreview, but it turns out that requires lv3 commit access, so I'm just gonna do it the old way :)
Assignee: nobody → scwwu
Keywords: checkin-needed
Pushed by philringnalda@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0ee3593e2315
Remove .popup-anchor visibility rule r=mconley
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0ee3593e2315
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Blocks: 1358487
The patch requires an uplift request for mozilla-release to solve Bug 1358487 with Firefox 53.0.1.
Flags: needinfo?(scwwu)
Comment on attachment 8839330 [details]
Bug 1341190 - Remove .popup-anchor visibility rule

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1109868
[User impact if declined]: Users won't see form validation errors. Date picker won't show if it's preffed on manually.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Only involves deletion of one line of CSS
[String changes made/needed]: No
Flags: needinfo?(scwwu)
Attachment #8839330 - Flags: approval-mozilla-release?
Hope I'm doing this right. Haven't done an uplift request before.

Sorry I didn't notice Bug 1109868 was uplifted. I should've referenced this regression on that bug.
Comment on attachment 8839330 [details]
Bug 1341190 - Remove .popup-anchor visibility rule

Let's uplift this now to release. I need to discuss whether to do a dot release for it; so far, there is no other major reason to do one.
Attachment #8839330 - Flags: approval-mozilla-release? → approval-mozilla-release+
Here, we missed landing this on 53 for a few reasons. 
 
* The bug was not marked as a regression in the keyword field (which would have given this more scrutiny in triage).
* We did figure out the regression window in Comment 3, but it wasn't indicated through status flags.
* The bug should have been flagged as status-firefox53:affected.  Then we could have caught the missed uplift opportunity through relman and QA queries, automated emails to developers, etc.

So, when you're triaging or fixing a bug, please take a minute to think about whether other versions are affected.  Then, we can uplift the fix as far as it should go, so it can get to users quickly, and if it's something release-blocking, we'll have a chance to notice and escalate our response.
Wes, can you land this on m-r today? Thanks!
Flags: needinfo?(wkocher)
Flags: needinfo?(wkocher)
Thanks Liz, and sorry for all the trouble. I'll be extra careful when I find a regression like this next time!
Reproduced the initial issue described in comment 0 on Firefox 53.0RC, verified that the issue is fixed using Firefox 53.0.2 build 1 across platforms (Windows 7 x64, Windows 10 x64, macOS 10.12.4 and Ubuntu 16.04 64bit).
Verified as fixed using the STR from comment 0 using Firefox Beta 54.0b5 and latest Nightly 55.0a1 (2017-05-07) on Ubuntu 16.04 x64, Windows 10 x64 and Mac OS X 10.11.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.