Drupal version: Latest 8.x HEAD
OS: Windows 7
Apache: 2.2.25 (Win32)
PHP: 5.4.25

Steps to reproduce

  1. Go to Appearance - Bartik - Settings. Color scheme Blue Lagoon (default) is selected.
  2. The controls to change UI colors are duplicated - see screenshot in #2
  3. Making changes in the top (normal) set of controls has no effect. After the [Save Configuration] button is clicked, the message says "The configuration options have been saved", but nothing has been changed, go back to the front page and the color scheme is still Blue Lagoon.
  4. Changes made in the second set of controls do stick however!

This worked a couple of weeks ago so something has changed recently.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marcingy’s picture

So this is strange. The issue is that there are infact too select dropdowns, the second one sticks and the former actually seems to be related to locking colours. So I think this is more a case where we need documentation to explain what the first field set is for vs the second.

StuartJNCC’s picture

FileSize
65.24 KB

I see what you mean - I didn't spot that! So all the controls are now duplicated:

Appearance - Bartik - Settings

I don't think this is intended. I think it is a bug! - and maybe should be higher priority than "normal" since it seems to be quite a significant regression?

StuartJNCC’s picture

Title: Cannot change UI colors » Appearance - settings controls are duplicated
Issue summary: View changes
sun’s picture

Can you try attached patch to see what causes the form to get invoked twice?

marcingy’s picture

The function in question is only called once. The 2 form elements are not the same.

The top item is built by $form['scheme']

js' => array(
        array(
          'data' => array(
            'color' => array(
              'reference' => color_get_palette($theme, TRUE),
              'schemes' => $schemes,
            ),
            'gradients' => $info['gradients'],
          ),
          'type' => 'setting',
        ),
      ),

The lower is

// Add palette fields.
  $palette = color_get_palette($theme);

So one is provide settings for a schema, while the other is too set the actual values for the theme. Yes it is totally confusing but I think the real issue is one needs better help text for the end user.

Jeff Burnz’s picture

Mistake in the template?

<div class="color-form clearfix">
  {{ form.scheme }}
  <div id="palette" class="clearfix">
    {{ form.palette }}
  </div>

  {{ form }} <---------------------   whats this doing here?

  <h2>{{ 'Preview'|t }}</h2>
  {{ html_preview }}
</div>
alexpott’s picture

Priority: Normal » Major
Status: Active » Needs review
FileSize
1.13 KB
660 bytes
1.13 KB
660 bytes

I opened #2257321: Color form is duplicated without searching - bad me. This is a major bug - we could not release with it.

Same patch from that issue.

markhalliwell’s picture

Attaching related issues which caused this regression. I'm tempted to say that this |without filter changed how the render arrays work. This patch shouldn't be necessary because the palette is already printed prior to the rest of the form.

edit: after chatting with @alexpott in IRC, I was unaware that this is, in fact, the desired behavior and fundamental change with how render arrays work. The patch is correct.

alexpott’s picture

marcingy’s picture

Status: Needs work » Reviewed & tested by the community

Looks good, I learned something new about twigg which is also good!

The last submitted patch, 7: 2221041.7.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: 2221041.7.will-fail.patch, failed testing.

marcingy’s picture

7: 2221041.7.patch queued for re-testing.

marcingy’s picture

Status: Needs work » Reviewed & tested by the community
markhalliwell’s picture

FileSize
122.53 KB
92.29 KB

RTBC from me.

Before:

After:

Tests also pass locally (was unsure with the testbot random failures):

Test only patch:

84.73 KB - /sandbox/sites/d8.sandbox (*2221041)
markcarver@Marks-MacBook-Air$ sudo -u _www php ./core/scripts/run-tests.sh --url http://d8.sandbox --class "Drupal\color\Tests\ColorTest"
Password:

Drupal test run
---------------

Tests to be run:
  - Drupal\color\Tests\ColorTest

Test run started:
  Saturday, May 3, 2014 - 15:46

Test summary
------------

Drupal\color\Tests\ColorTest                                 118 passes   2 fails

Test run duration: 26 sec

Full patch:

84.73 KB - /sandbox/sites/d8.sandbox (*2221041)
markcarver@Marks-MacBook-Air$ sudo -u _www php ./core/scripts/run-tests.sh --url http://d8.sandbox --class "Drupal\color\Tests\ColorTest"

Drupal test run
---------------

Tests to be run:
  - Drupal\color\Tests\ColorTest

Test run started:
  Saturday, May 3, 2014 - 15:38

Test summary
------------

Drupal\color\Tests\ColorTest                                 120 passes

Test run duration: 30 sec
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Very nice catch.

Committed and pushed to 8.x. Thanks!

  • Commit 96597bb on 8.x by webchick:
    Issue #2221041 by alexpott, sun | StuartJNCC: Color palette setting...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.