Problem/Motivation

Maintainers of distributions and themes have pointed out that there is some bleed through of css from themes into the settings tray, which can significantly impact experience and usability.

We did #2853208: [META] Determine best method ensure consistent theming of Off Canvas Tray
Which involved this issue and #2853222: Explore using an IFrame to sandbox CSS for the Off Canvas tray

It was determined that iframe though it would provide better CSS isolation it would introduce its own problems to solve.

Proposed resolution

Add an aggressive CSS reset to the off-canvas tray to the module that will to the extent possible offer a uniform look when using the tray across different themes.

No CSS reset will ever be absolute because we can't control the CSS used on a sites current theme.

Since we decided not do #2847522: Allow off-canvas links to be rendered by either "default" or "admin" renderer there does not need to be a different look for the off-canvas tray when used by the Settings Tray module and other uses of the tray.
This means the logic in outside_in.js that sets settings.dialogClass += ' ui-dialog-outside-in'; can be moved into OpenOffCanvasDialogCommand.php.

This module provides 2 libraries drupal.off_canvas and drupal.outside_in.

The CSS reset and other CSS related to the tray should be contained within the CSS files of the drupal.off_canvas library and should follow the off-canvas.*.css naming pattern.
The drupal.off_canvas library will be moved out of the Settings Tray module in #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it.

The drupal.outside_in library should not contain CSS that deals with tray. It should not reference classes that start with "ui-dialog" or use the selector "#drupal-off-canvas"
It should contain CSS that deals with other functionality of the Setting Tray module such as toolbar, highlighting regions, etc.

Remaining tasks

Test the patch.

User interface changes

Theme styles should not bleed into the tray.

API changes

none

Data model changes

none

CommentFileSizeAuthor
#146 2826722-146-no_csslint_ignore.patch.patch65.42 KBtedbow
#146 2826722-146.patch66.24 KBtedbow
#144 interdiff-139-144-no_csslint_ignore.txt7.75 KBtedbow
#144 2826722-144-no_csslint_ignore.patch65.7 KBtedbow
#144 interdiff-139-144.txt8.05 KBtedbow
#144 2826722-144.patch66.53 KBtedbow
#139 2826722-139.patch66.11 KBtedbow
#139 interdiff-136-139.txt5.29 KBtedbow
#136 2826722-136.patch65.61 KBtedbow
#136 interdiff-131-136.txt812 bytestedbow
#131 2826722-131.patch65.46 KBtedbow
#131 interdiff-128-131.txt3.63 KBtedbow
#130 menu_list-modal-400__nexus_theme.png66.49 KBtedbow
#130 menu_list__nexus_theme.png30.67 KBtedbow
#130 menu_list-modal-400__zircon.png79.32 KBtedbow
#130 menu_list__zircon.png31.91 KBtedbow
#130 menu_list-modal__zurb.png55.97 KBtedbow
#130 menu_list__zurb_foundation.png78.02 KBtedbow
#130 menu_list-modal__bootstrap_business.png39.16 KBtedbow
#130 menu_list-modal__bootstrap_business_400.png114.68 KBtedbow
#130 menu_list__bootstrap_business.png30.39 KBtedbow
#128 2826722-128.patch69.11 KBtedbow
#128 interdiff-112-128.txt5.92 KBtedbow
#119 interdiff-2826722-112-119.txt2.26 KBDyanneNova
#119 2826722-119.patch74.37 KBDyanneNova
#115 Welcome_to_RTL___RTL.png149.38 KBxjm
#114 tray_open.png123.54 KBxjm
#114 toolbar_tray.png63 bytesxjm
#112 2826722-112-fixed-link-size-in-description.png48.53 KBstarshaped
#112 interdiff-2826722-108-112.txt2.09 KBstarshaped
#112 2826722-112.patch65.91 KBstarshaped
#111 description_hyperlink.png21.68 KBrikki_iki
#108 interdiff-2826722-106-108.txt1.63 KBDyanneNova
#108 2826722-108.patch66.29 KBDyanneNova
#108 D8_Test-rtl-dropdowns.png42.5 KBDyanneNova
#107 rtl_106.png46.74 KBtedbow
#107 rtl_unpatched.png65.77 KBtedbow
#106 Home___D8_Test.png27.29 KBDyanneNova
#106 interdiff-2826722-105-106.txt1.84 KBDyanneNova
#106 2826722-106.patch66.81 KBDyanneNova
#105 interdiff-2826722-101-105.txt564 bytesDyanneNova
#105 2826722-105.patch66.61 KBDyanneNova
#102 rtl-messages.png51.29 KByoroy
#101 interdiff-2826722-99-101.txt383 bytesstarshaped
#101 2826722-101.patch66.74 KBstarshaped
#99 2826722-99-asterisk-changes.png36.44 KBstarshaped
#99 interdiff-2826722-92-99.txt1.17 KBstarshaped
#99 2826722-99.patch66.72 KBstarshaped
#98 asterisk-background.png49.3 KByoroy
#92 2826722-92.patch66.36 KBDyanneNova
#92 interdiff-2826722-90-92.txt1.39 KBDyanneNova
#91 animation_w_patch.gif2.61 MBtedbow
#91 animation_w_patch_low.gif334.99 KBtedbow
#91 animation_no_patch.gif1.48 MBtedbow
#91 animation_no_patch_low.gif217.91 KBtedbow
#90 interdiff-2826722-87-90.txt1.11 KBDyanneNova
#90 2826722-90.patch65.58 KBDyanneNova
#90 D8_Test-block-headers.png46.87 KBDyanneNova
#90 D8_Test-inline-headers.png42.33 KBDyanneNova
#87 2826722-dropbutton.png34.36 KBstarshaped
#87 interdiff-2826722-83-87.txt4.13 KBstarshaped
#87 2826722-87.patch65.47 KBstarshaped
#85 drop-button.png41.3 KBEclipseGc
#83 2826722-83.patch65.41 KBGrandmaGlassesRopeMan
#83 interdiff-2826722-81-83.txt418 bytesGrandmaGlassesRopeMan
#81 D8_Test-upload.png17.37 KBDyanneNova
#81 D8_Test-status.png42.05 KBDyanneNova
#81 interdiff-2826722-76-81.txt2.55 KBDyanneNova
#81 2826722-81.patch64.76 KBDyanneNova
#80 managed_file_button.png20.73 KBtedbow
#80 various_smaller.png111.34 KBtedbow
#80 tray_messages.png70.93 KBtedbow
#76 interdiff-2826722-74-76.txt3.18 KBstarshaped
#76 2826722-76.patch63.87 KBstarshaped
#74 2826722-74.patch64.31 KBstarshaped
#73 2826722-73-pencil-icon-fix.png46.5 KBstarshaped
#73 interdiff-2826722-69.73.txt523 bytesstarshaped
#73 2826722-73.patch64.5 KBstarshaped
#72 Screenshot 2017-06-23 14.44.09.png30.76 KBtedbow
#69 2826722-69.patch64.5 KBpk188
#67 interdiff-2826722-63-67.txt5.53 KBDyanneNova
#67 2826722-67.patch65.64 KBDyanneNova
#66 interdiff-63-66.txt3.14 KBjian he
#66 2826722-66.patch63.47 KBjian he
#64 2826722-63.patch64.71 KBtedbow
#64 interdiff-62-63.txt16.11 KBtedbow
#62 2826722-62.patch54.89 KBtedbow
#59 2826722-59.patch54.29 KBtedbow
#59 interdiff-56-59.txt575 bytestedbow
#56 2826722-settings-tray-fence-56.patch53.73 KBtkoleary
#53 2826722-53-settings-tray-fence.patch53.19 KBtedbow
#53 interdiff-51-53.txt1.74 KBtedbow
#51 2826722-settings-tray-fence-49.patch51.96 KBtkoleary
#48 2826722-settings-tray-fence-48.patch51.93 KBtkoleary
#47 interdiff-41-47.txt52.06 KBtkoleary
#47 2826722-settings-tray-fence-47.patch52.06 KBtkoleary
#41 2826722-settings-tray-fence-41.patch36.24 KBtkoleary
#39 interdiff-35-39.txt2.55 KBtkoleary
#39 2826722-settings-tray-fence-39.patch35.82 KBtkoleary
#35 interdiff-settings-tray-fence-32-35.txt56.17 KBtkoleary
#35 2826722-settings-tray-fence-35.patch35.72 KBtkoleary
#34 interdiff-settings-tray-patch-32-34.txt18.89 KBtkoleary
#34 2826722-settings-tray-fence-34.patch73.09 KBtkoleary
#32 2826722-settings-tray-fence-32.patch41.79 KBtkoleary
#32 interdiff-settings-tray-fence-28-32.txt0 bytestkoleary
#28 2826722-settings-tray-fence-28.patch40.89 KBtkoleary
#22 interdiff-2826722-19-22.txt16.26 KBtkoleary
#22 2826722-settings-tray-fence-22.patch45.84 KBtkoleary
#19 2826722-settings-tray-fence-19.patch52.29 KBtkoleary
#16 2826722-settings-tray-fence-16.patch46.07 KBtkoleary
#13 interdiff-10-13.txt19.83 KBtedbow
#13 2826722-13-tray-fence.patch14.65 KBtedbow
#10 2826722-tray-fence-10.patch14.42 KBtkoleary
#6 2826722-tray-fence-6.patch14.42 KBtkoleary
#2 2826722-tray-fence-2.patch18.92 KBtkoleary
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tkoleary created an issue. See original summary.

