Make WordPress Core

Opened 10 years ago

Closed 5 years ago

#29804 closed enhancement (wontfix)

Use HR instead of IMG as a placeholder for the more and nextpage tags

Reported by: iseulde's profile iseulde Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: TinyMCE Keywords: has-patch needs-refresh early needs-screenshots needs-unit-tests
Focuses: accessibility, javascript Cc:

Description

Read more here: #28335.

Attachments (5)

29804.patch (6.2 KB) - added by iseulde 10 years ago.
Screen Shot 2014-09-29 at 18.03.48.png (8.6 KB) - added by iseulde 10 years ago.
Screen Shot 2014-09-29 at 18.07.18.png (8.8 KB) - added by iseulde 10 years ago.
29804.2.patch (5.5 KB) - added by iseulde 9 years ago.
img_read_more_compare.png (123.1 KB) - added by pcarvalho 5 years ago.
Comparison of actual vs modfied in English vs modified in Portuguese

Download all attachments as: .zip

Change History (45)

@iseulde
10 years ago

#1 @iseulde
10 years ago

  • Focuses javascript added
  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Future Release to 4.1

#2 @iseulde
10 years ago

Now the tags are also translated! :)
I replaced "Read more..." with "(more…)". I though this would be good because it reflects what appears on the front-end, but that's not true since it can be overwritten by the theme or by a plugin. It's a string less to translate, but maybe "Read more..." is better.
Not sure if the the old images should be kept for back-compat.

#3 @iseulde
10 years ago

#28539 was marked as a duplicate.

#4 @afercia
9 years ago

Unfortunately, CSS generated content is not guaranteed to work on replaced elements If it works in some browsers, that's a non standard behavior and not future-proof. Technically, it's a bug. See for example Lea Verou's comment on the linked article:

Yes, although convenient, this behavior is technically a bug in these engines. I think extending :before and :after to apply to replaced elements was discussed in the CSSWG and rejected ...

That's the reason why in #28539 I was suggesting to consider a different approach.

For reference: http://www.w3.org/TR/CSS21/generate.html#before-after-content
...
Note. This specification does not fully define the interaction of :before and :after with replaced elements (such as IMG in HTML). This will be defined in more detail in a future specification.

#5 @iseulde
9 years ago

We can do some testing in different browsers and see where it works. In the worst case it's a just a line with a title text on hover.

#6 @afercia
9 years ago

yup, or test the TinyMCE noneditable plugin that would allow to use a (non editable) span instead of hr. I've tested a couple of months ago and it works pretty well. The CSS part would be basically the same :) it's also possible to center the text on the line and give that an inherited background color to match the body background color, just use :before for the line and :after for the title text, then some z-index.

TinyMCE would always wrap a p around the span and that would respect all the work @azaozz did to always have a paragraph around more and nextpage.

#7 @iseulde
9 years ago

That plugin doesn't work well atm. In that case you better use wpviews. See #28539.

#8 follow-up: @iseulde
9 years ago

@azaozz, does this look okay? Just … needs to go?

#9 in reply to: ↑ 8 @azaozz
9 years ago

  • Milestone changed from 4.1 to Future Release

Replying to avryl:

@azaozz, does this look okay?

No, it actually works worse than the current <img> placeholder because we replace is with <hr>. Introduces several inconsistencies/edge cases. Perhaps we can revisit in 4.2, but seems not worth it?

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

#10 @iseulde
9 years ago

Sure! :)

Has several inconsistencies/edge cases.

Could you list them here?

#11 @iseulde
9 years ago

  • Milestone changed from Future Release to 4.2

Let's sort out those inconsistencies and edge cases. I think using hr would be an improvement as we can translate the text.

#12 @azaozz
9 years ago

Using <hr> would let us use :before and :after CSS, but doesn't work well when inserting in the editor. As the More and Nextpage are HTML comments, we pretty much allow them in any HTML context. Image placeholders work pretty well anywhere too. However <hr> does not. Combined with autop, that can result in invalid HTML or blank lines being added before/after, etc.

#13 @iseulde
9 years ago

Combined with autop, that can result in invalid HTML or blank lines being added before/after, etc.

But we only do this in the TinyMCE editor, no? Not sure if autop is relevant here. Before any autop-ing it's an HTML comment again.

As the More and Nextpage are HTML comments, we pretty much allow them in any HTML context.

But do we really allow the more tag and nextpage tag in any context? The TinyMCE command to insert it will only insert it on the top level, you'd have to use the text editor to work around it. And I don't think something like nextpage should be allowed anywhere but on the top level. Anywhere else doesn't make sense.

#14 @iseulde
9 years ago

Also: #28335 :)

#15 follow-up: @DrewAPicture
9 years ago

  • Keywords reporter-feedback added

@iseulde: What's the status here?

Based on the feedback from @azaozz, seems like using pseudo elements here might not be advisable. And while I agree that realistically, the nextpage or more elements should really only be inserted at the top-level, we would want to be careful about breaking existing behavior, as seemed to be the case on #28335 before it was wontfixed.

#16 in reply to: ↑ 15 @iseulde
9 years ago

  • Keywords reporter-feedback removed

Replying to DrewAPicture:

@iseulde: What's the status here?

Based on the feedback from @azaozz, seems like using pseudo elements here might not be advisable. And while I agree that realistically, the nextpage or more elements should really only be inserted at the top-level, we would want to be careful about breaking existing behavior, as seemed to be the case on #28335 before it was wontfixed.

Inside TinyMCE you can only add them at the top level... But sure, in the text editor you can add it anywhere. Still, wouldn't TinyMCE validate the HTML and move it? I'll see what happens.

If we're not doing this I would remove the text and just leave a dotted/dashed line with a translatable title tag. The image is not translatable, so non English speakers won't understand it.

#17 @iseulde
9 years ago

TinyMCE Seems to handle this really well.

Combined with autop, that can result in invalid HTML or blank lines being added before/after, etc.

But we convert the HR back to a comment before autop, so it wouldn't affect that? :)

@iseulde
9 years ago

#18 @iseulde
9 years ago

Refreshed the patch.

This ticket was mentioned in Slack in #core-editor by iseulde. View the logs.


9 years ago

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


9 years ago

#21 @iseulde
9 years ago

  • Milestone changed from 4.2 to Future Release

#22 @swissspidy
8 years ago

  • Keywords needs-refresh added

#23 @iseulde
8 years ago

@azaozz Could you take a look at the last comments? What about adding this early now and see what comes up? Easy to revert.

#24 @iseulde
8 years ago

  • Milestone changed from Future Release to 4.6

#25 @iseulde
8 years ago

  • Keywords early added

This ticket was mentioned in Slack in #core-editor by iseulde. View the logs.


8 years ago

#27 @swissspidy
8 years ago

  • Keywords needs-screenshots added

Sounds like a nice enhancement.

Some before & after screenshots of this would be great.

#28 @afercia
8 years ago

  • Focuses accessibility added

Adding accessibility focus, this should be tested to at least check how screen readers will announce the HR.

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


8 years ago

#30 @iseulde
8 years ago

Another possibility: use spans and contenteditable="false".

This ticket was mentioned in Slack in #core-editor by ocean90. View the logs.


8 years ago

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


8 years ago

#33 @chriscct7
8 years ago

  • Milestone changed from 4.6 to Future Release

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


7 years ago

#35 @iseulde
7 years ago

  • Keywords needs-unit-tests added

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


5 years ago

This ticket was mentioned in Slack in #core-js by pcarvalho. View the logs.


5 years ago

@pcarvalho
5 years ago

Comparison of actual vs modfied in English vs modified in Portuguese

#40 @pcarvalho
5 years ago

Per recommendations, this ticket should be closed as it

  • introduces edges cases
  • classic editor will be deprecated

@iseulde, do you want to close this?

Last edited 5 years ago by pcarvalho (previous) (diff)

This ticket was mentioned in Slack in #core-js by pcarvalho. View the logs.


5 years ago

#42 @azaozz
5 years ago

  • Resolution set to wontfix
  • Status changed from new to closed

Another good idea but as @pcarvalho mentions above this is now superseded by the block editor. Also the edge cases inside the contenteditable introduced from switching the placeholder from an "inline" to a "block" element seem pretty bad/hard to fix. Thinking it is not worth proceeding, feel free to reopen if needed.

Note: See TracTickets for help on using tickets.