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.

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

Strikethrough format button not highlighted. #729

Closed
etoledom opened this issue Mar 8, 2019 · 13 comments
Closed

Strikethrough format button not highlighted. #729

etoledom opened this issue Mar 8, 2019 · 13 comments
Assignees
Labels
Core bug A bug that also happens on the web [OS] Android [Pri] High [Type] Bug Something isn't working Writing Flow

Comments

@etoledom
Copy link
Contributor

etoledom commented Mar 8, 2019

Highlighting attribute buttons on caret positions works for B, I and link but not for ABC.
It does work when typing with strikethough format.

Tested on iOS. Not sure if is multiplatform issue.

bug

@etoledom etoledom added the [Type] Bug Something isn't working label Mar 8, 2019
@hypest
Copy link
Contributor

hypest commented Mar 12, 2019

What is the exact html underneath that rich text content @etoledom ? Alternatively, can you add steps to reproduce?

I think that the format-lib doesn't support both del an s tags so, there might be a syncing issue with the toolbar when the html underneath is using the not supported tag.

@etoledom
Copy link
Contributor Author

etoledom commented Mar 12, 2019

This is the HTML generated:

<!-- wp:paragraph -->
<p>Hello <strike>world</strike> <strong>bold</strong></p>
<!-- /wp:paragraph -->

Steps:

  • Start on an empty paragraph block.
  • Press the ABC attribute button (Strikethrough).
  • Write something.
  • Put the cursor over the written text (with the strikethrough format)
  • Check that the ABC button is not active.

It happens for any striked text, is not necessary any combination of other attributes.
Tested on the last internal release (WPiOS 12.0, Hockey app)

Tested on WPAndroid beta (12.0-rc-1) and it does work well.

It would be good if someone can reproduce this on WPiOS.

@JavonDavis
Copy link
Contributor

Experiencing this on Pixel 3a - Android 9. I left a comment on #757 (comment) with gifs showing the weird behavior I was seeing.

@etoledom
Copy link
Contributor Author

So, It happens that Gutenberg uses and recognizes strikethrough with <s> tags and not with <strike> tags.

I'm not sure that not recognizing <strike> tag is a bug. It is also not recognized in web, so I'm inclined to close this issue.

#757 (comment) seems to be a different one.

@etoledom
Copy link
Contributor Author

Closing this since it works as expected with tags

@maxme
Copy link
Contributor

maxme commented Apr 2, 2020

Start on an empty paragraph block.
Press the ABC attribute button (Strikethrough).
Write something.
Put the cursor over the written text (with the strikethrough format)
Check that the ABC button is not active.

I was able to reproduce this on 14.5, the tag used was del. This is weird though, I tried again to record a video, but the tag s was added and everything was working fine (strikethrough button was highlighted).

@etoledom
Copy link
Contributor Author

etoledom commented Apr 2, 2020

I was able to reproduce this on 14.5, the tag used was del. This is weird though, I tried again to record a video, but the tag s was added and everything was working fine (strikethrough button was highlighted).

@maxme

I have noticed that if I start with Strikethrough on a clean text, it will work using s tag. But trying to add bold, then italic, and over a bold and italic text add strikethrough, it will add a del tag. And then it will continue using del tag even on over clean text.

I can add a video if it's not clear.

All this on Android. iOS is working fine.

@mchowning
Copy link
Contributor

mchowning commented Apr 2, 2020

I have noticed that if I start with Strikethrough on a clean text, it will work using s tag. But trying to add bold, then italic, and over a bold and italic text add strikethrough, it will add a del tag. And then it will continue using del tag even on over clean text.

I just noticed something similar (again only on Android). The distinction I'm seeing is that if I highlight text and apply strikethrough I get the s tag. If I have nothing highlighted, tap the strikethrough button, and then start typing strikethrough text I get the del tag. Also, when the strikethrough is using the del tag then the strikethrough button never enters a selected state appropriately, with the s tag the strikethrough text usually (but not always) activates the strikethrough button.

strikethrough mp4

Seems like we should reopen this issue.

cc: @maxme , @etoledom

@SergioEstevao
Copy link
Contributor

I think the main issue here is that Gutenberg web and the format library don't recognize the del, strike tags for the format.

@SergioEstevao SergioEstevao added the Core bug A bug that also happens on the web label May 4, 2020
@mchowning
Copy link
Contributor

mchowning commented Jun 24, 2020