tkoleary’s picture

Tested in Chrome and Firefox, mac, with the following themes enabled: Bartik, Stark, Zen, Mayo, Zurb Foundation 6.

Attempted to test in Bootstrap and Adaptive but settings tray did not work in either. Opening another issue for that.

There are still some minor unresolved issues around dropbutton border radius on hover and missing background arrow on selects in Foundation. There is a separate issue #2802457: Style selects in the Settings Tray to style all selects in the tray so it doesn't make sense to deal with that here.

tkoleary’s picture

Version: 8.3.x-dev » 8.2.x-dev
Category: Task » Bug report
tedbow’s picture

Status: Needs work » Needs review

Setting this to "Needs Review" so tests run.

tedbow’s picture

@tkoleary I know we had talked about some other issues that were also addressing similar problems maybe for toolbar or quickedit. If you know of any could you add them as related issue to this issue to make sure we aren't solving this same problem in different places different ways.

  1. +++ b/core/modules/outside_in/css/outside_in.form.css
    @@ -1,6 +1,6 @@
    - * Visual styling for forms in the Settings Tray module's off canvas tray.
    

    "forinput.forms"

  2. +++ b/core/modules/outside_in/css/outside_in.form.css
    @@ -25,65 +25,87 @@
    -  line-height: 16px;
    +  line-height: 2em;
    

    Is this height change related to this issue?

  3. +++ b/core/modules/outside_in/css/outside_in.form.css
    @@ -96,8 +118,8 @@
    -.ui-dialog-offcanvas .form-checkbox,
    -.ui-dialog-offcanvas .form-radio {
    ...
    +.ui-dialog-offcanvas input.form-radio {
    

    I think we are suppose to target classes not markup+classes when we can. Why was this changed? If search in core css for input.form-checkbox I don't find any results.

tkoleary’s picture

No interdiff because I couldn't get the older patch to apply cleanly.

@tedbow, I removed all the form stuff you mentioned above.

Status: Needs review » Needs work

The last submitted patch, 6: 2826722-tray-fence-6.patch, failed testing.

tkoleary’s picture

Version: 8.2.x-dev » 8.3.x-dev
Status: Needs work » Needs review
tedbow’s picture

Status: Needs review » Needs work

No interdiff because I couldn't get the older patch to apply cleanly.

I could not get the #6 to apply so that might be why. your local copy might not have been clean

tkoleary’s picture

Pulled core, rebased my branch and rerolled the patch.

tkoleary’s picture

Status: Needs work » Needs review

NR :)

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tedbow’s picture

Re-roll then changed to use .ui-dialog-outside-in instead of .ui-dialog-offcanvas for the reset. This means it will only reset when used with the Outside in.

I think we would really want to reset when used with forms or in "admin" mode. This would be possible with #2847522: Allow off-canvas links to be rendered by either "default" or "admin" renderer

tedbow’s picture

tkoleary’s picture

Close to having a new patch for this.

tkoleary’s picture

There are changes in most of the css files because of the cascade implications of switching scoping from classes to ids. Also took the liberty of re-organizing a lot of stuff.

tedbow’s picture

Status: Needs review » Needs work

@tkoleary re:

I think a lot of this nullifies #2815831: Move Off-canvas related CSS from drupal.outside_in library to drupal.off_canvas

The drupal.outside_in library should only have outside_in.*.css files and not any offcanvas.*.css files.

With the idea that in #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it

outside_in.*.css files would only be the files(lib drupal.outside_in) the module is loaded with the outside_in module is using the dialog.
The basic idea being that tray could be used in admin(backend) mode or regular(frontend) mode. The admin mode would the only case where you need the style reset.
See #2847522: Allow off-canvas links to be rendered by either "default" or "admin" renderer

Maybe the problem these related issues solved is

  1. #2856441: Off-canvas CSS files not included in libraries file this is just a problem with a previous commit.
  2. #2847522: Allow off-canvas links to be rendered by either "default" or "admin" renderer
    We could add to this issue letting themes override what styles are loaded for "Offcanvas Admin" mode(what Settings Tray uses)
  3. This issue, then all the structure would be set and we would just to add the CSS but it will obvious where it should live and how to override it.
tkoleary’s picture

The drupal.outside_in library should only have outside_in.*.css files and not any offcanvas.*.css files.

Yeah, that's why they have the @todos to move into the library. I think maybe this patch is just trying to do too much but I'm not sure how to break it up because of the intertwining dependencies.

I *think* what should be done is that this issue should be made a task of #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it. Then there will be no confusion around the css inheritance because all of the relevant tray css will already be in the dialog library.

tkoleary’s picture

This patch should be one of the last things we commit so I'm thinking it will need to be refactored a few times, but it works pretty well now.

tkoleary’s picture

Status: Needs work » Needs review

Repeated myself

Status: Needs review » Needs work

The last submitted patch, 19: 2826722-settings-tray-fence-19.patch, failed testing.

tkoleary’s picture

Fixed coding standards problems but I don't think that's why it's failing tests.

tkoleary’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 22: 2826722-settings-tray-fence-22.patch, failed testing.

droplet’s picture

Where's these CSS reset come from?

Drupal CORE default reset rule? If not, why not?

tkoleary’s picture

Drupal CORE default reset rule? If not, why not?

Because this is not a reset in that same sense. A core reset is something that simply rationalizes the base assumptions so that it's easier to develop cross-browser code.

The primary purpose of this reset is to create isolation in the tray. This requires a much stronger reset that sets a higher level of specificity by scoping every selector to the ID of the tray (#drupal-offcanvas). Put simply, a base reset does not need to anticipate and override *anything* that might be added by other themes.

As far as inspiration for the code, I started with Erik Myers reset, added quite a lot from Nathan Smith's formalize—because forms are the primary content of the tray—and then removed a bunch of outdated prefixes and parts of formalize that referenced browsers we no longer support. After that I did quite a bit of testing in different themes. One thing I discovered right away is that quite a few of the new themes available in D8 are using Bootstrap (not bootstrap base theme just the library) so I added a few things that specifically apply to elements that Bootstrap styles, for example ::selection (!?).

Bojhan’s picture

Can we make sure this gets buy-in from laurri or carver?

tkoleary’s picture

Status: Needs work » Needs review
FileSize
40.89 KB

No interdiff. I started on a clean codebase and moved in only the relevant work from the other branch to try to isolate my work from whatever was causing the failure in #22.

I also removed a bunch of outdated vendor prefixes and copied the library into stable theme.

Status: Needs review » Needs work

The last submitted patch, 28: 2826722-settings-tray-fence-28.patch, failed testing.

tedbow’s picture

I also removed a bunch of outdated vendor prefixes and copied the library into stable theme.
@tkoleary I don't think we can make any changes to Stable as long as this is an experimental module.

We can do that as part of #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it.

If you remove the Stable related changes I think the tests should pass.

tkoleary’s picture

@tedbow

Already had a new patch ready. I'll try that if this one fails.

tkoleary’s picture

@tedbow

If I understood correctly @dyannenova said *any* change to core libraries needs to be reflected in Stable or it will fail. So that's what this patch is doing.

I *think* we can put things in stable if they apply to features that are completely new.

tedbow’s picture

+++ b/core/core.libraries.yml
diff --git a/core/misc/dialog/offcanvas.reset.css b/core/misc/dialog/offcanvas.reset.css
new file mode 100644

new file mode 100644
index 0000000..9209157

index 0000000..9209157
--- /dev/null

@tkoleary yeah I don't think we should doing this yet.
My understanding is once we do this is no longer experimental.

I think we should keep offcanvas.reset.css in this module in Settings Tray module until #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it. Hopefully if everything is sorted before that then that issue will be simply moving files around.

tkoleary’s picture

Moved reset back in to module and removed from stable theme and misc.

tkoleary’s picture

tkoleary’s picture

droplet’s picture

I'm still thinking about if it's the best way to do this job. In the meaning of time. here's something I think that's errors:

  1. +++ b/core/modules/outside_in/css/offcanvas.reset.css
    @@ -0,0 +1,406 @@
    +#drupal-offcanvas html,
    

    Impossible?

  2. +++ b/core/modules/outside_in/css/offcanvas.reset.css
    @@ -0,0 +1,406 @@
    +/* Reset size and position on elements. */
    

    To my understand, this is a hard reset of all elements (should not only HTML5 but also any custom elements).

    About 80 lines of code is equal to

    #drupal-offcanvas *

  3. +++ b/core/modules/outside_in/css/offcanvas.reset.css
    @@ -0,0 +1,406 @@
    +#drupal-offcanvas input[type="button" i],
    +#drupal-offcanvas input[type="submit" i],
    +#drupal-offcanvas input[type="reset" i],
    +#drupal-offcanvas input[type="file" i]::-webkit-file-upload-button {
    

    what is the `i`?

  4. +++ b/core/modules/outside_in/css/offcanvas.reset.css
    @@ -0,0 +1,406 @@
    +#drupal-offcanvas * html textarea,
    +#drupal-offcanvas * html select {
    

    Impossible?

tkoleary’s picture

About 80 lines of code is equal to
#drupal-offcanvas *

Tried that but there were too many unintended consequences, then I'd need to start making overrides below and that starts to get messy.

+#drupal-offcanvas input[type="button" i],
+#drupal-offcanvas input[type="submit" i],
+#drupal-offcanvas input[type="reset" i],
+#drupal-offcanvas input[type="file" i]::-webkit-file-upload-button {
what is the `i`?

That's an odd thing that I discovered chrome inserting. It's not documented but it is there and does add unwelcome styles to those inputs.

tkoleary’s picture

@droplet

Patch addresses #1 and #4.

Status: Needs review » Needs work

The last submitted patch, 39: 2826722-settings-tray-fence-39.patch, failed testing.

tkoleary’s picture

Status: Needs work » Needs review
FileSize
36.24 KB

Rebased and re-rolled patch

mherchel’s picture

This is a crazy task to accomplish, but until web components becomes available, it's probably the right way to go. Some quick notes:

  1. +++ b/core/modules/outside_in/css/offcanvas.reset.css
    @@ -0,0 +1,406 @@
    +#drupal-offcanvas tt,
    +#drupal-offcanvas u,
    +#drupal-offcanvas ul,
    +#drupal-offcanvas var,
    +#drupal-offcanvas video,
    +#drupal-offcanvas xmp {
    

    Why aren't we using something like the following, which can then be overridden by selectors later in the same file?

    #drupal-offcanvas * { ... }

  2. +++ b/core/modules/outside_in/css/offcanvas.reset.css
    @@ -0,0 +1,406 @@
    +#drupal-offcanvas input {
    +  height: initial;
    +  width: initial;
    +  min-height: initial;
    +  max-height: initial;
    +  min-width: initial;
    +  max-width: initial;
    +  line-height: initial;
    

    Note that initial value isn't supported by IE11 :(

droplet’s picture

Tried that but there were too many unintended consequences, then I'd need to start making overrides below and that starts to get messy.

I could see the reason now. It dropped DIV. To me, it's same as other HTML elements. We still able to apply large margin to DIV and it will look wired in Setting Tray. Needs a comment there or TODO I think.

Anyway, :not(div) will work I think

That's an odd thing that I discovered chrome inserting. It's not documented but it is there and does add unwelcome styles to those inputs.

Be interesting to see what the diff is. can we comment it as "Chrome / Webkit user agent stylesheet" (/ shadow DOM things)

also,

I think #drupal-offcanvas must be a class. We could use it to do a hard reset in other places.

SKAUGHT’s picture

#42 "This is a crazy task to accomplish....until web component"

true enough.. but for 2cents worth: it will at some point be up to the theme used in the project (and those building the project) to ensure they're not breaking advanced tools like the drawer.

tkoleary’s picture

I think #drupal-offcanvas must be a class. We could use it to do a hard reset in other places.

Do you mean *also* a class? It's the fact that it's an ID that gives us the specificity to override the frontend theme.

tkoleary’s picture

@droplet

Trying #drupal-offcanvas *:not(div), looks good so far.

tkoleary’s picture

Ok new patch. Had to do a lot of cleanup work because

#drupal-offcanvas *:not(div) {
  all: initial;
}

means we can't inherit *anything* from core.

tkoleary’s picture

Re-rolled with whitespace removed

The last submitted patch, 47: 2826722-settings-tray-fence-47.patch, failed testing.

tkoleary’s picture

tkoleary’s picture

Ok this time I actually *comitted* the changes with whitespace removed.

The last submitted patch, 48: 2826722-settings-tray-fence-48.patch, failed testing.

tedbow’s picture

@tkoleary thanks your work on this!

I found 1 problem unrelated to actual CSS. Any links that aren't part of the Settings Tray module don't get the "ui-dialog-outside-in" class on the dialog itself which surrounds "#drupal-offcanvas".

You can test by
enabling the test module offcanvas_test
going to /offcanvas-test-links
Click "Display more links!"
In the tray click "Offcanvas link!"

Without these changes the final tray will not be fully styled. With the changes it will be.

Since we postponed #2847522: Allow off-canvas links to be rendered by either "default" or "admin" renderer the MVP does not need the Off-canvas dialog to work without this reset

So I moved adding the "ui-dialog-outside-in" class from outside_in.js to \Drupal\outside_in\Ajax\OpenOffCanvasDialogCommand

If a developer really didn't want ui-dialog-outside-in class they could use the existing dialogClass option to override this.

Also I was still getting a whitespace error in #51 so I fixed that.

The last submitted patch, 51: 2826722-settings-tray-fence-49.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 53: 2826722-53-settings-tray-fence.patch, failed testing.

tkoleary’s picture

Some fixes around buttons tabledrag and dropbuttons

tkoleary’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 56: 2826722-settings-tray-fence-56.patch, failed testing.

tedbow’s picture

Status: Needs work » Needs review
FileSize
575 bytes
54.29 KB

I haven't time to look at the other test fails but I know I introduced a test fail in #53 so just fixing that.

Status: Needs review » Needs work

The last submitted patch, 59: 2826722-59.patch, failed testing.

tedbow’s picture

Assigned: tkoleary » Unassigned

@tkoleary just assigning you because it has been a month. If you don't have to work on it now no problem. Thanks for pushing this issue so far!

tedbow’s picture

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

Adding a more detailed Issue Summary.

Also rerolled #59

Status: Needs review » Needs work

The last submitted patch, 62: 2826722-62.patch, failed testing.

tedbow’s picture

Status: Needs work » Needs review
FileSize
16.11 KB
64.71 KB

This patch renames a bunch files that were name outside_in*.css that should have been name off-canvas*.css

Also re-organized files so that all files that just dealt with off-canvas tray to the drupal.off_canvas library.

Now drupal.outside_in library has only the files that deal with "edit mode" and the toolbar changes

Here are notes about other files:

off-canvas.tabledrag.css

body div.tabledrag-changed-warning {
  margin-bottom: 0.5em;
  font-size: 14px;
}

All other selectors in this file start with #drupal-off-canvas So I am not sure if this part is a mistake. I left it.

outside_in.motion.css
Remove the todo about moving this file in #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it
These file deals with motion not specific to the tray
core/modules/outside_in/css/off-canvas.form.css
Still has this @todo which is correct.

outside_in.module.css
This file add off-canvas tray specific CSS in addition to Settings Tray "Edit mode" CSS
Moved off-canvas tray specific CSS off-canvas.css
was also using selectors starting with .ui-dialog-outside-in
Because the jQuery dialog
outside_in.theme.css
This file add off-canvas tray specific CSS in addition to Settings Tray "Edit mode" CSS
Moved off-canvas tray specific to new CSS off-canvas.theme.css
The CSS was also using selectors starting with .ui-dialog-outside-in
After rearranging the files to be in correct library this CSS was being overridden by jQuery UI's own CSS.
I think this is because the jQuery dialog is using .ui-dialog and this class comes before ours.
Anyways I was able to fix this by using the selector .ui-dialog.ui-dialog-outside-in

outside_in.theme.css also had this at the top

@import url("outside-in.button.css") screen;
@import url("outside-in.dropbutton.css") screen;

At the top of file. I am not sure if this was correct. I removed this and added the renamed off-canvas version to the library file to include these.

off-canvas.css
This file existed but was not included in a library. Is it needed?
include in library for now.

OpenOffCanvasDialogCommand
This class was adding the dialogClass ui-dialog-outside-in if no dialogClass was provided.
I changed this to ui-dialog-off-canvas because this used for CSS of the tray and will be moved out of this module in #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it

The last submitted patch, 62: 2826722-62.patch, failed testing.

jian he’s picture

FileSize
63.47 KB
3.14 KB

Reroll

DyanneNova’s picture

Overall, this looks good to me. I've checked over the tray for any odd styling issues and cleaned up a few details, fixed the select and autocomplete elements which had lost their styling, and cleaned up some of the formatting.

Status: Needs review » Needs work

The last submitted patch, 67: 2826722-67.patch, failed testing. View results

pk188’s picture

Status: Needs work » Needs review
FileSize
64.5 KB

Rerolled #67 and did some changes.

tedbow’s picture

@pk188 thanks for the patch.
Can you please provide an interdiff? https://www.drupal.org/documentation/git/interdiff

Also can explain the reason for the changes you made?

pk188’s picture

1. --- a/core/modules/outside_in/css/off-canvas.css
+++ b/core/modules/outside_in/css/off-canvas.css
@@ -6,18 +6,51 @@
*/
/* Position the off-canvas dialog container outside the right of the viewport. */
.ui-dialog-off-canvas {
- box-sizing: border-box;
- height: 100%;
- overflow: visible;
+ box-sizing: border-box;
+ height: 100%;
+ overflow: visible;
}

/* Wrap the form that's inside the off-canvas dialog. */
.ui-dialog-off-canvas .ui-dialog-content {
- padding: 0 20px;
- /* Prevent horizontal scrollbar. */
- overflow-x: hidden;
- overflow-y: auto;
+ padding: 0 20px;
+ /* Prevent horizontal scrollbar. */
+ overflow-x: hidden;
+ overflow-y: auto;

2. --- a/core/modules/outside_in/css/off-canvas.motion.css
+++ b/core/modules/outside_in/css/off-canvas.motion.css
@@ -12,17 +12,17 @@

/* Transition the off-canvas dialog container, with 2s delay to match main canvas speed. */
.ui-dialog-off-canvas .ui-dialog-content {
- -webkit-transition: all .7s ease 2s;
- -moz-transition: all .7s ease 2s;
- transition: all .7s ease 2s;
+ -webkit-transition: all .7s ease 2s;
+ -moz-transition: all .7s ease 2s;
+ transition: all .7s ease 2s;
}

@media (max-width: 700px) {
- .ui-dialog-off-canvas .ui-dialog-content {
- -webkit-transition: all .7s ease;
- -moz-transition: all .7s ease;
- transition: all .7s ease;
- }
+ .ui-dialog-off-canvas .ui-dialog-content {
+ -webkit-transition: all .7s ease;
+ -moz-transition: all .7s ease;
+ transition: all .7s ease;
+ }

These changes in #67 were already corrected by some other issue.

tedbow’s picture

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

@DyanneNova thanks for the patch and @pk188 thanks for the re-roll.

When opening a menu block I am getting a 404 on pencil icon.
pencil icon missing

This is the 404 url
http://www.octo2.dev/core/misc/icons/ffffff/pencil.svg

This is a similar icon used by contextual showing up
http://www.octo2.dev/d8_2_ux/core/misc/icons/bebebe/pencil.svg

Notice that my site is in sub directory and it is not being picked up.

UPDATE found the problem

+++ b/core/modules/outside_in/css/off-canvas.dropbutton.css
@@ -0,0 +1,308 @@
+  background: transparent url(/core/misc/icons/ffffff/pencil.svg) no-repeat center;

This will not work if the site is in subdirectory.
Other places in the css we are using
background-image: url(../../../misc/icons/ffffff/pencil.svg);
which should work in either case

I searched the module for "url(/core" and only found this 1 occurrence so maybe that is the only place.

starshaped’s picture

Status: Needs work » Needs review
FileSize
64.5 KB
523 bytes
46.5 KB

Changed the background image url to ../../../misc/icons/ffffff/pencil.svg as detailed in #72. Attached screenshot of the menu edit after making the change.

starshaped’s picture

Updated the patch to add newlines to the css files. No interdiff is needed here because I didn't make any updates to the code itself.

tedbow’s picture

Status: Needs review » Needs work

@starshaped thanks for the fix. I can confirm the pencil icon show up now.

Got a chance to go through patch. Looking good so far.

Here

  1. +++ b/core/modules/outside_in/css/off-canvas.base.css
    @@ -0,0 +1,196 @@
    +  margin: 0 0 0 10px 0;
    

    Too many values I think.

  2. +++ b/core/modules/outside_in/css/off-canvas.dropbutton.css
    @@ -0,0 +1,308 @@
    +  border: 0px solid transparent;
    

    Nit 0px should be just 0

  3. +++ b/core/modules/outside_in/css/off-canvas.dropbutton.css
    @@ -0,0 +1,308 @@
    +/* The arrow toogle arrow. */
    

    Spelling "toggle"

  4. +++ b/core/modules/outside_in/css/off-canvas.reset.css
    @@ -0,0 +1,382 @@
    +  /* For IE. http://css-tricks.com/ie-fix-bicubic-scaling-for-images */
    +  -ms-interpolation-mode: bicubic;
    

    The article linked references IE 6 & 7. Is this fix needed past that? We don't support IE 6 & 7.

  5. +++ b/core/modules/outside_in/css/off-canvas.reset.css
    @@ -0,0 +1,382 @@
    +  content: '';
    +  content: none;
    

    Doesn't the property get overwritten here?

  6. +++ b/core/modules/outside_in/css/off-canvas.reset.css
    @@ -0,0 +1,382 @@
    +/* Chrome / Webkit user agent stylesheet (/ shadow DOM things). */
    +#drupal-off-canvas input[type="button" i],
    +#drupal-off-canvas input[type="submit" i],
    +#drupal-off-canvas input[type="reset" i],
    +#drupal-off-canvas input[type="file" i]::-webkit-file-upload-button {
    

    I get a parsing error here. Can someone comment as to why this is here? Most browsers don't support shadow dom and nothing in the path mentions it?

  7. +++ b/core/modules/outside_in/css/off-canvas.table.css
    @@ -0,0 +1,96 @@
    +  min-width: calc(100%  40px);
    

    Is missing an operand here? Should this have a - or +?

  8. +++ b/core/modules/outside_in/css/off-canvas.tabledrag.css
    @@ -0,0 +1,114 @@
    +  margin: 0;
    +  margin-left: 0; /* LTR */
    

    I think the margin-left is not need because of the previous line.

starshaped’s picture

Made updates as suggested by #75.

  1. I discussed this issue with phenaproxima and he found only one other instance of .compact-link in core/modules, in system.admin.css, and it has a margin of 0 0 .5em 0, so I'm going to assume that the rule here should be margin: 0 0 10px 0.
  2. Removed the px from the 0 here.
  3. Changed the comment to say 'The toggle arrow'.
  4. I removed -ms-interpolation-mode because yes, it was deprecated as of IE9 according to this article: https://msdn.microsoft.com/en-us/library/aa752712(v=vs.85).aspx
  5. I did some research on why someone would set content: ' ' and then content: none, and according to https://stackoverflow.com/questions/6803250/content-vs-content-none, older versions of Webkit (circa 2008) did not support content: none, so content: ' ' was also used. I removed content: ' ' as Webkit does now support content: none.
  6. I did a quick search through core/modules and did not see shadow dom declarations used anywhere else, so I removed them. If anyone else knows why they were added to the css file, please comment why.
  7. Looks like this was missing a + sign, so I added it back.
  8. I noticed a margin-right: 5px below the margin: 0 declaration, so I condensed this to margin: 0 5px 0 0.
starshaped’s picture

Status: Needs work » Needs review
tedbow’s picture

@starshaped thank you for the patch, the research and the detailed explanations!

I confirm that my points in #75 were taken care of.

Going to give another look over now.

droplet’s picture

I'm interested how do you guys testing it? I meant... If we trying to reset and re-style all elements, shouldn't we build a single test dialog with all FORM API elements (and HTML elements)? It will help to review and has a global view of what we supported I think.

tedbow’s picture

Status: Needs review » Needs work
FileSize
70.93 KB
111.34 KB
20.73 KB

Ok. So to test various elements I decided to use the Styleguide module. I don't expect everything to look good in the tray but looked at some key elements that I think should.

I created a test module to put the elements in the tray https://github.com/tedbow/off_canvas_styleguide

Let me know if the this is not a good way to test the reset.

A few things stood out.

Messages


The background is being overriden here because of off-canvas.base.css

#drupal-off-canvas * {
  background: #444;
  font-family: "Lucida Grande", 'Lucida Sans Unicode','liberation sans', sans-serif;
  color: #ddd;
}

Misc


This shows that various tags including, a, b, em make the text be smaller than the surrounding text.

Also the h2 text is #ddd which is not very readable. Also legend

Managed file button


Spacing looks off here

DyanneNova’s picture

Status: Needs work » Needs review
FileSize
64.76 KB
2.55 KB
42.05 KB
17.37 KB

A few of those issues are caused by the specificity of *:not(div) overriding the elements covered under *, so I've added *:not(div) in to the base to make sure those elements get covered. This should fix the dark text and the odd text size changes.

This patch should also fix the messages styling, upload file element width, lists displaying as a jumble of text, and em and i elements losing their text styling.

Status styling

File upload styling

tedbow’s picture

Issue summary: View changes
Status: Needs review » Needs work

@DyanneNova thanks for the patch! It is looking really good.

I have done some checks:

Also found this 1 problem

+++ b/core/modules/outside_in/js/outside_in.js
@@ -156,7 +156,6 @@
-          settings.dialogClass += ' ui-dialog-outside-in';

Just noticed this change was made before es6.js changes were introduced to core. This change should be made directly to outside_in.es6.js and then the outside_in.js file should be automatically built.

Otherwise I think it is close to RTBC.

Updated the summary

GrandmaGlassesRopeMan’s picture

Status: Needs work » Needs review
FileSize
418 bytes
65.41 KB

- Applied changes to outside_in.es6.js.

tedbow’s picture

Status: Needs review » Needs work

@drpal thanks for the JS fix.

I found 1 other CSS problem.

+++ b/core/modules/outside_in/css/off-canvas.base.css
@@ -0,0 +1,221 @@
+  background: #f3faef url(/core/misc/icons/73b355/check.svg) no-repeat 10px 17px;
...
+  background: #fdf8ed url(/core/misc/icons/e29700/warning.svg) no-repeat 10px 17px;
...
+  background: #fcf4f2 url(/core/misc/icons/e32700/error.svg) no-repeat 10px 17px;

These icons get 404 if the site is a subdirectory. They should referenced like other files in these CSS files "../../../misc*"

I search the CSS files in this patch and these are the only instances of "(/core" so other file references should be fine.

EclipseGc’s picture

Issue summary: View changes
FileSize
41.3 KB

Drop buttons are broken right now. Lack of relative positioning on them make their drop link in the upper right corner of the next relative item which will likely cause them to all overlay each other. Also the basic styling is missing on them.

Eclipse

tedbow’s picture

@EclipseGc thanks for reporting this.

I had a couple links to the test module I am using to test this https://github.com/tedbow/off_canvas_styleguide

If you pull in that latest changes you could see 2 more links at "/off_canvas_styleguide-links" both link to core forms that have dropbuttons.

starshaped’s picture

Status: Needs work » Needs review
FileSize
65.47 KB
4.13 KB
34.36 KB

Updated with changes noted in both #84 and #85.

tedbow’s picture

@starshaped thanks for the fixes!

I visually checked the message icons and the dropbuttons and they seemed to be fixed for me.

pk188’s picture

So @tedbow, status should changed to RTBC?

DyanneNova’s picture

I've updated to make headers display as block instead of inline. This fixes the issues with headers colliding with text.

inline headers

block headers

I think that we have all of the important pieces working correctly now.

tedbow’s picture

Status: Needs review » Needs work
FileSize
217.91 KB
1.48 MB
334.99 KB
2.61 MB

@DyanneNova thanks looking much better!

Notice a couple things when giving the entire patch a review again.
1.

+++ b/core/modules/outside_in/outside_in.libraries.yml
@@ -24,6 +21,22 @@ drupal.off_canvas:
+      # @todo This file existed but was not included in a library. Is it needed?
+      css/off-canvas.css: {}

I added this todo in #64 because the file was in the patch but was not included in the library.
I just checked and it is needed for instance the tray needs to become %100 width at narrow widths.

@DyanneNova(or someone else) can you confirm this file is actually and remove this todo.

2. There is weird animation change that has a good effect and bad effect.
I think these changes my be related to #2856441: Off-canvas CSS files not included in libraries file
because the motion.css file was not actually included in the library before.

Here is the gif of the animation before the patch. (click on gifs to see higher res versions)

Body displacement does not have an animation: BAD
Resizing tray resizes form elements with no delay: GOOD

Here is the gif of the animation WITH the patch.

Body displacement DOES have an animation: GOOD
Resizing tray resizes form elements WITH delay: BAD

It seems the animation transitions are somehow being applied to form elements when they resize.

Should this be follow up? We could just remove the offcanvas.motion.css because before patch it existed but wasn't used. It has nothing todo with the CSS reset.
Or is this a super simple fix.

Setting to needs work to at least remove the todo if resolved.

DyanneNova’s picture

Status: Needs work » Needs review
FileSize
1.39 KB
66.36 KB

I think we could handle the resizing slowly in a follow up, but it was pretty simple to fix, so I went ahead and fixed it to not animate the elements in the tray on resize.

We should include off-canvas.css in the library, so I've also removed the todo.

tedbow’s picture

Status: Needs review » Reviewed & tested by the community

@DyanneNova ok yeah this fixes the problem for me, thanks!

Ok this look ready to me.

  1. All files are now included in the .libraries.yml file
  2. The 2 libraries are split up correctly so that drupal.off_canvas can easily be separated out in #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it
  3. Went through CSS code automated inspections and everything that comes up looks correct after manual inspection
  4. Manually checking links provided by test module off_canvas_test confirms that the tray using links that not provided by the Settings Module(not contextual) have reset and look as links provided by the Settings Tray module.

Thanks @DyanneNova, @tkoleary, @starshaped, @droplet, @drpal, @pk188, @jian he, @EclipseGc , @mherchel for all your work on this!

yoroy’s picture

I tested in simplytest and it looks good. The issue about styling messages in the tray was closed as a duplicate of this, but I could not find a way to actually show a message in the tray. Is that being handled in this patch?

DyanneNova’s picture

It is; we've been testing it using the module @tedbow has here: https://github.com/tedbow/off_canvas_styleguide. I have screenshots in #81.

tedbow’s picture

@yoroy another way to test the message though you will only see a "warning" message is to open up the "User Menu" or any menu block with more than 1 item in the tray. Then re-order the menu items and you will get a warning message about having unsaved changes.

Wim Leers’s picture

Nice work here!

yoroy’s picture

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

Apologies for missing those screenshot @DyanneNova. Thanks for that tip @tedbow. Tried again in simplytest and I'm seeing a dark background color behind the "required" asterisk:

CSS inspecting says this is applied through

#drupal-off-canvas *, #drupal-off-canvas :not(div)

on line 8 of off-canvas.base.css

starshaped’s picture

Status: Needs work » Needs review
FileSize
66.72 KB
1.17 KB
36.44 KB

I removed the background color from behind the * and fixed two other things:

1) Adjusted the required asterisk color in the "warning" message to be a little darker so it is noticeable. I checked to make sure this color worked on the "status" and "error" message types as well.

2) Moved the required asterisk next to "Log out" 5 pixels to the left (5 pixels to the right on RTL) so it's not running right up against the 'Log out' text.

