Followup to #1667742: Add abstracted dialog to core (resolves accessibility bug). We need to convert the Views modal to the new abstracted dialog. This will help resolve #1806308: Review Views JavaScript + generic modals for accessibility once the updated, accessible version of jQuery UI is released.

CommentFileSizeAuthor
#108 interdiff.txt1.01 KBnod_
#108 core-views-js-modal-1851414-107.patch55.43 KBnod_
#107 Screenshot of the views dialog squeezed into the center of the screen by displacing elements on each edge of the viewport55.07 KBjessebeach
#107 Screenshot of the dialog placement value presented as center--128px85.3 KBjessebeach
#107 Screenshot of the modal dialog located beneath the Toolbar tray and bar. It is broken.64.11 KBjessebeach
#105 core-views-js-modal-1851414-105.patch55.35 KBnod_
#104 core-views-js-modal-1851414-104.patch48.95 KBnod_
#97 fields.png72.9 KBdawehner
#94 Screen Shot 2013-10-08 at 01.55.09.png263.01 KByannickoo
#93 Screen Shot 2013-10-08 at 01.51.10.png266.92 KByannickoo
#93 Screen Shot 2013-10-08 at 01.51.19.png233.07 KByannickoo
#93 Screen Shot 2013-10-08 at 01.51.36.png220.96 KByannickoo
#91 core-views-js-modal-1851414-91.patch48.98 KBnod_
#91 interdiff.txt1.09 KBnod_
#89 interdiff.txt1014 bytesnod_
#89 core-views-js-modal-1851414-89.patch49.06 KBnod_
#88 core-views-js-modal-1851414-88.patch48.28 KBnod_
#88 interdiff.txt2.95 KBnod_
#85 core-views-js-modal-1851414-85.patch45.84 KBnod_
#85 interdiff.txt1.67 KBnod_
#84 interdiff.txt2.49 KBnod_
#84 core-views-js-modal-1851414-84.patch45.25 KBnod_
#81 core-views-js-modal-1851414-81.patch43.76 KBnod_
#81 interdiff.txt9.71 KBnod_
#78 core-views-js-modal-1851414-78.patch41.98 KBnod_
#78 interdiff.txt13.33 KBnod_
#77 core-views-js-modal-1851414-77.patch31.17 KBnod_
#77 interdiff.txt1.09 KBnod_
#74 core-views-js-modal-18511414-74.patch30.94 KBfrega
#74 interdiff-1851414-72-74.txt894 bytesfrega
#72 core-views-js-modal-18511414-72.patch31 KBfrega
#72 interdiff-69-72.txt3.21 KBfrega
#70 Screen Shot 2013-09-27 at 19.53.25.png212.31 KByannickoo
#69 core-views-js-modal-1851414-69.patch29.18 KBfrega
#67 core-views-js-modal-1851414-67.patch32.96 KBnod_
#67 interdiff.txt3.33 KBnod_
#65 Screen Shot 2013-09-12 at 9.24.20 AM.png162.64 KBtim.plunkett
#63 1851414-63.patch30.62 KBdamiankloip
#63 interdiff-1851414-63.txt737 bytesdamiankloip
#61 core-views-js-modal-1851414-61.patch29.9 KBnod_
#59 core-views-js-modal-1851414-59.patch24.18 KBnod_
#52 core-views-js-modal-1851414-52.patch51 KBnod_
#42 no-scroll.png15.1 KBdawehner
#42 scroll.png15.37 KBdawehner
#42 selected.png12.69 KBdawehner
#42 end.png13.81 KBdawehner
#38 interdiff.txt23.46 KBquicksketch
#38 views_convert_modals-1851414-38.patch53.52 KBquicksketch
#37 views_convert_modals-1851414-37.patch38.52 KBquicksketch
#37 views-dialog.png266.96 KBquicksketch
#30 interdiff.txt28.86 KBquicksketch
#19 views_convert_modals-1851414-19.patch19.11 KBquicksketch
#8 foo.patch8.31 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

Issue tags: +Accessibility, +VDC
xjm’s picture

Issue tags: +JavaScript
dawehner’s picture

Assigned: Unassigned » dawehner

Working on it.

dawehner’s picture

Assigned: dawehner » larowlan

It seems to be that larowlan is already working on that: http://drupal.org/node/1667742#comment-6700926

dawehner’s picture

So i think the way to implement is to introduce two new ajax commands:

dialogOpen($content)/dialogSet($content) and dialogClose() so custom ajax commands can control what's happening with the dialog.
Views needs this to implement it's custom logic when the dialog should be closed, and when you should just get another form.

effulgentsia’s picture

That might make sense, and I don't object to it if Views needs it. I removed it from the scope of #1667742: Add abstracted dialog to core (resolves accessibility bug), because it was not needed for confirmation forms, and I'm suspicious of adding APIs without use cases.