Just noticed on Android that if I select str?u?ckthrough text that has the del tag (so it was entered by pressing the strikethrough button without any selected text, and then typing some text), I cannot remove the strikethrough formatting. I do not see this issue if I select text and then apply the strikethrough formatting (which results in the s tag).

remove strikethrough mp4

@guarani
Copy link
Contributor

guarani commented Jul 16, 2020

After some tests on both platforms here's what I came across.

iOS

In terms of strikethrough, as far as I can tell, iOS is working fine (as @etoledom commented here). Tested on WPiOS 15.2. When I moved the cursor over text that had strikethough formatting, it always reflected the correct state. Also working when I selected a chunk of formatted text.

Android

Mostly confirms what was reported in above comments. Tested on WPAndroid 15.2 using both Gboard and Samsung keyboard.

  • Inconsistencies with regard to which HTML tag is used to strikethrough text. Consider two scenarios:
    A) Select existing text and add strikethrough, result: <s> tag ✅
    B) Tap strikethrough button first and then add text, result <del> tag ❌

    Scenario B causes two problems:

    • The toolbar button state does not stay in sync with what's under the cursor (reported by @mchowning here)
    • The strikethrough cannot be removed by selecting the affected text and tapping the appropriate toolbar button (reported by @mchowning here)

Both Android and iOS

There's a (less important) inconsistency between how the mobile editor and the web editor behave. It's not specific to strikethrough, it's the same for bold or italics.

Backspace partway into text that has strikethrough formatting, then stop and type new characters: on Gutenberg web the new chars don't pick up the formatting but on Android or iOS they do (it doesn't matter if the strikethrough uses <s> or <del>, same result). Also, the toolbar button doesn't turn on to reflect the fact that formatting is on.

Gif (19s)

Next steps

In addition to the above tests, I did a quick test combining strikethrough with bold and italics and found no additional issues on either platform. I didn't test links.

I'd say this is an Android-only ticket so I'll add the appropriate label.

Next step is to find out why <del> is used instead of <s> on Android in scenario B above. A quick check shows that the Classic editor on Android uses <del> whereas the Classic editor on iOS uses <s> — so this points to being an Aztec issue.

So the next step now is to know why Android Aztec uses <del> and see if it can be safely changed to <s> when used in Gutenberg. Could I get your opinion @mchowning if this makes sense?

@cameronvoell
Copy link
Contributor

I believe that the Android steps detailed in comments above are all fixed with the simple AztecAndroid fix of defaulting to <s> tag instead of <del> tag. I implemented that change in this Aztec PR wordpress-mobile/AztecEditor-Android#919, and it can be tested using the build / branch in this WPAndroid PR - wordpress-mobile/WordPress-Android#12483.

One question that we might consider is whether we should add handling to GB-Mobile for posts that already contain the <del> or <strike> tags so that we don't see the issue remaining on already created posts. However, I think this issue should be tackled separately. My reasoning is that I noticed that a similar problem can be created on Gutenberg Web, iOS, and Android by editing HTML and adding tags that render format updates in the editor and article, but can not be modified using the toolbar buttons as one would expect.

For example, on a Gutenberg Web Post, the follow steps show a similar issue:

  1. Add a paragraph block with some text
  2. Switch to the HTML view
  3. Add a <b></b> tag around some or all of the text. (Gutenberg would normally use <strong>)
  4. Return to visual mode and note that the text appears bold.
  5. Try and highlight the text and change its bold state using the toolbar, note that it does not work.

While I think the <del> case is more severe because the Gutenberg Android editor has been adding it as a tag to posts all along, I think that my example shows that there is a decision that needs to be made about either normalizing tags to update to be the ones expected by Gutenberg OR not rendering them in the editor. If we do neither of those, we'll always have the case where a format is rendered in the editor, but the toolbar buttons do not work as expected.

@cameronvoell
Copy link
Contributor

This issue is fixed with the change here: wordpress-mobile/AztecEditor-Android#919 which has been merged to Gutenberg Mobile.

I created a separate issue on Gutenberg for the issue that some tags manually added in the HTML editor update the style in the post in a way that cannot be manipulated by the expected toolbar buttons: WordPress/gutenberg#25389 .That seems like a separate, likely less urgent issue than our strikethrough button on mobile using the wrong tag, which is covered in this issue, and can now be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core bug A bug that also happens on the web [OS] Android [Pri] High [Type] Bug Something isn't working Writing Flow
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants