908441 - Convenience methods for interacting with HTML5 "date" and "time" input elements

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Mike Pennisi [:jugglinmike]]

Currently, modeling user interaction with "date" and "time" elements requires using the `send_keys` method with a correctly-formatted string. `set_date` and `set_time` methods would allow tests to specify these values more intuitively (i.e. with `date`, `time`, and `datetime` objects) without duplicating formatting logic.

Comment 1

[Mike Pennisi [:jugglinmike]]

Attachment: 908441-set-date-time.patch

Hi David,

This is my first patch to the Python Marionette client, so don't hesitate to nitpick it into oblivion :)

Comment 2

[David Burns :automatedtester]

Comment on attachment 794267 [details] [diff] [review]
908441-set-date-time.patch

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

That is a great first go but set_* is not very idiomatic of Python.

I would also rather have DateTimeElement class that abstracts things away.

E.g.

class DateTimeElement(object):

  def __init__(self, element):
      self.element = element

  @property
  def date(self):
      return self.element.get_attribute('value')

  @date.setter
  def date(self, date_value):
      #format date_value
      self.element.send_keys(formatted_date_according_to_html_spec)

  

  #same for time

  # These dont need to be done in the patch but we will need to add the following too
  #same for month
  #same for week
  

This class could live in a support module.

Thanks again for the great first patch!

Comment 3

[Mike Pennisi [:jugglinmike]]

Attachment: 908441-set-date-time-v2.patch

I've incorporated your feedback into this version. This patch also adds the new test file to the `unit-tests.ini` file.

Thanks for bearing with me :)

Comment 4

[William Lachance (:wlach)]

Comment on attachment 794699 [details] [diff] [review]
908441-set-date-time-v2.patch

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

Could you add some relevant documentation to the marionette documentation before this lands? We only just got inline documentation working, would be a shame if it went out of date. :) The relevant file to update is:

http://hg.mozilla.org/mozilla-central/file/tip/testing/marionette/client/docs/index.rst

Comment 5

[David Burns :automatedtester]

Comment on attachment 794699 [details] [diff] [review]
908441-set-date-time-v2.patch

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

This looks great. I would like a try push to make sure that everything is packaged up properly. In the mean time we should address :wlach's comments

Comment 6

[David Burns :automatedtester]

try push 

 https://tbpl.mozilla.org/?tree=Try&rev=b42d2a00164a
 https://tbpl.mozilla.org/?tree=Try&rev=b2dc9b8a0a02

Comment 7

[Mike Pennisi [:jugglinmike]]

Attachment: 908441-set-date-time-v3.patch

I've formatted this as a Mercurial patch and added documentation as William requested.

William: I'm positive I've fouled up the documentation. Would you help me fix it up?

1. The `DateTimeElement` is not really an "HTMLElement Object" (it's more of an interface for interacting with them), so it doesn't seem to fit in that location of the docs. (Really, this might even mean the `DateTimeElement` is the wrong name for this class.)
2. I wanted to include some references to the W3C spec and the RFC, but this didn't "feel" like appropriate `doc` string content, so I just included it as a comment. Is this right?

Comment 8

[William Lachance (:wlach)]

Comment on attachment 796267 [details] [diff] [review]
908441-set-date-time-v3.patch

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

Giving f+ because I like where this is going, but I have a bunch of issues that I'd like to see addressed before this lands...

::: testing/marionette/client/docs/index.rst
@@ +88,5 @@
>  .. automethod:: HTMLElement.is_displayed
>  .. automethod:: HTMLElement.value_of_css_property
>  
> +.. autoclass:: DateTimeElement
> +

I agree that putting this just below "HTMLElement Objects" is a bit confusing. How about we create a new section called "Modifying Document Content" and put both this and HTMLElement and DateTimeElement below it?

::: testing/marionette/client/marionette/date_time_element.py
@@ +1,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +class DateTimeElement(object):

Per your question about whether DateTimeElement is an appropriate name, I tend to agree that it might not quite be right. How about DateTimeValue?

@@ +4,5 @@
> +
> +class DateTimeElement(object):
> +    """
> +    Interface for setting the value of HTML5 "date" and "time" input elements.
> +    """

Could you put some example code of using this in the header. Something like the tests you've already written would be fine. See the execute_script function in marionette.py for an example of creating an example.

@@ +19,5 @@
> +
> +    # As per [the
> +    # specification](http://dev.w3.org/html5/markup/input.date.html),
> +    # this value is formatted according to [RFC
> +    # 3339](http://tools.ietf.org/html/rfc3339#section-5.6).

If this is just a comment, I would omit the markdown and just write:

RFC 3339: http://tools.ietf.org/html/rfc3339#section-5.

@@ +34,5 @@
> +
> +    # As per [the
> +    # specification](http://dev.w3.org/html5/markup/input.time.html),
> +    # this value is formatted according to [RFC
> +    # 3339](http://tools.ietf.org/html/rfc3339#section-5.6).

Same comment as above re: markdown.

Comment 9

[Mike Pennisi [:jugglinmike]]

Attachment: 908441-set-date-time-v4.patch

Thanks for the feedback, William. I believe I've implemented everything according to your recommendation, but I'd appreciate it if you could review once more.

David: I'm flagging you for review again because we've changed the name of the new class, and I'd rather err on the side of caution.

Comment 10

[David Burns :automatedtester]

The reason why I suggested DateTimeElement is that the WebDriver Spec is going to be changing "soon" to pass back more meta-data so we will get to a point. At that point we can change it from DateTimeValue to DateTimeElement.

Comment 11

[David Burns :automatedtester]

Comment on attachment 797199 [details] [diff] [review]
908441-set-date-time-v4.patch

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

Looks good to me

Comment 12

[William Lachance (:wlach)]

Comment on attachment 797199 [details] [diff] [review]
908441-set-date-time-v4.patch

>diff --git a/testing/marionette/client/docs/index.rst b/testing/marionette/client/docs/index.rst
>index eaab90b..617467d 100644
>--- a/testing/marionette/client/docs/index.rst
>+++ b/testing/marionette/client/docs/index.rst
>@@ -67,32 +67,37 @@ Script Execution
> 
> Debugging
> `````````
> .. autoattribute:: Marionette.page_source
> .. automethod:: Marionette.log
> .. automethod:: Marionette.get_logs
> .. automethod:: Marionette.screenshot
> 
>-HTMLElement Objects
>--------------------
>+Modifying Document Content
>+--------------------------

Let's change this to "Querying and Modifying Document Content".

Other than that, looks good!

Comment 13

[Mike Pennisi [:jugglinmike]]

Attachment: 908441-set-date-time-v5.patch

Thanks to you both for all the help!

Would you mind landing this for me, David?

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/f91ba68a888a

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/3597c51ab3ee

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/3597c51ab3ee