666223 - Add method to retrieve the value of an element's CSS property

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Maniac Vlad Florin (:vladmaniac)]

We must add a function in utils.js to have getComputedStyle(element)

Comment 1

[u415141]

A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/14876971

Comment 2

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.0

Asking for r from Henrik since he requested this patch

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 541355 [details] [diff] [review]
patch v1.0

>+ * @param {MozmillController} controller
>+ *        MozMillController of the window to operate on

We don't need the controller as paramter here. You can use el.ownerDocument.defaultView to get the window.

>+ * @return value of the CSS property
>+ * @type string

This can be a single line: @return {String} Description

>+function getCSSValue(controller, aElement, aProperty) {

Can we rename this function to getElementStyle? 

>+  var el = aElement.getNode();

How does getComputedStyle handle a null element?

>+  return controller.window.getComputedStyle(el, null).getPropertyValue(aProperty);

There is no need to have the 2nd parameter to getComputedStyle specified:
https://developer.mozilla.org/en/DOM/window.getComputedStyle#Syntax

Also please ensure it will fit into the 80 chars limit.

Comment 4

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.1

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 542135 [details] [diff] [review]
patch v1.1

>+function getElementStyle(aElement, aProperty) {
>+  var el = aElement.getNode();
>+  var elementStyle = el.ownerDocument.defaultView.getComputedStyle(el);

You will have to handle the null case of el. In those cases we have to throw a reasonable exception.

Also something I forgot last time, please check existing tests which make use of getComputedStyle, so we directly have tests for that API addition.

Comment 6

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.2

Patch with requested changes

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 542459 [details] [diff] [review]
patch v1.2

Code-wise it's fine but please include the patches for existing tests which can make use of this new API. There are a couple of those.

Comment 8

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch_with_tests v1.0

This include modifications in testBlueLarry.js and testGreenLarry.js from /tests/functional/testSecurity

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 542788 [details] [diff] [review]
patch_with_tests v1.0

>+/**
>+ * Returns the value of a CSS Property for a specific Element
>+ *
>+ * @param {ElemBase} aElement
>+ *        Element to get its property
>+ * @param {String} aProperty
>+ *        Property name to be retrieved
>+ *
>+ * @return {String} value of the CSS property

Nit: please check the capitalization of some of the doc items, i.e. Property, Element, and value.

>-  var cssInfoLockImage = controller.window.getComputedStyle(lockIcon.getNode(), "");
>-  controller.assertJS("subject.getPropertyValue('list-style-image') != 'none'", cssInfoLockImage);
>+  var cssInfoLockImage = utils.getElementStyle(lockIcon, 'list-style-image');
>+
>+  controller.assert(function () {
>+    return cssInfoLockImage !== 'none';
>+  }, "There is a lock icon - got '" +
>+     (cssInfoLockImage !== 'none') + "', expected 'true'.");

Lets do the comparison only once, so we can also get the message in a single line too.

Please update those parts, otherwise it looks fine.

Comment 10

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch_with_tests v1.1

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 542841 [details] [diff] [review]
patch_with_tests v1.1

> exports.formatUrl = formatUrl;
> exports.formatUrlPref = formatUrlPref;
> exports.emptyClipboard = emptyClipboard;
> exports.getDefaultHomepage = getDefaultHomepage;
> exports.getEntity = getEntity;
> exports.getProperty = getProperty;
> exports.handleWindow = handleWindow;
> exports.removePermission = removePermission;
>+exports.getElementStyle = getElementStyle;

Please always keep the sorting order and do not add lines at the end of the export list. I will update it right before check-in.

Otherwise looks fine and works as expected. Sorry for the delay. Somehow it fell off my radar.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/5f207730e62f (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/45c32cce910c (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/2e3be6ffd443 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/6809ba0ead9d (release)