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
Comment | File | Size | Author |
---|---|---|---|
#146 | 2826722-146-no_csslint_ignore.patch.patch | 65.42 KB | tedbow |
#146 | 2826722-146.patch | 66.24 KB | tedbow |
#144 | interdiff-139-144-no_csslint_ignore.txt | 7.75 KB | tedbow |
#144 | 2826722-144-no_csslint_ignore.patch | 65.7 KB | tedbow |
#144 | interdiff-139-144.txt | 8.05 KB | tedbow |
Comments
Comment #2
tkoleary CreditAttribution: tkoleary at Acquia commentedTested 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.
Comment #3
tkoleary CreditAttribution: tkoleary at Acquia commentedComment #4
tedbowSetting this to "Needs Review" so tests run.
Comment #5
tedbow@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.
"forinput.forms"
Is this height change related to this issue?
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.
Comment #6
tkoleary CreditAttribution: tkoleary at Acquia commentedNo interdiff because I couldn't get the older patch to apply cleanly.
@tedbow, I removed all the form stuff you mentioned above.
Comment #8
tkoleary CreditAttribution: tkoleary at Acquia commentedComment #9
tedbowI could not get the #6 to apply so that might be why. your local copy might not have been clean
Comment #10
tkoleary CreditAttribution: tkoleary at Acquia commentedPulled core, rebased my branch and rerolled the patch.
Comment #11
tkoleary CreditAttribution: tkoleary at Acquia commentedNR :)
Comment #13
tedbowRe-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
Comment #14
tedbowAdded this as a child of #2853208: [META] Determine best method ensure consistent theming of Off Canvas Tray
This issue is an either/or of #2853222: Explore using an IFrame to sandbox CSS for the Off Canvas tray to offer a consistent experience for styling
Comment #15
tkoleary CreditAttribution: tkoleary at Acquia commentedClose to having a new patch for this.
Comment #16
tkoleary CreditAttribution: tkoleary at Acquia commentedThere 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.
Comment #17
tedbow@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
We could add to this issue letting themes override what styles are loaded for "Offcanvas Admin" mode(what Settings Tray uses)
Comment #18
tkoleary CreditAttribution: tkoleary at Acquia commentedYeah, 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.
Comment #19
tkoleary CreditAttribution: tkoleary at Acquia commentedThis 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.
Comment #20
tkoleary CreditAttribution: tkoleary at Acquia commentedRepeated myself
Comment #22
tkoleary CreditAttribution: tkoleary at Acquia commentedFixed coding standards problems but I don't think that's why it's failing tests.
Comment #23
tkoleary CreditAttribution: tkoleary at Acquia commentedComment #25
droplet CreditAttribution: droplet commentedWhere's these CSS reset come from?
Drupal CORE default reset rule? If not, why not?
Comment #26
tkoleary CreditAttribution: tkoleary at Acquia commentedBecause 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 (!?).
Comment #27
Bojhan CreditAttribution: Bojhan commentedCan we make sure this gets buy-in from laurri or carver?
Comment #28
tkoleary CreditAttribution: tkoleary at Acquia commentedNo 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.
Comment #30
tedbowI 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.
Comment #31
tkoleary CreditAttribution: tkoleary at Acquia commented@tedbow
Already had a new patch ready. I'll try that if this one fails.
Comment #32
tkoleary CreditAttribution: tkoleary at Acquia commented@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.
Comment #33
tedbow@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.
Comment #34
tkoleary CreditAttribution: tkoleary at Acquia commentedMoved reset back in to module and removed from stable theme and misc.
Comment #35
tkoleary CreditAttribution: tkoleary at Acquia commentedIgnore last patch
Comment #36
tkoleary CreditAttribution: tkoleary at Acquia commentedComment #37
droplet CreditAttribution: droplet commentedI'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:
Impossible?
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 *
what is the `i`?
Impossible?
Comment #38
tkoleary CreditAttribution: tkoleary at Acquia commentedTried that but there were too many unintended consequences, then I'd need to start making overrides below and that starts to get messy.
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.
Comment #39
tkoleary CreditAttribution: tkoleary at Acquia commented@droplet
Patch addresses #1 and #4.
Comment #41
tkoleary CreditAttribution: tkoleary at Acquia commentedRebased and re-rolled patch
Comment #42
mherchelThis is a crazy task to accomplish, but until web components becomes available, it's probably the right way to go. Some quick notes:
Why aren't we using something like the following, which can then be overridden by selectors later in the same file?
#drupal-offcanvas * { ... }
Note that initial value isn't supported by IE11 :(
Comment #43
droplet CreditAttribution: droplet commentedI could see the reason now. It dropped
DIV
. To me, it's same as other HTML elements. We still able to apply large margin toDIV
and it will look wired in Setting Tray. Needs a comment there or TODO I think.Anyway,
:not(div)
will work I thinkBe 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.
Comment #44
SKAUGHT#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.
Comment #45
tkoleary CreditAttribution: tkoleary at Acquia commentedDo you mean *also* a class? It's the fact that it's an ID that gives us the specificity to override the frontend theme.
Comment #46
tkoleary CreditAttribution: tkoleary at Acquia commented@droplet
Trying #drupal-offcanvas *:not(div), looks good so far.
Comment #47
tkoleary CreditAttribution: tkoleary at Acquia commentedOk new patch. Had to do a lot of cleanup work because
means we can't inherit *anything* from core.
Comment #48
tkoleary CreditAttribution: tkoleary at Acquia commentedRe-rolled with whitespace removed
Comment #50
tkoleary CreditAttribution: tkoleary at Acquia commentedComment #51
tkoleary CreditAttribution: tkoleary at Acquia commentedOk this time I actually *comitted* the changes with whitespace removed.
Comment #53
tedbow@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.
Comment #56
tkoleary CreditAttribution: tkoleary at Acquia commentedSome fixes around buttons tabledrag and dropbuttons
Comment #57
tkoleary CreditAttribution: tkoleary at Acquia commentedComment #59
tedbowI haven't time to look at the other test fails but I know I introduced a test fail in #53 so just fixing that.
Comment #61
tedbow@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!
Comment #62
tedbowAdding a more detailed Issue Summary.
Also rerolled #59
Comment #64
tedbowThis 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
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
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
Comment #66
jian he CreditAttribution: jian he commentedReroll
Comment #67
DyanneNovaOverall, 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.
Comment #69
pk188 CreditAttribution: pk188 at OpenSense Labs commentedRerolled #67 and did some changes.
Comment #70
tedbow@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?
Comment #71
pk188 CreditAttribution: pk188 at OpenSense Labs commented1. --- 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.
Comment #72
tedbow@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.
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
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.
Comment #73
starshapedChanged the background image url to ../../../misc/icons/ffffff/pencil.svg as detailed in #72. Attached screenshot of the menu edit after making the change.
Comment #74
starshapedUpdated 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.
Comment #75
tedbow@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
Too many values I think.
Nit 0px should be just 0
Spelling "toggle"
The article linked references IE 6 & 7. Is this fix needed past that? We don't support IE 6 & 7.
Doesn't the property get overwritten here?
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?
Is missing an operand here? Should this have a - or +?
I think the margin-left is not need because of the previous line.
Comment #76
starshapedMade updates as suggested by #75.
Comment #77
starshapedComment #78
tedbow@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.
Comment #79
droplet CreditAttribution: droplet commentedI'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.
Comment #80
tedbowOk. 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
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
Comment #81
DyanneNovaA 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.
Comment #82
tedbow@DyanneNova thanks for the patch! It is looking really good.
I have done some checks:
Also found this 1 problem
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
Comment #83
GrandmaGlassesRopeMan- Applied changes to
outside_in.es6.js
.Comment #84
tedbow@drpal thanks for the JS fix.
I found 1 other CSS problem.
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.
Comment #85
EclipseGc CreditAttribution: EclipseGc commentedDrop 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
Comment #86
tedbow@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.
Comment #87
starshapedUpdated with changes noted in both #84 and #85.
Comment #88
tedbow@starshaped thanks for the fixes!
I visually checked the message icons and the dropbuttons and they seemed to be fixed for me.
Comment #89
pk188 CreditAttribution: pk188 at OpenSense Labs commentedSo @tedbow, status should changed to RTBC?
Comment #90
DyanneNovaI've updated to make headers display as block instead of inline. This fixes the issues with headers colliding with text.
I think that we have all of the important pieces working correctly now.
Comment #91
tedbow@DyanneNova thanks looking much better!
Notice a couple things when giving the entire patch a review again.
1.
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.
Comment #92
DyanneNovaI 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.
Comment #93
tedbow@DyanneNova ok yeah this fixes the problem for me, thanks!
Ok this look ready to me.
Thanks @DyanneNova, @tkoleary, @starshaped, @droplet, @drpal, @pk188, @jian he, @EclipseGc , @mherchel for all your work on this!
Comment #94
yoroy CreditAttribution: yoroy at Roy Scholten commentedI 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?
Comment #95
DyanneNovaIt is; we've been testing it using the module @tedbow has here: https://github.com/tedbow/off_canvas_styleguide. I have screenshots in #81.
Comment #96
tedbow@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.
Comment #97
Wim LeersNice work here!
Comment #98
yoroy CreditAttribution: yoroy at Roy Scholten commentedApologies 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
on line 8 of off-canvas.base.css
Comment #99
starshapedI 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.
Comment #100
tim.plunkettI believe the RTL rules have to cancel out the LTR rule, so this would need
margin-left: 0;
(or whatever the initial value was)Comment #101
starshapedAdded the margin-left: 0 to the patch.
Comment #102
yoroy CreditAttribution: yoroy at Roy Scholten commentedSomething looked odd in the screenshot from #99:
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?)
Comment #103
yoroy CreditAttribution: yoroy at Roy Scholten commentedSorry, let me double check first in simplytest. brb.
Comment #104
tedbow@yoroy I looked through the initial issues and I didn't see any mention of this. I would say out-of-scope!
Comment #105
DyanneNovaI 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.
Comment #106
DyanneNovaHere's a fix for the rtl messages styling. The tray title was still left aligned in rtl, so I've fixed that as well.
Comment #107
tedbow@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.
Comment #108
DyanneNovaThis should fix the rtl dropbuttons.
Comment #109
tedbow@DyanneNova thanks! Settings to Needs Review to test patch
Comment #110
tedbow@DyanneNova looks great! RTBC!
Comment #111
rikki_iki CreditAttribution: rikki_iki commentedThis 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.
Comment #112
starshapedFixed 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.
Comment #113
SKAUGHTComment #114
xjmThe 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.
Comment #115
xjmI 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.
Comment #116
xjmSo we just need review on the small changes in #112 prior to committer review?
Comment #117
xjmComment #118
tedbowRe #114
about my previous comment
@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.
Comment #119
DyanneNovaI 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.
Comment #120
tedbowJust 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 }
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
Comment #121
Wim Leers#120: It's documented in
outside_in_css_alter()
andoutside_in.libraries.yml
— #2784443 is just losing that comment. No need to bring that up here.Comment #122
tedbowLooking at the comment
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.
Comment #123
lauriiiI 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.
Why is div so special compared to all the other html elements that we are not overriding it?
This comment is confusing. It seems like this resets quite a lot more than just input form styles since it resets almost everything.
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.
Comment #124
Wim LeersI 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.
Comment #125
tedbowRe #123
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.
Comment #126
Wim LeersThis 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.
Comment #127
tedbow@lauriii thanks for taking a look
re:
Setting to needs work
Comment #128
tedbowHere 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.
Comment #130
tedbowRemoved 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:
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
Comment #131
tedbowWhoops I didn't get rid of all the unrelated changes.
Manually checked the patch this time. It should be good.
Comment #132
DyanneNova#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.
Comment #133
DyanneNovaSetting back to Major
Comment #134
tedbowSetting 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:
drupal.off_canvas
library CSS: https://www.drupal.org/node/2497313.dialog-off-canvas__main-canvas
that can be applied to the "main-canvas" as opposed to the "off-canvas".ui-dialog-off-canvas
Comment #135
lauriii@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.
Two easier things for someone to address would be:
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.
#123.2 is still not addressed.
Comment #136
tedbow@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
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.
Comment #137
Wim LeersFor #135.1, I'd have assumed that lauriii wanted https://github.com/stylelint/stylelint/blob/master/docs/user-guide/confi..., but he said , 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.
Comment #138
tedbowJust 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
Comment #139
tedbowre: #135
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
Because I know the reset needs these things.
Comment #141
lauriiiI 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.
Comment #142
tedbow@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.
Comment #143
lauriiiCould 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 :)
Comment #144
tedbowok 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
/*
andcsslint
. but this is actually a stylelint error 😜So to get this to work with stylelint you have to add
2826722-144-no_csslint_ignore.patch
removes the
/*csslint ids:false */
lines. So Drupalci will then have 500 more coding standards messages.
Comment #146
tedbow2826722-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
Comment #149
DyanneNovaI've checked over these and everything looks good to me.
Comment #150
lauriiiI 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!
Comment #152
tedbow@lauriii thanks for your reviews and the commit! 🙌 🎉
Comment #154
Wim LeersThanks, @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!
Comment #155
xjmWe should mention this in the Settings Tray section of the release notes.
Comment #156
Wim Leers#155++
Comment #158
tedbowChanging to new settings_tray.module component. @drpal thanks for script help! :)
Comment #159
Chris Burge CreditAttribution: Chris Burge commentedThis issue resulted in a bug with multiple select elements. See #3080698: Multiple select element is inappropriately targeted with CSS in settings tray.