WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#31373 closed task (blessed) (fixed)

Revamp Press This

Reported by: michael-arestad Owned by: azaozz
Milestone: 4.2 Priority: normal
Severity: normal Version: 4.2
Component: Press This Keywords:
Focuses: Cc:

Description (last modified by DrewAPicture)

This will be used as the master tracking ticket for the Press This Revamp feature plugin, which has been approved for merge in the 4.2 release.

Attachments (18)

31373.patch (159.2 KB) - added by azaozz 3 years ago.
31373.2.patch (256.5 KB) - added by DrewAPicture 3 years ago.
Docs: 1st pass audit
31373.3.patch (152.9 KB) - added by DrewAPicture 3 years ago.
Docs: 2nd pass audit + file fixes
31373.4.patch (160.4 KB) - added by azaozz 3 years ago.
31373.5.patch (4.8 KB) - added by ocean90 3 years ago.
31373.5.1.patch (5.1 KB) - added by ocean90 3 years ago.
31373.6.patch (5.6 KB) - added by ocean90 3 years ago.
31373.7.patch (491 bytes) - added by kraftbj 3 years ago.
Super minor inline docs typo (s/fr/for) in class-wp-press-this.php
length-less.diff (6.3 KB) - added by stephdau 3 years ago.
Cleans ip the use of unnecessary ( vartest && vartest.length ) in JS in press-this.php, as Ox did in bookmarklet.
31373-length-less.diff (6.3 KB) - added by stephdau 3 years ago.
Same as length-less.diff, but with ticket name in file (can't delete/overwrite other one, sorry)
31373-empty-fix.diff (580 bytes) - added by stephdau 3 years ago.
Fixes a bug where we test for empty( $key ), but $key can legitimately be 0 for the 1st key of an indexed array (_embed, _img).
31373-dailymotion.diff (871 bytes) - added by stephdau 3 years ago.
Adds DailyMotion embed detection in non-DailyMotion pages (as we do for others). Will discover DM embeds on any site/page.
31373-ytplaylists.diff (8.8 KB) - added by stephdau 3 years ago.
Adds support for Youtube playlists (supported by editor as WP embed), on youtube.com or when iframe embed is found on target page.
31373.8.patch (27.2 KB) - added by azaozz 3 years ago.
31373.9.patch (31.7 KB) - added by azaozz 3 years ago.
31373.10.patch (31.7 KB) - added by stephdau 3 years ago.
Same as 31373.9.patch, but with WP_Press_This::get_embeds() fixed (needed to use $data['_embed'], not $data['_embeds']).
31373-wpcom-stats.diff (643 bytes) - added by stephdau 3 years ago.
Better matching for WordPress.co stats gifs.
31373-better-embeds-and-docs.diff (8.2 KB) - added by stephdau 3 years ago.
Better Youtube URL matching, and better embed docs.

Download all attachments as: .zip

Change History (84)

#1 @DrewAPicture
3 years ago

  • Owner set to azaozz
  • Status changed from new to assigned

#2 @helen
3 years ago

#21551 was marked as a duplicate.

#3 @kraftbj
3 years ago

Tickets that can be modified once this is merged in:

Fixed:
#16922 - PT needs a class for the source attribution
#16935 - Selected text wrapped in blockquote
#18937 - Style PT header like admin bar
#27529 - Update media icons

Need Reporter Feedback:
#16940 - Does the new bookmarklet cause problems for mod_security?

IMO wontfix:
#7605 - Ability to edit slug via PT pop-up
#14650 - PT to use post-new.php

Should get a response updated with where PT now stands and the future roadmap:
#26681 - Grab HTML with selected text
#30803 - Selected image not used as featured image
#28873 - Improve keyboard accessibility of adding PT JS
#26794 - Remove PT code from Settings->Writing

Remaining tickets, as of now:
#14164 - do_meta_boxes on PT page
#12017 - Add site selector for multisite to allow one bookmarklet to be used on any blog user has an acceptable role on.

#4 @DrewAPicture
3 years ago

  • Reporter changed from DrewAPicture to michael-arestad

#5 @DrewAPicture
3 years ago

  • Description modified (diff)

@azaozz
3 years ago

#6 @azaozz
3 years ago

In 31373.patch: the plugin converted to a core patch (160KB...). Still some things to iron out/decide.

#7 @stephdau
3 years ago

@drewapicture: your version of the patch is at least missing src/wp-admin/js/bookmarklet.js, which leads to the following when someone drags the bookmarklet to their bookmark bar:

https://src.wordpress-develop.dev/wp-admin/%3Cbr%20/%3E%3Cb%3EWarning%3C/b%3E:%20%20file_get_contents(
/srv/www/wordpress-develop/src/wp-admin/js/bookmarklet.js):%20failed%20to%20open%20stream:%20No%20such
%20file%20or%20directory%20in%20%3Cb%3E/srv/www/wordpress-develop/src/wp-includes/link-template.php%3C
/b%3E%20on%20line%20%3Cb%3E2640%3C/b%3E%3Cbr%20/%3Ejavascript:

#8 @stephdau
3 years ago

@azaozz: your version works great!

Couple of things to think about:

#9 @stephdau
3 years ago

Upgrade notice: the culprit is !== and different types between data.v and data._version.
Using parseInt() when comparing them makes the problem go away.

src/wp-admin/js/press-this.php -> L640 (in 31373.patch):

// Prompt user to upgrade their bookmarklet if there is a version mismatch.
if ( data.v && data._version && parseInt( data.v ) !==  parseInt( data._version ) ) {
Last edited 3 years ago by stephdau (previous) (diff)

@DrewAPicture
3 years ago

Docs: 1st pass audit

@DrewAPicture
3 years ago

Docs: 2nd pass audit + file fixes

@azaozz
3 years ago

#10 @azaozz
3 years ago

In 31373.4.patch: some css fixes/enhancements for the Tools screen by @michael-arestad.

This ticket was mentioned in Slack in #core by azaozz. View the logs.


3 years ago

#12 @Michael Arestad
3 years ago

I've already found a few bugs. I created tickets against trunk in the Press This component. The titles are prefixed with "Press This:".

Tab order bug in Safari
Black border in IE
Convert categories list to li because of major display bugs

More testing in a bit.

#13 @azaozz
3 years ago

In 31534:

PressThis v2, first run. Props michael-arestad, stephdau, marcelomazza, DrewAPicture, iseulde, afercia, kraftbj, rachelbaker, AramZS, dd32. See #31373.

#14 @azaozz
3 years ago

In 31535:

PressThis:

  • Hard-code the minified bookmarklet js. Adding the non-minified bookmarklet to the browser bookmarks bar may have unexpected effect.
  • Fix type juggling when checking the bookmarklet version.

Props stephdau, see #31373.

#15 @azaozz
3 years ago

In 31537:

Press This: JSON encode the URL before appending it to the bookmarklet. See #31373.

#16 @kraftbj
3 years ago

Noting that I closed (as worksforme since best I could do) #16922, #16935, #18937, #26794, #27529 per the above list.

#17 @stephdau
3 years ago

W00t w00t.

#18 @azaozz
3 years ago

In 31545:

PressThis: go back to loading the minified bookmarklet content with file_get_contents(). Add Grunt task to minify bookmarklet.js on precommit and update it in /src. See #31373.

#19 @azaozz
3 years ago

In 31547:

Press This: add press-this.css to the list of stylesheets that are minified. Bump wp_version. See #31373.

#20 @Michael Arestad
3 years ago

Noting 31455 here. Should fix FF scrolling issues.

#21 @Michael Arestad
3 years ago

Added patch for 31456 (which is an awesome number) that just adds a class to make alert styles work properly.

#22 @Michael Arestad
3 years ago

Some feature request tickets:

31457: Close sidebar when cursor clicks out of sidebar or tab focus leaves sidebar
31458: Add Preview functionality

#23 @Michael Arestad
3 years ago

Noting accessibility patch 31449 by @afercia

#24 @kraftbj
3 years ago

Noting updated upgrade bookmarklet patch in #31461.

#25 @kraftbj
3 years ago

Open question on styling of tools.php with the "new" style of the Press This area in #31462

#26 @ocean90
3 years ago

In 31571:

Press This: Use boolean value instead of return_true(). Add missing hook docs.

see #31373.

#27 @ocean90
3 years ago

In 31572:

Press This: Add press-this to list of RTL styles.

see #31373.

@ocean90
3 years ago

@ocean90
3 years ago

@ocean90
3 years ago

#28 follow-up: @ocean90
3 years ago

In 31588:

Press This: Backwards compatibility enhancements.

  • Add missing actions for printing styles/scripts.
  • Since $hook_suffix is null, hardcode press-this.php.
  • Restore body classes, add filter.
  • Use boolean value instead of __return_false().
  • Use wp_json_encode().
  • Update docs for filters in script-loader.php.
  • Make <a href="%1$s">%2$s</a> not translatable.

see #31373.

#29 @Michael Arestad
3 years ago

Noting removal of class on suggested blockquote and source: 31493

#30 in reply to: ↑ 28 @azaozz
3 years ago

Replying to ocean90:

  • Add missing actions for printing styles/scripts.

Not sure this is a good idea. Many plugins do many things in the admin. Actually was thinking we shouldn't fire any of the default admin actions but only Press This specific actions.

  • Since $hook_suffix is null, hardcode press-this.php.

$hook_suffix is set in wp-admin/admin.php loaded from press-this.php: https://core.trac.wordpress.org/browser/trunk/src/wp-admin/press-this.php#L12

If it is not set in some circumstances, we still need to set it globally as many plugins use it to determine when to output scripts/styles and some are probably looking at the global.

  • Restore body classes, add filter.

Again, not sure that is "wise", at least not for all the body classes :) For example currently Press This doesn't support admin colors (the PT css is completely separate from the rest of wp-admin.css). Having a color class may trigger something we don't want. If a plugin adds something that triggers on a particular body class, and expects wp-admin.css to be loaded, it will fail and might affect Press This.

