Panopoly Magic (only panopoly module we still have) adds the ability to preview content prior to adding it to a panel pane which is a very nice feature. However, with th ecurrent layout if there are a lot of elements (fields, content panes, etc.) in a particular category then the add content modal window becomes difficult to use because each item takes too much space. It would be nice to come up with a method whereby the user sees more of the content links and still has the ability to see a preview.

Panopoly Magic Enabled

Magic

Panopoly Magic Disabled

Magic

Prototype

Magic

Still @todo to get this committed!

  1. Some more visual clean-ups. @sylus's styles look better in his screenshot than they do on a vanilla Panopoly site - I assume the extra styling is from his theme. Here's a screenshot of what this looks like on vanilla Panopoly:
  2. Make this the default on new sites. This is so cool, and generally more useful than the previous default, that I think this should be the default on new sites. This means adding a hook_update_N() to set the old default on sites that saved the 'panopoly_magic_pane_add_preview' variable, and changing the value of PANOPOLY_ADD_PREVIEW_DEFAULT.
  3. Adding Behat tests for the functionality. The panopoly_magic code is sooo complicated, I don't want to add any new functionality without having tests for it. Otherwise we're just going to break it later!
  4. Patch needs in-depth code review.
  5. I've quickly read through the patch, but not given it the full treatment yet!

  6. Preview shows "No Preview. Select a pane to show its preview." for panes that give an empty preview and "Add" button is missing! For example, if you select the "Inline differences" block (from the Diff module), it doesn't have the context to render a preview so it's empty. However, it's still instructing the user to select a pane even though one has been selected!
  7. Use the term "Widget" rather than "pane". Panopoly has standardized on the term "widget" as opposed to "pane" in user visible strings.
  8. Pane titles that contain HTML show the HTML tags rather than rendering. Something is probably being run through check_plain() when it should be filter_xss_admin() for pane titles:
  9. Fix accessibility issues! Several were identified in #53, #54 and #55
CommentFileSizeAuthor
#61 interdiff.txt2.95 KBdsnopek
#61 panopoly_magic-show-single-preview-21553377-61.patch16.31 KBdsnopek
#60 interdiff.txt345 bytesdsnopek
#60 panopoly_magic-show-single-preview-21553377-60.patch15.93 KBdsnopek
#56 interdiff.txt4.15 KBdsnopek
#56 panopoly_magic-show-single-preview-21553377-56.patch15.95 KBdsnopek
#52 panopoly_magic-show-single-preview-21553377-45.patch13.25 KBdsnopek
#51 panopoly_test-show-single-preview-2155377-51.patch11.87 KBdsnopek
#50 panopoly_test-show-single-preview-2155377-50.patch11.97 KBdsnopek
#46 panopoly-single-preview-buttons.png37.25 KBmglaman
#43 panopoly_magic-show-single-preview-21553377-43.patch17.65 KBdsnopek
#43 interdiff.txt2.52 KBdsnopek
#39 Selection_083.png53.96 KBdsnopek
#39 panopoly_magic-show-single-preview-21553377-39.patch13.14 KBdsnopek
#39 interdiff.txt1.73 KBdsnopek
#39 panopoly_magic-show-single-preview-21553377-36.patch12.26 KBdsnopek
#36 interdiff.txt1.75 KBdsnopek
#36 panopoly_magic-show-single-preview-21553377-36.patch12.26 KBdsnopek
#35 panopoly_test-show-single-preview-2155377-35.patch11.97 KBdsnopek
#32 panopoly_test-show-single-preview-2155377-32.patch11.98 KBdsnopek
#32 interdiff.txt1.35 KBdsnopek
#32 panopoly_magic-show-single-preview-21553377-32.patch11.6 KBdsnopek
#31 interdiff.txt3.79 KBdsnopek
#31 panopoly_magic-show-single-preview-21553377-31.patch10.82 KBdsnopek
#30 interdiff.txt7.98 KBdsnopek
#30 panopoly_magic-show-single-preview-21553377-30.patch10.77 KBdsnopek
#21 Selection_016.png59.02 KBdsnopek
#21 Selection_015.png13.82 KBdsnopek
#20 Selection_014.png88.57 KBdsnopek
#18 only_show_one_widget-2155377-18.patch10.49 KBsylus
#17 only_show_one_widget-2155377-17.patch24.91 KBsylus
#15 panels_magic_preview.png94.19 KBsylus
#12 panopoly_magic-single-preview-2155377-12.patch8.58 KBmpotter
#1 Prototype.png62.98 KBsylus
#1 After.png57.59 KBsylus
#1 Before.png86.14 KBsylus
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sylus’s picture

