Problem/Motivation
In Layout Builder, when the user chooses the options "Add Block", "Configure Block", "Remove Block", "Add Section", "Remove Section", "Configure Section", a dialog opens. Once the dialog is open, there is no visual indication of the Block/Section is is acting on. In some cases, this active element is pushed out of the viewport. This can make working with dialogs very confusing.
Proposed resolution
- The clicked element opening the dialog should remain within the viewport after the right panel appears (canvas resizing can push it out of view)
- The area of the page the dialog is acting on should be highlighted.
Video of the solution for the six scenarios to be addressed
- Add Section
- Configure Section
- Remove Section
- Add Block (also demonstrates the clicked element scrolling back into view)
- Configure Block
- Remove Block (also demonstrates that contextual link hover behavior when a block is highlighted)
Remaining tasks
Reviews
User interface changes
Element triggering dialogs will be highlighted. If the dialog appearing pushes the element offscreen, the page will automatically scroll to bring the triggering element back into view.
API changes
None
Data model changes
N/A
Release notes snippet
N/A
Comment | File | Size | Author |
---|---|---|---|
#129 | interdiff.txt | 889 bytes | lauriii |
#126 | 2994909-highlight-126-interdiff.txt | 698 bytes | tim.plunkett |
#126 | 2994909-highlight-126.patch | 32.46 KB | tim.plunkett |
#123 | 2994909-123.patch | 32.46 KB | tedbow |
#123 | interdiff-123.txt | 2.86 KB | tedbow |
Comments
Comment #2
cilefen CreditAttribution: cilefen as a volunteer commentedComment #3
krismaye CreditAttribution: krismaye at Drupal Diversity & Inclusion commentedComment #4
samuel.mortensonHere's an attempt at this - I added the same "highlight" effect to blocks, which felt more consistent.
Comment #5
xjmComment #6
xjmComment #7
tedbowFunctionality look good to me.
Now that #2978964: Use Prettier for formatting core JavaScript is fixed we need to run
yarn run prettier
on all js patches.
After running this that
yarn run lint:core-js-passing
Now passes
Comment #8
krismaye CreditAttribution: krismaye at Drupal Diversity & Inclusion commentedComment #9
tim.plunkettAre those backticks the right way to do this? I'm out of date on best practices here.
Can someone sign off on the JS stuff?
Comment #10
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedThere might be an accessibility dimension to this, I'm still thinking about it. The notion of "grey out" suggests some information is being conveyed by colour alone, so this idea would need to address WCAG SC 1.4.1 Use of Color (level A).
Can we add some indicator to emphasize the relevant area itself, rather than merely de-emphasising the irrelevant areas?
Update: I mean a non-colour indicator for the relevant area. An icon, or a change in border style perhaps.
Comment #11
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedComment #12
tedbowAddressing #10.
Changed the outline and made the text bolder in the link.
Here are screenshots
Comment #13
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedI like #12 - that's the sort of thing I had in mind.
RTBC-worthy from the screenshots in #12, but I'll leave the code review to others more familiar with the layout builder controllers and JS.
Comment #14
tedbowI don't for the data-layout-builder-highlight-id we actually need the storage_type and the storage_id.
The only case I could see for this is you have 2 layout builder administrations sections open on the same page at the same time. Meaning you configuring 2 separate layouts at once. I assume this is not something we support.
If we remove the extra elements in the array I think we can just use string concatenation.
Comment #15
GrandmaGlassesRopeManJavaScript here is 👍.
One thing to consider for future development would be to consider moving away from jQuery unless we absolutely need to use some of its features.
Comment #16
DyanneNovaThis looks good to me for CSS and should be a nice UX improvement.
Comment #17
lauriiiAny particular reason why this couldn't be just
.layout-builder-highlight
?Not sure if this is the correct
z-index
. For example, contextual links are now rendered on top of this.Is there a clear benefit in setting this? This font-weight is not commonly used in Drupal core and might be something that doesn't work very well with custom themes since they don't necessarily have font loaded for this weight.
Comment #18
tedbow@lauriii thanks for the review
[data-layout-builder-highlight-id]
was on the element too. I guess this also avoids accidently conflicting with a a theme is already working with the layout builder and is adding their own class.layout-builder-highlight
.Comment #19
tedbowComment #20
lauriiiCould you elaborate since I don't think I quite understand this?
I don't think we should worry about this given that the module is in
alphabeta stage. Also, this is not in Stable theme so we are allowed to make changes freely.We should also update the issue summary with the approach taken on the overlay and get UX review specifically on that since it works differently than other overlays in Drupal.
Comment #21
bnjmnmFrom #10,
Similar concerns were brought up at the Tues Jan 15 UX meeting. Attached are a few variations highlighting the relevant area without any grey-out
Comment #22
tedbow@bnjmnm thanks for the extra patches for the variations. Could attach screenshots that show the differences. Thanks
Comment #23
bnjmnm@tedbow Here are screenshot for each version. 2994909-21-slightly-louder-plus-one has a bit of css animation so I provided an animated gif for that one.
Comment #24
tedbowWould we still need these if we don't have overlay? Presummable nothing should be on top of the add section and block divs anyways.
I would vote against the animation in any of the solutions
Oh no! these already need rerolls
Comment #25
kostyashupenkoreroll of #21
Comment #26
bnjmnmThanks for the rerolls @kostyashupenko
I've hid the two variants we definitely wont be going with per #24 (2)
However, think we may need to explore something other than the current highlight-only approach. I tested this with a FE color scheme I used on a clients site last year. Unfortunately, the highlighting is almost indistinguishable from non-highlighted. I attached a gif demonstrating this.
The initially proposed gray-out approach would work with darker themes, but the reasons for moving away from this are valid ones.
I'll think over possible solutions that avoid the drawbacks of gray-out, while also accommodating a wider range of FE theme colors. Very interested in hearing suggestions.
Comment #27
bnjmnmDiscussed #26 outside of this issue and the conclusion was addressing darker color schemes is not in scope for this issue. Will provide a patch refining the approach selected in #24
Comment #28
bnjmnmRe:
#24.1 The z-index and position are still necessary for the black outline to fully replace the blue dashed one.
#24.2 Excellent selection, that's what we'll go with.
Patch also has a tiny JS optimization and a nit fix.
Comment #29
phenaproximaPatch looks good. There's test coverage, minimal changes, and everything that's going on is pretty clear. I like it! However, I do think this issue could use some before/after screenshots, so I'm tagging it for that. Beyond that, I have two small points of feedback:
This logic is confusing. Can it have a comment explaining what it's doing and, more importantly, why?
Pro-tip: I believe you could use $assert_session->elementCount() for a slightly cleaner assertion here, or anywhere you're asserting that some number of elements match a selector.
Comment #30
bnjmnmPatch addresses:
#29.1 - added some detailed comments so it's actually clear what the JS does.
#29.2 - nice protip, changes made
I also spotted & addressed an additional UX issue in the
dialog:aftercreate
event listener. In some cases, the target element is scrolled out of view when the off canvas dialog appears. highlight-out-of-viewport.gif shows this occurring.The solution in action can be seen with highlight-inside-viewport.gif
Screenshots:
Here's "Add Section" before the patch
Here it is after
Comment #31
Kristen PolThanks for the updates. I noticed some very minor things. I'll try to test it now.
Nitpick: For readability, perhaps:
Add Block/Add Section => "Add Block" and "Add Section"
Nitpick: 80 char wrapping.
Nitpick: 80 char wrapping.
Nitpick: 80 char wrapping.
Comment #32
bnjmnmNits in #31 addressed, TY @kristen-pol
Comment #33
Kristen PolChanges in interdiff look good. I tested it and the Add Section box is highlighted as expected. Marking RTBC.
Comment #35
tim.plunkettStraight reroll due to #3027938: Abstract the contents of LayoutBuilderController into a render element
Comment #36
bnjmnmComment #37
xjmEmbedding the SS in the IS.
Comment #38
lauriiiSorry to bump this back but it seems like #17.1 is still unresolved.
Comment #39
lauriiiComment #40
bnjmnmDiscovered that highlight disappears when adding blocks/sections that require more than one dialog. For example, when choosing a Two Column layout, the highlight disappears when the column width options are loaded. Adding the failing patch here for now as I wanted to get this visible without waiting on a fix.
This patch is also a reroll.
Comment #42
bnjmnmThis patch addresses additional use cases identified while testing
To address all the above issues, I added the
data-layout-builder-target-highlight-id
attribute to all (?) of the dialogs triggered by the layout builder UI. Addeddata-layout-builder-highlight-id
attributes to existing blocks and sections, previously this attribute was only present for "Add Block" and "Add Section".Tests were expanded to cover all the scenarios mentioned.
IS needs updating to reflect that highlighting should be added to all dialog-opening inputs, not just "Add Section"
Comment #44
bnjmnmHere are screenshots of the new areas receiving highlights. I'm also wondering if it would be beneficial to use a different outline color when "Remove" is selected, to make it more apparent what is being removed - particularly since the confirmation dialog does not visibly specify the item being removed.
Remove Section
Configure Section
Remove Block
Edit Block
Comment #45
tedbowDo we actually need to check if it is set? Would we ever want to preserve a different value? Could we just always set it?
data-layout-builder-target-highlight-id
I wonder if at this point it would make sense to havehighlightIdTrait
with justgetHightLightId()
Where maybe you send it in a array with the keys you have and it creates the id for you.
I think you should also wait for off-canvas or some elemet on it to be visible. To avoid random test fails.
need blank light before comment
This confirm there is only 1 highlight section and it has the class *new-section* but there are 2 new-section elements on the page. I don't think it confirms the correct one is highlight. Maybe we should switch to sending in a selector instead of a class.
need blank light before comment
need blank light before comment
need blank light before comment
I am wondering if it would be safer to copy
waitForNoElement()
(with the todo) from one of the other layout_builder js test. Then we can wait to explicitly that for the highlight to be take away and nto have to worry about timing issues.Probably also want to wait for no off-canvas dialog. before proceeding with test.
need blank light before comment
Here we should allow wait for dialog to close(noElement). I don't think there is any ajax request on close.
need blank light before comment
I think because of #2918718: Using ContextualLinkClickTrait::clickContextualLink() multiple times per page can cause random failures we want to reload the page before the second use of
clickContextualLink()
here.waitForNoElement()
Comment #46
bnjmnmThis patch addresses everything in #45, created a trait with four methods for generating a highlight ID, to cover the add & update scenarios for both sections and blocks.
Also added a x25 test patch because these UI-heavy tests (particularly ones with contextual links) seem prone to random failures.
This required a reroll, but the interdiff provided is pre-reroll, in case that aids the review process.
Comment #48
Kristen PolDoes this need manual testing?
Comment #49
bnjmnm@kristen-pol - thank you for asking! Manual testing would be great, particularly since the highlighting has been expanded to include existing blocks and sections, not just "Add Block" and "Add Section"
One of the things I'm most interested in is any scenarios where it seems like there should be focus and there isn't, especially since it took us a while to identify the need for highlighting existing Blocks/Sections.
Also interested in feedback about making the outline for "Remove Block"/"Remove Section" red instead of black. On the plus side: it could add a little gravitas to a potentially regretful operation -- particularly since currently it's kind of easy to mistakenly remove a section when it looks like it should just remove a block. On the down side: it deviates from the highlighting pattern people will be accustomed to at that point.
Comment #50
bnjmnmComment #51
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedputting this back on my list, the screenshots after #30 look significantly different to what I reviewed in #13
Comment #52
bnjmnm♻️🥖 Reroll
Comment #53
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedCan somebody summarize the design changes since the accessibility review in #13, and the reasons for them? I'm lost.
Comment #54
bnjmnmThat is a very reasonable request, @andrewmacpherson. The discussions that led to the changes since #13 took place at weekly UX meetings -- very little of that is documented here and it should have been.
My recollection of the the primary concerns about the gray-out approach:
(if this seems incomplete I can review the UX meeting recordings)
Several different approaches were discussed, and the agreement landed on highlighting the element that corresponds to the active off-canvas dialog with a 3px black outline.
Comment #55
tedbowwaitForLink()
doesn't wait for visibility so usually I do something like$this->assertNotEmpty($assert_session->waitForElementVisible('css', 'a:contains("Link test")');
Same here
This doesn't assert it is not empty.
Also should use
waitForElementVisible()
instead.This doesn't assert it is not empty.
Also should use
waitForElementVisible()
instead.Comment #56
bnjmnmUpdated tests per review in #55
Comment #57
tedbowThis should still be wrapped in
$this->assertNotEmpty()
There is another instance like this in the test.
I think this should say "to appear and for the page to be sized"
Isn't the resizing of the page the reason the element might get pushed out of the ViewPort?
Also created #3033410: Ensure that links that opening the off-canvas dialog are in the viewport after the dialog is open because I checked and this existing problem with the Setting Tray module. But could also affect any link using the off-canvas dialog.
So we should put a @todo here to find a general solution in that issue and get rid the custom solution here.
I tried to actually respond to one of the resize events fired in
off-canvas.es6.js
so that we won't need the wait but instead scroll after the page is resized. I couldn't get it work. So I think we general solution in the follow.Comment #58
bnjmnmPer #57
$this->assertNotEmpty()
...to fully appear
to...to appear and for the page to be sized
@todo
to that commentComment #59
tedbowChanges in #58 look good to address my review in #57
thanks, This issue is RTBC worthy in my opinion but we have needs accessibility review
Looks like @andrewmacpherson wants to take another look
Comment #60
bnjmnmComment #61
bnjmnmReroll
Comment #63
bnjmnmAfter the reroll, tests will not pass without disabling css animation. For good measure, I also added
assertWaitOnAjaxRequest()
after the tests AJAX requests.Comment #64
tedbowLooks good!
Comment #65
xjmI've seen the z-index brought up twice here already. @bnjmnm points out that it's necessary for it to be
> 0
, but what @lauriii asked I believe is whether1
is high enough.The JavaScript maintainer formerly known as @drpal (FKAdrupal) already signed off on the JS here, so I'm comfortable reviewing this for commit if we can confirm we've sufficiently address @lauriii's previous feedback about the z-index.
Comment #66
tedbowWe need to check here that
Drupal.offCanvas.isOffCanvas($element)
otherwise the block configure forms could actually open up modal themselves in the case they are using ckeditor and other custom code.. If we don't check we remove the highlight when the modal is closed.
Comment #67
tedbowhere is the fix for #66
Comment #68
bnjmnmVery good catch @tedbow. Checked this locally and it looks like a similar conditional needs to be added to
'dialog:aftercreate'
, otherwise the highlight is removed in the same manner. To reproduce, add a custom block and use ckeditor to add a link to the body field. Without adding a check it'dialog:aftercreate'
, the highlighting goes away as soon as the "add link" modal appears.Comment #69
tedbow@bnjmnm thanks for checking this and catching the other case.
The interdiff was created with
git diff -w
to ignore whitespace.Comment #70
bnjmnmWhile confirming the
z-index:
value is fine for #65, I adiscovered that if the cursor hovers over a highlighted block's contextual link button, the highlighting goes away due to a css rule from the contextual module.contextual-region.focus
taking priority. I addressed this by changing the.layout-builder-highlight
selector to#layout-builder .layout-builder-highlight
. Tests were modified to check for this, so a fail patch is included.The
z-index: 1
provides the results that I think work best to see, at least. It properly takes precedence over the blue dashed outlines, but contextual links appear over it and available for use.Anything else with static positioning should not be a concern as it would be contained inside the outline. If there are other use cases where the blocks contain non-statically positioned elements from core, hopefully the reviewer can point those out.
This patch includes the changes from #3035669: Block contextual links intermittently ignore mouseup, otherwise the tests would return a false positive.
Comment #72
tedbowcontextual misspelled.
should be surrounded by
$this->assertNotEmpty()
Comment #73
tedbowRunning the test locally it seems there fail I get is because the off canvas is in front of the contextual link. Does the off-canvas dialog have to open for the test to prove what is proving or could we close it again?
I testing adding a second
right before and it does fix that exact failure for me locally
Comment #74
bnjmnmAddressed the feedback in #72
Re: #73The test needs to happen with the dialog open. The problem is the supression of a mouseup event, so once another click happens the fail condition goes away. Hopefully the inconsistency is addressed as I rewrote the test so simulates the drag behavior that causes the problem with real-world use.
Comment #75
GrandmaGlassesRopeManI haven't had a chance to review all of patch, but I did want to strongly urge you to not use `setTimeout`. Any time you have to wait for an arbitrary amount of time for the browser to complete an action, it's a signal of a larger issue. Imagine this being run on a slow computer, or the test environment which will behave differently than the environment that the 700ms was decided on.
I saw that you have a `@todo` for this, and thats great. I would suggest blocking further development on this issue until you solve this problem. Perhaps dispatching an event when whatever you are waiting on is complete and listening for it here?
Comment #76
bnjmnmI wasn't particularly comfortable with the timeout either so I'm happy to get the push from @alwaysworking in #75 to revisit it - found a solution that should be much less fragile.
The 700ms was to wait for a css transition in off-canvas to complete.
I added an event listener that fires when the transition completes. The listener does roughly the same thing as the timeout, aside from the scroll amount being computed differently since there isn't access to the element's prior position.
Comment #77
bnjmnmReroll +
Also - because it may be easy to miss at this point - reminder from #70 that the JS changes in
behaviors.layoutBuilderDisableInteractiveElements
and changes toLayoutBuilderDisableInteractionsTest
come from #3035669: Block contextual links intermittently ignore mouseup and are necessary for the test to not return a false positive.Comment #78
lauriiiUsing this method isn't sufficient for overriding the contextual styles. If we want to specifically override the contextual selector, we could just use this selector:
.layout-builder-highlight.focus
. However, I'm a bit concerned if removing the focus styles will be acceptable.Comment #79
bnjmnmSince #78 was a bit open ended (overriding the focus styles vs. preserving what contextual provides), I've provided three options, gifs that demonstrate what they do, and the tests have been modified for each version to account for the differences.
2994909-79-highlight-over-focus
: contextual focus outlines are never visible if the element has.layout-builder-highlight
highlight-dashed-while-focused
- a focused contextual region with.layout-builder-highlight
will have a black dashed outline instead of the default bluecx-focus-beats-highlight
-.layout-builder-highlight
does not apply any outline on focused contextual regions. (note there is still overlap with the outline applied to.layout-builder__region
which would be outside the scope of this issue)Comment #80
tim.plunkettThis blocks a stable blocker.
Comment #81
tedbowre #75 @alwaysworking agreed we should get rid of the timeout
re #76 This works well and doesn't rely on the specific timeout value.
I have tested in manually and the scrolling still works as expected!
re #79
I think
cx-focus-beats-highlight
is the best solution here.It keeps the styles of the contextual module which also may have been customized by a particular theme. I don't like
highlight-dashed-while-focused
because it kind of assumes contextual will have dashed line and we don't this for any particular theme. 3) seem to assume the least about how contextual styling will look.Comment #82
xjmThere are three patches in #79. When we RTBC an issue, let's make sure the solution chosen is uploaded by itself so we don't accidentally commit the wrong thing. :)
I think @tedbow is saying that
#layout-builder .layout-builder-highlight:not(.focus)
is what he RTBCed, but I looked at the gif of that and it's a weird and disorienting behavior.It looks like a bug and it would confuse me if I were the user. I think this needs UX and a11y feedback.
This is already tagged for accessibility review, so we need that signoff before this is marked RTBC. I'm also going to specifically tag it for frontend framework manager review given recent discussion. Thanks!
Comment #83
xjmUpdating issue credit to include topic reviewers.
Comment #84
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedIt's not clear which patch the RTBC is about. I'm still confused about what is being proposed. There's talk of focus styles now?
Comment #85
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commented@xjm - "Weird and disorientating" is a bit vague. Can you explain in more detail? I'm having trouble following that animated GIF in #82. It's too fast and it loops so I can't tell what sequence of events it's supposed to depict.
Comment #86
tedbowre #72 sorry for RTBC'ing this to early.
I actually have change my opinion from #81. I detail my reasons below.
@andrewmacpherson hopefully I can clear up what is going on here with the focus styling.
core/modules/contextual/css/contextual.theme.css
from <code>core/modules/contextual/css/contextual.theme.css
.contextual-region.focus
selector is exactly the same as the div we highlight with.layout-builder-hightlight
After making the video and looking at the solutions in detail I now think we should use the patch 2994909-79-highlight-over-focus.patch
As you will see in the video in this solution if the block is highlighted via layout builder because it is being configured(or removed) in the off-canvas it does not apply the highlighting from the contextual module when the blocks is in focus. The contextual trigger is still highlighted.
The reason I think this is good is because the purpose of the contextual modules css to highlight the element that the contextual link trigger will effect is to let the user know what part of the page they will be affecting if they use the contextual links connected to the trigger.
Since in this case the part of the page they will be affecting is the block in the layout builder which already has a highlight then I don't think we need an extra highlight.
Comment #87
tim.plunkettFixing tag
Comment #88
DyanneNovaI'm inclined towards the first behavior. Highlighting the same way everywhere for contextual links makes that action feel more consistent.
Comment #89
lauriiiLack of focus effect doesn't necessarily cause critical problems for mouse users. If we completely remove the focus effect, it means that the only indication of the area being focused is the contextual link icon, which is very subtle.
@DyanneNova would your concerns be addressed if we could come up with an alternative design where focus and the highlight work together in a more elegant way?
Comment #90
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedI've watched the video from #86, and tried all the patches from #77 and #79
Firstly, there is a very confusing terminology problem in the comments here. Lots of comments talk about focus, but all the GIFs and videos show mouse hover behaviour only. Are you actually talking about focus? (i.e. document.activeElement, the :focus pseudoclass, the tabbing order, and all that.)
It seems what you are actually talking about is styling a container DIV, when the mouse hovers over it, or a control inside the container has keyboard focus (i.e. what :focus-within is for, once all the browsers support it). However the container itself doesn't get focus, so the WCAG "Focus visible" success criterion does not apply to it. The contextual menu button is what gets focus, and the patches here don't touch that.
From an accessibility viewpoint, I have no strong opinion about which patch from #79 is best. The simplest approach is
2994909-79-highlight-over-focus.patch
. There is no actual accessibility need for the extra styles in the other patches, at least as far as WCAG is concerned. The hover border style is a secondary signifier only; the primary signifier for a pointer user is the pointer itself (mouse arrow icon, screen magnifier cross-hairs, or actual finger).What I DO have strong opinions about:
.focus
is a misleading class name for the container outline styling. (Doesn't affect the user, but has made it hard to comprehend the comments here. That's a DX issue.)Comment #91
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedImportant related issue!
While I think that the highlight proposed here is helpful, it only helps sighted users. If we commit this issue on it's own, it will introduce a WCAG level A failure of "Info and relationships" because the only way you know what a remove block/section dialog refers to is by the visual highlight.
So we must not commit this issue, until after fixing:. #3037550: Clarify which block or section is being removed in layout builder dialog. I've made that a separate issue, because it's important for accessibility regardless of the highlight introduced here.
Comment #92
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedActive again now that #3037550: Clarify which block or section is being removed in layout builder dialog is in.
Re-queued tests.
Comment #95
tedbowComment #96
tedbowAfter meeting with UX team on Tuesday it was decided that the 2994909-79-highlight-over-focus.patch patch was the way to go from #79.
so only that patch needs a reroll.
The UX team also suggested we make a non-stable blocking follow-up Explore ways to make the highlighted Layout Builder element stand out more. This could be tricky because it is displayed in different front-end themes. So it might end up being just recommendation in the theming guide for Layout Builder.
Comment #97
lauriiiI tried to improve the design a bit from the black border.
Add section:
Highlighted add section:
Block:
Highlighted block:
Add block:
Highlighted add block:
Section with blocks:
Highlighted section with blocks:
Comment #99
tedbow@lauriii do you know what patch you started with if so I can make interdiff. I chatted with @lauriii and he said he only made CSS changes but want to confirm again with him.
Since #2938182: Design intuitive affordances for Layout Builder (for illustrating which parts of the page are editable in a given context) added a white background to the layout builder we are able now to actually do a background for the highlighted block too.
The class now here was changed from
'layout-builder-highlight'
which is why the test failed.I just was want to make sure this was intentional. Seems good to me.
This will also cause a fail since the actual styling has changed. I doubt we need to do this.
I don't think anywhere has in core we check that when apply a class it has certain style properties;
I removed it. If we wanted to keep it it would have be different based on whether it is block or "add section" or "add block" now since they are different.
Comment #100
lauriiiI used 2994909-79-highlight-over-focus.patch as a base for my patch in #97. Here's interdiff.
Comment #102
GrandmaGlassesRopeManThis should be split to a new line. Check Prettier.
This event, `transitionend`, can this be name-spaced? I am not sure where/what is triggering this event.
These are all numbers?
This should be split to a new line. Check Prettier.
Comment #103
xjmCan we get animations of the latest behavior with the final design and embed them in the IS? Thanks!
Comment #104
bnjmnmre #102
I double checked that height, scrollTop, outerHeight, and offset().top return numbers.
Additional things:
scrollBy()
with an object as the first arg, but it does acceptscrollBy(x-coord, y-coord)
. The options object is desirable when available (it provides the option to scroll smoothly instead of instantly - the default in some browsers), but not critical, so I added a check that usesscrollBy(options)
when compatible, but usesscrollBy(x-coord, y-coord)
otherwise. Here is a video of IE11 usingscrollBy(x-coord, y-coord)
https://youtu.be/0hkVsas8blMBefore:
After:
Comment #105
tim.plunkettComment #106
GrandmaGlassesRopeManI guess I should have mentioned that the changes look good to me. 🎉
Comment #107
lauriiiComment #108
tedbowAt UX meeting today there was unanimous that we switch back to highlight border colors that we had in before the changes in #97.
So this would mean always black border around highlighted elements not matter if they are new/existing block or section.
So this would just mean that
And we the consensus was that we remove the background change for blocks.
Since there were other JS and class name changes in #97 I don't think we can got back previous patches just need to update CSS for the new black border on highlighted elements.
Comment #113
tedbowcrediting those from UX meeting who wasn't already credited.
Let me know if I missed anyone
Comment #114
tedbowOk implementing the UX meeting recommendations re #108
Going to file 2 follow ups:
Comment #118
xjmAdding a couple other contributors from the UX meeting.
Comment #119
xjmI manually tested and confirmed this addresses the usability and accessibility feedback we received during the UX meeting yesterday. Nice work!
Comment #120
xjmI reviewed the docs and backend code. Just a bunch of docs nitpicks, mostly regarding small words like "the" and "is" being omitted from inline comments.
This formatting is a bit weird; I think we can combine these two half-paragraphs?
Should this be "an element's value"?
"Added to the element", I think. (As the sentence is currently written, "it" refers to the data attribute rather than the element itself.)
Nit: the word "the" is missing twice here ("the target element").
"If the element is in the viewport".
"If the element is not in the viewport"
"Check whether the browser supports scrollBy(options). If it does not, use..."
"Remove the highlight".
This isn't quite a sentence... maybe "The section region in which the block is placed"?
Nit: "dialog-opening elements", or, better, "elements that open the dialog".
"The highlight is..."
"Submit the formm to add the section and then confirm that no element is highlighted anymore."
"Add a custom block."
"The highlight should..."
"The highlight is..." Also,
doBuildForm()
should have parens."A block is..."
"Make sure the highlight remains when contextual links are revealed with the mouse."
@todo Remove the reload once https://issue is completed.
"Confirms the presence of the 'is-layout-builder-highlighted' class."
"Waits for the dialog to close and confirms no highlights are present."
Comment #121
tedbowIn #114
\Drupal\Tests\layout_builder\FunctionalJavascript\LayoutBuilderDisableInteractionsTest::assertContextualLinkRetainsMouseup()
failed because it checks that mouse up events are working correctly by moving the mouse after using the contextual link on the body field and making sure the body field is in exactly the same place. This is because if the mouseup event is not working correctly than the body field would actually dragged when the mouse moved.The problem is with our new border JS
getBoundingClientRect()
of the element will change by 4 pixels(the exact difference in the fail). We could change assert to take these 4 pixels into account but this seems we would be just asking for random fails if we measure to the exact pixel.Instead I changed the assert to take into account that the body might move very slightly but it should be much much less than the distance the mouse is move(because it is not moving because the mouse moved).
Please read he comments in the interdiff on core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderDisableInteractionsTest.php for more detailed explanation of the assert logic but it finally comes down to this assert.
$this->assertGreaterThan($distance_body_block_moved * 20, $minimum_distance_mouse_moved);
In
assertGreaterThan()
the 2nd should be greater.RE #120 @xjm thanks for the review. Fixed all the nits 🔎
Comment #122
tim.plunkettThe doc fixes look good.
I like the test change and the new method. One nit:
I think the default value should be removed and used explicitly in the method calls. If you disagree, can you expand the @param doc to mention the default value?
Comment #123
tedbow@tim.plunkett thanks for review. fixed
Comment #124
tim.plunkettThanks!
As of #119 this has no more signoffs needed, #120 is addressed by #121, and the patch on the whole looks great!
Comment #125
lauriiiIt would be great if we didn't change the margin of the
.block
element since it is not a layout builder element.Discussed with @tim.plunkett on a call and he proposed that we wrap blocks in the layout builder preview with another div that we could theme more safely. I think that is a good solution for fixing this problem.
s/css/CSS
Comment #126
tim.plunkett1) Attempted using a wrapper but that interfered with contextual links.
I opened #3042216: Only add .layout-builder-block to blocks when in the Layout Builder UI to discuss changing how that is done.
2) Fixed!
Marking RTBC for the capitalization fix
Comment #129
lauriiiI'm fine with postponing #125.1 to a follow-up. We do other modifications to the spacing in the layout builder preview as well, and the worst case scenario is that someone experiences different margin between blocks. This shouldn't render the site unusable or nothing of the sort.
I've manually tested the changes on Chrome, Firefox and IE 11 (normal and high contrast mode) and I couldn't find any regressions on either Umami or Bartik.
I've got a backend signoff from @xjm in #120. I also recognized #91 as a sign-off from accessibility topic maintainer since we've already solved the related issues.
There was an extra new line that was caught by PHPCS. I fixed this on commit and attached an interdiff.
Committed 40d6aca and pushed to 8.8.x. Also cherry-picked to 8.7.x. Thanks!