Was thinking we could/should move the HTML generation inside press-this.php, so it is inline with the rest of the admin "entry points" (the way it used to be). That means it will run directly in global scope, like the rest of wp-admin.

Last edited 3 years ago by azaozz (previous) (diff)

#31 @azaozz
3 years ago

Actually $hook_suffix should never be null, admin.php should always be loaded (in global scope) before calling WP_Press_This::html().

#32 @azaozz
3 years ago

In 31589:

PressThis:

  • Remove unneeded passing of post formats strings to JS.
  • Set the currently selected post format name with jQuery.

See #31373.

@kraftbj
3 years ago

Super minor inline docs typo (s/fr/for) in class-wp-press-this.php

#33 @azaozz
3 years ago

In 31595:

PressThis:

  • Simplify getSuggestedContent() and helpers. No need to override the global data.
  • Replace the press_this_source_string and press_this_source_link filters with press_this_suggested_html that allows filtering of the link and the wrapper HTML tags.

See #31373.

#34 @azaozz
3 years ago

In 31596:

PressThis:

  • Replace all %1$s and %2$s in suggestedHTML in case plugins repeat them.
  • Fix docs typo, props kraftbj.

See #31373.

#35 @Michael Arestad
3 years ago

Updated buttons here: 31498

#36 @azaozz
3 years ago

In 31597:

PressThis: make sure buttons.css is loaded before press-this.css. Use (int) $post_ID instead of $post->ID.
See #31373.

#37 @kraftbj
3 years ago

Forgot to note here #31494 (make the suggested HTML filterable) which @azaozz did today in [31595] and so is safe to mark as fixed.

#38 @azaozz
3 years ago

In 31609:

PressThis:

  • Improve handling of the data, both from the bookmarklet and from server-side parsing.
  • Standardize on processing the data in PHP and remove duplicate code from JS.
  • Improve the bookmarklet code and remove pre-filtering of the data.

Part props stephdau, see #31373.

@stephdau
3 years ago

Cleans ip the use of unnecessary ( vartest && vartest.length ) in JS in press-this.php, as Ox did in bookmarklet.

@stephdau
3 years ago

