Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comments
Comment #1
xjmComment #2
xjmComment #3
dawehnerWorking on it.
Comment #4
dawehnerIt seems to be that larowlan is already working on that: http://drupal.org/node/1667742#comment-6700926
Comment #5
dawehnerSo 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.
Comment #6
effulgentsia CreditAttribution: effulgentsia commentedThat 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.
Comment #7
dawehnerIt 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).
Comment #8
dawehnerThis is the current work which adds a new ajax commando to open/close modals
Comment #9
webchickSince there's a patch, marking needs review.
Comment #10
dawehnerYeah general review would be cool, but it's certainly not in the state of being strict-reviewable.
Comment #11
nod_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.
Comment #12
dawehnerFrega 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.
Comment #13
dawehneropened an actual issue to implement a proper API: #1870764: Add an ajax command which makes it easy to use the dialog API in complex cases
Comment #14
webchickAny progress on this? We added this dialog so that Views could use it and this critical bug has been open awhile now.
Comment #15
dawehnerProgress 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.
Comment #16
webchickSo #1909144: Allow #ajax['dialog'] to contain options other than 'modal' was committed awhile ago.. I don't suppose that's enough to unblock this?
Comment #17
dawehnerSadly 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.
Comment #18
jibranTagging
Comment #19
quicksketchThis 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.
Comment #21
nod_Resize shouldn't be in dialog.js, playing around with it it it seems that the whole views code can be replaced by
and it seems to be working just the same.
Comment #22
quicksketchWhy 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).
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.
Comment #23
nod_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.
Comment #24
Dave ReidThe 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.
Comment #25
quicksketchYeah 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).
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.
Comment #26
quicksketch.
Comment #27
quicksketchIn 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.
Comment #28
nod_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.
Comment #29
quicksketchYeah 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.
Comment #30
quicksketchI 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.
Comment #31
quicksketchOh, 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.
Comment #32
nod_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.
Comment #33
dawehnerThere is an issue for the OR dialog #1929070: Refactor views_ui_rearrange_filter_form to remove theme function for table rendering
Comment #34
effulgentsia CreditAttribution: effulgentsia commentedIs that true even when you apply it in CSS? As in:
The dialog is after all a DOM element, so I don't get what jQuery UI does to remove CSS features from it.
Comment #35
quicksketchHmm, 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.
Comment #36
quicksketchI'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.Comment #37
quicksketchHere'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.
Comment #38
quicksketchThis 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.
Comment #39
nod_Haven't tested yet, but it looks great :)
Nice to have the resizing out of the general dialog and ajax code.
Comment #41
xjmAwesome!
Related: #1832862: Make views add field scannable
Comment #42
dawehnerThanks for working on this issue. Tested it manually and spotted some issues:
Comment #43
quicksketchThanks @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.
Comment #44
mgifford#38: views_convert_modals-1851414-38.patch queued for re-testing.
Comment #46
quicksketch@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.
Comment #47
jessebeach CreditAttribution: jessebeach commentedThe 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.
Comment #48
quicksketchMy response to #47 (which was also in the other issue):
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.
Comment #49
larowlanCan we put the save button in the dialog as a normal form api field?
Tabbing still works.
Comment #50
quicksketchMaybe 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.
Comment #51
dawehnerThere is an issue for the button: #1836392: In the Views UI, the interaction pattern of “All displays”/ “Override this display” is confusing
Comment #52
nod_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.
Comment #54
falcon03 CreditAttribution: falcon03 commentedI 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.
Comment #55
quicksketch@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.
Comment #56
xjm@quicksketch, the current Views modal is not keyboard-navigable.
Comment #57
quicksketchIt 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.
Comment #58
tim.plunkettThe other day @quicksketch told me he was swamped recently, let's unassign this in case someone else wanted to work on it.
Comment #59
nod_Reroll. It's still pretty broken on the user side of things but at least it applies.
Comment #61
nod_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.
Comment #63
damiankloip CreditAttribution: damiankloip commentedGood job, thanks for doing this!
This should fix the failing test, then hopefully we have a passing patch to work with :)
Comment #64
jibranNice clean up. We need screen shots
Comment #65
tim.plunkett(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:
And obviously, a lot of the styling is broken. The CSS will need updates.
Comment #66
dawehnerI guess most of the bugs in https://drupal.org/node/1851414#comment-7218644 are still active.
Comment #67
nod_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.
Comment #68
joelpittettagging
Comment #69
frega CreditAttribution: frega commentedRerolled patch, functionality confirmed. Switching of button labels ("Apply (all pages)", "Apply (only this page/override)) is not yet implemented; needs styling.
Comment #70
yannickooHere is a screenshot so that you know what's missing.
Comment #71
nod_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.
Comment #72
frega CreditAttribution: frega commentedContinued 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.
Comment #73
nod_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…
Comment #74
frega CreditAttribution: frega commentedAdjusted as per #73. Rerolled patch attached.
Comment #75
frega CreditAttribution: frega commentedAs 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?
Comment #76
nod_If that works, let's try it in this one.
Comment #77
nod_changed a little the ternary operator and added #75 to the patch to see what testbot has to say.
Comment #78
nod_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 :)
Comment #79
yannickooWhitespace?
Comment #80
nod_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.
Comment #81
nod_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(-)
:)Comment #83
nod_#81: core-views-js-modal-1851414-81.patch queued for re-testing.
Comment #84
nod_Some more cleanup of collapsible.js
Comment #85
nod_Some margin adjustments to make it look more like before.
Comment #86
dawehnerIt would be great if someone could write some issue summary with screenshots and co.
Comment #87
nod_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 :)
Comment #88
nod_Had to reintroduce the
views_ui_build_form_url change
toviews_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.
Comment #89
nod_Sorry for patch spam but every time I think I'm done I find something new…
Here is the patch fixing the search.
Comment #90
Jelle_SShouldn't we use
.children('summary').find('span.details-summary-prefix')
? see #1931632: [META] Make core compatible with jQuery native-API selectorWhy is this commented out, shouldn't it just be removed then?
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...
Comment #91
nod_1. The selector is a legit CSS selector, no need to use jQuery for that.
2. fixed.
3. fixed.
Comment #92
cosmicdreams CreditAttribution: cosmicdreams commentedWhile I don't understand the removal of the CSS everything else looks good.
Comment #93
yannickooOops. 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.
Comment #94
yannickooOh yes, if you remove the toolbar
Comment #95
tim.plunkettThat's pre-existing:
#2067263: Drupal dialog placement must take displacing viewport elements, like the Toolbar, into account when calculating placement
See https://drupal.org/project/issues/search/drupal?issue_tags=modal%20place... for other related issues.
Comment #96
yannickooOh sorry guys, it was already mentioned in #61.
Comment #97
dawehnerSome more manually testing:
Comment #98
Wim Leers#97: hrm, I just tested again and closing the dialog with ESC definitely still works?
Comment #99
yannickooTested it and closing via ESC still works.
Comment #100
webchickI 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!!!!!"
Comment #101
webchickMeant to do that.
Comment #102
mgiffordCan 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
Comment #103
joelpittet.
Comment #104
nod_reroll. sad that it's blocked on #2067263: Drupal dialog placement must take displacing viewport elements, like the Toolbar, into account when calculating placement it's not going to be an easy one to fix.
Comment #105
nod_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.
Comment #106
nod_Setting real status of the issue. On #2067263: Drupal dialog placement must take displacing viewport elements, like the Toolbar, into account when calculating placement
Comment #107
jessebeach CreditAttribution: jessebeach commentedWe should take the absolute value of the Math.round result. Otherwise, we can end up with strings like this:
"--28px"
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.
Superfluous
console.log
I tested the patch to ensure that the dialog avoids collision with displacing elements on any edge of the screen:
It positions correctly!!!....except in the Overlay :(
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.
Comment #108
nod_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(-)
Comment #109
nod_major case of tagmonster.
Comment #110
jessebeach CreditAttribution: jessebeach commentedMy 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
Comment #111
nod_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.
Comment #112
nod_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
Comment #113
catchRan this through with simplytest.me and it looks fine now.
Committed/pushed to 8.x, thanks!
Comment #114
mgiffordSorry 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).
Comment #115
dawehnerOpened 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.
Comment #116
jessebeach CreditAttribution: jessebeach commentedMinor followup bug I just discovered. Should be simple to resolve.
#2114475: Latent Drupal dialogs throw JavaScript errors on window resize and scroll