However, I also suspect that Views might be doing too much logic server-side. The evolution of Drupal's Ajax system was primarily driven by PHP programmers who dabble in JavaScript. There's nothing wrong with that (Drupal gets written by the people who show up), but our front-end developer community has grown and is continuing to grow, and there might be benefit to rethinking some of the separation of concerns. Whether we want dedicated Ajax commands for controlling dialog open/close or whether we want JavaScript making that determination based on higher-level data returned by the server is an example of that. I don't currently know what the best approach is for Views, and ultimately, it should get decided by the people who write and review the patch.

dawehner’s picture

Assigned: larowlan » Unassigned

It feels like it would make at least the code easier to read, as just imagine you want to add a new Form to the stack, and you would have to get this information from PHP into JS.

If we go with the ajax commands we certainly should get views converted converted first to new ajax command objects, see #1843224: Convert Views Ajax commands to new Ajax API, but we could work on the generic ajax commands in the meantime (doing now).

dawehner’s picture

FileSize
8.31 KB

This is the current work which adds a new ajax commando to open/close modals

webchick’s picture

Status: Active » Needs review

Since there's a patch, marking needs review.

dawehner’s picture

Status: Needs review » Needs work

Yeah general review would be cool, but it's certainly not in the state of being strict-reviewable.

nod_’s picture

Commands should be in dialog.ajax.js, dialog.js should only have the basic code to make dialog work, see #1863478: Split Dialog API and optional integration helpers into separate files. Actually, happy to dup the other one and include the change here.

dawehner’s picture

Frega wanted to work on getting ajax commandos for the dialog though it seems to be that he didn't had time recently,
let's see.

I think we should keep the other issue to make at least some progress.

dawehner’s picture

webchick’s picture

Any progress on this? We added this dialog so that Views could use it and this critical bug has been open awhile now.

dawehner’s picture

Status: Needs work » Postponed

Progress is certainly happening on the other issue, so let's postpone this issue first.
It's funny but sadly the dialog API was changed during the issue to not support the slightly more complicated usecase of views :(
This is the reason why we need the other issue first.

webchick’s picture

So #1909144: Allow #ajax['dialog'] to contain options other than 'modal' was committed awhile ago.. I don't suppose that's enough to unblock this?

dawehner’s picture

Sadly not really, see #1870764: Add an ajax command which makes it easy to use the dialog API in complex cases. We not even need other options, but also proper ajax commands.

jibran’s picture

Issue tags: +modal dialog

Tagging

quicksketch’s picture

Status: Postponed » Needs review
FileSize
19.11 KB

This patch does a direct conversion of the current system based on #1870764-102: Add an ajax command which makes it easy to use the dialog API in complex cases. It should be a 100% conversion. It has a few nice improvements:

- Dialogs now reuse the existing commands for open/close. Previous commands for SetViewsForm and DismissForm removed.
- Dialogs are now draggable, since we're using the native jQuery UI header instead of making our own.
- Dialogs *could* be resizable, but since we auto-resize already I'm not sure if that's necessary.
- Lots of cruft code eliminated. Between this patch and the generic modal system, I think we're looking at a net reduction of code just through this abstraction. Of course other places in core will use modals too, so this individual reduction isn't too important.

Every situation I've tested works completely, but there are still a lot of areas for improvement:

- We could move the auto-resizing logic to /core/misc/dialog.js. Seems too useful to be part of Views only.
- We could move the theming of the dialog to system.base.css and system.theme.css. I like the Views-style theming of modals more than the default jQuery UI modals and it'd be nice if they looked the same everywhere.
- We could gut Views' custom form handling and use the approach being researched in #1944472: Add generic content handler for returning dialogs: utilize the router system to eliminate use of nojs/ajax in the paths and set up separate handlers for different versions of the page. (This will be a large undertaking, it'd probably be better to separate this problem).
- There's probably still more cruft code sitting around that no longer does anything that we need to remove. I tried to get all of it, but the Views architecture is so spread out it's hard to be sure that I got it all.

Status: Needs review » Needs work

The last submitted patch, views_convert_modals-1851414-19.patch, failed testing.

nod_’s picture

Resize shouldn't be in dialog.js, playing around with it it it seems that the whole views code can be replaced by

Drupal.viewsUi.resizeDialog = function ($dialog, noShrink) {

  "use strict";
  var $ = jQuery;
  if (!$dialog.length || $dialog.css('display') === 'none') {
    return;
  }
  
  var windowWidth = $(window).width();
  var maxWidth = parseInt(windowWidth * 0.85, 10);
  var minWidth = parseInt(windowWidth * 0.6, 10);

  $dialog.dialog('option', {
    maxWidth: maxWidth,
    minWidth: minWidth,
    position: { my: "center", at: "center", of: window }
  });
};

and it seems to be working just the same.

quicksketch’s picture

Resize shouldn't be in dialog.js

Why not? It seems like a deficiency of jQuery UI that it doesn't have a built-in ability to keep the dialog centered while resizing the window. While I know you want to keep the dialog.js file "pure" as an HTML5 shim, I'm positive that any browser-native version of a dialog would have the ability to adjust when the window was resized (though exactly *how* it would adjust we can't tell, since there aren't any implementations yet).

playing around with it it it seems that the whole views code can be replaced by

