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

[ticket/15572] Fix W3C validation error for quote BBCode #5932

Merged
merged 1 commit into from Apr 21, 2020
Merged

[ticket/15572] Fix W3C validation error for quote BBCode #5932

merged 1 commit into from Apr 21, 2020

Conversation

AlfredoRamos
Copy link
Contributor

@AlfredoRamos AlfredoRamos commented Apr 5, 2020

Checklist:

  • Correct branch: master for new features; 3.3.x & 3.2.x for fixes
  • Tests pass
  • Code follows coding guidelines: master, 3.3.x and 3.2.x
  • Commit follows commit message format

Tracker ticket:

https://tracker.phpbb.com/browse/PHPBB3-15572

@3D-I
Copy link
Contributor

3D-I commented Apr 5, 2020

Does it also takes into consideration the quotes in PMs? @msg_url

@AlfredoRamos
Copy link
Contributor Author

@3D-I I thought about it but I'm not sure if I should also check for the @msg_url (cite attribute) since that URL links to a private content, which is not publicly accessible.

@Dark1z
Copy link
Contributor

Dark1z commented Apr 5, 2020

Well !!! in PM , there is no way for a W3C validator to do any validation.
So this PR is perfect for @post_url.

Also nice that you changed div to span 😄

Best regards 👍

@rxu
Copy link
Contributor

rxu commented Apr 5, 2020

After this change, what will happen with BBCode in old posts (already saved in database)? Will they require reparsing?

@AlfredoRamos
Copy link
Contributor Author

AlfredoRamos commented Apr 5, 2020

@rxu no, it only requires the user to clear the cache so TextFormatter (trough phpbb/textformatter/s9e/factory.php) can read that file to setup those BBCodes again.

@rxu
Copy link
Contributor

rxu commented Apr 5, 2020

What I meant is whether quote BBCode in 'old' posts will be broken. So if they won't, that's good.

@AlfredoRamos
Copy link
Contributor Author

@rxu in my tests I didn't had problems with previously posted messages.

Should I keep the blockquote cite > div CSS selector just to be sure? Because the worst thing that can happen is that date won't be located at the right of the quoted message.

@rxu
Copy link
Contributor

rxu commented Apr 5, 2020

Tested locally: posted a message and then applied the changes from the PR.
Before:
image

After:
image

@Dark1z
Copy link
Contributor

Dark1z commented Apr 5, 2020

After:
image

I think you forgot the CSS part.
Here : https://github.com/phpbb/phpbb/pull/5932/files#diff-640d4c568fbd9176eefbdfbd5782e31aR487
& then purge the cache.

Best regards 👍

@rxu
Copy link
Contributor

rxu commented Apr 5, 2020

I did apply both of HTML/CSS parts while recompiling outdated templates setting is on, but purging cache made it right, my bad :)
image

@Dark1z
Copy link
Contributor

Dark1z commented Apr 5, 2020

I did apply both of HTML/CSS parts while recompiling outdated templates setting is on, but purging cache made it right, my bad :)

Ahh! Nice 😍 .

Best regards 👍

@ghost
Copy link

ghost commented Apr 14, 2020

@3D-I I thought about it but I'm not sure if I should also check for the @msg_url (cite attribute) since that URL links to a private content, which is not publicly accessible.

Could you expand a bit please, @AlfredoRamos?
Cause just because it's not accessible for bots, shouldn't mean it can have invalid markup.

@AlfredoRamos
Copy link
Contributor Author

@mrgoldy The missing cite attribute (<blockquote cite="...">) doesn't make the markup invalid.

I said that I didn't include that attribute on private messages because I didn't see the point for it.

@marc1706 marc1706 added this to the 3.3.1 milestone Apr 21, 2020
@marc1706
Copy link
Member

Agreed, the cite attribute will definitely help the search engines there. :)

@marc1706 marc1706 merged commit 1dc5c4f into phpbb:3.3.x Apr 21, 2020
@AlfredoRamos AlfredoRamos deleted the ticket/15572 branch April 21, 2020 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants