Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add sanitizer to disable auto lightbox #6936

Merged
merged 9 commits into from
Mar 9, 2022
Merged

Add sanitizer to disable auto lightbox #6936

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f9f096b
Add sanitiser to disable auto lightbox
dhaval-parekh Feb 24, 2022
e9766ae
update unit test case
dhaval-parekh Feb 24, 2022
42de8a9
Remove unwanted code from unit test cases
dhaval-parekh Mar 8, 2022
13e68b2
Update condition to enable auto lightbox disabled sanitizer
dhaval-parekh Mar 8, 2022
71a113c
Add unit test cases
dhaval-parekh Mar 8, 2022
12f0c94
Disable lightbox for images by default
dhaval-parekh Mar 9, 2022
2624459
Merge branch 'develop' of github.com:ampproject/amp-wp into enhanceme…
westonruter Mar 9, 2022
e02a9f9
Ensure consistent application of AMP_Auto_Lightbox_Disable_Sanitizer
westonruter Mar 9, 2022
7a3d259
Test amp_auto_lightbox_disabled being filtered true
westonruter Mar 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions includes/amp-helper-functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1608,6 +1608,12 @@ function amp_get_content_sanitizers( $post = null ) {
);
}

westonruter marked this conversation as resolved.
Show resolved Hide resolved
$is_auto_lightbox_disabled = apply_filters( 'amp_auto_lightbox_disabled', true );

if ( ! $is_auto_lightbox_disabled ) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an opposite of what should be. If $is_auto_lightbox_disabled is true, we should use AMP_Auto_Lightbox_Disable_Sanitizer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think it should be vice versa.
However current implementation is based on the Implementation brief of the issue.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I meant here is that the variable name says is_auto_lightbox_disabled and when it was true (i.e. disabled) we actually enabled the auto lightbox feature - so the opposite.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I meant here is that the variable name says is_auto_lightbox_disabled and when it was true (i.e. disabled) we actually enabled the auto lightbox feature - so the opposite.

$sanitizers[ AMP_Auto_Lightbox_Disable_Sanitizer::class ] = [];
}

/**
* Filters the content sanitizers.
*
Expand Down
24 changes: 24 additions & 0 deletions includes/sanitizers/class-amp-auto-lightbox-disable-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<?php
/**
* Class AMP_Auto_Lightbox_Disable_Sanitizer
*
* @package AmpProject\AmpWP
*/

/**
* Disable auto lightbox for images.
*
* @since 2.2.2
* @internal
*/
class AMP_Auto_Lightbox_Disable_Sanitizer extends AMP_Base_Sanitizer {

/**
* Add "data-amp-auto-lightbox-disable" attribute to body tag.
*
* @return void
*/
public function sanitize() {
$this->dom->body->setAttributeNode( $this->dom->createAttribute( 'data-amp-auto-lightbox-disable' ) );
delawski marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good to use createAttribute as then no attribute value will be shown. 👍

}
}
33 changes: 33 additions & 0 deletions tests/php/test-class-amp-auto-lightbox-disable-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
<?php
/**
* Class AMP_Auto_Lightbox_Disable_Sanitizer_Test.
*
* @package AmpProject\AmpWP
*/

use AmpProject\AmpWP\Tests\Helpers\MarkupComparison;
use AmpProject\AmpWP\Tests\TestCase;

/**
* Tests the auto lightbox disable sanitizer class.
*
* @coversDefaultClass AMP_Auto_Lightbox_Disable_Sanitizer
*/
class AMP_Auto_Lightbox_Disable_Sanitizer_Test extends TestCase {

use MarkupComparison;
delawski marked this conversation as resolved.
Show resolved Hide resolved

/**
* @covers ::sanitize()
*/
public function test_sanitize() {
delawski marked this conversation as resolved.
Show resolved Hide resolved

$source = '<html><body class="body-class"><span>Hello World!</span></body></html>';
$dom = AMP_DOM_Utils::get_dom_from_content( $source );

$sanitizer = new AMP_Auto_Lightbox_Disable_Sanitizer( $dom );
$sanitizer->sanitize();

$this->assertTrue( $dom->body->hasAttribute( 'data-amp-auto-lightbox-disable' ) );
}
}