Same as length-less.diff, but with ticket name in file (can't delete/overwrite other one, sorry)

@stephdau
3 years ago

Fixes a bug where we test for empty( $key ), but $key can legitimately be 0 for the 1st key of an indexed array (_embed, _img).

@stephdau
3 years ago

Adds DailyMotion embed detection in non-DailyMotion pages (as we do for others). Will discover DM embeds on any site/page.

#39 @azaozz
3 years ago

In 31613:

PressThis: remove the extra .length tests for strings from press-this.js.
Props stephdau, see #31373.

#40 @stephdau
3 years ago

Added 2 more small patches:

The equivalent of the length-less patches was committed in r31613.

@stephdau
3 years ago

Adds support for Youtube playlists (supported by editor as WP embed), on youtube.com or when iframe embed is found on target page.

#41 @stephdau
3 years ago

Added 31373-ytplaylists.diff to add support for Youtube playlists.

#42 @azaozz
3 years ago

In 31635:

PressThis:

  • Fix error when checking for empty array keys.
  • Add better DailyMotion embed support.

Props stephdau, see #31373.

#43 @azaozz
3 years ago

In 31636:

PressThis: fix toggling of aria-expanded attribute.
Props afercia, see #31373.

#44 @azaozz
3 years ago

In 31637:

PressThis: when server-side parsing, filter small images by the width and height attributes if set.
See #31373.

#45 @tyxla
3 years ago

Related: #31558.

In this ticket I've proposed a fix for an issue with duplicate initialization of tagBox in wp-admin/js/tags-box.js.

#46 @azaozz
3 years ago

In 31373.8.patch:

  • Move filtering of the data from JS to PHP. No need to send all data to JS and filter it there, can send only useful data.
  • Clean up the links passed from the bookmarklet. We are using only canonical, if found.
  • Load the suggested title and post content from PHP, no need to send to JS first.

@azaozz
3 years ago

#47 @azaozz
3 years ago

In 31680:

PressThis: fix styling of wpLink.
Props Michael-Arestad, see #31373.

@azaozz
3 years ago

#48 follow-up: @azaozz
3 years ago

In 31373.9.patch: fixed few typos and inconsistencies, added bookmarklet.min.js so it can be tested easier (no need to run grunt precommit before updating the bookmarklet).

@stephdau
3 years ago

Same as 31373.9.patch, but with WP_Press_This::get_embeds() fixed (needed to use $data['_embed'], not $data['_embeds']).

#49 in reply to: ↑ 48 @stephdau
3 years ago

Replying to azaozz:

In 31373.9.patch: fixed few typos and inconsistencies, added bookmarklet.min.js so it can be tested easier (no need to run grunt precommit before updating the bookmarklet).

See 31373.10.patch for a small fix to WP_Press_This::get_embeds().

#50 follow-up: @stephdau
3 years ago

Alternatively, and maybe for the better, we could also standardize on the use of _embeds and _images for those keys everywhere (bookmarklet, app JS/PHP), instead of now _img, _embed, _images, and _embeds. We uglify the bookmarklet now, so we no longer need to save on characters there. :)

Last edited 3 years ago by stephdau (previous) (diff)

#51 @azaozz
3 years ago

In 31693:

PressThis:

  • Filter and select the content on the PHP side. Then pass only the needed data to JS.
  • Add the suggested post title and contend directly to the HTML.
  • Standardise the data type names.
  • Some cleanup/reduction of the code in the bookmarklet.

See #31373.

#52 in reply to: ↑ 50 @azaozz
3 years ago

Replying to stephdau:

...we could also standardize on the use of _embeds and _images for those keys everywhere (bookmarklet, app JS/PHP), instead of now _img, _embed.

Right, good idea. Done in the commit. Also, thanks for the typo fixes :)

#53 @stephdau
3 years ago

We're having a problem with UTF-8 URLs: the characters are being stripped by sanitize_text_field() in WP_Press_This::_limit_string() right after the URL was html entity decoded, with UTF-8 compatibility, because WP_Press_This::_limit_url() uses it.

So the URL http://tekartist.org/2015/03/10/%e2%99%ab-phantogram-when-im-small-live-at-kexp/ becomes http://tekartist.org/2015/03/10/-phantogram-when-im-small-live-at-kexp/ after sanitization, which leads to a 404. The bookmarklet appears to work on that URL because it passes some valid meta data and does not fetch the source, but the attribution link is bad. If you use the same URL in the "Scan URL" form, the fetch fails altogether, for the same reason.

Should we consider not running sanitize_text_field() on strings that look like URL?

Index: src/wp-admin/includes/class-wp-press-this.php
===================================================================
--- src/wp-admin/includes/class-wp-press-this.php	(revision 31694)
+++ src/wp-admin/includes/class-wp-press-this.php	(working copy)
@@ -330,8 +330,10 @@
 				$return = $value;
 			}
 
-			$return = html_entity_decode( $return, ENT_QUOTES, 'UTF-8' );
-			$return = sanitize_text_field( trim( $return ) );
+			$return = trim( html_entity_decode( $return, ENT_QUOTES, 'UTF-8' ) );
+			if ( ! preg_match( '/^https?:/', $return ) ) {
+				$return = sanitize_text_field( $return );
+			}
 		}
 
 		return $return;

Or simply stop WP_Press_This::_limit_url() from using WP_Press_This::_limit_string(), since it starts with an is_string() test?

Index: src/wp-admin/includes/class-wp-press-this.php
===================================================================
--- src/wp-admin/includes/class-wp-press-this.php	(revision 31694)
+++ src/wp-admin/includes/class-wp-press-this.php	(working copy)
@@ -342,8 +342,6 @@
 			return '';
 		}
 
-		$url = $this->_limit_string( $url );
-
 		// HTTP 1.1 allows 8000 chars but the "de-facto" standard supported in all current browsers is 2048.
 		if ( mb_strlen( $url ) > 2048 ) {
 			return ''; // Return empty rather than a trunacted/invalid URL

@stephdau
3 years ago

Better matching for WordPress.co stats gifs.

@stephdau
3 years ago

Better Youtube URL matching, and better embed docs.

#54 @stephdau
3 years ago

Also added 2 small patches:

#55 @azaozz
3 years ago

In 31737:

PressThis: update _limit_url(), use esc_url_raw(). Fixes checking of urlencoded strings.
See #31373.

#56 @azaozz
3 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 31739:

PressThis: add inline comments and some cleanup for the images and embeds regex.
Props stephdau. Fixes #31373.

#57 @azaozz
3 years ago

In 31774:

PressThis: in Grunt, minify the bookmarklet before copying the files to /build.
See #31373.

#58 @azaozz
3 years ago

In 31778:

PressThis: increase the number of meta tags we check in the bookmarklet to 200. Sometimes there are empty meta tag nodes (no attributes) in some browsers.
See #31373.

#59 follow-up: @Stagger Lee
2 years ago

Dont know where to ask. Sorry if it is wrong place.

Did you reconsider making it easy to change CSS style of Press This editor container ? (press-this-editor.css)

Wordpress is about web editors beginners. And I personally find it very scary that they are faced with one screen where they have to insert something publicly visible on their websites, but CSS style of text formatting is light years away from style of their websites.

#60 in reply to: ↑ 59 @azaozz
2 years ago

Replying to Stagger Lee:

Did you reconsider making it easy to change CSS style of Press This editor...

Not sure what you mean by "make it easy". It is as easy as using a filter to pass another stylesheet URL. You can use one of the specific Press This actions and add another URL with mce_css. Something like this should work fine:

add_action( 'admin_head-press-this.php', 'my_pt_styles' );
function my_pt_styles() {
  add_filter( 'mce_css', 'my_press_this_editor_css' );
}

function my_press_this_editor_css( $css ) {
  if ( ! empty( $css ) {
    $css .= ',';
  }

  $css .= plugins_url( 'my-subdir/my-pt-style.css', __file__ );

  return $css;
}
Last edited 2 years ago by azaozz (previous) (diff)

#61 @Stagger Lee
2 years ago

Thank you. Will save snippet and use it. Best to link to editor style Advanced TinyMce plugin use, and both are done at once. Change path to theme root.

One more thing. Breaks and paragraphs in text. Can you bring it back and respect as it is on remote website ?
One question here is to be asked. Is it more harm done for look of post by few small problems on remote website, or it is more harm done by putting all text in one paragraph without any spacing. Think answer is oblivious, but it is just my personal opinion.

Last edited 2 years ago by Stagger Lee (previous) (diff)

#62 @michaelarestad
2 years ago

One more thing. Breaks and paragraphs in text. Can you bring it back and respect as it is on remote website ?

I'm not sure I understand. Can you give an example?

#63 follow-up: @Stagger Lee
2 years ago

Let us say you want to copy one editorial from Guardian newspapper. When all in Press This is finished all text ends in just one paragraph. No breaks, no lines, no other paragraphs as on original remote website (post).

If you use TinyMce Advanced plugin and button in editor "Ad as text" or what it is called dont remember now, before pasting text. After text pasting all breaks and paragraphs from remote website are preserved.

Mathematically speaking it takes much, much, much less time to fix formatting when just copy/paste text from remote website, than when dealing with Press This.

#64 in reply to: ↑ 63 @DrewAPicture
2 years ago

Replying to Stagger Lee:

Let us say you want to copy one editorial from Guardian newspapper. When all in Press This is finished all text ends in just one paragraph. No breaks, no lines, no other paragraphs as on original remote website (post).

@michaelarestad FWIW, I've seen this too. The formatting is correct in the Press This editor but once you make it over to the standard editor, all the formatting tags are in place but it's a giant block of text and markup with no line breaks or indentation. Haven't yet had a chance to track down where it's happening (it doesn't always).

#65 @michaelarestad
2 years ago

Oh! Right! Here's an open ticket for it. Would love someone to take a look. #33712

#66 @Stagger Lee
2 years ago

Talk to @azaozz, developer of the TinyMce Advanced plugin. His editor button "Paste as Text" makes wonder, all the time.

Note: See TracTickets for help on using tickets.