tim.plunkett’s picture

+++ b/core/modules/outside_in/css/off-canvas.tabledrag.css
@@ -55,6 +55,14 @@
+#drupal-off-canvas tr td abbr {
+  margin-left: 5px; /* LTR */
+}
...
+[dir="rtl"] #drupal-off-canvas tr td abbr {
+  margin-right: 5px;
+}

I believe the RTL rules have to cancel out the LTR rule, so this would need margin-left: 0; (or whatever the initial value was)

starshaped’s picture

Added the margin-left: 0 to the patch.

yoroy’s picture

Status: Needs review » Needs work
FileSize
51.29 KB

Something looked odd in the screenshot from #99:

rtl messages

If you change the dir="ltr" to dir="rtl" on the html tag you see that the styling of messages in rtl is not complete yet. This is what a rtl message looks like in admin:

Looks like this needs some work for that. (out of scope here: I almost daren't ask wheter the whole position of the settings tray should be left hand side in rtl language layouts. Did we ever consider that?)

yoroy’s picture

Sorry, let me double check first in simplytest. brb.

tedbow’s picture

(out of scope here: I almost daren't ask wheter the whole position of the settings tray should be left hand side in rtl language layouts. Did we ever consider that?)

@yoroy I looked through the initial issues and I didn't see any mention of this. I would say out-of-scope!

DyanneNova’s picture

Status: Needs work » Needs review
FileSize
66.61 KB
564 bytes

I found one issue that I think came in with #2809427: Update jQuery UI to 1.12. The class ui-state-default was removed from the close button, which caused the default jquery ui button to appear on top of our close button. I haven't seen any other side effects so far.

DyanneNova’s picture

Here's a fix for the rtl messages styling. The tray title was still left aligned in rtl, so I've fixed that as well.

tray rtl styling

tedbow’s picture

Status: Needs review » Needs work
FileSize
65.77 KB
46.74 KB

@DyanneNova thanks for the patch I can confirm messages are fixed RTL in the new patch.
Patched:


There is problem with the dropbutton though in RTL as you can above.

This is the unpatched version


You can see the dropbutton is usable here.

DyanneNova’s picture

This should fix the rtl dropbuttons.

RTL dropdowns

tedbow’s picture

Status: Needs work » Needs review

@DyanneNova thanks! Settings to Needs Review to test patch

tedbow’s picture

Status: Needs review » Reviewed & tested by the community

@DyanneNova looks great! RTBC!

rikki_iki’s picture

FileSize
21.68 KB

This is great!!

Only minor thing I noticed is hyperlinks inside .description are 14px instead of the 12px size of the rest of the description text.

starshaped’s picture

Fixed the font size issue in the description text, and cleaned up a couple other things:

1. I noticed that the same font styles for the a and .link were duplicated in off-canvas.theme.css and off-canvas.base.css, so I removed them from off-canvas.base.css. I haven't seen any issues arise from removing them from here, but if they are needed, they can easily be added back. Just figured we didn't need the duplicate styles :)

2. I also noticed the .placeholder class in some areas still had a background color of #444, so I updated this to remove the background color.

SKAUGHT’s picture

Status: Reviewed & tested by the community » Needs review
xjm’s picture

Issue summary: View changes
FileSize
63 bytes
123.54 KB

(out of scope here: I almost daren't ask wheter the whole position of the settings tray should be left hand side in rtl language layouts. Did we ever consider that?)

@yoroy I looked through the initial issues and I didn't see any mention of this. I would say out-of-scope!

The tray does already appear on the left side in an RTL language in HEAD. This is Drupal installed in Urdu:

So if that doesn't happen with the patch, then that is a serious regression.

xjm’s picture

FileSize
149.38 KB

I tested the patch and it seems to open the toolbar on the left for RTL as well. Maybe the screenshots above are just toggling RTL in a browser developer toolbar or such?

Edit: It's a little funky that the untranslated string gets truncated at the beginning rather than the end, but that happens in HEAD also, so we're fine there as well.

xjm’s picture

So we just need review on the small changes in #112 prior to committer review?

xjm’s picture

tedbow’s picture

Re #114
about my previous comment

@yoroy I looked through the initial issues and I didn't see any mention of this. I would say out-of-scope!

@xjm @yoroy 🙃 Sorry I missed that yoroy's comment was about RTL. i thought it was a suggest to move the tray to the other side.

Yeah it switches side in LTR vs RTL. We did this originally because the toolbar tray switches is always on the opposite side.

DyanneNova’s picture

I think we should keep the generic elements together, so I've moved the link styles from off-canvas.theme to off-canvas.base. I also cleared out some unused styles from off-canvas.theme. Now there are only .ui styles in there.

tedbow’s picture

Just a comment of @Wim Leers from #2784443-61: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it because they apply the CSS
misc/dialog/css/off-canvas.theme.css: { group: 200 }

This can't be right?

My memory of this was that was necessary to make the styles override "Jquery ui dialog" css which is by default loaded first.

It would be good to test if this is necessary. And get confirmation

Wim Leers’s picture

#120: It's documented in outside_in_css_alter() and outside_in.libraries.yml — #2784443 is just losing that comment. No need to bring that up here.

tedbow’s picture

Looking at the comment

      # @todo Set the group higher than CSS_AGGREGATE_THEME so that it overrides
      #   both jQuery UI and Classy's dialog.css, remove in
      #   https://www.drupal.org/node/1945262.
      css/outside_in.theme.css: { group: 200 }

I am actually wondering if we need this at all. It some point in this issue we realized the CSS reset would be much more effective if we switched to using an ID vs a class as our selector. I think because of this our CSS will override jQuery UI and Classy's dialog.css anyways.
I have removed this and outside_in_css_alter() and I could not see any visual changes.

lauriii’s picture

I took a quick look on the patch to get an idea of what is being done here. I'm slightly worried because it seems like the proposed solution is quite complex and potentially fragile. It is quite difficult to understand the whole change, and I'm worried that this will be yet another complex thing that we have to maintain. We should at least open up a follow-up to find the blockers for converting this to use Shadow DOM and web components. It seems like #2842298: [policy, no patch] Drop IE9 and IE10 support from Drupal 8.4.x is still blocking that.

  1. +++ b/core/modules/outside_in/css/off-canvas.reset.css
    @@ -0,0 +1,382 @@
    +#drupal-off-canvas *:not(div),
    

    Why is div so special compared to all the other html elements that we are not overriding it?

  2. +++ b/core/modules/outside_in/css/off-canvas.reset.css
    @@ -0,0 +1,382 @@
    + * Reset input form styles for the settings tray.
    

    This comment is confusing. It seems like this resets quite a lot more than just input form styles since it resets almost everything.

  3. --- a/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
    +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
    

    FYI: I reviewed the patch from #112 since the latest patch contains some unrelated changes

I will do another more in-depth review for this but I wanted to comment my thoughts so far.

Wim Leers’s picture

I share the concerns raised in #123. I raised similar ones in #2853222: Explore using an IFrame to sandbox CSS for the Off Canvas tray. By process of elimination, this approach was chosen. "The least bad choice", if you will.

Note that Shadow DOM is not an option, because http://caniuse.com/#feat=shadowdom and http://caniuse.com/#feat=shadowdomv1 show that this will still not be supported in Firefox and Edge, for example, and only partially in Safari. Shadow DOM is still very far out, unfortunately.

tedbow’s picture

Re #123

We should at least open up a follow-up to find the blockers for converting this to use Shadow DOM and web components

Since Shadow dom is far out do we even know that that would be best way to solve this problem without having to do the CSS reset?

We already have #2195695: Admin UIs on the front-end are difficult to theme which seems to be an issue to find the best solution for this kind of thing, whether it is Shadow Dom or something else.

Wim Leers’s picture

Priority: Normal » Major
Issue tags: +blocker

This is blocking #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it, which is major, so bumping this to major too. This should have been major all along. This is "Settings Tray-critical", really.

tedbow’s picture

Status: Needs review » Needs work

@lauriii thanks for taking a look

re:

FYI: I reviewed the patch from #112 since the latest patch contains some unrelated changes

Setting to needs work

tedbow’s picture

Status: Needs work » Needs review
FileSize
5.92 KB
69.11 KB

Here is the patch in @DyanneNova without the changes outside of the Settings Tray module that that module contained.

Doing the interdiff against 112 because this patch is the same 119 except without unrelated file changes.

Status: Needs review » Needs work

The last submitted patch, 128: 2826722-128.patch, failed testing. View results

tedbow’s picture

Removed an image from summary that wasn't suppose to be there

Ok to demonstrate how this reset works against different themes I will upload some screen shots.

These demonstrate:

  1. The reset keeps a consistent look across multiple themes.
  2. The existing modal dialog does not have consistent look across themes
  3. The existing dialog does not display forms well in many of these themes.
  4. IMHO the off-canvas looks better in all cases.

UPDATE: talked with @tim.plunkett and he pointed out that reason the modals don't look good on the front-end is that mostly these have been used on the admin side, so more likely using admin themes. Though I am not sure about contrib's usage. Anyways the screenshot still show that off-canvas reset is consistent across multiple themes

Bootstrap Business

Off-Canvas

Modal - default width

Modal 400px

Zurb Foundation

Off-Canvas

Modal 400px

Zircon

Off-canvas

Modal 400px

Nexus Theme

Off-canvas

Modal - 400px

tedbow’s picture

Whoops I didn't get rid of all the unrelated changes.

Manually checked the patch this time. It should be good.

DyanneNova’s picture

Priority: Major » Normal

#123 It looks like including div in the overrides required a much larger number of overrides per #38 and #43

I agree with #124. This is a complicated set of css, but if we want to accomplish this before Shadow DOM is fully ready it seems like this is the best way to go about it.

DyanneNova’s picture

Priority: Normal » Major

Setting back to Major

tedbow’s picture

Status: Needs review » Reviewed & tested by the community

Setting to RTBC because my patch in #131 is just removing unrelated files in @DyanneNova's patch in #119. I didn't make any changes under core/modules/outside_in

Also I think #130 proves that the reset works well in across multiple themes.

Obviously we can't guarantee that the reset will work across all possible CSS in a theme but that is true for any thing in Drupal core.

There are various ways that themes could easily handle conflicts with the reset:

  1. Libraries can now be overridden and extended by themes, this can be used add additions to the drupal.off_canvas library CSS: https://www.drupal.org/node/2497313
  2. There is explicit CSS selector, .dialog-off-canvas__main-canvas that can be applied to the "main-canvas" as opposed to the "off-canvas"
  3. There is an explicit CSS selector that can be applied to the off-canvas dialog, .ui-dialog-off-canvas
lauriii’s picture

Status: Reviewed & tested by the community » Needs work

@Wim Leers I agree that we shouldn't block this issue by the shadow DOM. Thank you for the reference to the issue that has more background on this.

@DyanneNova Thanks for pointing out the comments that include the conversation about the div exception.

I reviewed the latest patch and it has all of the unrelated changes removed from it.

  1. The largest outstanding issue is that we'll still have to solve the violations of CSS coding standards. We should fix all of the real violations and add overrides for the instances that we cannot fix (for example the use of ID selectors).


  2. Two easier things for someone to address would be:

  3. Could we document the div exception and the reason for it in the CSS? It would make it easier for anyone reading that piece of CSS to understand the backgrounds.

  4. #123.2 is still not addressed.

tedbow’s picture

Status: Needs work » Needs review
FileSize
812 bytes
65.61 KB

@lauriii thanks for the review!

#135.2 added comment
#135.3 Add fixed file comment per #123.2. Also fixed the file comment to start with "/**" and have a blank line after the first sentence.

re #131.1 I don't understand

and add overrides for the instances that we cannot fix (for example the use of ID selectors).

but that is probably just because I haven't done core CSS work.

Does this have to do with overriding rules in core/.stylelintrc.json? Do we put a .stylelintrc.json file in core/modules/outside_in/css as that extends it like documented here: https://stylelint.io/user-guide/configuration/#extends

I may be way off base.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

For #135.1, I'd have assumed that lauriii wanted https://github.com/stylelint/stylelint/blob/master/docs/user-guide/confi..., but he said extends, so I think #136 is right, and https://stylelint.io/user-guide/configuration/#extends is what he is asking.

OTOH, this is an experimental module, and cleaning up CSS standards violations can easily happen in a follow-up.

Therefore re-RTBC'ing.

tedbow’s picture

Just wanted to mention here that DrupalCI is NOT using csslint @see #2866840: Use stylelint as opposed to csslint in DrupalCI which is postponed so doing something like https://stylelint.io/user-guide/configuration/#extends or https://github.com/stylelint/stylelint/blob/master/docs/user-guide/confi... would not actually fix DrupalCI standards errors

Not sure how to do overrides currently with csslint because spent half a day researching this for stylelint. The whole situation is very confusing.

I commented here https://www.drupal.org/node/2868114#comment-12189267 that the change record should mention this is not yet in effect on Drupalci

tedbow’s picture

re: #135

We should fix all of the real violations and add overrides for the instances that we cannot fix (for example the use of ID selectors).

Ok I am still not positive what overrides means here. Overrides for stylelint won't make any difference for Drupalci because of reasons in #138

I did find a way that you can ignore rules per file for csslint though.

so this patch adds ignore for

  1. Heading (hX) should not be qualified.
  2. Don't use IDs in selectors.

Because I know the reset needs these things.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 139: 2826722-139.patch, failed testing. View results

lauriii’s picture

I checked the stylelint errors and they are much easier to get fixed than the csslint errors. Given that this only affects experimental modules and that we are planning to get rid of csslint, I can agree with #137. So let's ignore the csslint failures on this issue and try to resolve them later if #2866840: Use stylelint as opposed to csslint in DrupalCI doesn't get resolved.

tedbow’s picture

Status: Needs work » Reviewed & tested by the community

@lauriii ok #141 sounds good.
#139 does add ignoring 2 csslint rules that are causing most of the csslint errors. So it brings down from 536 errors to 32
If we don't want to mess with ignoring csslint errors right now then #136 should be fine.

#139 just failed because of an unrelated failure in FileFieldWidgetTest which is breaking 8.4.x right now also
So I am setting back to RTBC.

lauriii’s picture

Status: Reviewed & tested by the community » Needs work

Could we still fix the stylelint errors even though they are not run on the CI since they are run on the committer hooks. Those should be pretty easy to get fixed :)

tedbow’s picture

ok fixed stylelint errors.

So now I can run:
node_modules/.bin/stylelint modules/outside_in/css/*.css
from the core folder and get no errors.

I added 2 versions of this patch.

2826722-144.patch

leaves in the
/*csslint ids:false */
Lines to to ignore these csslint rules which require no space between /* and csslint. but this is actually a stylelint error 😜
So to get this to work with stylelint you have to add

/* stylelint-disable */
/*csslint ids:false */
/* stylelint-enable */

2826722-144-no_csslint_ignore.patch

removes the
/*csslint ids:false */
lines. So Drupalci will then have 500 more coding standards messages.

Status: Needs review » Needs work

The last submitted patch, 144: 2826722-144-no_csslint_ignore.patch, failed testing. View results

tedbow’s picture

2826722-144-no_csslint_ignore.patch failed because of random fail in HEAD

Needed re-roll to apply cleanly after #2897306: Remove dead CSS

UPDATE: Not waiting for 8.4.x branch to pass because I think the fail is random because in #144 only 1 of those patches failed on this error

The last submitted patch, 146: 2826722-146.patch, failed testing. View results

The last submitted patch, 139: 2826722-139.patch, failed testing. View results

DyanneNova’s picture

Status: Needs review » Reviewed & tested by the community

I've checked over these and everything looks good to me.

lauriii’s picture

Status: Reviewed & tested by the community » Fixed

I hope we can simplify this in future but for now this is the right way to do this. I've reviewed this couple of times and tested manually and that is the best I can do to ensure this is good enough to be committed.

I committed the 2826722-146-no_csslint_ignore.patch.patch since we should deal with the csslint problems later if we have to.

Committed 7a27101 and pushed to 8.4.x. Thanks!

  • lauriii committed 7a27101 on 8.4.x
    Issue #2826722 by tkoleary, tedbow, DyanneNova, starshaped, drpal, jian...
tedbow’s picture

@lauriii thanks for your reviews and the commit! 🙌 🎉

  • lauriii committed 36ee6b3 on 8.5.x
    Issue #2826722 by tkoleary, tedbow, DyanneNova, starshaped, drpal, jian...
Wim Leers’s picture

Thanks, @lauriii! I know this must've taken a lot of time to review. And it's indeed choosing "the least bad" option, but unfortunately that's what technical limitations in the web force us to do :( Can't wait to vastly improve this when these limitations are no longer present!

xjm’s picture

Issue tags: +8.4.0 release notes

We should mention this in the Settings Tray section of the release notes.

Wim Leers’s picture

#155++

Status: Fixed » Closed (fixed)

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

tedbow’s picture

Component: outside_in.module » settings_tray.module

Changing to new settings_tray.module component. @drpal thanks for script help! :)

Chris Burge’s picture