1318317 - [DateTimePicker] Delete button appears before time values are entered in the input field

Status: VERIFIED FIXED

(Toolkit :: UI Widgets, defect)

Description

[roxleitan]

Attachment: time input.html

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20161115030213

[Affected versions]:
Nightly 53.0a1

[Affected platforms]:
All platforms: Windows 10 x 64, Ubuntu 16.04, Mac OS X 10.11

[Steps to reproduce]:
1.Launch Nightly 53.0a1 with a new profile
2.Set "dom.forms.datetime" pref to True in about:config
3.Open attached html

[Expected result]:
Delete button should be displayed only after any time values are entered in the input field

[Actual result]:
Delete button is displayed before time values are entered in the input field

Comment 1

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v1.

Comment 2

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8828207 [details] [diff] [review]
patch, v1.

Hi Mike, may I have your review on this small patch? I have also fixed a typo on the reset-button (anoid -> anonid), so now it works correctly.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8828207 [details] [diff] [review]
patch, v1.

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

Seems okay, though I have one suggestion (see below about moving the reset button click event listener).

It's a bit of a shame that there's no centralized place where we can call updateResetButtonVisibility().

Anyhow, thanks for the patch!

::: toolkit/content/widgets/datetimebox.xml
@@ +1013,5 @@
>                        size="2" maxlength="2"
>                        xbl:inherits="disabled,readonly,tabindex"/>
>          </html:span>
>  
> +        <html:button class="datetime-reset-button" anonid="reset-button"

Yikes. Good catch on that typo.

@@ +1031,5 @@
>          this.mIsPickerOpen = false;
>  
> +        this.mResetButton =
> +          document.getAnonymousElementByAttribute(this, "anonid", "reset-button");
> +        this.mResetButton.addEventListener("click", () => {

Unlike the other events we're adding here, this one doesn't get removed.

Perhaps we should put this in the onClick handler instead if the original target is the reset button, near where we're already checking for it?

Comment 4

[Jessica Jong [:jessica] (inactive)]

(In reply to Mike Conley (:mconley) from comment #3)
> Comment on attachment 8828207 [details] [diff] [review]
> patch, v1.
> 
> Review of attachment 8828207 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Seems okay, though I have one suggestion (see below about moving the reset
> button click event listener).
> 
> It's a bit of a shame that there's no centralized place where we can call
> updateResetButtonVisibility().

Hmm, you're right, might do that in a follow-up patch.

> 
> Anyhow, thanks for the patch!
> 
> ::: toolkit/content/widgets/datetimebox.xml
> @@ +1013,5 @@
> >                        size="2" maxlength="2"
> >                        xbl:inherits="disabled,readonly,tabindex"/>
> >          </html:span>
> >  
> > +        <html:button class="datetime-reset-button" anonid="reset-button"
> 
> Yikes. Good catch on that typo.
> 
> @@ +1031,5 @@
> >          this.mIsPickerOpen = false;
> >  
> > +        this.mResetButton =
> > +          document.getAnonymousElementByAttribute(this, "anonid", "reset-button");
> > +        this.mResetButton.addEventListener("click", () => {
> 
> Unlike the other events we're adding here, this one doesn't get removed.
> 
> Perhaps we should put this in the onClick handler instead if the original
> target is the reset button, near where we're already checking for it?

Good idea, will provide a new patch to do this.

Comment 5

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v2.

addressed review comment 3.

Comment 6

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8830107 [details] [diff] [review]
patch, v2.

carrying r+.

Comment 7

[Jessica Jong [:jessica] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9582f2ae0ea01a399bc871fd0b37af1e7fb4c287&group_state=expanded

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f17e441ce4d0
[DateTimeInput] hide reset button when there is no value. r=mconley

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[roxleitan]

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170127030206

Verified as fixed using the latest Firefox Nightly 54.0a1 on Windows 10 x64, Ubuntu 16.04 and Mac OS X 10.10.