FileSize
86.14 KB
57.59 KB
62.98 KB
sylus’s picture

Issue summary: View changes
sylus’s picture

Issue summary: View changes
sylus’s picture

Issue summary: View changes
sylus’s picture

Version: 7.x-1.0 » 7.x-1.x-dev
Status: Active » Needs work
sylus’s picture

Project: Web Experience Toolkit (7.x) » Panopoly
Component: Code » Magic
Category: Feature request » Bug report

I am going to move this to panopoly so we can all work on solving this UX issue.

I am also going to upgrade this to a bug because will no doubt be a problem for larger sites.

lsolesen’s picture

Status: Needs work » Active

Moving to active, as there is no patch.

lsolesen’s picture

Category: Bug report » Feature request

And this is not a bug, but a feature request.

sylus’s picture

I disagree that this is not a bug.

A feature request implies there is nothing currently breaking whereas this design does represent a break in the default UX of panels.

Once you reach a certain threshold of content / panes it becomes impossible to use the new design to find and work with panes properly.

Does a break in UX not represent a bug?

lsolesen’s picture

Category: Feature request » Bug report

@sylus If it is because you cannot scroll in the ctools modal to see the other panes, it is a bug. After doing a little more investigation, it seems at least in Chrome on a Mac that the scrollbar is not visible. I just never noticed because I use the scroll wheel on my mouse. So I think this should be investigated as a bug. Sorry.

joel_osc’s picture

I would argue that the best solution from a UX perspective is to not have the scrollbar at all. I really like the concept of this feature, but for every site that I have worked on I have needed to disable this module because once you have more than 10 view panes the add content modal become unusable. Just my 2 cents. Cheers.

mpotter’s picture

We really wanted to improve this for Open Atrium also, so I got started on a patch for this.

It adds a new option to the "Enable previews when adding panes" setting called "Single". It shows a single preview on the Add Content form. Clicking each pane will update the preview. There is an Add button next to each pane so you can add it without needing to see the preview first.

What this patch needs now is the css changes to make it look nicer. But I think it handles the backend functionality. If I get more time I'll submit a css update. We might need to add a class somewhere so this can be themed properly.

mpotter’s picture

Status: Active » Needs work
dsnopek’s picture

Title: Preview widgets are too large when adding content in panels » Only show one widget preview at a time when adding content in panels

Updated issue title to better reflect the current proposal and patch.

sylus’s picture

FileSize
94.19 KB

Thanks for the awesome patch I did some custom css and modify the above patch to wrap the add button and title inside a div to ease css.

Attached a picture of how it currently looks for us. If looks satisfactory can commit the patch.

dsnopek’s picture

@stylus: Can you share the patch? Even if it's not finished, it'd be great to have it here so that others can carry on your work later. Thanks! :-)

sylus’s picture

Apologies for the whitespace correction as my IDE automatically corrects this.

This patch includes the CSS additions, as well fixes for watchdog issues such as undefined index 'preview' etc.

sylus’s picture

FileSize
10.49 KB

Attaching a cleaner patch without most white space corrections.

cboyden’s picture

Issue tags: +Accessibility

This is also an accessibility issue: If you're using the keyboard to add panes, you'll have to tab through every link that's rendered in each of the previews in order to select something that's further along in the list.

dsnopek’s picture

Issue summary: View changes
FileSize
88.57 KB

Thanks again to @mpotter and @sylus! I just manually tested this patch and it works beautifully. :-) I've just started the automated tests on Travis-CI: https://travis-ci.org/dsnopek/panopoly/builds/51424011

However, there's a couple things that we need to do before we can commit:

  1. Some more visual clean-ups. @sylus's styles look better in his screenshot than they do on a vanilla Panopoly site - I assume the extra styling is from his theme. Here's a screenshot of what this looks like on vanilla Panopoly:
  2. Make this the default on new sites. This is so cool, and generally more useful than the previous default, that I think this should be the default on new sites. This means adding a hook_update_N() to set the old default on sites that saved the 'panopoly_magic_pane_add_preview' variable, and changing the value of PANOPOLY_ADD_PREVIEW_DEFAULT.
  3. Adding Behat tests for the functionality. The panopoly_magic code is sooo complicated, I don't want to add any new functionality without having tests for it. Otherwise we're just going to break it later!
  4. Patch needs in-depth code review.
  5. I've quickly read through the patch, but not given it the full treatment yet!

