widget id's are not unique #80

Closed
gregory-hk opened this Issue Jul 5, 2012 · 10 comments

Comments

Projects
None yet
2 participants

hello Chip.

I updated to the theme as it stands today.

you've probably seen the issue that I'm seeing. the view/hide actions on the sidebar widgets don't work because not all of the widgets are getting unique id values. try it on your own site. you'll see the same problem. I just tested it.

I've looked at the problem for a while, but haven't found the cause yet. mt_rand() does indeed create a new random value each time it's called to register a widget, but duplicates are appearing in the final html page nonetheless, and the function that encloses mt_rand() gets called a lot of times every time a page is loaded (at least 9 times in my case) which doesn't seem to be a very efficient thing for WP to do. (I placed an echo '*#*' . $showhide_id; command into the oenology_showhide_widget_content_open() function to monitor its activity.)

if you need help reproducing or testing this, please let me know.

I'll keep looking into it for a short while longer just in case I find something.

cheers,
Gregory

observation #1.

if two widgets are moved into the same widget area, they are assigned the same widget-id value.

move one of the widgets into a different widget area and it'll be assigned its own unique widget-id value.

I'm not very smart so this is hard to tackle for me, but...

the after_title property is only declared with respect to the sidebars and never individually for each widget; i.e., an after_title property declared for a sidebar will be shared by all widgets within that sidebar. does that sound right?

I guess you're trying to find a way to apply the view/hide switch to every widget in any of the sidebars, hence adding the code to 'after_title' and using mt_rand().

in wp-includes/widgets.php::class WP_Widget::display_callback(), there's a filter hook just before the widget is drawn:
$instance = apply_filters('widget_display_callback', $instance, $this, $args);

I wonder if this is something you could use, perhaps applying a placeholder for $showhide_id in the 'after_title' showhide code within register_sidebar(), and then replacing the placeholder with a random unique id through this hook/filter each time the widget is drawn.

got to go. my wife is waiting...

cheers,
Gregory

back again...

just a thought... from a cache point of view, it would be better to not use a random id, especially one that is regenerated every time the widget is drawn.

I'm going to have a look at the filter hook and see if there's something in there that could be used to build a unique but consistent id.

cheers,
Gregory

nope. the 'widget_display_callback' filter won't work. it could have except that developers aren't making the most of it. i.e., widget( $args, $instance ) specifies a widget class's registered arguments in $args, including the after_title argument, and specifies arguments specific to the widget being handled in $instance; i.e., the properties within $instance should override the same properties within $args. unfortunately, the WP developers (and yourself) have not allowed for that ability, looking instead for specific properties within $instance and nothing else. so that filter hook won't work in this case.

for reference, my code looked like this:


function oenology_showhide_widget_content_open() {
    $showhide_id = '%showhide_id%'; // Gregory's edit.

    $showhide = '<span class="showhide">';
    $showhide .= 'Click to <span style="color:#5588aa;" onclick="document.getElementById(\'' . $showhide_id . '\').style.display=\'inline\';">view</span> / <span style="color:#5588aa;" onclick="document.getElementById(\'' . $showhide_id . '\').style.display=\'none\';">hide</span>';
    $showhide .= '</span>';
    $showhide .= '<br /><br />';
    $showhide .= '<div id="' . $showhide_id . '" style="display:none;">';

    return $showhide;
}

function gregory_widget_temp($instance, $widget, $args) {
    $instance['after_title'] = str_replace( '%showhide_id%', 'widget-'.$widget->id, $args['after_title'] );
    return $instance;
}
add_filter('widget_display_callback','gregory_widget_temp',11,3);

there are two other methods that we might try including (1) JQuery and (2) a different JavaScript command.

more later.
cheers.

Chip, something for you to try. I'm not sure how well the js will translate to other browsers:

function oenology_showhide_widget_content_open() {
    $showhide = '<span class="showhide">';
    $showhide .= 'Click to <span style="color:#5588aa;" onclick="this.parentElement.nextElementSibling.style.display=\'inline\';">view</span> / ';
    $showhide .= '<span style="color:#5588aa;" onclick="this.parentElement.nextElementSibling.style.display=\'none\';">hide</span>';
    $showhide .= '<br /><br /></span>';
    $showhide .= '<div style="display:none;">';

    return $showhide;
}

the <br /> elements are now within the <span> element so that the view/hide commands can refer to the widget div by parent/sibling association.

cheers.

and yet another version. this one toggles 'view' and 'hide'.

function oenology_showhide_widget_content_open() {
    $showhide = '<span class="showhide">';
    $showhide .= 'Click to ';
    $showhide .= '<span style="color:#5588aa;" onclick="if(this.textContent==\'view\'){this.parentElement.nextElementSibling.style.display=\'inline\';this.textContent=\'hide\';}';
    $showhide .= 'else{this.parentElement.nextElementSibling.style.display=\'none\';this.textContent=\'view\';}">view</span>';
    $showhide .= '<br /></span>'; // Gregory's note: I prefer just one line break.
    $showhide .= '<div style="display:none;">';

    return $showhide;
}

my lazy testing shows that it works in Safari, Firefox and Chrome on my Mac running Lion.

I was using innerText but Firefox doesn't support it reliably. textContent works well.

I have no idea if IE will support this js.

cheers,
Gregory

Owner

chipbennett commented Aug 30, 2012

Hey Gregory, thanks for all the detailed investigation into this. I'm going to test your suggested script changes. I'd love to get rid of the random-number generation altogether.

hello Chip.

I wanted to be able to specify which widgets were expanded on loading, and which were collapsed, but my script didn't allow that, so I had to change it. It's much closer now to what you had before.

I also checked the Oenology and WP code very carefully to see if it might be possible to include unique IDs in the widget view/hide code for each widget, but it doesn't appear to be. before_title and after_title are sidebar-specific properties, given to each and every widget loaded within the sidebar.

Here's my code.

function oenology_showhide_widget_content_open() {
    $showhide = '<span class="showhide">';
    $showhide .= 'Click to ';
    $showhide .= '<span style="color:#5588aa;" onclick="d=this.parentElement.nextElementSibling; d.style.display==\'none\' ? d.style.display=\'block\' : d.style.display=\'none\';">view/hide</span>';
    $showhide .= '<br /></span>'; // I prefer just one line break.
    $showhide .= '<div>';

    return $showhide;
}

cheers,
Gregory

Owner

chipbennett commented Nov 26, 2012

Looks like this one works just fine!

chipbennett added a commit that referenced this issue Nov 26, 2012

@chipbennett chipbennett reopened this Nov 26, 2012

Owner

chipbennett commented Nov 26, 2012

Oops; a regression. DIVs are no longer display:none by default now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment