Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
From Wim's review in #164 of the parent issue:
Accessibility: Another accessibility problem is that you can't stop quick editing by pressing ESC.
This might possibly be easy to fix, so tagging as Novice.
Comment | File | Size | Author |
---|---|---|---|
#35 | 2784571-35-esc.patch | 3.64 KB | tedbow |
#31 | interdiff.txt | 2.17 KB | GrandmaGlassesRopeMan |
#31 | 2784571-31.patch | 4.19 KB | GrandmaGlassesRopeMan |
| |||
#29 | interdiff-2784571-26-29.txt | 1.88 KB | tedbow |
#29 | 2784571-29.patch | 2.42 KB | tedbow |
Comments
Comment #2
webchickComment #3
tedbowComment #4
miwayha CreditAttribution: miwayha as a volunteer commentedI'm at the Midwest developers summit. I'm going to try to do some JS to fix it.
This is my first time working on a core issue.
Comment #5
nod_Don't spend too much time on this, the related issue fixes this problem. Might be better to give the other patch a review or continue the clean-up.
Comment #6
miwayha CreditAttribution: miwayha as a volunteer commentedOK. I'll focus effort there instead.
Comment #7
miteshmapComment #8
SKAUGHT#7 only seems to toggle toolbar's edit/editing toggle visually. tray does not close. needs work
Comment #9
miwayha CreditAttribution: miwayha as a volunteer commentedI wonder whether @nod_ should update us on the status of this re: using the dialog system. If this issue is moot, we should close it.
Comment #10
tedbowRe #9
Once #2786459: "Offcanvas" tray should be using the existing dialog system is committed ESC will close the offcanvas dialog. It will not exit out of editing mode.
I am not sure what the best behavior is.
Maybe if dialog open close dialog but stay in "editing" mode(this would happen now)
If dialog is not open exit "editing" mode.
In that case #7 could just check if dialog is currently open.
Comment #11
SKAUGHTif this is to be tackled from the current outside_in module...
interactions of the tray and the toolbar are 2 separate behaviours.. an ESC would need to exist for both script sets.
Comment #12
miwayha CreditAttribution: miwayha as a volunteer commentedRe #10:
Thanks for that information. That was really helpful.
I would love to work on this issue, but I guess at this point, I'm still confused about where work needs to happen. It sounds like #7 was wasted effort due to less-than-ideal issue hygiene, and it would be really discouraging if more effort was spent on this due to confusion about whether this issue still needs work or not, and ambiguity about what code to work against. It looks like #11 has the same confusion I have.
So again, to avoid wasted effort and ambiguity, I'm not sure what if any work needs to be done here, and what base to work off of. But form my perspective, it looks like this issue should be closed — and another one opened? Perhaps another one should be opened once the commit you're referring to occurs? And perhaps a better issue summary?
Comment #13
tedbowre nod_ in #5
#2786459: "Offcanvas" tray should be using the existing dialog system fixes closing the offcanvas try but exiting Outside In edit module(which is disables links/forms etc)
@miteshmap's patch will exit edit mode but also will enter edit mode.
I am updating with small change to check if the site is currently in Edit mode.
Re @SKAUGHT in #11
Yes that sounds right.
Once #2786459: "Offcanvas" tray should be using the existing dialog system lands ESC will close the offcanvas tray b/c the existing dialog system works this way.
This patch will make it so hitting ESC will exit Edit mode.
So with the patch and #2786459: "Offcanvas" tray should be using the existing dialog system 1 time pressing ESC will both close the tray and exit Edit mode.
re @miwayha
Could you review the patch and confirm that it exits Outside In edit mode? Currently the tray will remain open until #2786459: "Offcanvas" tray should be using the existing dialog system lands. thanks
Comment #14
tedbow#2786459: "Offcanvas" tray should be using the existing dialog system just got committed.
Patch in #13 still applies. It should now close offcanvas and exit edit mode at the same time.
Comment #15
GrandmaGlassesRopeManThis looks good to me.
Comment #16
nod_I'd rather we use the API instead of ad-hoc code.
Comment #17
naveenvalechaRemoved the extra spacing.
Comment #18
tedbow@nod_ the patch in #16 exits "Edit" mode whenever you close the tray.
#13 exits the "Edit" mode only when you close the tray via ESC. Maybe that was bad idea but we do want to remain in "edit" mode when tray is closed.
So maybe for ESC we should go with
So if you had a tray open, first ESC closes tray, second ESC exits "Edit" mode
Comment #19
nod_see #18.
Comment #20
SKAUGHTi'm not clear on which patches to apply at this point. it seems they are all individual commits.
Comment #21
tedbowI talk with @nod_ on irc about patch in #16(#17 is the same but with spacing fix)
The original UX design in #2762505: Introduce "outside in" quick edit pattern to core should that you don't exit "Edit" mode every time you close the tray. That is what the patch in #16 does.
He also didn't like my idea:
But rather prefered
(from IRC)
This not possible as far as I can tell with the approach in #16. This because
At this point you can't actually tell what key was pressed to close the dialog. I spent some time debugging the event here is the dialog close not the key or click event that closed it. Even if you could figure out the last key pressed and the key press event could have happened before the tray was open.
Re nod_'s comment #16
It doesn't seem we can use the dialog system's API because of the reasons above.
So for
This is actually done by my patch in #13 actually does this.
I will upload it again just to be clear, it is the same as #13.
Comment #22
Everett Zufelt CreditAttribution: Everett Zufelt at Myplanet commentedAs a screen-reader user, I often press escape to exit "forms mode" or interactive mode. I find that this can have undesired results of dismissing a dialog or other interactive mode of a web application.
The behaviour of not using escape in a particular context can be learned. Could it be worth adding a Drupal.announce to indicate that context has changed? Perhaps this is already happening, I have not tested.
"Exited edit mode" or something similar.
Comment #23
mgiffordThat sounds like a good approach Everett.
Comment #24
tedbow@Everett Zufelt my guess would this is already happening but I haven't tested.
Exiting the dialog is the same as closing any core dialog. Drupal.announce I think should already be called.
Exiting "edit" mode is actually controlled by the Contextual module. Settings Tray doesn't create edit mode but enhance it. I am not sure if the Contextual module is using Drupal.announce but it seems like it should be.
If someone could confirm these it would be good.
Comment #25
Bojhan CreditAttribution: Bojhan as a volunteer and commentedFrom my point of view, closing should be 1-on-1 with going out of a mode. Explaining closing + going out of edit mode, is a really though thing from a UI perspective.
Comment #26
GrandmaGlassesRopeMan8.3.x
Drupal.announce()
for esc key actionComment #27
tedbow@drpal thanks for patch.
Marking as RTBC.
For those reviewing
ESC exits edit mode.
It will also close the current form but that is from the dialog system.
There is also Drupal.announce called "Exited edit mode"
Comment #28
alexpottIt'd be great to test this using a JavascriptTetsBase test. One of tests that extend \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInJavascriptTestBase might be a good place for this.
Comment #29
tedbow@alexpott good point.
Instead of creating a new test I added to \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest::testBlocks
This has to go in and out of Editing mode to test the functionality. I substituted 1 time with the ESC key and tested for announce also.
Comment #30
droplet CreditAttribution: droplet commentednamespace:
keyup.outsideIn
Comment #31
GrandmaGlassesRopeMan@droplet - Fixed the namespace, and fixed another issue with namespace collision between
outsidein
andoffcanvas
.Comment #32
tkoleary CreditAttribution: tkoleary at Acquia commentedComment #35
tedbowJust needed a re-roll
Comment #36
tedbowI reviewed again. Since I just did a re-roll and didn't most of the work marking as RTBC.
Comment #37
alexpottWhy are these changes necessary? I'm guessing this is because:
But if that is caused by namespacing the keyup event then why not choose a different namespace.
Comment #38
tedbow@alexpott thanks for the taking a look at this issue.
UPDATE AND CORRECT EXPLANATION
Because in offcanvas.js we were already performing
To remove listeners in offcanvas.js when we added in this patch outside_in.js
offcanvas.js was all of sudden removing listeners in outside_in.js which was not intended.
So we switch all event namespaces in offcanvas.js to '.offcanvas'
Offcanvas should work without outside_in.js and not conflict so that eventually we can do #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it
Comment #39
alexpottThanks @tedbow.
Comment #40
alexpottCommitted 2161fb2 and pushed to 8.3.x. Thanks!
Comment #43
webchickSince this only touches Outside In module, and it's an accessibility fix, cherry-picked this to 8.2.x too. Let me know if that was the wrong thing to do.
Comment #47
tedbowChanging to new settings_tray.module component. @drpal thanks for script help! :)