The code introduced in #2287805: Caption filter does not work with Quarter Size image format - or with floated captions. does not have any tests written for it.

CommentFileSizeAuthor
#4 wysiwyg_image_captions-2466823-4.patch3.47 KBsegovia94
#2 wysiwyg_image_captions-2466823-2.patch3.42 KBcarsonblack
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

segovia94’s picture

segovia94 CreditAttribution: segovia94 commented
Assigned: Unassigned » segovia94
carsonblack’s picture

carsonblack CreditAttribution: carsonblack commented
Assigned: segovia94 » carsonblack
Issue tags: +sprint
FileSize
wysiwyg_image_captions-2466823-2.patch3.42 KB

This patch creates a new test that checks the caption when the Quarter Size image style is applied to an image added via WYSIWYG as well as adds a new method, iSwitchToTheWysiwygDialogFrame to the WysiwygSubContext class that add a new step definition that will make it possible to switch to the dialog iframe.

carsonblack’s picture

carsonblack CreditAttribution: carsonblack commented
Assigned: carsonblack » Unassigned
Status: Active » Needs review
segovia94’s picture

segovia94 CreditAttribution: segovia94 commented
FileSize
wysiwyg_image_captions-2466823-4.patch3.47 KB

I rerolled this with an addition to float the image to the right and check that the output class is attached to the .caption class.

One thing to think about here is that the test doesn't visually check any of the possible outcomes brought up in the related issue this is testing. It only checks that the image has a caption. It doesn't check to make sure that the image is not overflowing the caption container or other problems the related issue fixed. I'm not totally sure how this could be done without some screenshot comparisons or using javascript to calculate positions on the page.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented

Thanks @segovia94 and @carsonblack!

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
Status: Needs review » Needs work

One small bit of review:

+++ b/tests/steps/panopoly_test_wysiwyg.behat.inc
@@ -168,6 +168,19 @@ class WysiwygSubContext extends BehatContext implements DrupalSubContextInterfac
+   * @When /^I switch to the wysiwg dialog frame$/

I'd prefer we used "WYSIWYG" (in all caps) in the step text to be consistent with the other WYSIWYG steps.

Trying on Travis-CI quick:

https://travis-ci.org/panopoly/panopoly/builds/63692054

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
Version: 7.x-1.20 » 7.x-1.x-dev

Passed on Travis! So, what we have so far works. :-)

One thing to think about here is that the test doesn't visually check any of the possible outcomes brought up in the related issue this is testing. It only checks that the image has a caption. It doesn't check to make sure that the image is not overflowing the caption container or other problems the related issue fixed. I'm not totally sure how this could be done without some screenshot comparisons or using javascript to calculate positions on the page.

I think the best approach would be Javascript that checks the width of the caption compared to the width of the image. That should be enough verify that everything is working as it should!

It'd be really great to get something like that included in this test, so leaving at "Needs work".

ergophobe’s picture

ergophobe CreditAttribution: ergophobe commented

That's probably true. I have Selenium IDE tests that grab screenshots, but the verification is strictly manual and I can't imagine how it could be automated simply, so your idea is probably a reasonable compromise. I've used a similar method with Selenium IDE to test on a Views Slideshow that had special resizing requirement and it seemed to effectively trip if something breaks (i.e. a class changes so the selector breaks).

The issue that jumps out is that if a shared selector changes, I could see the caption and the image both being returned as zero. So it should probably test that they are not only equal, but numeric and non-zero.

The original patch this test is being written for already had code to grab the widths of the image, but was found not to be robust enough. The updated patch includes a more robust method for grabbing the image width.
#2486713: Caption filter doesn't select the correct image size when an image's size is manually adjusted.

That patch code also sets the caption width and could easily be adapted to grab it for comparison.

Unfortunately, I still don't know how to integrate this into a test, but I'm happy to help as I can.

[EDIT]
Another note - I believe this too will need to use the imagesLoaded method as in the original patch. Here's my note on that from the original issue:

1. imagesLoaded. Because of the way browsers handle images, you can't use ready() (or Drupal.behaviors or DOMDocumentLoaded (which we all know), but you also can't use load() or window.onload. Those will not work with cached images or in other circumstances with webkit browsers (see #28 below). So we use the imagesloaded libary, which is already widely used in Drupal both within Panopoly (with the Manual Crop module) and elsewhere (the Equal Heights module, the OpenFed distro, etc). This lets us reliably wait for images to be loaded before trying to get the size.

Since the tests are run on Panopoly and should already fail if Manual Crop is unavailable, this should be fine and I assume the same imagesLoaded() method can be used here.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented

The issue that jumps out is that if a shared selector changes, I could see the caption and the image both being returned as zero. So it should probably test that they are not only equal, but numeric and non-zero.

Actually, since we're using a known image for the tests and running under a known theme, we can (and should!) specify a hard-coded width in pixels in the test. I'm imagining a step like:

Then the ".caption" element should be 300 pixels wide

... or something like that.

Another note - I believe this too will need to use the imagesLoaded method as in the original patch.

Yeah, that would make sense. We could make a step like:

And I wait for all the images to load

... which uses imagesLoaded to make sure everything is loaded before checking the width.

ergophobe’s picture

ergophobe CreditAttribution: ergophobe commented

>>we're using a known image

Check.

One other issue just to put it on the radar is the manual resize issue as in #2486713: Caption filter doesn't select the correct image size when an image's size is manually adjusted.)

Ideally the test would follow the logic of that most recent patch where you would take the value of the width attribute if available and check the caption width against that. Again, it could be a hard-coded width in the test just like the test for the default image.

Anyway, I don't want to make it more complex than it is, but did want to just flag the manual resize issue.