That's not too surprising. I couldn't figure out why in the world that code was so complicated. It may be left over from the conversion to jQuery UI, where the old dialog system didn't have built-in calculations for "center". We should add a setTimeout() counter to prevent repeated calculations on scroll/resize too. While resizing you can easily end up with hundreds of calls in just a few seconds.

nod_’s picture

We have a debounce function to limit the maddness with resize events.

As for the resizing and centering, the browsers will only use CSS to do that. if you have a #dialogsomething {position:absolute;top:20%;left:20%;bottom:20%;right:20%;} that should work fine in browsers.

Since jquery can't use CSS to do the sizing and all we're stuck with using their API to do it. and that's starting to be tying things down to jQuery UI more than i'm comfortable with.

One could argue that if we make the dialog resizable and draggable It doesn't make sense to keep the centering and resizing on window resize too.

Dave Reid’s picture

The re-sizing code seems good for re-use if the percentage of height and width, in addition to orientation were parameters rather than hard-coded since that seems to hard-coded to the Views UI specific use case.

quicksketch’s picture

Issue tags: -VDC

The re-sizing code seems good for re-use if the percentage of height and width, in addition to orientation were parameters rather than hard-coded since that seems to hard-coded to the Views UI specific use case.

Yeah I agree. We could simply re-set the all the size and position settings on resize, just like #21 only with the user-provided values. Of course you'd want to be able to toggle this setting as it might not be appropriate for all situations. I think it would make sense to enable it by default, and have a disable toggle though by overloading the existing $options array (which currently is for jQuery UI options only).

We have a debounce function to limit the maddness with resize events.

Cool, I hadn't seen Drupal.debounce() before. It looks like toolbar library depends on drupal.debounce already, so there's a very high chance it'll be loaded on the page for administrators. Unfortunately since we'll be using dialogs in non-admin situations (i.e. WYSIWYG dialogs), this means an additional HTTP request when the dialog is first opened. Kind of unfortunate, but may be mitigated by other libraries adding debounce.js as a requirement at page-load time.

quicksketch’s picture

Issue tags: +VDC

.

quicksketch’s picture

We could simply re-set the all the size and position settings on resize, just like #21 only with the user-provided values.

In practice, this doesn't work so well. The first problem is that jQuery UI Dialog doesn't support height in percentages, so you can't have a dialog that is "75%" height. Additionally, even if we convert these to pixels on resize, we end up cutting off the buttons at the bottom of the dialog when the window gets too small. The previous implementation always kept the buttons visible. #21's simplification has the same problems. jQuery UI has a dedicated "buttons" wrapper to prevent this kind of problem, but moving our buttons into that wrapper and still having our AJAX behaviors work is going to be tricky business.

nod_’s picture

but moving our buttons into that wrapper and still having our AJAX behaviors work is going to be tricky business.

not that bad actually I've done it for ctools modal #1397370-6: Make modal.js use jQuery dialog (look for // Transform submit in dialog buttons. in the patch) it's ugly but it works. Ideally we'd rewrite the JS ajax API so that it's more flexible and not tied to a DOM element so it's less workaround-ish.

Yes for the height issue it's more complicated than #21, but it won't be more code than currently.

quicksketch’s picture