dsnopek’s picture

Issue summary: View changes
FileSize
13.82 KB
59.02 KB

Er, and a found a couple more small bugs in manual testing:

  • Preview shows "No Preview. Select a pane to show its preview." for panes that give an empty preview and "Add" button is missing! For example, if you select the "Inline differences" block (from the Diff module), it doesn't have the context to render a preview so it's empty. However, it's still instructing the user to select a pane even though one has been selected!
  • Use the term "Widget" rather than "pane". Panopoly has standardized on the term "widget" as opposed to "pane" in user visible strings.
  • Pane titles that contain HTML show the HTML tags rather than rendering. Something is probably being run through check_plain() when it should be filter_xss_admin() for pane titles:
dsnopek’s picture

Oops, accidentally hid the latest patch!

dsnopek’s picture

Below is my in-depth code review! Sorry for so many nitpicks. You guys have done great work so far, thanks so much!

I'm just a little hard on changes to panopoly_magic because it's simultaneously some of the ugliest code and most important code in Panopoly. I want each addition to help de-uglify as much as possible within the scope of its changes. :-)

  1. +++ b/css/panopoly-modal.css
    @@ -550,3 +550,55 @@
    +  color: #666 !important;
    

    Having !important in a module is usually a bad sign and points to something that should probably really be done in the theme.

    Removing this doesn't change the visual appearance for me at all.

  2. +++ b/css/panopoly-modal.css
    @@ -550,3 +550,55 @@
    +  color: #000 !important;
    

    Same for this one.

  3. +++ b/panopoly_magic.module
    @@ -1229,31 +1231,68 @@ function panopoly_magic_preprocess_panels_ipe_toolbar(&$vars) {
    +      foreach ($vars['categories'][$vars['category']]['content'] as $key => $plugin) {
    +        if ($plugin['subtype_name'] == $sub_type) {
    +          $vars['preview_single_title'] = $key;
    +          break;
    +        }
    +      }
    

    We should probably check both $type_name and $sub_type: $sub_type isn't guaranteed to be globally unique, only unique within its type.

  4. +++ b/panopoly_magic.module
    @@ -1229,31 +1231,68 @@ function panopoly_magic_preprocess_panels_ipe_toolbar(&$vars) {
    +        // Convert the link to generate a preview of itself.
    +        $options = array(
    +          'query' => array(
    +            'type_name' => $plugin['type_name'],
    +            'subtype_name' => $plugin['subtype_name'],
    +          ),
    +          'attributes' => array('class' => array('use-ajax button'))
    +        );
    +        $plugin['title'] = l($plugin['title'], current_path(), $options);
    

    This is probably where check_plain() is getting run on the pane title.

    Likely fix: $options['html'] should be set to TRUE, and the title should be run through filter_xss_admin().

  5. +++ b/panopoly_magic.module
    @@ -1296,6 +1336,25 @@ function panopoly_magic_process_panels_add_content_modal(&$vars) {
    +        '<fieldset class="widget-preview"><legend><div class="widget-preview-title">' .
    +        (!empty($vars['preview_single']) ?
    +          theme('panels_add_content_link', array(
    +            'renderer' => $vars['renderer'],
    +            'region' => $vars['region'],
    +            'content_type' => $content[$title]
    +          )) : '') .
    +        '</div><span class="fieldset-legend">' . $vars['preview_single_title'] . '</span></legend><div class="fieldset-wrapper">' .
    

    We do almost this same exact lump of code three times. Here, and ...

  6. +++ b/panopoly_magic.module
    @@ -1305,8 +1364,31 @@ function panopoly_magic_process_panels_add_content_modal(&$vars) {
    +        $vars['columns'][$which] .=
    +          '<div class="content-wrapper">' .
    +          theme('panels_add_content_link', array(
    +            'renderer' => $vars['renderer'],
    +            'region' => $vars['region'],
    +            'content_type' => $content[$title]
    +          )) .
    +          '<span class="fieldset-legend">' . $legend_title . '</span>' .
    

    ... and here, and ...

  7. +++ b/panopoly_magic.module
    @@ -1305,8 +1364,31 @@ function panopoly_magic_process_panels_add_content_modal(&$vars) {
    +        $vars['columns'][$which] .=
    +          '<fieldset class="widget-preview"><legend><div class="widget-preview-title">' .
    +          theme('panels_add_content_link', array(
    +            'renderer' => $vars['renderer'],
    +            'region' => $vars['region'],
    +            'content_type' => $content[$title]
    +          )) .
    +          '</div><span class="fieldset-legend">' . $legend_title . '</span></legend><div class="fieldset-wrapper">' .
    +          (!empty($content[$title]['preview']) ? $content[$title]['preview'] : t('No Preview')) .
    +          '</div></fieldset>';
    

    ... and here. Would it be possible to refactor this out into a function?

    That would also probably make it easier to understand from reading the code what it's doing, and what makes each of these sections a little different (since it'd be represented in function arguments).

    If it's not possible (I haven't actually tried), well, I guess we have to live with it, but I'd rather not. :-)

  8. +++ b/panopoly_magic.module
    @@ -1296,6 +1336,25 @@ function panopoly_magic_process_panels_add_content_modal(&$vars) {
    +        (!empty($vars['preview_single']) ? $vars['preview_single'] : t('No Preview.  Select a pane to show its preview.')) .
    

    Strings in t() should use a single space to separate sentences (there's two spaces here).

Thanks again!

EDIT: Fixed a bunch of type-o's!

dsnopek’s picture

Issue tags: +sprint
dsnopek’s picture

Issue tags: +Needs tests

Running on Travis-CI again: https://travis-ci.org/panopoly/panopoly/builds/53462520

This one still needs new Behat tests to test the new functionality!

Andrew_Mallis’s picture

Excited to see these improvements.

Please excuse these observations if they distract us from the functional task at hand.

It would be really, really awesome if instead of a live preview, we could optionally load an image.
I could not find a hook for this.
Probably its own request, but thought I would mention it in the context of the broader UX considerations being brought to bear.

Widgets out of context can look really strange sometimes.
Many widgets require a context to render an data at all.

A preview, to achieve its goal of helping a site admin select the appropriate widget would be well served by such an option.

dsnopek’s picture

@Andrew_Mallis: That's a great idea! And it shouldn't be too hard to implement - I'd imagine having a special key on the CTools content type plugin which points to the image. Then we could have panopoly_magic check if the content type it's rendering the preview has that key set, and show the image instead. However, this should definitely be a new issue! This issue already has a clear path to completion and these changes don't really depend on each other - they could be worked on simultaneously.

dsnopek’s picture

Assigned: Unassigned » dsnopek

Going to do some work on this!

dsnopek’s picture

FileSize
10.77 KB
7.98 KB

Here's a new patch that fixes all the code review that I did on #23. The hardest bit to get right was the refactoring for #23.5, #23.6 and #23.7, but the code is way easier to read and understand now!

I also fixed the bugs identified in #21.

But I still need to address the functional review in #20, so leaving this as "Needs work"

dsnopek’s picture

FileSize
10.82 KB
3.79 KB

Found some more opportunities for clean-up and refactoring. The codes even simpler now!

dsnopek’s picture

Here's a patch for panopoly_test that adds tests for all the different 'Add content' preview modes. It required a small change to panopoly_magic to make it easier to test the manual previews. I'm going to run this on Travis in a moment.

EDIT: Here's the Travis build: https://travis-ci.org/panopoly/panopoly/builds/61644652

dsnopek’s picture

Unassigning for now. I may come back to this soon-ish, though!

#20.1 and #20.2 still need to be done. If the tests pass on Travis, then that's all that needs to be done to finish this!

dsnopek’s picture

Assigned: dsnopek » Unassigned

Er, helps to actually unassign when I'm saying that I'm unassigning. :-)

dsnopek’s picture

Assigned: Unassigned » dsnopek
FileSize
11.97 KB

Tests didn't pass on Travis. :-( Here's a new patch that attempts to fix them!

EDIT: Here's the new Travis build: https://travis-ci.org/panopoly/panopoly/builds/61763936

dsnopek’s picture

Assigned: dsnopek » Unassigned
Status: Needs work » Needs review
FileSize
12.26 KB
1.75 KB

Here's a new patch that fixes TODO #2 and #5 in the issue summary. I tried to make some visual clean-ups, but the CSS for this is a mess. In a lot of ways these aren't new visual issues, but pre-existing ones so it might make sense to address them in a new issue? But I'll try and take one more stab at it before deferring until later.

EDIT: Another Travis build for this patch (including upgrade tests): https://travis-ci.org/panopoly/panopoly/builds/61767759

dsnopek’s picture

Issue summary: View changes

Updating TODO's in the issue summary.

dsnopek’s picture

Category: Bug report » Feature request
Issue tags: -Needs tests

For my own personal organization, I'm marking this as a "Feature request". This is no way affects my commitment to finishing this and getting it merged into Panopoly!

FYI, the last test build on Travis-CI succeeded so the only thing remaining now is the visual fixes. :-)

dsnopek’s picture

Alright! Here is my attempt to fix the visual styling issues that I laid out in #20.1 (or point #1 in the issue summary). I also fixed a logic issue in the hook_update_N() function: I didn't take into account that one of the valid values was 0 (for manual previews).

And here is a screenshot after the visual clean-ups:

At this point, all this really needs is lots and lots of review! I'd really appreciate any code reviews or manual testing. :-)

dsnopek’s picture

Issue summary: View changes

Updating issue summary.

segovia94’s picture

Status: Needs review » Needs work

Functionally it all works well. Great job! There are just a couple code cleanup items.

  1. +++ b/css/panopoly-modal.css
    @@ -551,3 +556,57 @@
    +  top: 50%; margin-top: -10px;
    

    margin-top should be on its own line

  2. +++ b/panopoly_magic.module
    @@ -1389,11 +1440,55 @@ function panopoly_magic_preprocess_panels_add_content_modal(&$vars) {
    +    $content_type['description'] = $title;
    

    The $title is not set anywhere

Also, the ajax throbbers don't look very good. I don't think that should block this though since a lot of the issues are already present in past releases. We should have another issue to go in and clean up the appearance of throbbers.

One possible help visually would be to add a background color on the preview link row when hovering over it so that the "Add" button isn't just floating off on the side unrelated to anything. Maybe something like:

#modal-content .content-wrapper:hover {
    background-color: #f1f1f1;
}

I didn't have a chance to run the test patch yet. I'll try to get to that later tonight.

segovia94’s picture

The tests passed for me. There is one TODO sitting in a function that I'm not sure if it got missed or not.

+++ b/plugins/content_types/panopoly_test_simple.inc
@@ -0,0 +1,33 @@
+function panopoly_test_simple_content_type_edit_form_submit($form, &$form_state) {
+  // TODO
+}

Does something need to go here?

dsnopek’s picture

Status: Needs work » Needs review
FileSize
2.52 KB
17.65 KB

@segovia94: Thanks for the review! Sorry it took me a bit to get to it. :-)

#41.1: Fixed.

#41.2: Great catch! That was actually a mistake I made in refactoring that bit of code, and missing $title would have broken a the fix that was originally there for views.

Also, the ajax throbbers don't look very good. I don't think that should block this though since a lot of the issues are already present in past releases. We should have another issue to go in and clean up the appearance of throbbers.

Sure! Can you create a new issue describing exactly what doesn't look good about throbbers now and how they should work? Thanks!

One possible help visually would be to add a background color on the preview link row when hovering over it so that the "Add" button isn't just floating off on the side unrelated to anything.

Yeah, good idea, that does look better! I've added your suggestion and adjusted the padding a little bit.

#42: That TODO is more a placeholder for future tests. Eventually, we'll probably need to flesh out the configuration form and submit function. I'm fine leaving this as is, since it's support code for the tests anyway.

Leaving open for a bit more to see if I can't get some more review!

mglaman’s picture

+++ b/panopoly_magic.module
@@ -1041,6 +1028,15 @@ function panopoly_magic_form_ctools_entity_field_content_type_formatter_options_
+  // Ensure form id, form token, and form build id are on the root of the form.
+  // Image fields in particular have issues with this.
+  $form['form_id'] = $form['general_settings']['form_id'];
+  $form['form_token'] = $form['general_settings']['form_token'];
+  $form['form_build_id'] = $form['general_settings']['form_build_id'];
+  unset($form['general_settings']['form_id']);
+  unset($form['general_settings']['form_token']);
+  unset($form['general_settings']['form_build_id']);
+

Didn't we remove this stuff in another Panopoly Magic test? Can't recall which, was committed during sprint.

dsnopek’s picture

FileSize
13.25 KB

@mglaman: Ah, yeah, you're right! I forgot to merge 7.x-1.x back into my branch for this issue, so I generated a bad patch. Here's a better one!

mglaman’s picture

Ok, so I'm at 90% to RTBC. I think the patch needs review against changes to Panopoly Magic from the sprint that happened in LA (see #44.)

I was also a bit confused by the buttons - but loved the way it worked (HUGE IMPROVEMENT.)

The Add isn't a button, but the title is, and clicking the button pushes it to the preview. I'd probably se this option as our default for our product, but customers would probably be confused (even more) on how to work this.

mglaman’s picture

Status: Needs review » Reviewed & tested by the community

:) I applied the patch in #45 and I love it. I don't know what happened in #46 for me.

sylus’s picture

Just chiming in here that tested this out and is working great for us!

dsnopek’s picture

Did some testing with this in the Radix theme and found some problems, because Radix copied a bunch of code from panopoly_magic. :-/ Of course, we can patch Radix to take newer copies of the code, I create a follow-up issue for a better solution: #2496027: Generate preview markup in a theme function/template

Anyway, I don't want the Radix issues to hold this up! We can fix that afterward...

dsnopek’s picture

FileSize
11.97 KB

I re-rolled the test patch after #2371247: Upgrade to Behat 3 / Drupal Extension 3 was merged!

The tests pass locally, but I'm going to run them on Travis in a moment.

EDIT: Here's the Travis build: https://travis-ci.org/panopoly/panopoly/builds/64564699

dsnopek’s picture

FileSize
11.87 KB

Blergh! The last re-roll was bad. Here's a new panopoly_test patch that will hopefully pass!

EDIT: Here's the build on Travis: https://travis-ci.org/panopoly/panopoly/builds/64588915

dsnopek’s picture

Hrm. Test re-roll looking good now, but the patch for #45 seems to have disappeared from the main issue summary? Re-uploading it and hiding the patch from #43. Now THIS should pass on Travis. :-)

EDIT: Build on Travis: https://travis-ci.org/panopoly/panopoly/builds/64605673

cboyden’s picture

Status: Reviewed & tested by the community » Needs work

This patch needs some accessibility work. The "Add" buttons that you can use to add a widget without clicking to see the preview first are only available on mouseover. They should also appear on focus.

I'm going to apply the Panels and CTools keyboard accessibility patches to see if they resolve one of the other problems with keyboard focus.

cboyden’s picture

After applying the patches from #2280853: Keyboard trap when using ctools modal and #2390803: Keyboard focus should be on the first element in the selected category tab in the "Add content" model (accessibility problem), there are still some focus issues.

To reproduce:

  1. Apply the above 2 patches and the panopoly_magic patch for this issue.
  2. On a demo page such as Lovely Vegetables, tab to and activate the Customize this page button.
  3. Tab to and activate an Add Content button.
  4. Tab to and activate the Page Content category link.
  5. Tab to Attached Files (first item in the list).
  6. Note that the "Add" link does not appear (as mentioned in previous comment).
  7. Tab forward once.
  8. Note that keyboard focus indicator disappears - this is an invisible tab stop.
  9. Tab forward once more.
  10. Note that you are now focused on the next item (Field body)
  11. Hit Enter to activate the link.
  12. Note the Preview area updates to show the Body field preview.
  13. Note also your keyboard focus indicator has disappeared.
  14. Tab forward once.
  15. Note you are focused on Attached files (first option in the category) again.
  16. Tab forward to get to the green Add button.
  17. Note there is an invisible tab stop after each item.

The behavior I'd expect is:

  • Tabbing to one of the items reveals the Add button for that item, same as mouseover. The inline Add button should remain visible while focus is on either itself or its related preview link.
  • Tabbing away from an item in the list (forward from the inline Add button, or backward from the link) hides the inline Add button.
  • After activating the link to show the preview, keyboard focus moves to the inline Add button for that item.
  • There are no invisible tab stops in the list - non-visible Add buttons should be display:none or removed from the DOM.
cboyden’s picture

Also, the Add buttons are before the links that reveal them in the source code. Ideally the source code order should match the visible order, so the Add buttons would go after the preview links.

dsnopek’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
15.95 KB
4.15 KB

@cboyden: Thanks for the accessibility review! Here is a patch that should fix the issues identified in #53, #54 and #55.

However, I think tabbing through and getting both the preview link and "quick add" buttons is kinda awkward, because you're always going over two links that do sort of the same thing. What would you think of just hidding the "quick add" buttons from the tab order entirely? Since the focus will be on the "Add" button once the preview loads, its just a matter of pressing enter twice to add something, rather than having to tab twice as many times when trying to find the widget you want.

Please let me know what you think!

dsnopek’s picture

Oh, and I forgot to mention that my last patch doesn't do this from #54:

Tabbing to one of the items reveals the Add button for that item, same as mouseover. The inline Add button should remain visible while focus is on either itself or its related preview link.

This is because we're hiding/showing these buttons purely via CSS and I couldn't come up with a good way to do it without Javascript. However, I'm hoping you'll be OK with just removing the "quick add" buttons from the tab order anyway. :-)

mpotter’s picture

Testing this in the latest -dev of Panopoly. Seems to be working well for me.

My only minor complaint is that when you click an item in the list of widgets to update the preview, the ajax spinner image is shown on the next line, rather than to the right of the widget name. Just makes it a bit ugly since the items below it shift down to show the spinner, then shift back up when the preview is done loading. Probably just needs some simple css for inline-block or something.

cboyden’s picture

In the latest patch, when you activate the widget link that shows the preview, you are focused on the green Add button that's on top of the preview. The problem is that the focus highlight on the button isn't distinguishable. Since the button appears all at once already focused, there's no contrast to its unfocused state. And since it isn't styled like a regular button or link, you don't have any previous knowledge to draw on to tell that it's focused.

dsnopek’s picture

FileSize
15.93 KB
345 bytes

@mpotter: Thanks for testing!

My only minor complaint is that when you click an item in the list of widgets to update the preview, the ajax spinner image is shown on the next line, rather than to the right of the widget name.

Attached is a patch that should fix this! Please let me know what you think.

@cboyden wrote:

In the latest patch, when you activate the widget link that shows the preview, you are focused on the green Add button that's on top of the preview. The problem is that the focus highlight on the button isn't distinguishable.

How far would we have to go with the style so that it'd be acceptable? (ex. more color contrast? added border?)

