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

(3.0 Alpha 2) - Repeater - ID's are duplicated #432

Closed
wplit opened this issue Jul 24, 2019 · 81 comments
Closed

(3.0 Alpha 2) - Repeater - ID's are duplicated #432

wplit opened this issue Jul 24, 2019 · 81 comments
Labels
bug Something isn't working confirmed bug Oxygen support staff has replicated and confirmed issue
Projects

Comments

@wplit
Copy link

wplit commented Jul 24, 2019

Describe the bug
Every post inside the loop has the same ID

To Reproduce
Steps to reproduce the behavior:

  1. Add repeater element
  2. Query posts
  3. Add any elements inside repeater div
  4. Inspect html on front end

Expected behavior
Each new post & all elements inside should have new unique IDs, or no IDs at all, just classes if they all need to be the same.

What actually happens
Every element inside the loop has the same ID in each post. It causes an error in chrome lighthouse

Screen Shot 2019-07-24 at 5 00 26 pm

Screenshots
Screen Shot 2019-07-24 at 4 56 02 pm

@wplit wplit added the bug Something isn't working label Jul 24, 2019
@Spellhammer Spellhammer added this to New Bug Reports in Bug Reports via automation Jul 25, 2019
@Spellhammer Spellhammer moved this from New Bug Reports to Pending Development in Bug Reports Jul 25, 2019
@louisreingold
Copy link
Member

I can't think of a good way to address this. If we change the IDs, all the styles will stop working, so we'd have to output duplicate styles for every ID in the repeater. When we originally built Oxygen we should have skipped IDs entirely and used exclusively classes, but we didn't, and it's too late to do that now.

@albertso
Copy link

Incrementing the ids would make them unique, and if we use classes to style the blocks, there shouldn't be any styles from the id's to repeat.

What would be helpful is some way to copy styles between classes and ids, and also a button to reset the styles in a block or class.

@wplit
Copy link
Author

wplit commented Jul 25, 2019

hmmm..

Incrementing the ids, then having to duplicate all the CSS to make it work doesn't sound ideal.

I would argue the IDs aren't needed at all with repeater elements, but i get it's built heavily into how the components work. If you could somehow...

  • Prevent styles from being added by the user until a class is added, on any element inside the repeater.
  • Don't output the IDs for these elements at all on the front end.

Or have a little warning above the class/id field saying, you're styling an element inside a repeater, please use a class first, Less ideal, but slightly easier to implement.

Update: I've just realised this won't work, as users won't be able to place existing elements into repeaters. As they may have already been styled via ID. Hmm.

@Spellhammer Spellhammer moved this from Pending Development to Confirmed / Replicated in Bug Reports Jul 26, 2019
@albertso
Copy link

albertso commented Jul 26, 2019

How about if there is an option to disable using id for any kind of block, inside or outside a repeater?

For example a toggle button next to the id field that grays out the id so the user knows it's disabled.

The id could still be there if that's necessary for oxygen internally, but we'd have the option to not use it in frontend.

Also, to help in the workflow of transferring styles from one thing to another, and seeing what's in use, we could have a button to inspect all the styles applied to a block or class, in raw css.

If we could copy/edit/paste/clear the css there and have them converted back to oxygen properties, it would really help in moving stuff between stylesheets, ids and classes.

Whatever property isn't handled by oxygen can go into the custom css option.

@Hanleymade
Copy link

What is the limitations and fall out of Oxygen being built this way in technical terms? IF it can't be changed as Louis says, then if Oxygen continues to be more dynamic but can't change to classes, then what? Does it affect site performance, speed, bloat, etc.?

@alpabuz
Copy link

alpabuz commented Aug 27, 2019

I can't think of a good way to address this. If we change the IDs, all the styles will stop working, so we'd have to output duplicate styles for every ID in the repeater. When we originally built Oxygen we should have skipped IDs entirely and used exclusively classes, but we didn't, and it's too late to do that now.

It may be possible to implement the generation of styles for each element with reference to the parent ID and this is what to display in the user interface, such as: _dinamic_list-60-43> *: nth-child(0). And by the same principle for nested elements of each element: _dinamic_list-60-43> *: nth-child(0)> *: nth-child(0). Due to the impossibility of deleting the ID for each element, I think it is possible to implement ID regeneration with the addition of a new index after the element is added to the repeater (e.g. div_block-61-43-1, div_block-61-43-2, div_block-61-43-3 ...). At the same time, such IDs should not be involved in the generation of styles and not displayed in the user interface.

I apologize if I have not explained clearly or my idea is too delusional.

@srikat
Copy link

srikat commented Sep 3, 2019

@knytl
Copy link

knytl commented Sep 17, 2019

The jQuery workaround is handy, but still need to add code. I think it would be enough to be able to remove ID of element. Now when I delete ID it still output "id" which is not much better.

@wpsumo
Copy link

wpsumo commented Oct 30, 2019

@knytl Yeah but we also remove all IDs in within the dynamic fields. Such as if the user adds an ID to any element such as H1-H6 in the WYSIWYG editor. Those will be removed with the Jquery workaround as well.

@wpsumo
Copy link

wpsumo commented Nov 4, 2019

Same issue in Gutenberg blocks. If using the same block on the same page we have duplicate IDs. So technically the same issue and root cause.

@wplit
Copy link
Author

wplit commented Nov 6, 2019

Just linking to a workaround to search for duplicate IDs across the whole page, then remove them. Again, just jQuery code tho.

(see code in comments, not at the top)
https://gist.github.com/wplit/7861b18ef34ffd955b43ddcacc9ef6e8

This means any Oxygen sections created for client use with Gutenberg have to be 100% styled with classes, as we can't control when/where they are used on the site and if multiple versions of the same block will be used on the same page.

@fadlee
Copy link

fadlee commented Dec 10, 2019

I created simple plugin to fix this issue.

https://github.com/fadlee/oxygen-repeater-fix

image

This plugin doesn't modify any of Oxygen functionality. It just modify the output, the HTML and CSS code, using simple preg_replace.

HTML mod

  • modify oxy_dynamic_list shortcode output, move defined ID to class
  • store oxy_dynamic_list children IDs for CSS modification

CSS mod

  • monitor CSS xlink request, and buffer the output
  • for cached CSS, modify Oxygen CSS URI, so the request can be intercepted
  • modify CSS based on IDs stored before
  • CSS selector modification: #child_id => #parent_id .child_id

@wpsumo
Copy link

wpsumo commented Dec 10, 2019

@fadlee Great and good initiative! The ID is really starting to become a blocker when using TOC and once you need to use ID for other purposes. Which is why duplicated ID is bad practice. But found two issues:

  1. Does not cover Gutenberg blocks
  2. Have to cater for custom added ID in Gutenberg blocks.
  3. Breaks all design: https://tppr.me/SSv1T
    Could we try a version or chose to not move ID to class and manipulate the CSS mod. Since this is causing everything break for me and I do not need it. I assume more users will experience the broken style as I do and do not need the CSS mod. To cater for both best would be to have a toggle on/off for ID to class or to just remove ID's.

Would be great if it could remove the Oxygen IDs from gutenberg blocks but keep my automated/manually added ID's for the TOC:

function auto_id_headings( $content ) {
	$content = preg_replace_callback( '/(\<h[1-6](.*?))\>(.*)(<\/h[1-6]>)/i', function( $matches ) {
		if ( ! stripos( $matches[0], 'id=' ) ) :
			$matches[0] = $matches[1] . $matches[2] . ' id="' . sanitize_title( $matches[3] ) . '">' . $matches[3] . $matches[4];
		endif;
		return $matches[0];
	}, $content );
    return $content;
}
add_filter( 'the_content', 'auto_id_headings' );

So would assume it would require some regex headline-* etc would require your plugins php script to run before my php snippet above which also run preg_replace_callback but checks if the "ID slot is busy" and won't add if it's already an ID assigned to the headline.

@fadlee
Copy link

fadlee commented Dec 10, 2019

@alriksson Thanks for the feedback.

  1. What kind of Guternberg blocks do you mean?

This plugin developed specifically for Oxygen Repeater. I've tested Repeater in Guterberg block, and it works as expected. At least for me.

  1. I have no idea.

  2. I need more information to analyze this problem. Can you share the URLs that give 404 errors? Do you use caching plugin?

@wpsumo
Copy link

wpsumo commented Dec 11, 2019

What kind of Guternberg blocks do you mean?

All gutenberg blocks created in Oxygen have duplicated ID's as well if same block is used more than once.

This plugin developed specifically for Oxygen Repeater. I've tested Repeater in Gutenberg block, and it works as expected. At least for me.

For me, once I enable your plugin it breaks all styling and the universal.css file.

I need more information to analyze this problem. Can you share the URLs that give 404 errors? Do you use caching plugin?

Yes using WP Rocket.

@fadlee
Copy link

fadlee commented Dec 11, 2019

@alriksson I have updated the plugin. Can you give another try?

Don't forget to Regenerate Oxygen CSS.

@wpsumo
Copy link

wpsumo commented Dec 11, 2019

@fadlee Thanks, I will let you know as soon as I had time to test the new version.
Does this one include gutenberg ID fix as well or only the bug fix for the repeater?

@fadlee
Copy link

fadlee commented Dec 11, 2019

@alriksson I fixed the CSS issue.

Gutenberg issue is a different problem, its more complicated. I don't think to implement in my simple plugin, that focus on specific repeater issue.

@wpsumo
Copy link

wpsumo commented Dec 12, 2019

I fixed the CSS issue.

You did, it is now working as intended for the <div> but not for child elements in the repeater such as <p>. Could you make support to catch those as well?

Gutenberg issue is a different problem, its more complicated. I don't think to implement in my simple plugin, that focus on specific repeater issue.

Okay, how come the ID issue in gutenberg blocks is more complicated once created with oxygen? Just curious. Maybe something you would like to look into as well? Really a bottleneck in my case and maybe for more people when I use TOC and can not do so with headlines created in Oxygen.

Would be very much appreciated. Maybe more code friendly guys like @wplit would enjoy to contribute 😄

@benhohner
Copy link

@louisreingold Sometimes apps need a major refactor to pave the way forward. ;) What's best for the long term growth of the community? More and more of the WordPress ecosystem is moving toward web standards compliance, and it's actually a boon because native web technologies are now so powerful. Maybe Oxygen can move away from an ID based styling system and toward BEM for v4.0? It would solve all sorts of cascade issues.

@KittenCodes KittenCodes added the confirmed bug Oxygen support staff has replicated and confirmed issue label Jan 1, 2020
@vecchionico
Copy link

@KittenCodes Should we file a separate bug for the Gutenberg Block issue first mentioned by @alriksson or is it ok to deal with both issues in this very report? Just stumbled upon the Gutenberg Block issue myself and found this report while searching if it was already reported by someone else.

@KittenCodes
Copy link
Contributor

@vecchionico There's no need to file a separate report.

@Lupul
Copy link

Lupul commented Feb 23, 2020

This issue also affects repeated js blocks like those created by the gallery element. (Ex: lightbox link works only on the first image)

@wpsumo
Copy link

wpsumo commented Feb 28, 2020

Only me that experience duplicated CSS selectors and styling with @fadlee oxygen fixer?
Anyone that has a similar solution to the Gutenberg duplicated ID issue.

Default ID styling starts to become an issue and a bigger bottleneck. Good luck using the Gutenberg content block with quick links/jump links. Which only works if we use the_content and native clean headlines without oxygen IDs. Which is limiting.

@albertvisuals
Copy link

albertvisuals commented Feb 12, 2021

I have maybe found a workaround if you need to create an unique ID for each Div inside the repeater.
Using a code block and create a DIV manually:
<div id="av-pdf-id<?php echo $file['id'];?>" style="height: 360px; width: 500px;"></div>
This works with the Adobe PDF embed API, which was broken beforehand, so maybe that can be useful for others too.

Could this be changed so that we can simply add a code snippet to the advanced php area of each element and that changes the ID, which is set by oxygen to <post/file id>?

@shoelaced
Copy link

If we're okay with adding a checkbox that would remove the IDs entirely, why aren't we okay with adding a checkbox that would increment them? It seems like there are a lot of situations where a unique ID for each repeater element would be helpful for Javascript.

@brian-grider
Copy link

Not sure if this solution would work, but...

What if we just have an option to not output IDs if an element is inside a Repeater? The option could be a checkbox on the Repeater, and not enabled by default.

@louisreingold I would go one step further and offer this checkbox on every element (and even offer a global option to default the disabling IDs checkbox to checked).

One of my biggest (and really only) frustrations with Oxygen beyond the repeater ID issue, is that I have to either leave a bunch of random IDs for all of the elements in my markup, or add an ID name manually for every single one. In my opinion, this is the one thing that stops the Oxygen code output from being as clean as hand coding. I would prefer to just work with classes except for when a specific element needs to be targeted (as I'm sure many other people would) and this would make Oxygen pretty much perfect in my book as far as code output goes.

@wpsumo
Copy link

wpsumo commented Feb 25, 2021

Just migrate to classes without adding clsses to unviseral, let classes run in specific templates etc.

The duplicate ID issue is not limited to repeater. It's present in the gutenberg blocks as well.

@webenadam
Copy link

Are u kidding? remove those id's!

Is @louisreingold from oxygen's team? because "When we originally built Oxygen we should have skipped IDs entirely and used exclusively classes, but we didn't, and it's too late to do that now." answer is a warning sign to stay out of this plugin if it is. because it means this plugin is stuck! it won't stay up to date. this is so basic. if you don't fix this simple issue how can someone build a series website and count on this plugin to live with it through the years coming?

@wplit
Copy link
Author

wplit commented Feb 28, 2021

this is so basic... if you don't fix this simple issue

i wouldn't be so quick to use words such as basic or simple when describing the problem of finding a way to remove one of the fundamental parts of Oxygen components. It's not just about preventing one attribute from being output on the front end, there's all the issues of the styling & JS using IDs to identify elements for certain functionality etc.

@webenadam
Copy link

webenadam commented Feb 28, 2021

@wplit if you ever followed a page builder's development life cycle, you would know that there is so much to consider and add with time. and yes, treating the id's is a very very basic fundamental feature. i'm not saying it's that easy (btw - it's also not that hard), but i'm saying it's very fundamental and if a developer decides not to treat that right away (this issue is 2 years old, and this problem is older) you can probably guess it's going to be a very very slow development down the road. and it is (you can tell by other issues also, it's not the only one).

Btw - it might be because until now they didn't make any money, and maybe now with the new subscription thing it will give them more time to develop and fix those things and add a lot of other missing basic stuff (right click menu, dragging margins and so...)

@Firsh
Copy link

Firsh commented Apr 13, 2021

until now they didn't make any money

😅 Nah, they do https://www.wpallimport.com too.

@conlumina
Copy link

Please try this, fresh out of the oven: https://conlumina.com/fix-oxygen-repeater-duplicate-ids/

@ayhanmalkoc
Copy link

Please try this, fresh out of the oven: https://conlumina.com/fix-oxygen-repeater-duplicate-ids/

Thanks for this code. But it didn't work for me.

@KittenCodes
Copy link
Contributor

Thanks for this code. But it didn't work for me.

Did you activate the code after importing it to Code Snippets or Advanced Scripts?

@ayhanmalkoc
Copy link

ayhanmalkoc commented Jun 7, 2021

Thanks for this code. But it didn't work for me.

Did you activate the code after importing it to Code Snippets or Advanced Scripts?

Yes, it's loaded with code snippets and it's active. I checked the repeater outputs, the ID is duplicated.

@KittenCodes
Copy link
Contributor

Yes, it's loaded with code snippets and it's active. I checked the repeater outputs, the ID is duplicated.

Do you have any caching on the site that may need clearing?

@sandyhuner
Copy link

@conlumina the script is causing some problems with certain repeaters, see screenshot, please advise.

repeater

@v-marinkov
Copy link

Yes, it's loaded with code snippets and it's active. I checked the repeater outputs, the ID is duplicated.

Do you have any caching on the site that may need clearing?

Yeah, it didn't make a difference for me either:
parent repeater: id="_dynamic_list-57-11"
childred all have: id="div_block-58-11"

@hsnyc
Copy link

hsnyc commented Jun 21, 2021

I recently had the need to select a couple of individual elements inside a repeater when I came across this bug. I have addressed it in the mean time by selecting the link items inside the repeater and changing their ids incrementally.

//Get all elements in Dynamic List (Update id to target your specific list of course)
var ctLinks = document.querySelectorAll('#_dynamic_list-75-1584 .ct-link');

// update IDs
var newId = 'cm-pbm-ct-link_';

for (i = 0; i < ctLinks.length; i++) {
    ctLinks[i].id = newId + i.toString();
}

Then you can target individual elements. In my case I needed to prevent default on a few of the links so I just added those to an array and looped over it:

var pdLinks = ['cm-pbm-ct-link_1', 'cm-pbm-ct-link_4'];

for (let index = 0; index < pdLinks.length; index++) {
	let tlink = document.getElementById(pdLinks[index]);

	tlink.addEventListener('click', (e) => {
		e.preventDefault();
                //etc..
	});
}

Make sure you copy the Id styles to a class on those links.

Sharing in case it helps anyone else out there.

Hope this bug gets resolved in a future release of Oxygen.

@nathanbitcheno
Copy link

Please try this, fresh out of the oven: https://conlumina.com/fix-oxygen-repeater-duplicate-ids/

On my site it looks like this only works on "pages" that have the repeater element. Oxygen templates which also have the repeater element which is still defined by shortcode oxy_dynamic_list_3 don't seem to be affected by this. Any thoughts on how to address this or if I'm overlooking something simple?

@wpsumo
Copy link

wpsumo commented Oct 28, 2021

Would make sense to add an option to disable ID per element and globally. That way it wont break anything for users who swift it on if default would be enabled.

Then more and more users can migrate over to class as default styling. Ideally disable IDs only and secondary settings to switch to classes. meaning #div_repeater_example just becomes .div-repeater-example

@puipaktin
Copy link

puipaktin commented Dec 11, 2021

Can the Oxygen team ADD (not replace) a New Repeater element which solves the duplicate ID problem, while keeping the old one?
I think it's easier and "safer" to implement.
It's better to solve "future problems" and leave the old problems behind rather than doing nothing.

@andresbrx
Copy link

+1

@wpsumo
Copy link

wpsumo commented Dec 21, 2021

It would not lighten the HTML document as well and make it cleaner and easier to read by removing all IDs. It could be an option which is disabled by default for years until the default switch will be on to later on fully switch to no ID.

As we style all elements with classes and don't think anybody leaves display attribute empty, why do we need ID and default styling on those. Users who want to style elements individually and not add classes can manually add IDs or keep the legacy item on. If users don't select a class or add a class a generated oxy-class will be generated on the item once they start using the UI to style and no class is assigned.

@wpsumo
Copy link

wpsumo commented Jan 4, 2022

Same issue in Gutenberg blocks. If using the same block on the same page we have duplicate IDs. So technically the same issue and root cause.

@Spellhammer This issue is also present in gutenberg blocks. A second opinion, keeping all IDs and adding them to data attributes is good for backwards compatibility but still clutter down for users who don't use IDs on repeaters, which I guess is many due to the duplicated issue. Maybe an option for us to use only class styling. Per repeater element? And another globally. ID should only be needed once a user add an ID just like classes today. Time to move away from auto generated IDs? :) And please consider removing the wrapper div on the repeater element once you anyhow are in the repeater code.

You always say ACF repeaters is complex to handle. How come, it's fairly simple using repeaters directly in php. Is it complex how oxygen is built and within oxygen not necessary to build a repeater based UI?

@Spellhammer
Copy link
Collaborator

Fixed in Oxygen 4.0.

@wpsumo
Copy link

wpsumo commented Jun 2, 2022

@Spellhammer Will you fix the issue in gutenberg blocks as well? As once you add same gutenberg block on the same page you have the same issue. Is really moving them to data-attribute the solution, we still create a lot of IDs in Oxygen not needed for styling if they are needed for styling we should add custom IDs just as we do with classes.

Remove oxygen auto generated IDs on elements.

@0x7466
Copy link

0x7466 commented Oct 11, 2022

This issue is still present when you use a repeater in a repeater.

@wplit
Copy link
Author

wplit commented Oct 12, 2022

This issue is still present when you use a repeater in a repeater.

Just tested it, not seeing any duplicate IDs. Are you doing something custom perhaps that could be causing it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working confirmed bug Oxygen support staff has replicated and confirmed issue
Projects
No open projects
Bug Reports
  
Confirmed / Replicated
Development

No branches or pull requests