Closed Bug 908441 Opened 6 years ago Closed 6 years ago

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

Categories

(Testing :: Marionette, defect)

x86_64
Linux
defect
Not set

Tracking

(firefox24 wontfix, firefox25 wontfix, firefox26 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

RESOLVED FIXED
mozilla26
Tracking Status
firefox24 --- wontfix
firefox25 --- wontfix
firefox26 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed

People

(Reporter: jugglinmike, Assigned: jugglinmike)

Details

Attachments

(1 file, 4 obsolete files)

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.
Attached patch 908441-set-date-time.patch (obsolete) — Splinter Review
Hi David,

This is my first patch to the Python Marionette client, so don't hesitate to nitpick it into oblivion :)
Attachment #794267 - Flags: review?(dburns)
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!
Attachment #794267 - Flags: review?(dburns) → review-
Attached patch 908441-set-date-time-v2.patch (obsolete) — Splinter Review
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 :)
Attachment #794267 - Attachment is obsolete: true
Attachment #794699 - Flags: review?(dburns)
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 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
Attachment #794699 - Flags: review?(dburns) → review+
Attached patch 908441-set-date-time-v3.patch (obsolete) — Splinter Review
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?
Attachment #794699 - Attachment is obsolete: true
Attachment #796267 - Flags: review?(dburns)
Attachment #796267 - Flags: feedback?(wlachance)
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.
Attachment #796267 - Flags: feedback?(wlachance) → feedback+
Attached patch 908441-set-date-time-v4.patch (obsolete) — Splinter Review
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.
Attachment #796267 - Attachment is obsolete: true
Attachment #796267 - Flags: review?(dburns)
Attachment #797199 - Flags: review?(dburns)
Attachment #797199 - Flags: feedback?(wlachance)
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 on attachment 797199 [details] [diff] [review]
908441-set-date-time-v4.patch

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

Looks good to me
Attachment #797199 - Flags: review?(dburns) → review+
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!
Attachment #797199 - Flags: feedback?(wlachance) → feedback+
Thanks to you both for all the help!

Would you mind landing this for me, David?
Attachment #797199 - Attachment is obsolete: true
Attachment #797292 - Flags: review?(dburns)
Attachment #797292 - Flags: review?(dburns) → review+
https://hg.mozilla.org/mozilla-central/rev/f91ba68a888a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Whiteboard: [checkin-needed-b2g18]
You need to log in before you can comment on or make changes to this bug.