This is a variation on your original suggestion to focus on the inline "Add" button in #54, but (a) I'd like to make those not tabbable/focusable because it seems awkward to me to tab twice as many times to find the item you want (could be a lot of items!) and (b) targeting that item is kinda tricky since it's Panels that's rebuilding the dialog (although, I do have a couple of "interesting" ideas about how it could be done :-)).

Anyway, please ping me on IRC when you can jam about this so we can discuss the options!

dsnopek’s picture

Alright, here is a patch that is much closer to @cboyden's suggestions in #54! It does the following:

  1. Tabbing to the preview link shows the inline "Add" button for that item
  2. Tabbing away from the preview link hides the inline "Add" button
  3. After activating the link to show the preview, keyboard focus moves to the inline "Add" button for that item

However, for the record, I still don't like all tabbing from the inline "Add" buttons! Will hopefully jam with @cboyden about this soon on IRC...

  • dsnopek committed d4b45d9 on 7.x-1.x
    Update Panopoly Magic and Test for Issue #2155377 by dsnopek, sylus,...
dsnopek’s picture

Status: Needs review » Fixed

Alright! Jammed with @cboyden about this, and decided that the patch in #61 is acceptable from an accessibility standpoint, but that we'd need to do some more work to come up with a way to remove all the extra tabbing. So, we're deferring that to a follow-up issue:

#2499269: Improve usability of "Add content" dialog from the keyboard

I also just got the CTools and Panels keyboard accessibility patches in:

#2499273: Include Panels and CTools patches for improved keyboard accessibility

So, it's time to finally commit this!

Thanks to everyone who helped write, test, review or come up with ideas for this! This is a huge win for Panopoly on several levels. :-)

Here is the final test build on Travis-CI for this commit: https://travis-ci.org/panopoly/panopoly/builds/65161123

dsnopek’s picture

FYI, the tests are failing because of #2499595: Build failing because host of jquery.imgareaselect (odyniec.net) has gone offline, not because of anything wrong with this commit! I'll post a comment here with the new Travis build with the fix once there is one.

dsnopek’s picture

This Travis build should work and includes the changes from this issue:

https://travis-ci.org/panopoly/panopoly/builds/65259007

Status: Fixed » Closed (fixed)

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