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

VERIFIED FIXED in Firefox 53

Status

()

Toolkit
XUL Widgets
VERIFIED FIXED
3 months ago
14 days ago

People

(Reporter: scottwu, Assigned: scottwu)

Tracking

(Blocks: 2 bugs, {regression})

unspecified
mozilla54
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53blocking verified, firefox54 verified, firefox55 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

3 months ago
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.
Comment hidden (mozreview-request)
Blocks: 1323674
Comment hidden (mozreview-request)
(Assignee)

Comment 3

3 months ago
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)

Comment 4

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

Comment 5

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

Updated

3 months ago
Attachment #8839330 - Flags: review?(mconley)

Comment 6

3 months ago
mozreview-review
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.
(Assignee)

Comment 7

3 months ago
mozreview-review-reply
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 8

3 months ago
mozreview-review-reply
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 9

3 months ago
mozreview-review
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+
(Assignee)

Comment 10

3 months ago
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

Comment 11

3 months ago
Pushed by philringnalda@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0ee3593e2315
Remove .popup-anchor visibility rule r=mconley
Keywords: checkin-needed

Comment 12

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0ee3593e2315
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54

Updated

a month ago
Blocks: 1358487
The patch requires an uplift request for mozilla-release to solve Bug 1358487 with Firefox 53.0.1.
status-firefox53: --- → affected
Flags: needinfo?(scwwu)
(Assignee)

Comment 14

28 days ago
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?
(Assignee)

Comment 15

28 days ago
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.
tracking-firefox53: --- → blocking
Keywords: regression
Wes, can you land this on m-r today? Thanks!
Flags: needinfo?(wkocher)
Flags: needinfo?(wkocher)

Comment 19

18 days ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-release/rev/058fb1eeefc9
status-firefox53: affected → fixed
(Assignee)

Comment 20

18 days ago
Thanks Liz, and sorry for all the trouble. I'll be extra careful when I find a regression like this next time!
Duplicate of this bug: 1361714
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).
status-firefox53: fixed → verified
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
status-firefox54: fixed → verified
status-firefox55: --- → verified
You need to log in before you can comment on or make changes to this bug.