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 5 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', false );
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't it be true by default? The acceptance criteria in #5122 says:

By default the data-amp-auto-lightbox-disable attribute should be added to the body.

The code above doesn't add the data-amp-auto-lightbox-disable attribute by default.

Copy link
Member

@westonruter westonruter Mar 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm late to respond to #5122 (comment).

After thinking about it some more, I think it's probably good to indeed disable by default. Otherwise, users who see the Lightbox toggle will be confused what purpose it serves if the images are lightboxed anyway.

Suggested change
$is_auto_lightbox_disabled = apply_filters( 'amp_auto_lightbox_disabled', false );
$is_auto_lightbox_disabled = apply_filters( 'amp_auto_lightbox_disabled', true );

delawski marked this conversation as resolved.
Show resolved Hide resolved

if ( $is_auto_lightbox_disabled ) {
$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. 👍

}
}
20 changes: 20 additions & 0 deletions tests/php/test-amp-helper-functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1900,6 +1900,26 @@ static function( $classes ) {
$this->assertEquals( AMP_Tag_And_Attribute_Sanitizer::class, $ordered_sanitizers[ count( $ordered_sanitizers ) - 1 ] );
}

/**
* @covers ::amp_get_content_sanitizers()
*/
public function test_amp_get_content_sanitizers_for_lightbox_sanitizer() {

add_filter( 'amp_auto_lightbox_disabled', '__return_true' );

$this->assertContains(
AMP_Auto_Lightbox_Disable_Sanitizer::class,
array_keys( amp_get_content_sanitizers() )
);

remove_all_filters( 'amp_auto_lightbox_disabled' );

$this->assertNotContains(
AMP_Auto_Lightbox_Disable_Sanitizer::class,
array_keys( amp_get_content_sanitizers() )
);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$this->assertContains(
AMP_Auto_Lightbox_Disable_Sanitizer::class,
array_keys( amp_get_content_sanitizers() )
);
remove_all_filters( 'amp_auto_lightbox_disabled' );
$this->assertNotContains(
AMP_Auto_Lightbox_Disable_Sanitizer::class,
array_keys( amp_get_content_sanitizers() )
);
$this->assertArrayHasKey(
AMP_Auto_Lightbox_Disable_Sanitizer::class,
amp_get_content_sanitizers()
);
remove_all_filters( 'amp_auto_lightbox_disabled' );
$this->assertArrayNotHasKey(
AMP_Auto_Lightbox_Disable_Sanitizer::class,
amp_get_content_sanitizers()
);

}

/**
* Test amp_get_content_sanitizers().
*
Expand Down
30 changes: 30 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,30 @@
<?php
/**
* Class AMP_Auto_Lightbox_Disable_Sanitizer_Test.
*
* @package AmpProject\AmpWP
*/

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 {

/**
* @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' ) );
}
}