(look for // Transform submit in dialog buttons. in the patch) it's ugly but it works.

Yeah I was thinking along the same lines as the only possible solution: hide the button in the original place and have the second button click the first one... Seems like a pretty good trick. I like that your version actually just reuses the existing labels too, that solves any translation problems. The theming for jQuery UI dialogs needs some work, the default CSS for their dialogs have slightly larger/rounder buttons than we use elsewhere in the Seven theme. However since this needs to affect the front-end too... we may want a dedicated stylesheet for dialog theming.

quicksketch’s picture

FileSize
28.86 KB

I have another revision to this issue that is looking pretty good. Unfortunately all this moving files around requires some additional planning when making patches, which I didn't anticipate, so rolling a nice clean patch is will take me a bit. Here's an interdiff that includes most of the changes that will go on top of #19.

Changes:
- New /core/misc/dialog.theme.css file for general presentation CSS for dialogs.
- Removed almost all CSS from Seven and Views regarding dialogs. There's still a bit more to trim out.
- Images for dialogs (close.png, progress-small.gif) have been moved to /misc.
- Moved jquery.ui.dialog.patch.js into dialog.js, with a JS setting toggle to turn it off in the event that D8 ships with jQuery UI 1.9 and jQuery Update wants to undo the "patch". It doesn't seem like it would do any harm running on jQuery UI 1.10, but for cleanliness there's a way to turn it off. Because it fixes a general problem with dialogs, dialog.js is an appropriate place for it.
- Moved the resizing code into dialog.js. It works great but is kind of clunky (100x better than the previous version though). Again, because this is a general improvement to dialogs, it makes sense in dialog.js.
- Buttons are now placed in the jQuery UI dialog button area, which fixes issues with the buttons being hidden. I used a trick similar to _nod's suggestion in #28, but the new way we're handling AJAX makes this problem a bit easier than it had been in D7/Ctools. This code lives in jquery.ajax.js, since it's Drupal-implementation specific and for the most part only needed on AJAX-loaded forms.

A bug I noticed: The interface for filtering AND/OR groups is broken. There's some additional JS that needs to be updated in there.

I'll try to post a real reroll that can be applied directly and moves the files around properly tomorrow. I'm coming up on a busy week so I may not get back to this immediately. I think the new home for images/CSS/JS makes sense and is inline with our current patterns (though obviously adding 4 more files to /misc isn't ideal). Feedback about the locations of files and CSS and general review that can be done at this point would be appreciated.

quicksketch’s picture

Oh, and regarding the jankiness of button presentation (previously we had a nice blue "primary button" in the dialog, now they're both gray), that problem already has its own issue in #1848292: Consolidate Seven button styles.

nod_’s picture

Assigned: Unassigned » quicksketch

For the resize i'd use the dialog:* events instead of putting that in directly, that's why we have those events.

I wouldn't be selecting <button> elements to add them to the dialog buttons since #1719640: Use 'button' element instead of empty links.

i got issues applying the patch over #19 So I guess I'll wait for your reroll to try it out. I like where it's going though.

dawehner’s picture

effulgentsia’s picture

jQuery UI Dialog doesn't support height in percentages, so you can't have a dialog that is "75%" height.

Is that true even when you apply it in CSS? As in:

.views-ui-dialog {
  height: 75%;
}

The dialog is after all a DOM element, so I don't get what jQuery UI does to remove CSS features from it.

quicksketch’s picture

Is that true even when you apply it in CSS? As in:

Hmm, well I suppose that may be true. If you set a height of "auto", jQuery UI dialogs won't set a specific height property at all, so you could control it with CSS. For some reason jQuery UI Dialogs discard percentage height values but pass through percentage width values

However one problem with CSS-only approach is that the dialog will *always* be that particular height. So a simple dialog that only needs 100px of height ends up displaying 75% of the height, resulting in a huge empty part of the dialog. So even if it's possible using CSS, we'll probably want to use a JS-based solution anyway. The current patch functionality really works pretty well, sorry I haven't rolled it properly so that it can be easily tested. I hope to get to it tonight.

quicksketch’s picture

However one problem with CSS-only approach is that the dialog will *always* be that particular height. So a simple dialog that only needs 100px of height ends up displaying 75% of the height, resulting in a huge empty part of the dialog.

I'm incorrect. Of course you could use height: auto; max-height: 75%;. That accommodates for everything. I'll try rerolling this without the current height component and do it all with CSS.

quicksketch’s picture

Status: Needs work » Needs review
FileSize
266.96 KB
38.52 KB

Here's a patch that is pretty much a straight reroll from #30, just combining the two patches (and the file renames) into a single patch that should be testable.

I experimented around with the resizing options quite a bit but I don't think a CSS-only solution is going to work in this situation (I hope I'm wrong). I struggled for quite a while trying to figure out how to do this, but this Views example is really much more complicated that it seems. The trouble is that we have areas inside the dialog that are not definable either in static pixel heights nor percentages. The top and bottom of the dialog show expand as needed to show all the text without scrolling, but the area in the middle needs to scroll the remaining space.

As far as a generic resizing solution goes, I think this is generally solvable through CSS-only after we update to jQuery UI 1.10. Our current version seems to have a bug in it that prevents the use of position: fixed; on the dialog.

I removed jquery.ui.dialog.patch.js, since it will be irrelevant after #1175830: Update to jQuery UI 1.10.2 is committed.

Here's an image depicting the problem with the Views dialog. I'm thinking we might be able to solve it through a custom position callback... but I'm really not sure at this point what the best way to handle it is.

quicksketch’s picture

This version moves the resizing code back to views-admin.js (but still trims it down to utilize jQuery UI and the new dialog API). Now that I'm certain we can do an always-centered modal dialog with CSS only (after updating to jQuery 1.10), the Views-dialog becomes a special-case rather than a universal problem. I didn't really like that resizing code anyway.

I removed a dozen "use strict" and jQuery assignments in this version, instead just wrapping the whole file in a jQuery/strict wrapper, as is typical of most JS files in Drupal, hence the bigger patch, but it's all more removals so that's probably a good thing.

nod_’s picture

Haven't tested yet, but it looks great :)

Nice to have the resizing out of the general dialog and ajax code.

Status: Needs review » Needs work

The last submitted patch, views_convert_modals-1851414-38.patch, failed testing.

xjm’s picture

dawehner’s picture

FileSize
13.81 KB
12.69 KB
15.37 KB
15.1 KB

Thanks for working on this issue. Tested it manually and spotted some issues:

  • If you start with a modal and you don't scroll at all, the window size seems wrong, see no-scroll.png
  • Then you scroll with your mouse, and the first resize happens, see scroll.png
  • One thing you see is some flickering compared to the old modal.
  • If you add a new field and scroll down to the end of the listing, you see "selected::", see selected.png
  • At the end of the page, you also see some white background, see end.png
quicksketch’s picture

Thanks @dawehner. I can also tack on several other problems:

- The "Selected" item is supposed to populate with the checkboxes you've selected, but it does not.
- The buttons are supposed to update their labels based on affecting the master vs. overridden displays.

I fiddled around with the jerkiness issue a little bit but haven't solved it yet. Part of the reason for the jerkiness is that this version uses the debounce.js file to add a 25ms delay. The previous version was firing on every scroll/resize event, which could be as frequent as 1-5ms. So by effect things will be slightly more jerky but not nearly so CPU consuming.

mgifford’s picture

Status: Needs work » Needs review
Issue tags: -JavaScript, -Accessibility, -modal dialog, -VDC

Status: Needs review » Needs work
Issue tags: +JavaScript, +Accessibility, +modal dialog, +VDC

The last submitted patch, views_convert_modals-1851414-38.patch, failed testing.

quicksketch’s picture

@nod_ and others interested in dialogs: I've incorporated a large chunk of this patch's code into #1879120: Use Drupal-specific image and link plugins — use core dialogs rather than CKEditor dialogs, containing alterable Drupal forms, including the CSS cleanup, the porting of Views-style loading throbbers to the dialog CSS file, and the auto-centering code. If that patch lands first, this issue can be rerolled with a patch that is quite a bit smaller.

jessebeach’s picture

The following was incorrectly posted to #1879120-65: Use Drupal-specific image and link plugins — use core dialogs rather than CKEditor dialogs, containing alterable Drupal forms.

Using core dialogs means we will need to alter the buttons to use dropbuttons if we're going to solve #1831894: Users miss "save" button and can't distinguish "editable" and "preview" areas by hiding the "apply to this display/apply to all displays" by moving the choice to the confirmation.

quicksketch’s picture

My response to #47 (which was also in the other issue):

Hmm, this could be a significant problem. jQuery UI has some strange ways of providing buttons. We don't have full control over the buttons within a dialog because they have to be provided as configuration to the $.dialog() method. See this documentation on setting the buttons option. Using the jQuery UI buttons is necessary for properly handling resizing the dialog and accessibility (I think). So in short, making the dialogs use drop buttons is going to be difficult.

So the thing here is that we're going to turn the Save button into a drop button? Are there mocks of which buttons will consolidate into a drop button? I'm not quite following what the suggested change is, but as I said in the other issue, it'd be preferable if we could avoid dropbuttons in the dialog because of technical challenges associated with it.

larowlan’s picture

Can we put the save button in the dialog as a normal form api field?
Tabbing still works.

quicksketch’s picture

Maybe in the case of Views we could build a special exception, but in general using the built-in button area of jQuery UI has advantages because it is outside of the .dialog-content class, which jQuery UI will automatically adjust for you if room is not available. However since Views needs special height-handling anyway (see the picture in #37) we could specifically not use the jQuery UI button area. However as a general practice, I think we should try to use the button area of jQuery UI because it reduces the special handling we have to do in our own code.

dawehner’s picture

nod_’s picture

Status: Needs work » Needs review
FileSize
51 KB

reroll of #38, just to get something not too broken if someone feels like testing. The whole "use strict" thing shouldn't be included in this patch. That's the job of #1839130: Refactor modules/views_ui/js/views-admin.js.

Status: Needs review » Needs work

The last submitted patch, core-views-js-modal-1851414-52.patch, failed testing.

falcon03’s picture

I just want to remark how critical this issue is. I talked about it with some members of the Views team at Drupalcon Portland and we agreed that this issue is blocking a full review of Views UI accessibility, which is something that we should definitely do before releasing D8.

quicksketch’s picture

@falcon03: Could you elaborate on how accessibility is affected by this issue? Views is already using jQuery UI's dialog, it's just not using our abstracted API for opening and displaying that dialog. Most of the changes that would happen here wouldn't affect the front-end display in any significant way. I'd like to make the buttons use the official jQuery UI area for buttons rather than leaving them in the content area of the dialog, which might have some impact, but other than moving the buttons I don't think any change is expected on the front-end.

xjm’s picture

@quicksketch, the current Views modal is not keyboard-navigable.

quicksketch’s picture

@quicksketch, the current Views modal is not keyboard-navigable.

It looks like the dialog is completely keyboard-navigable, but when trying to save a field/filter/sort by using the spacebar on the "Apply" button, it just keeps reloading the same form in the dialog. That problem probably would be fixed by our abstraction here, but could easily be fixed separately.

tim.plunkett’s picture

Assigned: quicksketch » Unassigned

The other day @quicksketch told me he was swamped recently, let's unassign this in case someone else wanted to work on it.

nod_’s picture

Status: Needs work » Needs review
FileSize
24.18 KB

Reroll. It's still pretty broken on the user side of things but at least it applies.

Status: Needs review » Needs work

The last submitted patch, core-views-js-modal-1851414-59.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review
FileSize
29.9 KB

Reroll, some more refinments, took out most (all?) of views positioning code. It's still really painful to use because of #2067263: Drupal dialog placement must take displacing viewport elements, like the Toolbar, into account when calculating placement but that's another issue.

Status: Needs review » Needs work

The last submitted patch, core-views-js-modal-1851414-61.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
737 bytes
30.62 KB

Good job, thanks for doing this!

This should fix the failing test, then hopefully we have a passing patch to work with :)

jibran’s picture

Issue tags: +Needs screenshots

Nice clean up. We need screen shots

tim.plunkett’s picture

(There are pre-existing bugs with the modal that prevent you from scrolling properly, so don't bother trying to add fields/filters)

When adding a new Header handler, the checkboxes no longer update the "Selected" section at the bottom:
Screen Shot 2013-09-12 at 9.24.20 AM.png

And obviously, a lot of the styling is broken. The CSS will need updates.

dawehner’s picture

I guess most of the bugs in https://drupal.org/node/1851414#comment-7218644 are still active.

nod_’s picture

Still a WIP, hoping someone else would put the finishing touches.

Few improvments, fixed the select issue, the scrolling thing by putting #2067263-9: Drupal dialog placement must take displacing viewport elements, like the Toolbar, into account when calculating placement in this patch and some styling things.

joelpittet’s picture

tagging

frega’s picture

Rerolled patch, functionality confirmed. Switching of button labels ("Apply (all pages)", "Apply (only this page/override)) is not yet implemented; needs styling.

yannickoo’s picture

Here is a screenshot so that you know what's missing.

Screen Shot 2013-09-27 at 19.53.25.png

nod_’s picture

On of the problems we're going to have with those modals is that views expects a header area outside thescollable one and taht's not what jquery ui gives us by default. it might be possible to fix it with a few lines of CSS but most likely not.

I'm referring to the 'for display' header thing and the 'selected' thing at the bottom when adding a new field.

frega’s picture

Continued working on the patch.

- Partially fixed: updating 'Selected:' properties in "Add Field" work again; but i did not moving the 'selected' stuff to a separate pane; I would argue that this should *not* be moved into the "buttons pane" (like it is in views-7.x) but that the "form" at the top at the list should be sticky and the selected stuff should be there).
- Fixed: updating 'button panes' - added an custom event triggered on $dialog-element (dialogButtonsChange) that keeps buttonpane buttons and (hidden) bu
- Fixed: Drupal.behaviors.addItemForm: the .is check did not cater for e.g.

being prepended to the form.

I think, I figured out why the "submit" button sometimes does not work on some views; if the page contains another "edit-submit" that has been ajaxified, this prevents the "Apply" button in the Dialog from being ajaxified (STR: 1. Place the "Search Block" in first sidebar and 2. Click on "Title" 3. Click "Apply" 4. Loads the JSON response in the full page). This could be a huge problem for dialogs in general (/me shakes fist at ajax.js).

nod_’s picture

 var $form = $context.is('form[id^="views-ui-add-item-form"]') ?
+      $context.once('views-ui-add-item-form') :
+      // In some case this can be wrapped or contain other elements.
+      $context.find('form[id^="views-ui-add-item-form"]').once('views-ui-add-item-form');

I'd rather have an if/else here to fill in $form then do the $form.once() after the condition, that would avoid having 2 .once() call that are the same in the source.

Nice one on the dialogButtonsChange.

The duplicate submit might be a big problem. 2 elements with same ids…

frega’s picture

Adjusted as per #73. Rerolled patch attached.

frega’s picture

As a follow-up. Thanks to @jbeach's additional sleuthing we found a way to fix the duplicate submit button issue. Not including this in the patch, because this might impact too many other bits. Should we open another issue for this?

diff --git a/core/modules/views_ui/js/views-admin.js b/core/modules/views_ui/js/views-admin.js
index b157c43..379200b 100644
--- a/core/modules/views_ui/js/views-admin.js
+++ b/core/modules/views_ui/js/views-admin.js
@@ -869,7 +869,7 @@ Drupal.behaviors.viewsUiOverrideSelect = {
     $(context).find('#edit-override-dropdown').once('views-ui-override-button-text', function () {
       // Closures! :(
       var $context = $(context);
-      var $submit = $context.find('#edit-submit');
+      var $submit = $context.find('[id^=edit-submit]');
       var old_value = $submit.val();
 
       $submit.once('views-ui-override-button-text')
diff --git a/core/modules/views_ui/lib/Drupal/views_ui/ViewUI.php b/core/modules/views_ui/lib/Drupal/views_ui/ViewUI.php
index 5df0656..f9240f2 100644
--- a/core/modules/views_ui/lib/Drupal/views_ui/ViewUI.php
+++ b/core/modules/views_ui/lib/Drupal/views_ui/ViewUI.php
@@ -290,6 +290,7 @@ public function getStandardButtons(&$form, &$form_state, $form_id, $name = NULL)
       $form['actions']['submit'] = array(
         '#type' => 'submit',
         '#value' => $name,
+        '#id' => 'edit-submit-' . drupal_html_id($form_id),
         // The regular submit handler ($form_id . '_submit') does not apply if
         // we're updating the default display. It does apply if we're updating
         // the current display. Since we have no way of knowing at this point
nod_’s picture

Component: views.module » views_ui.module

If that works, let's try it in this one.

nod_’s picture

changed a little the ternary operator and added #75 to the patch to see what testbot has to say.

nod_’s picture

Made the strange views fixed elements inside modals work. not an especially pretty code but looks like it's working.

Once the style have been polished a little it'll be done. Closing in on RTBC :)

yannickoo’s picture

Status: Needs review » Needs work
+++ b/core/modules/views_ui/js/dialog.views.js
@@ -1,10 +1,30 @@
+          ¶

Whitespace?

nod_’s picture

Status: Needs work » Needs review

let's not hold further review on a technicality.

you're right. it's what I get for making a patch on windows.

it's not Rtbc material yet. need more review.

nod_’s picture

Looks pretty good now.

I had to take out the animation from the collapsible pseudo-polyfill we have. Otherwise I had issues with resizing the dialog properly when details elements were opened/closed inside the modal.

From what I could click around everything looked good and worked on FF and Chrome.

(edit) 30 files changed, 158 insertions(+), 513 deletions(-) :)

Status: Needs review » Needs work
Issue tags: -JavaScript, -Needs issue summary update, -Accessibility, -modal dialog, -VDC

The last submitted patch, core-views-js-modal-1851414-81.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review
Issue tags: +JavaScript, +Needs issue summary update, +Accessibility, +modal dialog, +VDC
nod_’s picture

Some more cleanup of collapsible.js

nod_’s picture

Some margin adjustments to make it look more like before.

dawehner’s picture

Issue tags: +Novice, +Needs screenshots

It would be great if someone could write some issue summary with screenshots and co.

nod_’s picture

For those testing, the opening of the modal is pretty slow right? it's because of our #states script. Try #1589176: Follow-up: Use data-* to store #states api informations for a very nice speed-up :)

nod_’s picture

Had to reintroduce the views_ui_build_form_url change to views_ui_build_form_path because there were issue when adding more than one field. The forms wouldn't work as expected. Not sure why but renaming everything as before fixes the issue. probably just the $form['url'] that needs to be changed to $form['path'] but anyway.

Looks like the search field doesn't work in modals.

nod_’s picture

Sorry for patch spam but every time I think I'm done I find something new…

Here is the patch fixing the search.

Jelle_S’s picture

  1. +++ b/core/misc/collapse.js
    @@ -91,59 +81,27 @@ $.extend(CollapsibleDetails.prototype, {
    +    var $summaryPrefix = this.$node.find('> summary span.details-summary-prefix');
    

    Shouldn't we use .children('summary').find('span.details-summary-prefix')? see #1931632: [META] Make core compatible with jQuery native-API selector

  2. +++ b/core/modules/views_ui/css/views_ui.admin.theme.css
    @@ -902,16 +891,10 @@ ul#views-display-menu-tabs li.add ul.action-list li{
    +/*
     .views-ui-dialog .form-buttons {
       background-color: #f3f4ee;
       padding: 8px 13px;
    @@ -920,7 +903,7 @@ ul#views-display-menu-tabs li.add ul.action-list li{
    
    @@ -920,7 +903,7 @@ ul#views-display-menu-tabs li.add ul.action-list li{
       margin-bottom: 0;
       margin-right: 0;
     }
    -
    +*/
    

    Why is this commented out, shouldn't it just be removed then?

  3. +++ b/core/modules/views_ui/js/ajax.js
    @@ -113,20 +77,9 @@
    +      }*/
    

    Again commented out code

Thats all I could find from a visual review (meaning I haven't tested the actual patch). It's mainly small things so I'm not sure if this should be set to NW for these remarks...

nod_’s picture

1. The selector is a legit CSS selector, no need to use jQuery for that.

2. fixed.

3. fixed.

cosmicdreams’s picture

Status: Needs review » Reviewed & tested by the community

While I don't understand the removal of the CSS everything else looks good.

yannickoo’s picture

Oops. Just tested the patch from #91 and got some crazy positioning issues because of the Toolbar module. I attached some screenshots where you can see that the toolbar overlays the dialog and in one case it works fine.

Screen Shot 2013-10-08 at 01.51.10.png

Screen Shot 2013-10-08 at 01.51.19.png

Screen Shot 2013-10-08 at 01.51.36.png

yannickoo’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
263.01 KB

Oh yes, if you remove the toolbar

you can see that the position is correct. Otherwise we should update the status right?

Screen Shot 2013-10-08 at 01.55.09.png

tim.plunkett’s picture

yannickoo’s picture

Oh sorry guys, it was already mentioned in #61.

dawehner’s picture

FileSize
72.9 KB

Some more manually testing:

Wim Leers’s picture

#97: hrm, I just tested again and closing the dialog with ESC definitely still works?

yannickoo’s picture

Tested it and closing via ESC still works.

webchick’s picture

I think this still needs accessibility review.

And then (please don't hit me, this really is AWESOME work) I think we might need to postpone this on #2067263: Drupal dialog placement must take displacing viewport elements, like the Toolbar, into account when calculating placement because with this patch, that bug suddenly goes from an annoyance where you have to fold up the toolbar in order to work to "OMFFFFFG I CAN'T USE VIEWS ARRGHGHHGHHGHHG!!!!!"

webchick’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs accessibility review

Meant to do that.

mgifford’s picture

Status: Needs review » Needs work

Can someone please re-roll this patch?

error: patch failed: core/misc/dialog.ajax.js:119
error: core/misc/dialog.ajax.js: patch does not apply

joelpittet’s picture

Issue tags: +Needs reroll

.

nod_’s picture

Status: Needs work » Needs review
FileSize
48.95 KB
nod_’s picture

the two patches conflicts.

Here is a patch with both of them merged. I'd close the other issue and commit this patch but if not, let me know when you commit the other one so that I can reroll this one and get it committed at the same time.

nod_’s picture

jessebeach’s picture

  1. +++ b/core/misc/dialog.position.js
    @@ -0,0 +1,81 @@
    +    var leftString = (left > 0 ? '+' : '-') + Math.round(left/2) + 'px';
    +    var topString = (top > 0 ? '+' : '-') + Math.round(top/2) + 'px';
    

    We should take the absolute value of the Math.round result. Otherwise, we can end up with strings like this:

    "--28px"

    Screenshot of the dialog placement value presented as center--128px

    I did some testing and jQuery UI handles this string just fine. It actually takes the last operator in the list, so "--+---+5" would be plus 5 and "+++++-5" would be minus 5. But I don't like relying on a weird parsing behavior of jQuery UI when we can give it a clean, single operator by using the absolute value of the top and left value.

  2. +++ b/core/misc/dialog.position.js
    @@ -0,0 +1,81 @@
    +    console.log(options);
    

    Superfluous console.log

  3. I tested the patch to ensure that the dialog avoids collision with displacing elements on any edge of the screen:

    Screenshot of the views dialog squeezed into the center of the screen by displacing elements on each edge of the viewport

    It positions correctly!!!....except in the Overlay :(

    Screenshot of the modal dialog located beneath the Toolbar tray and bar. It is broken.

    Wah Wah. Good news is, there's a high likelihood that the Overlay will be removed from Drupal. Bad news is is it's not removed yet. So we need to continue to make concessions for it while it is still in core. The problem can be easily solved by changing the IIFE invocation line of the dialog.position.js file from this:

    })(jQuery, Drupal, drupalSettings, Drupal.debounce, Drupal.displace);

    to this:

    })(jQuery, Drupal, drupalSettings, Drupal.debounce, window.parent.Drupal.displace);

    With a little todo to remove this extra code once #787896: Add a link so that administrators can return to their most recently visited non-admin page is committed.

I vote we keep the combined patch here and simply close #2067263: Drupal dialog placement must take displacing viewport elements, like the Toolbar, into account when calculating placement when this is committed. This issue has been under work for many months. It's been well banged on and tested. nod_'s additional code looks really good. At it's heart, it's just math utilizing existing APIs (Drupal.displace) to figure out positioning values to pass to the jQuery UI dialog.

nod_’s picture

wow I didn't think it'd be that easy to fix the overlay issue.

Fixed the feedback. Thanks!

32 files changed, 260 insertions(+), 599 deletions(-)

nod_’s picture

Issue tags: +JavaScript, +modal dialog, +VDC

major case of tagmonster.

jessebeach’s picture

My vote is RTBC given the changes in #108. I know mgifford wanted to review this patch as well, so I will leave it as needs review.

Test this patch on simplytest.me

nod_’s picture

can't be worst than it currently is. How about we RTBC that and leave #1806308: Review Views JavaScript + generic modals for accessibility for a11y? already a lot going on in this patch.

nod_’s picture

Status: Needs review » Reviewed & tested by the community

It's a fact that modals are here to stay in core and that the situation is already better after the patch than it was before.

Also I don't think much a11y testing has been done for modals because when we got the dialog API in core, jQuery UI 1.10 (which address our a11Y concerns) wasn't out then (remember the original feature freeze? that was before that #1667742-105: Add abstracted dialog to core (resolves accessibility bug)). Let's use #1806308: Review Views JavaScript + generic modals for accessibility to do the a11y review of modals in a broader way.

with that, RTBC per #110

catch’s picture

Status: Reviewed & tested by the community » Fixed

Ran this through with simplytest.me and it looks fine now.

Committed/pushed to 8.x, thanks!

mgifford’s picture

Sorry I wasn't able to get to reviewing this. We need more people on the Accessibility Team doing testing.. This is of course true for Core development in general. Glad to see it's standardized now so that it can all be fixed in one place now (if needed).

dawehner’s picture

Opened a followup as getting rid of being able to close via escape is quite annoying to be honest: #2113567: Views dialogs don't close via escape.

jessebeach’s picture

Minor followup bug I just discovered. Should be simple to resolve.

#2114475: Latent Drupal dialogs throw JavaScript errors on window resize and scroll

Status: Fixed » Closed (fixed)

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